Page MenuHomePhabricator

Enhanced recent changes JavaScript: clicking arrow should keep it in focus, keyboard accessibility
Closed, ResolvedPublic

Description

In RC when multiple edits to the same page are made there is a blue arrow which one may click to show each specific edit. (well, if enhanced changes are enabled in the users preferences)

When the arrow is clicked it loses focus (since another arrow is displayed). When navigating using the keyboard this is very inconvenient - so the JS should actively change focus to the new arrow.


Version: 1.20.x
Severity: enhancement

Details

Reference
bz17616

Event Timeline

bzimport raised the priority of this task from to Low.Nov 21 2014, 10:35 PM
bzimport set Reference to bz17616.

a simple patch (as long as the HTML looks like it currently does, no problems)

attachment enhancedchanges.js ignored as obsolete

simple patch (no problem as long as HTML is the same)

Trying to create a real patch.

Attached:

The enhanced changes have changed a fair bit since 1.17, notably the use of CSS and jQuery utility functions instead of images directly in the DOM.

I think this is no longer an issue, someone else should confirm as well though (ideally the bug reporter) since I'm not entirely sure what the bug reporter meant and don't have a 1.15 around to experience the bug there.

(In reply to comment #3)

The enhanced changes have changed a fair bit since 1.17, notably the use of CSS
and jQuery utility functions instead of images directly in the DOM.

I think this is no longer an issue, someone else should confirm as well though
(ideally the bug reporter) since I'm not entirely sure what the bug reporter
meant and don't have a 1.15 around to experience the bug there.

It seems like you are confused about the version numbers. The problem however still *does* exist at Wikimedia sites, e.g.:

Perhaps the version number should be updated?
Also, as of the deployed version, <img> tags are put directly into the HTML source; I don't recognize the "use of CSS and jQuery utility functions instead of images directly in the DOM".

Has this changed in 1.18?

Steps to reproduce:
TAB through links in the page till you get to an arrow
Hit Enter
Focus is gone

It has changed in trunk / 1.18, but the bug is stil there.

Instead of replacing the image itself, in 1.18 it has both and depending on whether the collapsible element is collapsed or expanded, one is display:none and the other display:block.

*Bulk BZ Change: +Patch to open bugs with patches attached that are missing the keyword*

sumanah wrote:

+reviewed since it seems that we have reviewed the code submitted, although if I'm wrong someone should remove that and add the need-review keyword. Thank you for the patch, Dan; do you have the time and interest to update your fix?

I sure am interested in updating the fix, but I am preoccupied at the moment.
If I haven't provided an update within a month, my priorities have changed and you shouldn't count on me doing it.

Note:
Such a fix should also remove the newly introduced (and related) bug:

  1. Tab to an arrow
  2. Press enter
  3. Notice that the URL has changed (a # is added)

Relevant code: http://svn.wikimedia.org/viewvc/mediawiki/trunk/phase3/resources/jquery/jquery.makeCollapsible.js?revision=94389&view=markup#l180

Krinkle, is it necessary to prevent an <a> element from initiating the toggle?

We want to be able to click the <a> to toggle (or press enter while it is focused), so that can't be prevented.

I can think of a few solutions:

  • Don't prevent <a> elements (is this prevention needed anywhere?)
  • Add a parameter allowLinkAsTarget
  • Add a class to $that element to indicate that links (i.e. .mw-collapsible-toggle.mw-collapsible-allow-link)
  • Add a new function which doesn't prevent <a> elements

Also, as a note: We'll need to change the HTML to only have *one* <a> for both images, since we don't want to focus the <a> when the page loads (the toggling function is performed once).

Krinkle and I have spoken briefly about this bug and it seems like the reason for preventing an <a> from initiating the toggle is to allow other links within toggle elements (use case: a complete table row is used for toggling, except that we want to have links in there too).

A fix that would solve this bug as well as allowing those links would be replacing line 180 above with:

if ( $(e.target).is('a') && !$that.is(e.target) ) {

So it's 2013 now.

The script that does this has been rewritten again approximately two times since the last comment, and in my testing using MW 1.22 alpha (deployed on Wikipedias) using Opera 12.15 the focus is kept when using keyboard navigation (Shift+arrows, Enter).

Could someone verify? If this is the case, then it looks like this can be closed.

I can confirm that focus is kept in Opera 12.15 (Linux) using shift+arrows, enter.

However, it does *not* work in Firefox 24 (Linux), not even when the setting to use cursor keys to navigate pages is active. In fact I can't focus it all in Firefox. I can't get it to work in Chromium 28 (Linux) either.

Looking at the source, there's no link, nor tabindex. My guess is that Opera makes items which have a click handler focusable as well.

Hmm, yeah, Opera tends to be smarter than other browsers in places you don't expect. Let's try adding a tabindex and see what happens.

[Updating keywords.]

Change 72371 had a related patch set uploaded by Matmarex:
jquery.makeCollapsible: Togglers accessibility

https://gerrit.wikimedia.org/r/72371

Change 72371 merged by jenkins-bot:
jquery.makeCollapsible: Togglers accessibility

https://gerrit.wikimedia.org/r/72371

Okay, that's merged. Can you check it out locally and see if it fixes the issue?

(Or feel free to register an account and test on my test wiki at http://users.v-lo.krakow.pl/~matmarex/testwiki/ , where I have just deployed it. Please ignore the spambots. All edits must have a non-empty summary or they will be rejected by the spam filter.)

@Bartosz #17:
I tried creating an account on http://users.v-lo.krakow.pl/~matmarex/testwiki/. The arrow *is* focusable, and doesn't lose focus when clicking, but when it is focused, clicking Enter does nothing. So I'd say that so far this is *not* solved. I expect it to toggle when clicking Enter.

Change 72719 had a related patch set uploaded by Matmarex:
jquery.makeCollapsible: Togglers keyboard accessibility

https://gerrit.wikimedia.org/r/72719

Hmph. I though browsers handle this magically, but it turns out only Opera is smart enough. I deployed the pending patch above (comment 19) as well – can you test again now? :)

(Sorry about the back-and-forth.)

(In reply to comment #20)
I tested it again in Firefox 24 (Linux) and Chromium 28 (Linux) and it does work (using the deployed version at http://users.v-lo.krakow.pl/~matmarex/testwiki/).

I'd say that that commit resolves this bug. Thanks!

Change 72719 merged by TheDJ:
jquery.makeCollapsible: Togglers keyboard accessibility

https://gerrit.wikimedia.org/r/72719

Change 78071 had a related patch set uploaded by TheDJ:
Accessibility: Don't arbitrarily override role

https://gerrit.wikimedia.org/r/78071

Change 78071 merged by jenkins-bot:
Accessibility: Don't arbitrarily override role

https://gerrit.wikimedia.org/r/78071