Last modified: 2011-03-13 18:04:34 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T15389, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 13389 - Browser detection in wikibits.js
Browser detection in wikibits.js
Status: RESOLVED WONTFIX
Product: MediaWiki
Classification: Unclassified
Interface (Other open bugs)
unspecified
All All
: Lowest enhancement (vote)
: ---
Assigned To: Nobody - You can work on this!
: easy
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-03-16 13:10 UTC by Lupo
Modified: 2011-03-13 18:04 UTC (History)
1 user (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description Lupo 2008-03-16 13:10:21 UTC
wikibits.js currently does

if (clientPC.indexOf('opera') != -1) {
  var is_opera = true;
  var is_opera_preseven = (window.opera && !document.childNodes);
  var is_opera_seven = (window.opera && document.childNodes);
  var is_opera_95 = (clientPC.search(/opera\/(9.[5-9]|[1-9][0-9])/)!=-1);
}

Could this be changed to 

var is_opera = (clientPC.indexOf('opera') != -1);
var is_opera_preseven = is_opera && (window.opera && !document.childNodes);
var is_opera_seven = is_opera && (window.opera && document.childNodes);
var is_opera_95 = is_opera && (clientPC.search(/opera\/(9.[5-9]|[1-9][0-9])/)!=-1);

This ensures that these variable are indeed defined.

Also, could we have additional variables is_IE and is_IE_pre_7? I currently use

var is_IE       = !!window.ActiveXObject;
var is_IE_pre_7 = is_IE 
               && (function(agent) {
                     var version = new RegExp('MSIE ([\\d.]+)').exec(agent);
                     return version ? (parseFloat(version[1]) < 7) : false;
                   })(navigator.userAgent);

(This is tested and working, see [[:commons:MediaWiki:Tooltips.js]].)
Comment 1 Lupo 2008-03-16 18:36:24 UTC
Just noticed bug 12766 (adding IE detection; closed by Tim as WONTFIX)...
Comment 2 Bryan Tong Minh 2008-03-16 21:00:14 UTC
We probably need a generic object BrowserDetection, and then do BrowserDetection.isIE() etc.
Comment 3 Lupo 2008-03-17 08:36:02 UTC
Well, if you want to wrap it in an object BrowserDetection, then I'd rather have one function BrowserDetection.getBrowser () returning "ie", "gecko", "opera", etc (or numeric "constants" defined for these, such as BrowserDetection.IE, BrowserDetection.GECKO, etc.), plus an additional function BrowserDetection.getVersion () returning the browser version as a float (5.5, 2.0, 7.0 etc) or null if unknown.

Cache the results of the browser detection inside the BrowserDetection object to avoid having to re-evaluate it each an every time one of these functions is called.
Comment 4 Brion Vibber 2008-03-17 22:23:38 UTC
As much as possible, "browser detection" of this sort should be taken out and shot.

When it's necessary to check browser capabilities to perform some conditional operations, you should check *only the specific browser capabilities* relevant.

More 'general' browser detection should be a last resort only, if there is *no way whatsoever* to detect the specific feature needed or bug to work around.
Comment 5 Lupo 2008-03-18 12:46:58 UTC
Sure, we know that. But as you say, sometimes it still can't be helped, because there is no capability to test for, or the test would be exceedingly expensive. Having variables is_IE etc. for these cases is useful, and it wouldn't need to be reinvented.

The problem with the BrowserDetection object proposed in comment 2 is just that it'd break many existing scripts if the existing is_gecko, is_opera, etc. variables were removed.

var is_IE = !!window.attachEvent;

would be even better.
Comment 6 Brion Vibber 2008-03-18 18:24:53 UTC
That would be obviously wrong, as other browsers may well choose to implement window.attachEvent for IE compatibility. You should *only* check for window.attachEvent if you want to use window.attachEvent; not for general "am I running IE?" checks.

I strongly recommend removing these generic checks where possible. Scripts incorrectly using them should be corrected to do proper checks.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links