Last modified: 2010-05-15 16:03:18 UTC

Wikimedia Bugzilla is closed!

Wikimedia has migrated from Bugzilla to Phabricator. Bug reports should be created and updated in Wikimedia Phabricator instead. Please create an account in Phabricator and add your Bugzilla email address to it.
Wikimedia Bugzilla is read-only. If you try to edit or create any bug report in Bugzilla you will be shown an intentional error message.
In order to access the Phabricator task corresponding to a Bugzilla report, just remove "static-" from its URL.
You could still run searches in Bugzilla or access your list of votes but bug reports will obviously not be up-to-date in Bugzilla.
Bug 14717 - wikibits includes nonexistent stylesheet(s)
wikibits includes nonexistent stylesheet(s)
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Interface (Other open bugs)
1.13.x
All All
: Normal minor with 1 vote (vote)
: ---
Assigned To: Nobody - You can work on this!
http://en.wikipedia.org/wiki/?useskin...
: patch, patch-need-review
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-07-03 22:04 UTC by Jack Schmidt
Modified: 2010-05-15 16:03 UTC (History)
5 users (show)

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


Attachments
Patch to wikibits (1.05 KB, patch)
2008-08-10 00:19 UTC, Chad H.
Details
Keep it simple (1.13 KB, patch)
2009-02-21 01:12 UTC, bluehairedlawyer
Details
add skin specific config to choose fixes for wikibits.js to include (4.06 KB, patch)
2010-01-13 19:53 UTC, Jools Wills
Details
add skin specific config to choose fixes for wikibits.js to include (4.04 KB, patch)
2010-01-13 19:57 UTC, Jools Wills
Details
alternative fix for fixes css inclusion (610 bytes, patch)
2010-01-13 20:41 UTC, Jools Wills
Details

Description Jack Schmidt 2008-07-03 22:04:04 UTC
One gets harmless errors that a stylesheet was given with content-type text/html on some browsers (like Safari) because a non-existent stylesheet was requested, and the browser got the nice html error page instead.  See the similar bug #14520, https://bugzilla.wikimedia.org/show_bug.cgi?id=14520

It should be easy to fix, either by including blank such files, or adding a more complicated check to wikibits.js:

Line 96 of wikibits.js ( http://svn.wikimedia.org/svnroot/mediawiki/trunk/phase3/skins/common/wikibits.js rev 37012 or http://en.wikipedia.org/skins-1.5/common/wikibits.js ) is the following:

		document.write('<link rel="stylesheet" type="text/css" href="'+stylepath+'/'+skin+'/KHTMLFixes.css">');

I believe this could be changed to:

		if(skin=='monobook') document.write('<link rel="stylesheet" type="text/css" href="'+stylepath+'/'+skin+'/KHTMLFixes.css">');

since this is the only skin using the file.


The missing files from SVN are:
skins/chick/KHTMLFixes.css
skins/modern/KHTMLFixes.css
skins/myskin/KHTMLFixes.css
skins/simple/KHTMLFixes.css

The missing files from wikipedia are:
skins-1.5/chick/KHTMLFixes.css
skins-1.5/cologneblue/KHTMLFixes.css
skins-1.5/modern/KHTMLFixes.css
skins-1.5/myskin/KHTMLFixes.css
skins-1.5/nostalgia/KHTMLFixes.css
skins-1.5/simple/KHTMLFixes.css
skins-1.5/standard/KHTMLFixes.css
Comment 1 Chad H. 2008-07-04 16:44:02 UTC
Fixed in r37063.
Comment 2 Brion Vibber 2008-07-04 21:23:29 UTC
Reverted in r37067.
Comment 3 Chad H. 2008-08-10 00:19:10 UTC
Created attachment 5157 [details]
Patch to wikibits

Added a new function in wikibits called "doesFileExist". Given a relative path beyond wgServer, the function will return true if the HTTP request to it returns with status 200. 

Applied it to importStyleSheetURI, so it will only add a stylesheet if said file exists.
Comment 4 Aryeh Gregor (not reading bugmail, please e-mail directly) 2008-08-10 20:24:19 UTC
Um, so this adds an extra HTTP request on every page load for every stylesheet, unconditionally?  This doesn't strike you as a bad idea?  I would suggest that the skin set an extra JS var that can be checked, as Brion suggested in his commit message.
Comment 5 Brion Vibber 2008-08-11 00:54:21 UTC
Indeed, this check would do much more harm than good. The browser already does this 404 check for us by... trying to load it... :)

