Fix incorrect useAnchor positioning when switching from virtual to rich text elements #58900
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
…ch text elements When creating a link or inline text color, we start with a virtual element and then after creating the link/adding a color, it switches from a virtual to rich text element (<a> or <mark>). We want to recompute that we've changed elements and reanchor appropriately when this happens. Also, useAnchor was adding focus and selectionChange events that were only used by a previous version of the link control UX where the link popover would show based on the caret position. If the caret was within the link, we would show the link popover. This is no longer the case, so we can remove these event listeners.
|
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: -53 B (0%) Total Size: 1.71 MB
ℹ️ View Unchanged
|
jeryj
added
[Type] Bug
jeryj
commented
Feb 9, 2024
Comment on lines
-156
to
-176
| function attach() { | ||
| ownerDocument.addEventListener( 'selectionchange', callback ); | ||
| } | ||
|
|
||
| function detach() { | ||
| ownerDocument.removeEventListener( 'selectionchange', callback ); | ||
| } | ||
|
|
||
| if ( editableContentElement === ownerDocument.activeElement ) { | ||
| attach(); | ||
| } | ||
|
|
||
| editableContentElement.addEventListener( 'focusin', attach ); | ||
| editableContentElement.addEventListener( 'focusout', detach ); | ||
|
|
||
| return () => { | ||
| detach(); | ||
|
|
||
| editableContentElement.removeEventListener( 'focusin', attach ); | ||
| editableContentElement.removeEventListener( 'focusout', detach ); | ||
| }; |
There was a problem hiding this comment.
I believe all of these were used to show/hide the link control popover when the caret was placed inside the link boundaries. This is no longer the UX as of #57986
jeryj
commented
Feb 9, 2024
| @@ -195,28 +194,9 @@ function InlineLinkUI( { | |||
|
|
|||
| const popoverAnchor = useAnchor( { | |||
| editableContentElement: contentRef.current, | |||
| settings, | |||
| settings: { ...settings, isActive }, | |||
There was a problem hiding this comment.
Is it better to add an isActive property instead of shoving it in the settings array? Probably... But I didn't want to have to update the docs to always support it if so. If that isn't a concern, I think we should add isActive as its own prop on useAnchor.
draganescu
previously approved these changes
Feb 12, 2024
There was a problem hiding this comment.
I think this solution is much more solid and unerstandable:
! wasActive && isActive ) || ( wasActive && ! isActive
is much more easy to grasp compared to:
const cachedRect = useCachedTruthy( popoverAnchor.getBoundingClientRect() );
popoverAnchor.getBoundingClientRect = () => cachedRect;
and also solves the problem at the source (in useAnchor) instead of sprinkling logic across many files.
In my testing this works as expected and shows no problems compared to trunk
draganescu
approved these changes
Feb 12, 2024
There was a problem hiding this comment.
I think we should merge both this and #58896 and adress the problem with custom fields UI separately.
|
Is this a bug fix for something during the 6.5 lifecycle. In other words, should we backport this to 6.5? |
|
Yes. |
draganescu
added
the
Backport to WP 6.7 Beta/RC
aristath
added a commit
that referenced
this pull request
Feb 13, 2024
* trunk: (273 commits) Remove preffered style variations legacy support (#58930) Style theme variations: add property extraction and merge utils (#58803) Migrate `change-detection` to Playwright (#58767) Update Changelog for 17.6.6 Docs: Clarify the status of the wp-block-styles theme support, and its intent (#58915) Use `data_wp_context` helper in core blocks and remove `data-wp-interactive` object (#58943) Try double enter for details block. (#58903) Template revisions API: move from experimental to compat/6.4 (#58920) Editor: Remove inline toolbar preference (#58945) Clean up link control CSS. (#58934) Font Library: Show error message when no fonts found to install (#58914) Block Bindings: lock editing of blocks by default (#58787) Editor: Remove the 'all' rendering mode (#58935) Pagination Numbers: Add `data-wp-key` to pagination numbers if enhanced pagination is enabled (#58189) Close link preview if collapsed selection when creating link (#58896) Fix incorrect useAnchor positioning when switching from virtual to rich text elements (#58900) Upgrade Floating UI packages, fix nested iframe positioning bug (#58932) Site editor: fix start patterns store selector (#58813) Revert "Rich text: pad multiple spaces through en/em replacement (#56341)" (#58792) Documentation: Clarify the performance reference commit and how to pick it (#58927) ...
youknowriad
pushed a commit
that referenced
this pull request
Feb 13, 2024
…ch text elements (#58900) When creating a link or inline text color, we start with a virtual element and then after creating the link/adding a color, it switches from a virtual to rich text element (<a> or <mark>). We want to recompute that we've changed elements and reanchor appropriately when this happens. Also, useAnchor was adding focus and selectionChange events that were only used by a previous version of the link control UX where the link popover would show based on the caret position. If the caret was within the link, we would show the link popover. This is no longer the case, so we can remove these event listeners.
|
I just cherry-picked this PR to the backports/beta1 branch to get it included in the next release: 5f03c58 |
youknowriad
removed
the
Backport to WP 6.7 Beta/RC
youknowriad
pushed a commit
that referenced
this pull request
Feb 13, 2024
…ch text elements (#58900) When creating a link or inline text color, we start with a virtual element and then after creating the link/adding a color, it switches from a virtual to rich text element (<a> or <mark>). We want to recompute that we've changed elements and reanchor appropriately when this happens. Also, useAnchor was adding focus and selectionChange events that were only used by a previous version of the link control UX where the link popover would show based on the caret position. If the caret was within the link, we would show the link popover. This is no longer the case, so we can remove these event listeners.
youknowriad
pushed a commit
that referenced
this pull request
Feb 13, 2024
…ch text elements (#58900) When creating a link or inline text color, we start with a virtual element and then after creating the link/adding a color, it switches from a virtual to rich text element (<a> or <mark>). We want to recompute that we've changed elements and reanchor appropriately when this happens. Also, useAnchor was adding focus and selectionChange events that were only used by a previous version of the link control UX where the link popover would show based on the caret position. If the caret was within the link, we would show the link popover. This is no longer the case, so we can remove these event listeners.
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.
When creating a link or inline text color, we start with a virtual element and then after creating the link/adding a color, it switches from a virtual to rich text element ( or ). We want to recompute that we've changed elements and reanchor appropriately when this happens.
Also, useAnchor was adding focus and selectionChange events that were only used by a previous version of the link control UX where the link popover would show based on the caret position. If the caret was within the link, we would show the link popover. This is no longer the case, so we can remove these event listeners.
What?
Fixes scrolling bug related to caching the popover position when switching from a virtual to rich text element (and back from rich text to virtual). When the popover position is cached, it will scroll down the page in a fixed position rather than be anchored near the element.
Why?
How?
Recompute anchor position when going from selection to a active rich text element and vice versa.
Remove unnecessary event listeners.
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast
Before
before.popover.positioning.mov
After
after.popover.positioning.mov