-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Implementing new UX for invoking rich text Link UI #57986
Conversation
Size Change: +2.5 kB (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
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.
I have some of the other work for this issue I can push up to this branch.
@@ -39,6 +40,42 @@ function Edit( { | |||
contentRef, | |||
} ) { | |||
const [ addingLink, setAddingLink ] = useState( false ); | |||
const [ clickedLink, setClickedLink ] = useState( false ); |
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.
Instead of managing a clickedLink
state, can we set the state of the thing we want to do? I.e. - display the link preview.
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.
I don't see how we could do that. The Popover
is within the React tree. Therefore we must setState to trigger a re-render of the component with the new state.
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.
Can we use setAddingLink( true )
instead of adding clickedLink
state?
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.
Yeh try that. Might not need two states.
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.
Done :) b322bbd
|
||
useLayoutEffect( () => { | ||
// log tagNAME of anchorElement | ||
if ( anchorElement?.tagName?.toLowerCase() === 'a' ) { |
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.
Can't you just return early?
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.
Also if anchorElement is defined and isActive is true, it's guaranteed to be a link no?
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.
No because it can be a VirtualAnchorElement returned from useAnchor
.
I've ended up avoiding the useAnchor
entirely now by attaching listeners to the contenteditable.
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.
No because it can be a VirtualAnchorElement returned from useAnchor.
How can I test VirtualAnchorElements? What blocks use them?
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.
You can't 😄 Take a look at useAnchor
and you'll see it returns a "virtual" anchor representing a selection within RichText. So for example, when you highlight some text and you want to make a link, the Link UI needs to have an anchor point to "attach" the Popover to, but the link (<a>
TAG) doesn't yet exist in the DOM. So useAnchor
will return a coordinate which approximates the selection in the richtext.
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.
Nice work!
We'll need some new e2e test coverage for these changes. Specifically it would be super easy for regressions to be introduced around this new behaviour so we need to validate the new expectations.
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.
I think fixing all the broken e2e tests will validate it too.
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.
Sure, let's try.
Thank you Ella. @jeryj Let's refine the PR UX / UI before this gets merged |
Flaky tests detected in 196d888. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7646931654
|
✅ Update: I implemented this in a8a418a I think the next step should be that if you are within a link and...
...then
I think we can safely trap focus and constrain tabbing. We should then consider incorporating the a11y tweaks to labelling the dialog that I started in #54063. |
I suppose the link control in the toolbar should open/close the LinkControl, just like when the other formats are applied (and not subsequently removed upon a second click): link.mp4 |
I've been looking at this, and it is unfortunately quite tricky due to how/when states are set. Are you OK with that behavior in a follow-up? |
Sure. |
b322bbd
to
e6e0e98
Compare
Update: I can't replicate this once I rebased and then reverted 74a3e9e @jeryj I found a bug where we show the wrong UI:
Screen.Capture.on.2024-01-24.at.09-29-54.mp4It only seems to happen if you click exactly on the point where the cursor is placed. |
29ac34b
to
2783e3c
Compare
2783e3c
to
2526cda
Compare
Rebased to bring in changes made in #57726 |
@@ -39,6 +39,36 @@ function Edit( { | |||
contentRef, | |||
} ) { | |||
const [ addingLink, setAddingLink ] = useState( false ); | |||
// We only need to store the button element that opened the popover. We can ignore the other states, as they will be handled by the onFocus prop to return to the rich text field. | |||
const [ openedBy, setOpenedBy ] = useState( null ); |
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.
I didn't want to add this openedBy
state, as the popover has its own system for returning focus. For some reason, it's not working correctly with the Rich text element and split between iframe editor and toolbar in the header.
Great work @jeryj 👏
@richtabor already confirmed he's happy with this in a followup. I don't think we should block this PR further on this unless there's a very important reason why it has to be in the initial release. |
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.
This is testing well for me.
The state being tested is no longer possible with the new ux.
Tests are hopefully fixed to account for the new states of the Link Control being visible/hidden. 🤞🏻 Also added tests to cover focus outside click sending focus to the right area and returning focus from the keyboard to the button that opened 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.
LGTM ✨
…zy-hydration * origin/trunk: (47 commits) Interactivity API: Break up long hydration task in interactivity init (#58227) Add supports.interactivity to Query block (#58316) Font Library: Make notices more consistent (#58180) Fix Global styles text settings bleeding into placeholder component (#58303) Fix the position and size of the Options menu, (#57515) DataViews: Fix safari grid row height issue (#58302) Try a fix (#58282) Navigation Submenu Block: Make block name affect list view (#58296) Apply custom scroll style to fixed header block toolbar (#57444) Add support for transform and letter spacing controls in Global Styles > Typography > Elements (#58142) DataViews: Fix table view cell wrapper and BlockPreviews (#58062) Workflows: Add 'Technical Prototype' to the type-related labels list (#58163) Block Editor: Optimize the 'useBlockDisplayTitle' hook (#58250) Remove noahtallen from .wp-env codeowners (#58283) Global styles revisions: fix is-selected rules from affecting other areas of the editor (#58228) Try: Disable text selection for post content placeholder block. (#58169) Remove `template-only` mode from editor and edit-post packages (#57700) Refactored download/upload logic to support font faces with multiple src assets (#58216) Components: Expand theming support in COLORS (#58097) Implementing new UX for invoking rich text Link UI (#57986) ...
What?
Changes UX for links within rich text to require explicit activation before displaying the Link UI interface.
Closes #57821
Why?
Currently on
trunk
moving the cursor within an existing link (format) will cause the Link UI to appear. This is providing a sub par a11y experience because:How?
disables the behaviour to immediately activate the Link UI is the link format is active
adds a click handler to the contenteditable element which will trigger the Link UI when it is clicked.
changes the block toolbar button to be "Edit link" if the link format is active. Clicking this will cause the Link UI to appear.
New Post
Create a paragraph block
Add some text
Create 3 or 4 links
Click on each link and see you can activate and edit the link using the Link UI.
Using keyboard attempt the same thing but notice that you must either click the toolbar button or press
CMD + K
to edit the linkTesting Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Screen.Capture.on.2024-01-18.at.21-03-34.mp4