WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 6 years ago

#32843 closed defect (bug) (fixed)

Menu Customizer: empty elements get announced by screen readers

Reported by: afercia Owned by: valendesigns
Milestone: 4.3 Priority: normal
Severity: normal Version: 4.3
Component: Customize Keywords: has-patch commit
Focuses: accessibility, javascript Cc:

Description

Sometimes the Customizer outputs empty elements, see screenshot below. An empty paragraph (first part of the screenshot) would be a minor issue. Empty lists (second part of the screenshot) get announced by screen readers though. They're also invalid code. Bit confusing for users when they hear "Lists with 0 items".

https://cldup.com/xhwXadPo6V.png

Attachments (6)

32843.diff (1.9 KB) - added by valendesigns 6 years ago.
before patch.png (38.9 KB) - added by westonruter 6 years ago.
Screenshot prior to patch
with 32843.diff applied.png (38.6 KB) - added by westonruter 6 years ago.
With 32843.diff applied
32843.2.diff (1.9 KB) - added by valendesigns 6 years ago.
32843.3.diff (1.9 KB) - added by valendesigns 6 years ago.
32843.4.diff (2.3 KB) - added by valendesigns 6 years ago.
Move CSS

Download all attachments as: .zip

Change History (21)

#1 @ocean90
6 years ago

The reason for the empty .customize-section-description element is because the element is a p tag but $description includes p tags too, see trunk/src/wp-includes/class-wp-customize-nav-menus.php#L422.

#2 @afercia
6 years ago

Mostly concerned about the ul because an empty p doesn't get announced by screen readers but an empty ul does, as "Lists with 0 items".
As far as I see that ul is used as sort of placeholder when dragging menu items so it can't be removed. Maybe we could try to hide it until it's actually needed?

#3 follow-up: @valendesigns
6 years ago

  • Owner set to valendesigns
  • Status changed from new to assigned

So we just need to hide the ul when it's empty to stop the screen readers from announcing it?

#4 in reply to: ↑ 3 @afercia
6 years ago

Replying to valendesigns:

So we just need to hide the ul when it's empty to stop the screen readers from announcing it?

Yes, display: none or visibility: hidden would work for screen readers. Would be invalid code yet but I guess we can live with that?

#5 @valendesigns
6 years ago

I'll see if it can be added on the fly without any issues, but I suspect that's tied to something dynamic that's expecting it to always be there and might not be removable.

@valendesigns
6 years ago

#6 @valendesigns
6 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

Patch 32843.diff changes the p to a div for customize-section-description and adds styles so it appears the same as it did when it was a p tag. You can't add paragraphs inside a paragraph. As well, I've added CSS that hides menu-item-transport when it's empty. Should work correctly now, but needs testing with a screen reader.

Last edited 6 years ago by valendesigns (previous) (diff)

#7 @celloexpressions
6 years ago

Patch looks good to me. Html in section descriptions is generally discouraged, but it's allowed, and it's encouraged in panel descriptions. This should be ready for commit as soon as it's tested with a screen reader.

#8 @afercia
6 years ago

Tested with ChromeVox and it works nicely. For reference, see in the screenshot below what was happening before the patch when arrowing around elements. ChromeVox also highlights with an (ugly) orange outline the element being announced so it's pretty clear:

https://cldup.com/4PoniC_V7k.png

And after the patch nothing gets read out between the menu items:

https://cldup.com/bd9WlIqlV9.png

#9 @ocean90
6 years ago

I think we should move .menu-item .menu-item-transport:empty { display: none; } to nav-menus.css since we have the placeholder there too.

#10 @valendesigns
6 years ago

@ocean90 why would we move it into the Admin Menus CSS file, and does that file even load when you view the Customizer?

This ticket was mentioned in Slack in #core-customize by valendesigns. View the logs.


6 years ago

@westonruter
6 years ago

Screenshot prior to patch

@westonruter
6 years ago

With 32843.diff applied

#12 @westonruter
6 years ago

  • Keywords needs-patch added; has-patch needs-testing removed

Margins need to be updated on the CSS.

@valendesigns
6 years ago

@valendesigns
6 years ago

#13 @valendesigns
6 years ago

  • Keywords has-patch added; needs-patch removed

@valendesigns
6 years ago

Move CSS

#14 @westonruter
6 years ago

  • Keywords commit added

#15 @westonruter
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 33282:

Customizer: Hide the transport list when it's empty, especially for screen readers.

Also prevent invalid markup for .customize-section-description.

Props valendesigns.
Fixes #32843.

Note: See TracTickets for help on using tickets.