WordPress.org

Make WordPress Core

Opened 5 months ago

Last modified 8 weeks ago

#53951 accepted defect (bug)

Twenty Twenty: Search modal aria-expanded

Reported by: utz119 Owned by: alexstine
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch has-testing-instructions needs-testing
Focuses: accessibility, javascript Cc:

Description

Theme: Twenty Twenty

When opening the search modal using the Tab key, then closing it using the Esc key causes aria-expanded to remain “true” even after the modal is closed.

To reproduce the issue:

  1. Navigate to the search button using the Tab key
  2. Use the Enter key to open the search form
  3. Use the Esc key to close

Attachments (5)

TwentyTwenty-Search-ARIA-Expanded.gif (3.1 MB) - added by utz119 5 months ago.
53951.diff (1.4 KB) - added by utz119 5 months ago.
53951-2.diff (1.4 KB) - added by alexstine 2 months ago.
53951-3.diff (3.4 KB) - added by alexstine 2 months ago.
53951-4.diff (3.4 KB) - added by alexstine 8 weeks ago.

Change History (20)

#1 @mukesh27
5 months ago

  • Component changed from General to Bundled Theme
  • Version 5.8 deleted

Hi and welcome to WordPress Trac! Thanks for the ticket.

I reproduce the issue.

Additionally, if we click outside or the form it show same aria-expanded.

@utz119
5 months ago

#2 @utz119
5 months ago

  • Keywords has-patch added

#3 @utz119
5 months ago

  • Keywords needs-testing added

#4 @Hareesh Pillai
5 months ago

  • Keywords has-testing-instructions added
  • Milestone changed from Awaiting Review to 5.9

Thanks for the ticket and patch @utz119.

It is noteworthy that the aria-expanded attribute is set correctly when the search modal is clicked in the responsive screen sizes. It is handled through the twentytwentyToggleAttribute() function. We should perhaps refresh the patch to make use of the currently written code.

#5 @sabernhardt
3 months ago

  • Focuses javascript added
  • Keywords needs-refresh added; needs-testing removed
  • Summary changed from Twenty Twenty Search modal aria-expanded to Twenty Twenty: Search modal aria-expanded

#6 @sabernhardt
3 months ago

With either the desktop or mobile search toggle button, the patch correctly toggles the aria-expanded attribute to false when pressing the Escape key. The desktop search toggle button also switches to false when clicking outside the modal to close it.

However, the mobile search button seems to toggle twice the first time I click outside the modal, which results in a true value when the modal is closed. Then clicking the mobile search button and closing by clicking outside again, the aria-expanded value continues to toggle to the incorrect value.

The code below does not fix the double toggling, but it might be worth considering for these two situations because the value should always become false.

			document.querySelectorAll( '.search-toggle' ).forEach( function( element ) {
				element.setAttribute( 'aria-expanded', 'false' );
			} );

(Accurate toggling is probably trickier due to the two search buttons.)

This ticket was mentioned in Slack in #accessibility by ryokuhi. View the logs.


2 months ago

This ticket was mentioned in Slack in #accessibility by alexstine. View the logs.


2 months ago

@alexstine
2 months ago

#9 @alexstine
2 months ago

  • Keywords needs-testing added; needs-refresh removed

I uploaded 53951-2.diff and I believe it now works correctly with Escape key. I am not sure about the click outside because that is hard to do for screen reader, but I was able to get Escape working I think.

Does this patch work any better?

Thanks.

#10 @sabernhardt
2 months ago

  • Keywords needs-refresh added
  • Milestone changed from 5.9 to 6.0

53951-2.diff does not change the aria-expanded to false for me when pressing the Escape key (on desktop).

With only a few hours before Beta, let's revisit this later, for next release.

Last edited 2 months ago by sabernhardt (previous) (diff)

#11 @alexstine
2 months ago

  • Owner set to alexstine
  • Status changed from new to accepted

@sabernhardt Does pressing Escape only work on mobile or does it not work at all? My tests showed it working fine but interested in the specifics. I'll take ownership of this ticket and make sure we get something that works for 6.0.

Thanks.

@alexstine
2 months ago

#12 @alexstine
2 months ago

Just uploaded 53951-3.diff. Instead of playing around with aria-expanded further, I made the search form a proper dialog. I believe this is a much more accessible approach. Would be interested in getting others feedback.

Thanks.

This ticket was mentioned in Slack in #accessibility by alexstine. View the logs.


8 weeks ago

#14 @sabernhardt
8 weeks ago

  • Keywords needs-refresh removed

53951-3.diff seems to work.

With Firefox and NVDA, I press Escape twice to close the dialog when focus is in the search input, which was also necessary before the patch. However, then I need to press Enter twice on the Search button (either mobile or desktop) to re-open the dialog.

@alexstine
8 weeks ago

#15 @alexstine
8 weeks ago

@sabernhardt This should now be fixed in latest patch. After pressing Escape twice to exit the modal, Enter once should open it back up. Tested for both desktop/mobile.

Note: See TracTickets for help on using tickets.