RichText: Do not focus the input unless user is in an editing context #61374
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
alexstine
added
[Focus] Accessibility (a11y)
alexstine
requested review from
ellatrix,
fluiddot,
dcalhoun and
ajitbohra
as code owners
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -94 B (-0.01%) Total Size: 1.76 MB
ℹ️ View Unchanged
|
|
@jorgefilipecosta Looks like we all missed this. :( Your fix was creative but we've introduced another bug in the process. When the Would appreciate some eyes here. |
|
I think I understand quite well what's going on, but I have trouble reproducing the bug. Suppose I'm editing the navigation link's 
and I disabled the Still I can't reproduce a situation where the focus would be stolen back to the |
ellatrix
reviewed
May 15, 2024
packages/rich-text/src/to-dom.js
Outdated
| // | ||
| // See: https://github.com/WordPress/gutenberg/issues/61315 | ||
| if ( | ||
| activeElement.closest( 'iframe' ) && |
There was a problem hiding this comment.
This will always return false, the iframe element is not in the same document as the rich text elements?
There was a problem hiding this comment.
You could check something like
const { frameElement } = activeElement.ownerDocument.defaultView;
frameElement ? frameElement === frameElement.ownerDocument.activeElement : true
|
@jsnajdr, try applying the patch below, then delete the navigation item link in the sidebar and start typing a new one. This is a fix for #60758. diff --git packages/block-library/src/navigation-link/update-attributes.js packages/block-library/src/navigation-link/update-attributes.js
index 6aa28d1818d..fdf6d10c46a 100644
--- packages/block-library/src/navigation-link/update-attributes.js
+++ packages/block-library/src/navigation-link/update-attributes.js
@@ -88,7 +88,7 @@ export const updateAttributes = (
setAttributes( {
// Passed `url` may already be encoded. To prevent double encoding, decodeURI is executed to revert to the original string.
- ...( newUrl && { url: encodeURI( safeDecodeURI( newUrl ) ) } ),
+ ...{ url: newUrl ? encodeURI( safeDecodeURI( newUrl ) ) : newUrl },
...( label && { label } ),
...( undefined !== opensInNewTab && { opensInNewTab } ),
...( id && Number.isInteger( id ) && { id } ),ScreencastCleanShot.2024-05-15.at.16.53.55.mp4 |
|
@ellatrix Would appreciate another review. I still don't have this right. Thanks. |
scruffian
reviewed
Jun 18, 2024
|
@alexstine Thank you so much for your PR. I tested the Accessibility from your last commit and for me it works fine and better :) |
|
I would love to get this PR merged but we've still got failing E2E tests. Assistance would be appreciated. |
|
I'll put it on my list of things to look at later this week if no one else gets to it first. |
…se focus shift. Change the code to only move focus if user is in iFrame to avoid side-effects where text fields could be updating the value.
|
@alexstine Unfortunately, the e2e test is failing for a valid reason. It only occurs in Firefox:
In other browsers, focus is moved back to the list item so you can continue typing. I've been debugging it some but haven't made any clear progress. |
jeryj
reviewed
Jun 27, 2024
| // Do not take focus if user is not in iFrame editing canvas. | ||
| // | ||
| // See: https://github.com/WordPress/gutenberg/issues/61315 | ||
| if ( isInsideFrame && activeElement !== ownerDocument.activeElement ) { |
There was a problem hiding this comment.
We need the activeElement.focus() on line 304 to run for Firefox to move focus after a selection change.
When that change comes from the toolbar (indent/outdent), we want the cursor to get applied to the list item and take focus. When a change comes from the sidebar, we don't want to move focus to the canvas (like a navigation label text change). I can't find a way to differentiate between these two situations. Fixing one breaks the other.
@ellatrix, do you have any insights?
There was a problem hiding this comment.
I'm failing to understand what problem this modification to rich text solves. I remove this change, tried to edit the text in the inspector for a navigation link, and focus seems to remain there. How do I reproduce the problem without this change to rich text?
These lines in rich text are only meant to maintain focus only if the active element right before changing the range is different after changing the range.
There was a problem hiding this comment.
On Trunk in Chrome:
- In Chrome
- Go to a navigation item
- Edit it in the canvas (add some letters)
- Tab to the sidebar
- Continue tabbing and you'll end up in a focus loop
Screen.Recording.2024-06-28.at.9.25.15.AM.mov
On this PR in Firefox
- In Firefox
- Add a list block
- Type something
- Add another list item
- Move to the list item toolbar
- Indent the list item using the Indent button
- Focus is lost. The active focus should be moved to the list item for typing.
Screen.Recording.2024-06-28.at.9.29.14.AM.mov
This PR without the isInsideFrame check in Firefox
- In Firefox
- Go to a navigation item
- Edit it in the canvas (add some letters)
- Go to the block settings sidebar
- Type in the label field
- Focus will get stolen back to the canvas after one keypress
Screen.Recording.2024-06-28.at.9.30.49.AM.mov
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #61315
This was a hard bug to unravel. Essentially, every time a user types into a
RichTextfield, we're building some type of virtual tree in memory. Apparently this can at times cause focus to shift unpredictibly so the solution was to hardcode a focus back to the input receiving theonChangevalues. The issue with this is if the user is changing the value from say aTextControlin the inspector sidebar,RichTextstill sees this as anonChangeand will take focus back to the block editor canvas. I added a check to skip this focus check if the user was not in the context of an iFrame.Worth noting, I have no idea if I have possibly broken anything. I would greatly value input from @ellatrix on if this is a viable solution or not.
Finally, removed the hack in the navigation link block to keep focus in one place.
Why?
Broken accessibility.
How?
Explained above.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast