Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#5367 closed defect (fixed)

dojo.isSafari is broken. Here is a patch.

Reported by: guest Owned by: James Burke
Priority: high Milestone: 1.0.2
Component: Core Version: 1.0
Keywords: Safari Cc: daniel.carrera@…
Blocked By: Blocking:

Description (last modified by Adam Peller)

Using the dojo.js.uncompressed file from the Dojo 1.0.0 distribution:

Line 934:

- d.isSafari = (vi) ? parseFloat(dav.substring(vi+8)) : 2;
+ d.isSafari = (vi > 0) ? parseFloat(dav.substring(vi+8)) : 2;

Why? Here is the original block:

if(dav.indexOf("Safari") >= 0){
     var vi = dav.indexOf("Version/");
     d.isSafari = (vi) ? parseFloat(dav.substring(vi+8)) : 2;
}
  • The variable dav does not contain the string "Version/".
  • dav.indexOf("Version/") => -1
  • vi is -1
  • '(vi) ?' evaluates to true because vi is not 0.
  • dav.substring(vi+8) => dav.subString(7)
  • Of course, that returns some general string. No what we want.
  • When you give that string to parseFloat() you get NaN.
  • So d.isSafari is set to NaN.
  • And of course, that evaluates to false in a conditional.

This is a severe bug because all the Safari-specific functions don't work if dojo.isSafari doesn't work.

Change History (7)

comment:1 Changed 11 years ago by James Burke

Owner: changed from anonymous to James Burke
Status: newassigned

I assume this is an issue with Safari 2? With Safari 3, the code seems correct. For Safari 3, this is the navigator.appVersion (what the dav variable above should hold):

5.0 (Macintosh; U; Intel Mac OS X; en) AppleWebKit/523.12 (KHTML, like Gecko) Version/3.0.4 Safari/523.12

Version is in that string. I rewrote the parseFloat operation with navigator.appVersion(so I can copy paste into firebug lite):

parseFloat(navigator.appVersion.substring((navigator.appVersion.indexOf("Version/") + 8)))

When I run that in Firebug Lite on this page, then I get "3". Also testing for dojo.isSafari returns 3.

So if you are using Safari 2, perhaps you can include navigator.appVersion in this bug report so I can use it to test the logic (I no longer have access to Safari 2).

comment:2 Changed 11 years ago by guest

I assume this is an issue with Safari 2? With Safari 3, the code seems correct.

It is never "correct" to use "vi ?" to test if vi is positive. That much should be self evident. It only works on Safari 3 because with Safari 3 vi is >0. For that matter, you could remove the test entirely and it'd still work with Safari 3.

Think about it for a second. What's the purpose of "vi ?" in that line? It is clearly to test if there was a match for "Version/". And as I explained, if there isn't a match for version, the test will give the wrong result. Because when there isn't a match, the .indexOf() method returns -1 and that evaluates to "true".

Here navigator.appVersion for my computer:

0.5 (Macintosh; U; Intel Mac OS X; en) AppleWebKit/419.3(KHTML, like Gecko) Safari/419.3

comment:3 Changed 11 years ago by Adam Peller

Description: modified (diff)

made the text readable

comment:4 Changed 11 years ago by Adam Peller

how about the even more concise

d.isSafari = parseFloat(navigator.appVersion.split("Version/")[1]) || 2

which leads me to realize we can remove the split and try/catch around the isFF and isIE lines (I think?) this is addictive. I'll attach my pass at a cleaned up hostenv_browser.js for review.

Do we need to stop 1.0.2? I'd say we should probably shoot for 1.0.3, since Safari 2 isn't technically supported anymore.

comment:5 Changed 11 years ago by James Burke

Resolution: fixed
Status: assignedclosed

(In [11815]) Fixes #5367: dojo.isSafari was not reporting 2 if using Safari 2.

comment:6 Changed 11 years ago by James Burke

(In [11816]) Fixes #5367: dojo.isSafari was not reporting 2 if using Safari 2. (merge to trunk)

comment:7 Changed 11 years ago by James Burke

Cc: daniel.carrera@… added
Note: See TracTickets for help on using tickets.