onlink handlers wouldn't help anything much here, since the totally-JS-dependent *optional display mode* would still not function properly. :)

happy_melon wrote:

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?

etdp01 wrote:

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.

*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.

Done in r42514.

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

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)

(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.

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.

Reverted r42528 in r42531. Links with href="#" make firefox scroll to the top of the page, regardless of onclick handler.

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.

Fixed again in r42576. The arrow links are no longer displayed if JS is disabled.