WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

#30083 closed defect (bug) (fixed)

Twenty Fifteen: Sub Menu Navigation Toggles are Invalid HTML

Reported by: davidakennedy Owned by: iandstewart
Milestone: 4.1 Priority: normal
Severity: blocker Version: 4.1
Component: Bundled Theme Keywords: has-patch needs-testing
Focuses: accessibility Cc:
PR Number:

Description

While testing Twenty Fifteen for accessibility, the Accessibility team found that:

  • Nesting a <button> element inside of an <a> element is invalid HTML.
  • The W3C Validator reports: "The element button must not appear as a descendant of the a element."
  • This happens in the main navigation, in the sub-menus.
  • All in all, the keyboard accessibility of the current code works well.
  • For screen readers, the addition of the ARIA is excellent, but we could also make the text more specific, so screen reader users know what they are expanding. So: "Expand Sub-Menu" instead of just "Expand". Or something similar. But first we need to figure out the best solution for the toggle.

I am researching some solutions.

Attachments (3)

30083.patch (3.8 KB) - added by afercia 5 years ago.
30083.2.patch (3.5 KB) - added by davidakennedy 5 years ago.
30083.3.diff (3.6 KB) - added by iamtakashi 5 years ago.

Download all attachments as: .zip

Change History (15)

#1 @iandstewart
5 years ago

  • Milestone changed from Awaiting Review to 4.1
  • Severity changed from normal to blocker

#2 @afercia
5 years ago

Hi, what about to insert the <button> as a sibling of the <a> element?
Example patch attached, might need some CSS adjustment.

Worth noting that currently there's one more issue: tested with Firefox + NVDA, the button will be announced as part of a link, which make sense given the current structure but may be confusing for screen reader users: "it's a button or a link?"

Currently NVDA announces:

EXPAND
button collapsed
link

COLLAPSE
button expanded
link

after the patch:

EXPAND
button collapsed

COLLAPSE
button expanded

@afercia
5 years ago

#3 follow-up: @joedolson
5 years ago

Just had a conversation with David Kennedy, and he's also working on a patch with that solution. I think he's also addressing any styling changes that might need to happen due to the change of location.

#4 @afercia
5 years ago

  • Keywords has-patch needs-testing added

#5 in reply to: ↑ 3 @afercia
5 years ago

Replying to joedolson:

Just had a conversation with David Kennedy, and he's also working on a patch with that solution.

Great :)

#6 follow-up: @davidakennedy
5 years ago

I added my patch as well, but it's pretty close to what afercia has. Beat me to it. :)

I:

  • Moved the <button> outside the <a>
  • Adjusted the JS to accommodate that.
  • Adjusted the CSS to accommodate that.
  • Added some more descriptive text to the Expand/Collapse verbiage so screen reader users know better what they're expanding.

#7 in reply to: ↑ 6 @afercia
5 years ago

Replying to davidakennedy:

I added my patch as well, but it's pretty close to what afercia has. Beat me to it. :)

Yours is better :) I forgot to improve the button text.

Btw I think I've found something interesting. I was wondering why the button text was reported in the NVDA Speech Viewer as all uppercase:

EXPAND CHILD MENU
button collapsed

https://cldup.com/6Dw-O2YPCt.png

as in the markup is just capitalized: "Expand Child Menu".

Depending on user settings, this may be annoying. NVDA (and I guess other screen readers too) has some settings about capital letters:

  • Capital pitch change percentage (the voice will change when speaking a capital letter)
  • Say "cap" before capitals
  • Beep for capitals

When tabbing, nothing special happens but when using the left and right arrow keys NVDA will do all of that, if set. It will pitch the voice, "beep" and say "cap" before *any* single letter:
"beep" cap E, "beep" cap X, "beep" cap P, "beep" cap A, "beep" cap N ...

I really don't know why, and to me it sounds like a bug: it shouldn't assume button text as capitalized.
Anyway, just setting text-transform: lowercase; on .dropdown-toggle in the main CSS does the trick. No more voice pitch, "beeps" or saying "cap". Also, the NVDA Speech Viewer will report the text as lowercase:

expand child menu
button collapsed

I would also recommend to don't capitalize the text in the markup, just to be sure.

@iamtakashi
5 years ago

#8 @iamtakashi
5 years ago

I've adjusted CSS little and implemented afercia's recommendations in the patch from davidakennedy.

#9 follow-up: @afercia
5 years ago

Turns out the real reason for the uppercase thing is in the general CSS rule for input buttons:

button,
input[type="button"],
input[type="reset"],
input[type="submit"] {
	...
	text-transform: uppercase;
}

so it's not a NVDA bug, just something that needs to be reset for screen reader's text.

#10 in reply to: ↑ 9 @davidakennedy
5 years ago

Replying to afercia:

Turns out the real reason for the uppercase thing is in the general CSS rule for input buttons:

Great catch! Thanks!

Patch is looking good @iamtakashi!

#11 @iandstewart
5 years ago

Thanks everyone for your efforts here! Open source FTW.

#12 @iandstewart
5 years ago

  • Owner set to iandstewart
  • Resolution set to fixed
  • Status changed from new to closed

In 30013:

Twenty Fifteen: valid HTML for accessible child page menu expanding and collapsing -- now with even better accessiblity.

Props afercia, davidakennedy, iamtakashi, fixes #30083.

Note: See TracTickets for help on using tickets.