Last modified: 2014-06-17 17:02:45 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 T46385, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 44385 - The situation with $.collapsibleTabs and its Vector ext. counterpart sucks badly
The situation with $.collapsibleTabs and its Vector ext. counterpart sucks badly
Status: RESOLVED FIXED
Product: MediaWiki skins
Classification: Unclassified
Vector (Other open bugs)
unspecified
All All
: Low major
: ---
Assigned To: Bartosz Dziewoński
:
Depends on:
Blocks: code_quality javascript 44386
  Show dependency treegraph
 
Reported: 2013-01-26 16:13 UTC by Bartosz Dziewoński
Modified: 2014-06-17 17:02 UTC (History)
9 users (show)

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


Attachments

Description Bartosz Dziewoński 2013-01-26 16:13:48 UTC
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.
Comment 1 Bartosz Dziewoński 2013-02-04 19:57:49 UTC
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 ;)
Comment 2 Bartosz Dziewoński 2013-02-05 19:23:46 UTC
I have submitted I7c0f42d92 (core) and Ic6b622279 (Vector) to move the file from core to Vector. This is the first part.
Comment 3 Bartosz Dziewoński 2013-02-06 08:18:23 UTC
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.
Comment 4 Bartosz Dziewoński 2013-02-15 20:54:31 UTC
First two changesets were merged today by Reedy.
Comment 5 Jeffrey Gill 2013-02-16 14:56:28 UTC
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}
Comment 6 Bartosz Dziewoński 2013-02-16 14:59:01 UTC
Yes, Vector after this change is incompatible with older MediaWiki.
Comment 7 Bartosz Dziewoński 2013-03-03 12:23:01 UTC
All patches merged.

While there are still some code quality issues with this raised by Ori, IMO it's good enough to close this.
Comment 8 James Forrester 2013-03-03 15:59:15 UTC
... 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?
Comment 9 Bartosz Dziewoński 2013-03-03 16:13:52 UTC
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.
Comment 10 Artem A Klevtsov 2013-05-27 14:20:44 UTC
I faced with issue on the MW 1.21 and latest git version (master brnach). Are you saure that it fixed?
Comment 11 Bartosz Dziewoński 2013-05-29 23:37:13 UTC
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.

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


Navigation
Links