-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve code commenting around focus mount behaviour of rich text Link UI #54085
Conversation
Size Change: +1.24 kB (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
60e96d7
to
5791996
Compare
Just found a bug whereby Also if we are going to trap focus in both edit and preview modes, then we need a means for keyboard users to close the |
Flaky tests detected in 5791996. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6037602102
|
@youknowriad As this code goes way back I wonder if you recall why we set |
@getdave If I'm not wrong, when there's already a link in RichText and you click on the link element in the RichText you want to continue typing and not move the focus to the link popover. |
@@ -230,7 +230,7 @@ function InlineLinkUI( { | |||
return ( | |||
<Popover | |||
anchor={ popoverAnchor } | |||
focusOnMount={ focusOnMount.current } | |||
focusOnMount={ focusOnMountWithTrap } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this is all we need, we can basically remove this line because it's the prop's default value.
On the other hand though, it kind of seems to me like the desired behavior here is actually addingLink ? 'firstElement' : true
.
Excellent context. Thank you 🙇 Screen.Capture.on.2023-09-01.at.14-02-57.mp4This is what Riad is refering to. When creating a link we want focus to jump directly into the popover because it is a dialog that contains the control to do the creation. However if you already have a link then when entering into the link format (the bit that is highlighted) you do not want to steal focus into the popover because it is informational only. Instead you want to continue typing until such time as you choose to move into the link dialog. |
I've updated the PR to be focused on code quality by improving the inline comment. The objective now is that:
The focus does now seem to be behaving as I expected. I'm not sure why yesterday it didn't seem to be. Hopefully this PR leaves the code in a better place than when we found it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment seems true to functionality today. 👍
What?
Improves the documentation around why the focus on mount handling is implemented in the way it is on the Link UI popover for rich text.
Complements #54063
Why?
The code around managing focus seems unusual, especially as for most dialogs the focus should always be moved into the dialog on mount.
The comment now fully documents the code and avoid potential future regressions and also avoids relying on stakeholder knowledge.
How?
Adds comment to explain why the code is as it is.
Testing Instructions
trunk
.Testing Instructions for Keyboard