Last modified: 2008-10-26 17:17:33 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 16073 - Expanded watchlist javascript should use 'onclick' handlers
Expanded watchlist javascript should use 'onclick' handlers
Status: RESOLVED FIXED
Product: MediaWiki
Classification: Unclassified
Parser (Other open bugs)
unspecified
All All
: Normal minor (vote)
: ---
Assigned To: Nobody - You can work on this!
:
Depends on:
Blocks: javascript
  Show dependency treegraph
 
Reported: 2008-10-23 10:46 UTC by Happy-melon
Modified: 2008-10-26 17:17 UTC (History)
5 users (show)

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


Attachments

Description Happy-melon 2008-10-23 10:46:39 UTC
The discussion at http://en.wikipedia.org/wiki/Wikipedia:Deletion_review/Log/2008_October_21 was prompted by this error: for readers using a security plugin (mainly FireFox NoScript plugin) that suppresses javascript, the show/hide links for the extended watchlists and recent changes are converted into links to the articles [[RCI0]] and [[RCI1]].  Although there is clearly a bug in the FireFox plugin, it would be elementary defensive programming to use onclick event handlers in these links, which would resolve the issue and probably be more elegant.  To be explicit, the links are currently formatted:

<a href="javascript:toggleVisibility('RCI0','RCM0','RCL0')">

When they should be either 

<a href="#" onclick="toggleVisibility('RCI0','RCM0','RCL0')">
or
<span class="jslink" onclick="toggleVisibility('RCI0','RCM0','RCL0')">

with a suitable definition of the 'jslink' class.  Credit for this suggestion belongs to [[User:Anomie]].
Comment 1 Brion Vibber 2008-10-23 21:39:11 UTC
onlink handlers wouldn't help anything much here, since the totally-JS-dependent *optional display mode* would still not function properly. :)
Comment 2 Happy-melon 2008-10-24 07:48:19 UTC
The issue is (AFAIK) that the plugin converts the show/hide links into links to other pages because it mucks about with the contents of the href= parameter to try and get rid of the javascript.  So wouldn't using onclick mean that the links would just become nonfunctional? Surely that's better than diverting users to obscure and probably nonexistant pages?
Comment 3 Gavia immer 2008-10-24 18:47:24 UTC
Right, the issue is not that the links will be broken regardless, because the links will be broken regardless. The issue is that there's a miniscule chance of users being sent to a random-seeming link based on the first parameter of the javascript URL, and we can prevent that, at least. 
Comment 4 Brion Vibber 2008-10-24 18:52:58 UTC
*nod*

I'd probably recommend combining a couple things...

1) Have a sensible fallback layout for Enhanced RC mode when JS isn't there -- for instance, showing all sections as expanded.

This would make it so you don't have a near-useless page if you have JS turned off for a bit.

2) Do the expand/hide actions via onclick so they're less likely to turn funky from weird proxies/plugins like noscript.
Comment 5 Alex Z. 2008-10-24 23:11:42 UTC
Done in r42514.
Comment 6 Brion Vibber 2008-10-25 00:10:41 UTC
I've reverted this for now as it has some issues:

* The expand/contract icons no longer have regular link behavior (eg no hand icon, can't be reached by keyboard tabbing)
* It looks like the stuff-to-be-hidden doesn't get hidden until after the </body>, which feels a little sketchy to me. On long lists and slow connections you may see odd behavior with the items being shown expanded, then suddenly hiding when things reach the end. Adding the style immediately in the JS instead of waiting for the body load completion should avoid that
* mw-rc-jshidden class seems to be applied to things that shouldn't have it sincec they are explicitly given display:none?
* Instead of style='display:none' etc, consider using clear classes for expanded and hidden modes, then switch the classes instead of the styles in the JS
Comment 7 Danny B. 2008-10-25 00:22:13 UTC
Copying my comment from codereview page:

Switching from <a> to <span> is very bad solution, since it decreases the accessibility of the page. All actions should be placed on <a> tags, so user immediately knows, which items on page are active. Use the correct code for javascript-only links: <a href="#" onclick="toggleVisibility('RCI0','RCM0','RCL0')">. Also don't forget to use the title attribute to display the description such as "toggle the details (needs JavaScript)" or so.

Try to turn off the stylesheets and see.

See also bug 13984.

(with edit conflict with Brion, regardless on what he wrote) 
Comment 8 Alex Z. 2008-10-25 02:28:15 UTC
(In reply to comment #6)

redone in r42528

> * The expand/contract icons no longer have regular link behavior (eg no hand
> icon, can't be reached by keyboard tabbing)

changed to <a> tags again

> * It looks like the stuff-to-be-hidden doesn't get hidden until after the
> </body>, which feels a little sketchy to me. On long lists and slow connections
> you may see odd behavior with the items being shown expanded, then suddenly
> hiding when things reach the end. Adding the style immediately in the JS
> instead of waiting for the body load completion should avoid that

fixed, CSS added immediately after the JS loads 

> * mw-rc-jshidden class seems to be applied to things that shouldn't have it
> sincec they are explicitly given display:none?

that one was just my mistake, the "close arrow" needed to be hidden by default or else it displayed both arrows next to each other with JS disabled, but I forgot to remove the extra class

> * Instead of style='display:none' etc, consider using clear classes for
> expanded and hidden modes, then switch the classes instead of the styles in the
> JS
> 

sort of done, the problem is that it needs various things depending on the situation and it doesn't lend itself easily to just a couple classes. When the page is first opened:
*the "open" arrow needs to always be shown by default
*the "close" arrow needs to always be hidden by default
*the inner parts need to be hidden, unless JS is disabled

The inner text kind of screws things up since technically it needs to be "hidden" but should be expanded for fallback purposes
I created 2 classes (hidden/expanded), with the close arrow still set to style=display:none, overridden with a "style:inline !important" in the CSS defined by the JS so it can be changed by JS when necessary.

This way, everything except the close arrow is shown by default with JS disabled since the classes are defined by JS and the classes define how things are supposed to be shown if JS is enabled.


(In reply to comment #7)
> Also don't forget to use the
> title attribute to display the description such as "toggle the details (needs
> JavaScript)" or so.

Also done, and removed the cryptic little "+" "-" img alt text that it was setting before, which didn't even work in Firefox behind the <a> tag.

Comment 9 Ilmari Karonen 2008-10-25 03:13:46 UTC
Unfortunately, changing from <a href="javascript:toggleVisibility('RCI0','RCM0','RCL0')"> to <a href="#" onclick="toggleVisibility('RCI0','RCM0','RCL0')"> doesn't actually help with the original issue: NoScript treats both exactly the same way, as if they were links to [[RCI0]].  Of course, having the collapsible parts visible unless JS is enabled is still an improvement.
Comment 10 Tim Starling 2008-10-25 05:53:29 UTC
Reverted r42528 in r42531. Links with href="#" make firefox scroll to the top of the page, regardless of onclick handler.
Comment 11 Alex Z. 2008-10-25 06:33:37 UTC
Okay, looks like appending ";return false" to the end of the onclick handler might work - http://codingforums.com/showthread.php?t=96262#2

Will do some more testing on this later in more browsers. 
Comment 12 Alex Z. 2008-10-26 17:17:33 UTC
Fixed again in r42576. The arrow links are no longer displayed if JS is disabled.

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


Navigation
Links