Make WordPress Core

Opened 3 years ago

Closed 2 years ago

Last modified 15 months ago

#53951 closed defect (bug) (fixed)

Twenty Twenty: Search modal aria-expanded

Reported by: utz119's profile utz119 Owned by: joedolson's profile joedolson
Milestone: 6.0 Priority: normal
Severity: normal Version:
Component: Bundled Theme Keywords: has-patch has-testing-instructions commit
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 (8)

TwentyTwenty-Search-ARIA-Expanded.gif (3.1 MB) - added by utz119 3 years ago.
53951.diff (1.4 KB) - added by utz119 3 years ago.
53951-2.diff (1.4 KB) - added by alexstine 3 years ago.
53951-3.diff (3.4 KB) - added by alexstine 3 years ago.
53951-4.diff (3.4 KB) - added by alexstine 3 years ago.
53951.2.diff (1.9 KB) - added by afercia 2 years ago.
53951.3.diff (2.1 KB) - added by alexstine 2 years ago.
53951.4.diff (3.0 KB) - added by afercia 2 years ago.

Change History (32)

#1 @mukesh27
3 years 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
3 years ago

#2 @utz119
3 years ago

  • Keywords has-patch added

#3 @utz119
3 years ago

  • Keywords needs-testing added

#4 @Hareesh Pillai
3 years 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 years 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 years 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.


3 years ago

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


3 years ago

@alexstine
3 years ago

#9 @alexstine
3 years 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
3 years 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 3 years ago by sabernhardt (previous) (diff)

#11 @alexstine
3 years 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
3 years ago

#12 @alexstine
3 years 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.


3 years ago

#14 @sabernhardt
3 years 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
3 years ago

#15 @alexstine
3 years 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.

#16 @audrasjb
2 years ago

  • Keywords dev-feedback added; needs-testing removed

53951-4.diff works when pressing esc twice, but it doesn't remove the aria-expanded="true" attribute when I only press esc once. Is it expected?

@afercia
2 years ago

#17 @afercia
2 years ago

  • Keywords needs-testing added; dev-feedback removed

Looking a bit into this issue, seems to me the root problem is that aria-expanded needs to be handled on multiple elements at once. Ideally, the state of the toggle search buttons for mobile and desktop should be synced.

53951.2.diff changes the twentytwentyToggleAttribute function so that:

  • It selects all the toggles with the same data-toggle-target attribute of the passed element.
  • Loops through the collected toggles to update their aria-expanded attribute.

I kept the role="dialog" added in previous patches and also added an aria-label to the dialog.

Some testing would be greatly appreciated. Things to test:

  • Both the desktop and mobile Search toggle buttons get their aria-expanded attribute updated, in all cases: click, Escape key press, click outside. On both the desktop and responsive view.
  • Check the mobile menu toggle still works as expected.
  • Check the mobile sub-menus toggles still work as expected.

@alexstine
2 years ago

#18 @alexstine
2 years ago

In 53951.3.diff, I made one improvement to ensure the close search button did not receive aria-expanded attribute. This is one extra conditional check added to @afercia patch.

@afercia
2 years ago

#19 @afercia
2 years ago

Thanks @alexstine, good catch.

I just noticed there's one more issue related to another button. When the mobile navigation menu is open, there's a 'Close menu' button. The aria-expanded attribute on this button isn't handled correctly.

Also, there's one more inconsistency:

  • The 'Close Menu' button does have an initial aria-expanded attribute which is printed out on the PHP side.
  • The 'Close search' button does not.

We could argue whether a 'Close' button really needs an aria-expanded attribute. To me, a 'Close' button implies that something is open / expanded. Removing this aria-expanded attribute would also make things easier to fix on the JavaScript side.

The latest patch 53951.4.diff

  • Removes aria-expanded from the 'Close Menu' button.
  • On the JavaScript side, skips both the 'Close search' and 'Close Menu' buttons by checking whether the attribute to toggle exists.

Some testing would be nice.

#20 @alexstine
2 years ago

@afercia Your patch works perfectly. None of the close buttons get aria-expanded anymore and attribute values seem to stay in sync.

Thanks!

#21 @afercia
2 years ago

  • Keywords needs-testing removed

#22 @joedolson
2 years ago

  • Keywords commit added
  • Owner changed from alexstine to joedolson

#23 @joedolson
2 years ago

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

In 53051:

Twenty Twenty: Fix aria-expanded handling in search toggle.

Transform search into a dialog role and fix the handling of aria-expanded to synchronize mobile and desktop buttons.

Props utz119, alexstine, mukesh27, hareesh-pillai, sabernhardt, audrasjb, afercia.
Fixes #53951.

This ticket was mentioned in Slack in #core by sabernhardt. View the logs.


15 months ago

Note: See TracTickets for help on using tickets.