Skip to content
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

Inline Highlight ColorPicker - Typing hex codes default to 000000, causes colorpicker to jump location #49286

Closed
cuemarie opened this issue Mar 22, 2023 · 4 comments · Fixed by #54736
Assignees
Labels
[Feature] Colors Color management [Package] Format library /packages/format-library [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended

Comments

@cuemarie
Copy link

cuemarie commented Mar 22, 2023

Description

On a test site using WordPress 6.1.1, but Gutenberg inactive, the Inline Highlight ColorPicker moves when I try typing in a hex code, and auto-completes the code to 000000 - making it quite difficult to type in a color.

Step-by-step reproduction instructions

  1. On a test site with WordPress 6.1.1 and all other plugins inactive, create a new page/post with text content
  2. Highlight text, then use the Toolbar > Highlight to set a custom color
  3. Try typing a color code into the hex code field

Screenshots, screen recording, code snippet

Screen.Capture.on.2023-03-22.at.13-14-34.mp4

Environment info

  • WordPress Version 6.1.1
  • No plugins active
  • Theme: Twenty Twenty-Three v1.0
  • Browser: FireFox 111.0

This report stems from troubleshooting on this open report: Automattic/wp-calypso#59566

Please confirm that you have searched existing issues in the repo.

Yes

Please confirm that you have tested with all plugins deactivated except Gutenberg.

Yes

@t-hamano
Copy link
Contributor

I was able to reproduce this problem with the latest Gutenberg, but with slightly different results.

  • In the color picker, the first time the following operation is performed, the popover is misaligned:
    • Select a color palette
    • Change the hex value in the custom color picker
    • Change the pointer position in the custom color picker
  • Autocompletion of color codes by 000000 appears not to occur.
1ce2ce1084259524fe65eb05d3e6d91c.mp4

@ciampo
Copy link
Contributor

ciampo commented Apr 5, 2023

Thank you for the thorough report, @cuemarie !

Following up to @t-hamano 's reply, I think that the reason why the color popover jumps around is related to its anchor becoming stale. For reference, here's the bit of code where the anchor for this particular instance of Popover is defined:

/*
As you change the text color by typing a HEX value into a field,
the return value of document.getSelection jumps to the field you're editing,
not the highlighted text. Given that useAnchor uses document.getSelection,
it will return null, since it can't find the <mark> element within the HEX input.
This caches the last truthy value of the selection anchor reference.
*/
const popoverAnchor = useCachedTruthy(
useAnchor( {
editableContentElement: contentRef.current,
settings,
} )
);

As you can see, there's already a comment about how this value may be jumping while editing the highlight.

Here's what I think may be happening:

  • when unhighlighted text is selected, a virtual anchor is passed to the Popover to represent the bounding rect of the text being highlighted
  • when a color is picked, the text gets wrapped in a <mark> element — it's at that point that that anchor may be going "out of sync". I tested by logging popoverAnchor.getBoundingClientRect().top and, in fact, it jumps suddenly from the correct value to 0

Therefore, it doesn't look like the issue here is in the Popover component, but rather in how the popover anchor is defined / updated.

@ntsekouras , is this something that you or @ellatrix could take a look at ?

@t-hamano
Copy link
Contributor

t-hamano commented Apr 6, 2023

Thank you for the analysis, @ciampo!

The useCachedTruthy() to cache the anchor was introduced in #36263 to solve a similar problem that occurred in the past. The argument useAnchorRef() was then changed to useAnchor().

I suspect that the change in the return value of this hook from {Element|Range} to {Element|VirtualAnchorElement|undefined|null} might have caused the cache to not work properly.

Perhaps not the ideal approach, but caching only getBoundingClientRect(), as shown below, appears to solve this problem.

diff --git a/packages/format-library/src/text-color/inline.js b/packages/format-library/src/text-color/inline.js
index 7a2c2f92ea..3c73984437 100644
--- a/packages/format-library/src/text-color/inline.js
+++ b/packages/format-library/src/text-color/inline.js
@@ -144,12 +144,12 @@ export default function InlineColorUI( {
         it will return null, since it can't find the <mark> element within the HEX input.
         This caches the last truthy value of the selection anchor reference.
         */
-       const popoverAnchor = useCachedTruthy(
-               useAnchor( {
-                       editableContentElement: contentRef.current,
-                       settings,
-               } )
-       );
+       const popoverAnchor = useAnchor( {
+               editableContentElement: contentRef.current,
+               settings,
+       } );
+       const cachedRect = useCachedTruthy( popoverAnchor.getBoundingClientRect() );
+       popoverAnchor.getBoundingClientRect = () => cachedRect;
 
        return (
                <Popover

Also, although it may not be directly related to this issue, I have discovered a problem with the initial position of the inline color popover being misaligned. If the content contains a mark element, the mark element present on the same line is recognized as an anchor.

781da47483cd35c34025f96321b802c8.mp4

@ciampo
Copy link
Contributor

ciampo commented Apr 6, 2023

Perhaps not the ideal approach, but caching only getBoundingClientRect(), as shown below, appears to solve this problem.
...
Also, although it may not be directly related to this issue, I have discovered a problem with the initial position of the inline color popover being misaligned. If the content contains a mark element, the mark element present on the same line is recognized as an anchor.

Thank you for looking further into it. Feel free to propose a fix, although I'll defer to folks who are more experienced with the block editor and the rich text packages to see if there's a better solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Colors Color management [Package] Format library /packages/format-library [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants