Make WordPress Core

Opened 9 months ago

Closed 6 months ago

Last modified 6 months ago

#58912 closed defect (bug) (fixed)

Mobile: Admin menu unexpectedly closes with Safari

Reported by: afercia's profile afercia Owned by: joedolson's profile joedolson
Milestone: 6.4 Priority: normal
Severity: normal Version: 6.2
Component: Administration Keywords: has-screenshots has-patch has-testing-info commit
Focuses: ui, accessibility, javascript Cc:

Description

Turns out the change introduced in #53587 / [55326] makes the admin menu unusable with Safari.

My testing environment:
macOS 13.4.1
Safari 16.5.2 (18615.2.9.11.10)
VoiceOver 10 (869.24)

To reproduce:

  • Emulate a smaller screen by using the 'Responsive Design Mode' in the Safari 'Develop' menu.
  • Go for example to the posts list page in the admin.
  • Click the hamburger icon at the top of the page to expand the admin menu.
  • Click, for example, the menu item: Users
  • Expected: the menu item to expand and reveal its sub-items.
  • Actual: the whole admin menu closes. Nothing else happens.
  • Click on a menu item that doesn't have any sub-item e.g.: Comments.
  • Expected: the browser to navigate to the Comments page.
  • Actual: nothing happens.

Although this happens only with Safari, the inability to navigate through the admin pages seems to me a pretty serious issue.

The implementation in #53587 / [55326] relies on detecting whether the clicked menu link has focus and is contained within the menu. Safari is known to _not_ set focus on clicked elements or at least, it _may_ decide to not set focus on some kind of elements.

After some debugging, at this point of the code:

With Chrome:

focusIsInToggle is false 
focusIsInSidebar is true 
$( ':focus' )[0] returns the clicked link 
document.activeElement returns the clicked link 

With Safari:

focusIsInToggle is false 
focusIsInSidebar is false 
$( ':focus' )[0] returns undefined 
document.activeElement returns the body element 

This confirm Safari doesn't set focus on the clicked link and the scripts fails.

Worth noting #53587 was reopened at some point because a new ticket reported a similar problem with iOS Safari, see #54837. The bug seem nnot limited to mobile Safari though.

See attached animated GIF to illustrate the unexpected collapsing of the menu.

Attachments (3)

safari.gif (413.5 KB) - added by afercia 9 months ago.
58912.2.diff (1.4 KB) - added by joedolson 8 months ago.
Updated patch
58912.3.diff (5.1 KB) - added by joedolson 6 months ago.
Combined patch to handle focus location & click behavior

Download all attachments as: .zip

Change History (30)

@afercia
9 months ago

#1 @afercia
9 months ago

For reference: in the Gutenberg editor there is an ad-hoc component to detect when focus is outside of an element. See useFocusOutside where focus on some elements gets 'normalized' to avoid this issue. Some eyes from the Editor team would probably help a lot to solve this bug and abstract a bit the code so that it can be reused.

This ticket was mentioned in PR #4917 on WordPress/wordpress-develop by @rajinsharwar.


9 months ago
#2

  • Keywords has-patch added

Using blur instead of focusout for fixing browser issues in Safari

Trac ticket: https://core.trac.wordpress.org/ticket/58912

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


9 months ago

#4 @joedolson
9 months ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 6.4
  • Owner set to joedolson
  • Status changed from new to accepted

@Travel_girl commented on PR #4917:


9 months ago
#5

I have tested the patch and it works much better now.

But I have noticed, that some menu items ( Tools - Import or Apperance) only opens after 2 or 3 times klicking.

I have tested on:

Mac OS 12.6.5
Safari 16.4.1

#6 @oglekler
8 months ago

  • Keywords changes-requested added

I tested patch with Windows 10 and Chrome 115, everything is working fine as was before, but I don't have Mac OS and Safari.

@rajinsharwar can you please check the issue reported in the previous comment.

#7 @rajinsharwar
8 months ago

I don't have MacOS either @oglekler :'/, I tested the patch in Chrome, and Firefox and it does seem to work. Can anyone with Mac devices please check on the issue reported on #comment:5?

#8 @joedolson
8 months ago

In my testing on Mac/Safari, it didn't appear that this patch worked; the close behavior didn't fire on losing focus at all. Looking at alternatives.

@joedolson
8 months ago

Updated patch

#9 @joedolson
8 months ago

Added an alternate option. I think that monitoring blur is flawed because it depends on something having focus, which isn't always the case.

I switched this to a click event, firing on any click event outside of the menu. This doesn't close the menu if you're using the keyboard and exit the menu, however.

#10 @oglekler
8 months ago

It looks like click and focusout can be combined, or it is possible to add additional listener for keyup check the element which got focus is outside the menu or focus is loosed entirely 🤔

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


8 months ago

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


8 months ago

#13 @marybaum
8 months ago

  • Keywords changes-requested removed

THis could use a little more testing before getting declared commit-ready.

#14 @rcorrales
8 months ago

I locally tested @joedolson patch in Safari last week, and it solved the issue. However, I cannot successfully navigate the menu with the keyboard, not even before the change, with or without the emulator on a smaller screen. In other browsers, when the menu is open, I can press the tab to go through the items, but on Safari, it shifts the focus to the next element (the Screen Options dropdown). The navigation in Safari could be a separate problem, though.

Last edited 8 months ago by rcorrales (previous) (diff)

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


7 months ago

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


7 months ago

#17 @binsaifullah
7 months ago

I have tested it with my iMac and found it is working fine now.

Mac OS Ventura 13.5.2
Safari 16.6

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


7 months ago

#19 @shubhamsedani
7 months ago

I have tested it in Safari, looks good to me.

@joedolson
6 months ago

Combined patch to handle focus location & click behavior

#20 @joedolson
6 months ago

  • Keywords has-testing-info added

Updated patch combines both behaviors to handle both scenarios.

  • Uses 'click' event to detect activity outside of menu.
  • Usees 'keyup' event to detect actions that trigger leaving the menu.
  • Adds an event on esc to close the menu regardless of location that moves focus back to the menu trigger.

To test:

  • Use tab and shift+tab to navigate in and out of the sidebar menu. The menu should close when focus moves anywhere other than within the menu.
  • Use esc to trigger closing the menu.
  • Click anywhere not in the menu to close the menu.
  • Click on menu items to verify links work as expected.

Patch tested in Safari/MacOS and Chrome/Windows. Additional testing welcome.

This ticket was mentioned in Slack in #core-test by joedolson. View the logs.


6 months ago

#22 @ashikur698
6 months ago

Tested this in Safari. Seems to work just fine.

Localhost Server: Nginx
PHP: 8.1.23
WordPress: 6.4 Beta 2
Browser: Safari
Theme: Twenty Twenty-Four (TT4)
Active Plugins: None

#23 @joedolson
6 months ago

  • Keywords commit added; needs-testing removed

I've now also tested it on Android/Chrome, Windows/Edge, and iOS/Safari and everything worked according to expectations. Marking for commit; the existing version has a clear significant bug in Safari, and this appears to be a significant improvement.

#26 @joedolson
6 months ago

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

In 56810:

Administration: Fix unusable mobile admin menu in Safari.

Replace the focusout event handler added in [55326] with a combination of blur and keyup handler. This change handles Safari not setting focus on clicked elements.

Props afercia, joedolson, travel_girl, oglekler, rajinsharwar, marybaum, rcorrales, binsaifullah, shubhamsedani, ashikur698.
Fixes #58912.

Note: See TracTickets for help on using tickets.