Page MenuHomePhabricator

Seemingly broken skins under Resource Loader
Closed, ResolvedPublic

Description

Results of skin tests on SVN head, but expected 1.17 will be all but the same:

Chick - Fine
Classic (Standard) - Sidebar below content
Cologne Blue - Sidebar below content
Modern - Fine
Monobook - Fine
MySkin - Sidebar below content, but is also like that on 1.16wmf4 http://en.wikipedia.org/wiki/Main_Page?useskin=myskin
Nostalgia - Fine
Simple - Fine
Vector - Fine

So for me, 2, possibly 3 skins with the same issues, that need looking at for 1.17


Version: 1.17.x
Severity: major

Details

Reference
bz26649

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 21 2014, 11:17 PM
bzimport set Reference to bz26649.
Reedy created this task.Jan 10 2011, 4:26 AM

I remember the sidebar-below-content issue, and it had a particularly nasty cause, something to do with user preferences-based CSS. I'll poke at this when I have time, but I won't have much time this week I'm afraid.

Created attachment 7969
repair quickbar of standard skin

Patch fixes this for standard. The problem is that cologneblue uses different CSS for the same option. Is it safe to introduce skin-dependant behavior into ResourceLoaderUserOptionsModule.php ?

Attached:

(In reply to comment #2)

Created attachment 7969 [details]
repair quickbar of standard skin

Patch fixes this for standard. The problem is that cologneblue uses different
CSS for the same option. Is it safe to introduce skin-dependant behavior into
ResourceLoaderUserOptionsModule.php ?

Safe, yes, good idea, no. Isn't there some existing Skin method that returns this #quickbar stuff?

Attached:

TheDJ added a comment.Jan 11 2011, 8:23 PM

Mostly fixed in r80034

The CSS is now flipped, because the rest of the CSS is flipped as well. This means that default behavior for rtl implementations has changed from left aligned quick bar to right aligned quickbar.

Might require message update for the preferences ?

Simple mostly done in r80163. Still requires porting of the highlightbroken user option, probably in the same style as done in r80034

(In reply to comment #5)

Simple mostly done in r80163. Still requires porting of the highlightbroken
user option, probably in the same style as done in r80034

Was this done yet? What remains to be done for this bug?

Roan, is it possible you could check see if a problem still exists?

TheDJ added a comment.Jan 22 2011, 9:33 AM

I'll try to look at this today.

TheDJ added a comment.Jan 22 2011, 9:17 PM

r80771 fixes the Simple skin.

TheDJ added a comment.Jan 22 2011, 9:29 PM

r80772 fixes Chick skin

r80776 ports Modern to RL

All SkinTemplate generation skins should now be ported.

Stuff that is still broken:

  • IE stylesheets of Monobook and Chick
  • All non-SkinTemplate skins, although working, are not yet ported to RL

Stuff that needs cleaning up:

  • RTL stylesheets: monobook/rtl.css, modern/rtl.css
  • myskin/main.css (empty, see no need to keep it)
TheDJ added a comment.Jan 23 2011, 1:43 AM

With r80785, all skins should be ported to ResourceLoader

  • common_rtl.css needs to be taken care of.

These rtl files of the skins seem to contain mostly redundant declarations atm, with the exception of some styling for lists under older gecko browsers. Perhaps factor this all into one file and only include when in RTL mode ?

(In reply to comment #11)

r80776 ports Modern to RL

Is this needed for Modern to work in 1.17, or can it be left out of the release?

Stuff that is still broken:

  • IE stylesheets of Monobook and Chick

At least Monobook should be fine in 1.17. Not sure about Chick.

Could you (or someone) identify which skins are broken in REL1_17 right now so I know which fixes to backport?

TheDJ added a comment.Jan 24 2011, 8:22 AM

No, these skin ports are not needed for 1.17, they are just ports to RL. I think I marked everything 1.17 that needs porting in order to make the skins work.

(In reply to comment #14)

No, these skin ports are not needed for 1.17, they are just ports to RL. I
think I marked everything 1.17 that needs porting in order to make the skins
work.

Thanks

TheDJ added a comment.Jan 25 2011, 7:38 PM

I say we can mark this as fixed then. If someone spots an error in the 1.17 branch for these skins, they can reopen.