Page MenuHomePhabricator

The situation with $.collapsibleTabs and its Vector ext. counterpart sucks badly
Closed, ResolvedPublic

Description

Okay, so.

There is a file called [mediawiki/core]/resources/jquery/jquery.collapsibleTabs.js. It's unused in core, as far as I could find: there is only a jquery.collapsibleTabs module defined for it, and for some bizarre reason it also greps in /tests/selenium/data/SimpleSeleniumTestDB.sql. Nothing else.

Then there's a file called [mediawiki/extensions/Vector]/modules/ext.vector.collapsibleTabs.js. This one is used all right, and depends on the other one.

And there's at least four things wrong with these two:

  • ext.vector one *overwrites* two internal functions of $.collapsibleTabs, just like that. You were using that in your own extension, buddy? Tough luck.
  • ext.vector also changes two default settings of the jquery one. Which is fine, except that Vector's ones differ by explicitly handling directionality (which the core version doesn't do). This seems like a bug in core, or dead code in Vector.
  • ext.vector depends on h5 headers in the navigation portlets, which were ages ago changed to h3s. I guess this is dead code if nobody noticed during all this time on all Wikimedia wikis, but I'm afraid to clean this up myself.
  • And last but not least, why do *two* files for this exist? If this is used only in Vector skin/extension, it should be moved entirely to one place. (And personally I couldn't care less whether this would be the skin (thus core) or the extension. Just plese clean this up.)

I was trying to figure out why are things this way, but the commit that creates this file in Vector has a cryptic message of "Ported JavaScript from UsabiltyIntiative/Vector" [sic], so I gave up.

I came upon this trying to fix bug 20234.


Version: unspecified
Severity: major
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=46513

Details

Reference
bz44385

Event Timeline

bzimport raised the priority of this task from to Low.Nov 22 2014, 1:14 AM
bzimport set Reference to bz44385.

Okay, so since it looks like no one's willing to take action unless I do it first, here's what I'm going to do. Comments welcome, but if you come back later and -2 my commits once I create them, I'll be very angry.

Regarding points 1 and 4:
I'm just going to merge both the core file with the Vector extension one, and keep the result in Vector (completely killing core version). The core one is entirely Vector-specific anyway (so very unlikely to have been used by anything external), and I think that this functionality (being really a partial workaround for low-res issues with the skin) belongs to the extension instead of the skin.

Regarding point 2:
I'll choose whichever version works properly and kill the overriding.

Regarding point 3:
I'll just fix the heading selectors, whatever the code does.

Speak now or forever hold your peace ;)

I have submitted I7c0f42d92 (core) and Ic6b622279 (Vector) to move the file from core to Vector. This is the first part.

Also submitted I62aeb2b6 and Ib4ffef1b for Vector ext, to fix respectively points 2 and 3. Once all four are merged, this will be good enough to close this bug.

First two changesets were merged today by Reedy.

jeffrey.p.gill wrote:

With commit 8e01e294b632bcd340789133c3ea360319e7977d (Fri Feb 15 18:50:40 2013 +0000), I am receiving this error (when $wgShowExceptionDetails = true) on an otherwise vanilla wiki running MediaWiki 1.20.2 (perhaps I need to be running the bleeding edge version of MediaWiki?):

MediaWiki internal error.

Original exception: exception 'MWException' with message 'ResourceLoader duplicate registration error. Another module has already been registered as jquery.collapsibleTabs' in /var/www/wiki/includes/resourceloader/ResourceLoader.php:238
Stack trace:
#0 /var/www/wiki/includes/resourceloader/ResourceLoader.php(206): ResourceLoader->register(Array)
#1 /var/www/wiki/includes/OutputPage.php(2504): ResourceLoader->__construct()
#2 /var/www/wiki/includes/OutputPage.php(432): OutputPage->getResourceLoader()
#3 /var/www/wiki/includes/OutputPage.php(457): OutputPage->filterModules(Array, 'bottom')
#4 /var/www/wiki/includes/OutputPage.php(527): OutputPage->getModules(true, 'bottom', 'mModuleMessages')
#5 /var/www/wiki/includes/OutputPage.php(2757): OutputPage->getModuleMessages(true, 'bottom')
#6 /var/www/wiki/includes/OutputPage.php(2870): OutputPage->getScriptsForBottomQueue(false)
#7 /var/www/wiki/includes/Skin.php(583): OutputPage->getBottomScripts()
#8 /var/www/wiki/includes/SkinTemplate.php(390): Skin->bottomScripts()
#9 /var/www/wiki/includes/OutputPage.php(1989): SkinTemplate->outputPage()
#10 /var/www/wiki/includes/Wiki.php(543): OutputPage->output()
#11 /var/www/wiki/includes/Wiki.php(446): MediaWiki->main()
#12 /var/www/wiki/index.php(59): MediaWiki->run()
#13 {main}

Exception caught inside exception handler: exception 'MWException' with message 'ResourceLoader duplicate registration error. Another module has already been registered as jquery.collapsibleTabs' in /var/www/wiki/includes/resourceloader/ResourceLoader.php:238
Stack trace:
#0 /var/www/wiki/includes/resourceloader/ResourceLoader.php(206): ResourceLoader->register(Array)
#1 /var/www/wiki/includes/OutputPage.php(2504): ResourceLoader->__construct()
#2 /var/www/wiki/includes/OutputPage.php(432): OutputPage->getResourceLoader()
#3 /var/www/wiki/includes/OutputPage.php(457): OutputPage->filterModules(Array, 'bottom')
#4 /var/www/wiki/includes/OutputPage.php(527): OutputPage->getModules(true, 'bottom', 'mModuleMessages')
#5 /var/www/wiki/includes/OutputPage.php(2757): OutputPage->getModuleMessages(true, 'bottom')
#6 /var/www/wiki/includes/OutputPage.php(2870): OutputPage->getScriptsForBottomQueue(false)
#7 /var/www/wiki/includes/Skin.php(583): OutputPage->getBottomScripts()
#8 /var/www/wiki/includes/SkinTemplate.php(390): Skin->bottomScripts()
#9 /var/www/wiki/includes/OutputPage.php(1989): SkinTemplate->outputPage()
#10 /var/www/wiki/includes/Exception.php(227): OutputPage->output()
#11 /var/www/wiki/includes/Exception.php(272): MWException->reportHTML()
#12 /var/www/wiki/includes/Exception.php(620): MWException->report()
#13 /var/www/wiki/includes/Exception.php(690): MWExceptionHandler::report(Object(MWException))
#14 /var/www/wiki/includes/Wiki.php(449): MWExceptionHandler::handle(Object(MWException))
#15 /var/www/wiki/index.php(59): MediaWiki->run()
#16 {main}

Yes, Vector after this change is incompatible with older MediaWiki.

All patches merged.

While there are still some code quality issues with this raised by Ori, IMO it's good enough to close this.

... this seems to fly against the move to "coreify" the Vector extension. Won't we just have to undo this in a few weeks/months when someone does that?

Probably, yes. I started this before I started pushing for coreifying the extension, and at the time didn't care *where* this is fixed, but decided my changes would be easier to get merged this way.

unikum.pm wrote:

I faced with issue on the MW 1.21 and latest git version (master brnach). Are you saure that it fixed?

What issue? The error posted by Jeffrey above shouldn't occur if you use compatible core and extension versions. git master is not necessarily always compatible with releases; use the versions tagged REL1_XX for 1.XX MediaWiki version.