Page MenuHomePhabricator

Page actions should use semantic 'a' rather than 'li' as a button and be tabbable
Closed, ResolvedPublic3 Estimated Story Points

Description

Nav elements are skipped when tabbing through links in Minerva

Developer notes

The current page actions code looks like this:

<ul id="page-actions" class="hlist ">
<li id="ca-edit" class="mw-ui-icon mw-ui-icon-element mw-ui-icon-minerva-edit-enabled" title="Edit the lead section of this page.">
  <a class="edit-page" href="#/editor/0" title="Edit the lead section of this page.">Edit</a>
</li>
<li id="ca-watch" class="mw-ui-icon mw-ui-icon-element watch-this-article mw-ui-icon-mf-watch view-border-box" title="Watch this page">
  <button>Watch this page</button>
</li>
<div class="view-border-box">
  <li class="mw-ui-icon mw-ui-icon-minerva-download mw-ui-icon-element   " title="Download"></li>
</div>
<li class="mw-ui-icon mw-ui-icon-element mw-ui-icon-minerva-language-switcher language-selector" title="Read in another language">
  <a href="/wiki/Special:MobileLanguages/Alabama">Read in another language</a>
</li>
</ul>

Potentially all three buttons are links to other pages (when JS is disabled). We should make sure that the buttons are links not li elements. This will be useful for screen readers and search engines and us developers because it's easier to see what is an actionable item and what is not.

Here is a screencast of Mac OS X voiceover. Notice how the page actions are described:

screen reader.gif (584×738 px, 688 KB)

Suggested fixes:

  • Move the ID, classes, and titles from li elements to their corresponding children. Style the icon not the container.
  • The watch button will be dealt with in T141936. It's rendered as a link in the back-end, but replaced with a button element in the front end.
  • Change JS code to work with child a's rather than li's.

AC

  • Should be possible to tab between page actions
  • The markup on the page actions is consistent and semantic (li's and a's for buttons)
  • Voice over and other assistive technologies behave properly with the buttons
  • Ensure each icon is accessible by tabbing
  • Download link should not be wrapped in a div!
  • Pages pass the "Elements Are Well Structured" Chrome audit

QA

Steps to Reproduce

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

The title attribute as currently just duplicating the text like in the first two actions is actually not helping assistive technology, but negatively impacting f.e. screen readers, as some of them are reading the text twice.
The page actions are altogether not visible when JavaScript is disabled. From an SEO perspective that could have negative impact as well.

The page actions are altogether not visible when JavaScript is disabled. From an SEO perspective that could have negative impact as well.

This is fixed as of today. You may need to bust the cache.

jhobs added a project: Web-Team-Backlog.
MBinder_WMF set the point value for this task to 5.Sep 26 2016, 4:39 PM

Could some rationale for high priority be added to the task description? I assume it's accessibility related but it's not clear whether the status quo is broken.

phuedx subscribed.

I'm moving this to Triaged but Future as it's been in Sprint +1 for almost 5 months.

Jdlrobson lowered the priority of this task from High to Medium.Apr 13 2017, 7:05 PM

The culprit is Skin::makeListItem which lives in core. We'd incur technical debt if we do this as written as flagged by ""EDIT: maybe this may not be a good idea. Maybe we should stick to the mark-up defined in core and not add extra classes to li elements.""

Do we want to do this?
Do we plan to improve the code in core?

This part of the description is also unclear and needs clarification:
"Add a link child to the language button."

Jdlrobson lowered the priority of this task from Medium to Low.May 2 2017, 4:54 PM

This would benefit from a talk in front end standards group. Ideally we'd revisit implementation of Skin::makeListItem

Jdlrobson changed the task status from Open to Stalled.Aug 23 2017, 4:03 PM

We talked about this in the frontend standards group. We don't need to use the same HTML hierarchy as Vector etc..
This is a Minerva problem. Let's render these consistently by making it impossible to render them in an inconsistent manner.

$actions = [
{ name: 'edit' },
{ name: 'watch' },
];

{{#pageactions}}<li><a/></li>{/pageactions}}

This is however blocked on a decision in T141936.

Jdlrobson removed the point value for this task.Aug 23 2017, 4:04 PM
Jdlrobson changed the task status from Stalled to Open.Sep 7 2017, 6:38 PM
Jdlrobson raised the priority of this task from Low to Medium.Feb 24 2018, 12:36 AM
Jdlrobson set the point value for this task to 3.Mar 13 2018, 4:50 PM
Jdlrobson renamed this task from Page actions should use semantic 'a' rather than 'li' as a button to Page actions should use semantic 'a' rather than 'li' as a button and be tabbable.Apr 24 2018, 3:46 PM
Jdlrobson updated the task description. (Show Details)

Exciting progress here from @Jdrewniak
A few notes:

  • The anchor gets role=button assigned, why not making it a button out of box? That would make it simpler for assistive technology
    • Even if disabled, the code is

<a id="" href="" class="mw-ui-icon mw-ui-icon-element mw-ui-icon-minerva-language-switcher language-selector disabled" role="button" title="Read in another language">Read in another language</a>
which the disabled class alone doesn't provide any useful addition to screen readers. It should carry an aria-disabled or if changed to a button a disabled attribute!

@Volker_E I can speak to the button vs link.

The reason this is an <a> and not a <button> is because the default actions, (languages, watchlist, edit) actually all have non-js code-paths as well, which simply link to a different page, e.g. Special:MobileLanguages and ?action=watch.

This is why they should still come down from the server as links and not buttons. However, they should be given the role=button on the client, instead of on the server, which is what happens currently.

I agree the links need the aria-disabled attribute.

Also to your earlier point about the title attribute. I agree the title is being duplicated, and it's probably unnecessary. The reason I left it in there is to provide the "tooltip" when mousing-over on desktop. If you have any thoughts on this pattern, (yay or nay) please share.

Jdlrobson claimed this task.

Nav elements are skipped when tabbing through links in Minerva

This is no longer the case and fixed, so I'm calling this resolved (especially given the age of this ticket).

Here's the proof:

focus.gif (719×1 px, 198 KB)

Please open new tasks for other accessibility problems.