It would probably be best if we can dump these extra workaround scripts as much as possible, though, or make the code for them very skin-specific.
Comment 6 Siebrand Mazeland 2008-08-13 12:00:07 UTC
Assigned to ^demon, who has already put the most work in this, and is probably the right person to fix it, after adressing comment 4 and comment 5.
Comment 7 Roan Kattouw 2009-02-18 16:34:06 UTC
I suggest moving all skin-specific JS to /skins/skinname/common.js and *requiring* that file to exist for every skin (albeit possibly empty) so it can just be included unconditionally and forgotten about.
Comment 8 Jools Wills 2009-02-18 18:30:42 UTC
Is there really a need for a requirement for a common.js? If someone creates a skin that needs an additional javascript, then it is a trivial task to include it in the Skinname.php. Right now only monobook requires those additional javscript css includes. so perhaps monobook should just include this javascript itself, and if another skin wants to do so, they can also include it.

Comment 9 Aryeh Gregor (not reading bugmail, please e-mail directly) 2009-02-19 00:40:34 UTC
It's sloppy to include empty scripts.  It increases HTTP requests and latency unnecessarily.  Some browsers will block all parsing while waiting for the script to arrive, even though it will turn out to be empty.  Includes should be included only if they're actually needed.
Comment 10 bluehairedlawyer 2009-02-21 01:12:09 UTC
Created attachment 5839 [details]
Keep it simple

Not much need to make it more complicated than it needs to be. If it doesn't exist, don't request it!
Comment 11 Chad H. 2009-07-23 01:53:15 UTC
Not an issue anymore, as KTHMLFixes was removed in r53141.
Comment 12 Jools Wills 2009-07-23 05:32:58 UTC
Doesn't the javascript still include opera fixes etc, which may not exist on a custom skin.
Comment 13 Jools Wills 2010-01-13 19:53:29 UTC
Created attachment 6953 [details]
add skin specific config to choose fixes for wikibits.js to include

Earlier patch was rejected by Brion, as it was monobook specific, and didn't address the other css files for other browsers. Here is a patch that has an additional skin variable that contains an associative array where each "buggy browser" can have an associated css fix file with it. I have included this for monobook. I don't think any of the other skin's need these fixes.

I also changed the parameter for makeGlobalVariablesScript to be the skin object/class rather than just the name of the skin, so it can get the data about the required css' from the skin to pass to the javascript. I chose this over adding an additional parameter, in case more stuff was needed in the future, but feel free to make it two parameters if preferred,
Comment 14 Jools Wills 2010-01-13 19:54:52 UTC
reopening bug : see previous patch
Comment 15 Jools Wills 2010-01-13 19:57:24 UTC
Created attachment 6954 [details]
add skin specific config to choose fixes for wikibits.js to include

Sorry. wrong version. here is correct diff
Comment 16 Aryeh Gregor (not reading bugmail, please e-mail directly) 2010-01-13 20:21:56 UTC
Is there a big advantage here over just hardcoding it into wikibits.js?  In practice, it looks like nothing but Monobook (and Chick, which is closely patterned off Monobook) actually uses separate fixes files.  Third-party skins can always inject their own CSS fixes through other means if they really want them, or just use CSS selector hacks like Vector does.
Comment 17 Jools Wills 2010-01-13 20:27:01 UTC
well hardcoding it would have been much easier based on skin name, but a previous patch got reverted for this reason by brion, so i made it as was "requested" here. Im happy with any fix. I just want the 404's outta my stats ;-)
Comment 18 Aryeh Gregor (not reading bugmail, please e-mail directly) 2010-01-13 20:40:08 UTC
Committed a hardcoded check for Monobook in r61023.
Comment 19 Jools Wills 2010-01-13 20:41:29 UTC
Created attachment 6955 [details]
alternative fix for fixes css inclusion

here is an alternative css fix which hardcodes the skinnames. I removed the typedef checks, as I couldn't see how you could get to this point without skinname/stylepath being defined.

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


Navigation
Links