Pattern Explorer category selection causes the modal to close unexpectedly

[richtabor]

When running 6.4 beta 2, or Gutenberg trunk, I cannot select alternate categories within the Pattern Explorer. The modal closes unexpectedly. Chrome seems fine though.

CleanShot.2023-10-05.at.17.25.37.mp4

[talldan]

@richtabor There's a similar report here - #55043. Though in a different part of the UI.

I can't reproduce any issues with it closing (though I just spotted an issue that makes it crash completely). Is it happening for every category or just some? Are there patterns you've created in the affected categories?

[Mamaduka]

@richtabor, is this a Browser/Safari-specific issue? What version are you using?

I can't reproduce the bug on Chrome.

[richtabor]

Ah yes, it's Safari.

[richtabor]

Is it happening for every category or just some? Are there patterns you've created in the affected categories?

Seems like any category switch, regardless of pattern.

[Mamaduka]

@richtabor, can you share the version? I can't reproduce it with Safari Version 16.4.

[glendaviesnz]

I couldn't replicate this in Safari 16.2 either.

[aaronrobertshaw]

I can replicate this issue with Safari Version 17.1

[kevin940726]

I had a look into this and I think I know the reason.

It happens only in Safari under two separate but interwound conditions:

1. Clicking on a button in Safari doesn't focus the button, this is a known issue in Safari. However, there's a behavior that I don't know if it's only reproducible in Safari 17: it will instead focus the next focusable element in the parent. In our case, clicking on the category button doesn't focus the button, but instead focus the scrollable wrapper (<div class="components-modal__content is-scrollable" role="document" tabindex="0" aria-label="Scrollable section">) of the modal.
2. Once the category changes, the patterns list will be empty for a moment and then populate itself with the filtered list. This change triggers the resize observer and the isContentScrollable check in the modal, which then updates the tabIndex of the scrollable wrapper to undefined for a brief moment. Since we have this element focused, changing the tabIndex to undefined (-1 for div) will lose its focus. The focus is then lost (moves to body), thus triggers useOnFocusOutside check and close the modal.

I'm still not sure how to fix it though. I feel like making the scrollable wrapper always focusable (tabIndex={0}) seems like a safer option, but I'll defer to @afercia for accessibility feedback (#47426).

[afercia]

@kevin940726 thank you for looking into this and for the ping.

I'm still not sure how to fix it though. I feel like making the scrollable wrapper always focusable (tabIndex={0}) seems like a safer option,

No because setting the 'focusability' of that wrapper dynamically is the main point of that implementation. We want to make that div focusable only when there is an actual content overflow that triggers its 'scrollability'. Instead, we don't want it to be focusable when there is no actual content overflow. This point is well explained in the original PR. Native browser behavior with scrollable divs is inconsisntent and we want to standardize it. Please have a look at the original PR and also the codepen linked there, test with Chrome and Firefox.

That said, there's a few issues here.
First of all, this is a browser bug. I'm pretty sure Apple folks would call it a 'feature' but no, this is a browser bug. The fact Safari doesn't set focus on clicked buttons is super annoying and non-standard. I would greatly appreciate Apple to just fix their broken browser but I'm afraid that's not going to happen.

In general, I don't think we should fix browser bugs. Certainly we should not do that by removing important accessible features. We can try to workaround them, when possible.

I spent a few time debugging this issue and turns out there's a lot to say.

First, it is important to highlight that anything that triggers a focus loss within the modal dialog is going to make it close.

Category buttons

I was wondering why the Preferences modal dialog works correctly with Safari while this Patterns modal dialog does not. Comparing the behavior in the below animated GIF. I'm using a thick red outline to highlight where focus is:

In the Preferences modal dialog:

• Initial focus is set to the modal dialog container, as expected.
• Safari does set focus on the clicked buttons.

In the Patterns explorer modal dialog:

• Initial focus goes for a moment to the modal dialog container, then it seems it gets stolen and set somewhere else. This should be further investigated.
• Safari does not set focus on the clicked buttons.
• When clicking a button, focus goes to the scrollable div (when there is a content overflow)

Turns out in the Preferences modal dialog the buttons do have a tabindex="-1" attribute (except the active one) because they're part of the Tabs component. It appears that the tabindex attribute persuades Safari to focus the buttons when they are clicked.
Thus, I tried to set a tabindex="0" attribute on the Pattern categories buttons and that makes Safari happy. The modal dialog stays open. This tabindex="0" is technically redundant on elements that are already focusable, but seems to me it's the best way to workaround this Safari bug.

Pagination buttons

These buttons get a disabled attribute dynamically, depending if it's the first or last page. when they are clicked and then get disabled, there is a focus loss. This triggers again the same problem:

• With Safari, the modal dialog closes.
• With Chrome, the modal dialog stays open but when pressing the Tab key the focus loss is evident, as the tab sequence starts from the document root outside of the modal dialog. For no reason, ever, focus should land outside the modal dialog.

To reproduce:

• Use Chrome.
• Open the Patterns modal dialog.
• Click a category that does have pagination.
• Click the 'Next page' button until you reach the last page.
• At that point, do not click anywhere.
• Press the Tab key.
• Onbserve focus goes to the 'View Posts' link outside of the modal dialog.

Animated GIF to illustrate the issue with Chrome:

This needs to be fixed as well.

I'd argue that making the modal dialog content populate dynamically by fetching content and even using pagination within the modal dialog content seems to be less than ideal in the first place. I'm not sure it's the best user experience. Also, this implementation exposes to potential further bugs.

Anyways, as pointed out several times in the history of this project, using the disabled attribute should be avoided in most cases. I think it's beneficial to highlight yet another time a fundamental usability and accessibility principle:

Important

Elements that are focused must not get disabled on the fly, or get hidden, or be removed from the DOM. That triggers a focus loss. Unless focus is managed and the focus loss is prevented programmatically.

These pagination buttons need to be changed.
Instead of using disabled, they need to use aria-disabled and be nooped so that they are still focusable.

We should also check if there are other interactive elements in this modal dialog that get disabled/hidden/removed.

This principle applies to any focusable element: they should never be disabled/hidden/removed while they are focused.

[afercia]

Important: I also suspect that re-populating the content area makes useConstrainedTabbing fail anyways. I will prepare a quick PR to test it.

[afercia]

I created #56162 to explore a possible fix.
I'd greatly appreciate some extended testing with Safari and all the other browsers. 🙏

[afercia]

Noticed one more thing: I'm not sure I understand this change:

https://github.com/WordPress/gutenberg/pull/54590/files#diff-7d9147900701280de3fb09897ba9b4e36f424b18412332fc2f72774ef1afe035L286-R310

It comes from the PR that refactored the modal initial focus behavior. To my understanding it's getting as ref for the focus handling an element that is not always focusable? In fact, the childrenContainerRef element becomes focusable only when it is scrollable? I may be missing something though.
Cc @getdave @kevin940726 any thoughts about this?

[getdave]

To my understanding it's getting as ref for the focus handling an element that is not always focusable?

From what I recall, if you opt into the API that that PR provided, it will attempt to focus an element within the contents of the Modal rather than the entire Modal.

If the Modal contents does not contain a focusable element then it might have undesired behaviour. However, it's an optional API and thus when you opt into it it's your responsibility (as noted in the accompanying docs) as a developer to ensure a focusable element is available.

Maybe I'm misunderstanding something though...?

[afercia]

I just found another case where a button within the Modal component gets a disabled attribute while it is focused. This is the same issue of the 'Pagination buttons' explained above and it does trigger a focus loss.

In the install Font modal dialog, the 'Install' button gets disabled after it is activated with the keyboard. As such, a focus loss occurs. At that point, pressing the Tab key restarts the tab sequence fron the document root outside of the modal dialog.

It should never, ever, happen that focus lands outside of a modal dialog.

I'm not sure educating developers to avoid these focus losses is sufficient. I think there should be a strict rule, enforced via code, to not allow the disabled attribute to be set dynamically on interactive elements.

Screenshot of the inatall fonts modal dialog with the focus loss: see focus is on the 'Open navigation' button behind the modal dialog:

[afercia]

I've tested a little more in depth the 'Fonts' modal dialog and it's literally full of cases where focusable elements are dynamically added / removed / hidden / disabled. Each of those cases has an impact on:

• the detection of 'tabable' elements within the modal
• focus losses

I'm not sure a modal dialog should ever used this way, for highly dynamic UIs, but, regardless, we should make sure no focus lossess ever occur.

[kevin940726]

It should never, ever, happen that focus lands outside of a modal dialog.

I agree, but as we can see in some parts of the code base it's not trivial to fix them all at once.

The closing of the modal is the most unexpected, and in some cases it leaves users no choices to recover from it. I wonder if we should detect if a focus loss happens, and don't close the modal in that case. We can instead put the focus back in the modal again if needed. It's obviously sub-optimal, but I think the focus misplacement is still better than completely destroying users' intent of opening a modal. We can even put some console logs when they happen so that they're more obvious to the developers working on these issues.

In use-dialog hook, we can do and ad-hoc check in useFocusOutside to see if the current active element is the <body> or not and decide if we want to close the modal.

const focusOutsideProps = useFocusOutside( ( event ) => {
+	if (document.activeElement === document.body) {
+		console.warn('Focus lost. Please check the code of the modal.');
+		return;
+	}
	// This unstable prop  is here only to manage backward compatibility
	// for the Popover component otherwise, the onClose should be enough.
	if ( currentOptions.current?.__unstableOnClose ) {
		currentOptions.current.__unstableOnClose( 'focus-outside', event );
	} else if ( currentOptions.current?.onClose ) {
		currentOptions.current.onClose();
	}
} );

What do you think of this temporary approach before we go on to fix the root cause?

[afercia]

@kevin940726 that could be an option. However, I'd would like to first understand what is really happening here.

For the Safari bug:

useFocusOutside does contain code that appears to me to prevent the Safari bug:

gutenberg/packages/compose/src/hooks/use-focus-outside/index.ts

Lines 102 to 123 in 6e532f4

	/**
	* Handles a mousedown or mouseup event to respectively assign and
	* unassign a flag for preventing blur check on button elements. Some
	* browsers, namely Firefox and Safari, do not emit a focus event on
	* button elements when clicked, while others do. The logic here
	* intends to normalize this as treating click on buttons as focus.
	*
	* @param event
	* @see https://developer.mozilla.org/en-US/docs/Web/HTML/Element/button#Clicking_and_focus
	*/
	const normalizeButtonFocus: React.EventHandler<
	React.MouseEvent | React.TouchEvent
	> = useCallback( ( event ) => {
	const { type, target } = event;
	const isInteractionEnd = [ 'mouseup', 'touchend' ].includes( type );
	if ( isInteractionEnd ) {
	preventBlurCheck.current = false;
	} else if ( isFocusNormalizedButton( target ) ) {
	preventBlurCheck.current = true;
	}
	}, [] );

seems it is not working as intended though?

For buttons that are dynamically added / removed / hidden / disabled

That's just a bad pattern that developers should never use within the modal dialog. Also, I'm not sure such highly dynamic user interfaces like the content of the Fonts modal dialog should ever be placed within a modal dialog.

Consider the font 'Install' button. When it appears and users Tab to it, useConstrainedTabbing sets it to the 'next tabbable' element. But then the button is removed before users Tab again, so useConstrainedTabbing fails. This is not a bug in useConstrainedTabbing. It's just that focusable elements within a modal should never get added / removed / hidden / disabled while they are focused. I'm not sure a fallback mechanism would be OK in al cases. There are also modal dialogs that contain iframes (e.g. the Classic block) which makes thins even more complicated.

To me, we should just make clear that we must not make buttons added / removed / hidden / disabled dynamically.

[stokesman]

I believe the first point raised in this issue—Pattern Explorer unexpectedly closing—is no longer reproducible. I tried a bit and couldn't. I didn't try very hard but the following should help explain why.

First, it is important to highlight that anything that triggers a focus loss within the modal dialog is going to make it close.

Since 0816b4c this should no longer be true. That commit removed useFocusOutside from the Modal component.

It happens that the aforementioned commit was merged less than an hour before this issue was created. So I believe it was technically solved before it was made. However, it did seem to help Andrea discover other issues.

[talldan]

It happens that the aforementioned commit was merged less than an hour before this issue was created. So I believe it was technically solved before it was made.

That explains why so many of us couldn't reproduce it.