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

Keep a copy of anchor ref to preserve highlighted text selection #36263

Merged
merged 2 commits into from
Nov 5, 2021

Conversation

alshakero
Copy link
Contributor

@alshakero alshakero commented Nov 5, 2021

Description

When highlighting a text to change its color, we use document.currentView.getSelection to determine what's selected on the screen. But when you try to type in the color you want using the custom color input, you're also discarding the selection you'd made of the text you want to colorize.

So when useAnchorRef goes to look for the <mark> element within the selected text, it won't find it, since it's actually looking inside the input field.

This PR proposes caching the truthy value of the selection anchor, so user can select other text (like the color string inside the input) without losing track of the text they want to format.

How has this been tested?

Luckily, the issue is reproducible on trunk here. This change demonstrably fixes it.

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

Fixes #36254 and Automattic/wp-calypso#57687

Copy link
Member

@fullofcaffeine fullofcaffeine left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This tests well!

@fullofcaffeine
Copy link
Member

@jorgefilipecosta @ellatrix I’ve tested that this fixes issue #36254 in v11.8.0 and from this branch (closer to trunk / 11.9.0 RC1), but I’m not sure if this is how you’d prefer to handle this kind of focus/selection related issue long-term. We can follow up with another iteration afterward, if needed.

@fullofcaffeine fullofcaffeine merged commit 787fd3e into WordPress:trunk Nov 5, 2021
@github-actions github-actions bot added this to the Gutenberg 12.0 milestone Nov 5, 2021
fullofcaffeine pushed a commit that referenced this pull request Nov 5, 2021
)

* Keep a copy of anchor ref to preserve selection

* Update native files as well
@fullofcaffeine
Copy link
Member

This has been cherry-picked into https://github.com/WordPress/gutenberg/tree/release/11.8.

* @param {any} value
* @return {any} value
*/
export function useCachedTruthy( value ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This hook seem a bit misplaced. If we need to be a public API, it should be on the "compose" package (more generic as it has nothing to do with block-editor). But personally, I see it only used once and I'm not sure this hook is very valuable outside that use-case so why not just move it close to its current usage?

@youknowriad
Copy link
Contributor

Also was this cherry-picked to 10.9 as well, since 10.9 is now branched, any cherry-pick made to 10.8 should also be made to 10.9 or the commit won't be on the next major release.

fullofcaffeine pushed a commit that referenced this pull request Nov 5, 2021
)

* Keep a copy of anchor ref to preserve selection

* Update native files as well
@fullofcaffeine
Copy link
Member

@youknowriad jinx! I just did that, and also released RC2 with it https://github.com/WordPress/gutenberg/releases/tag/v11.9.0-rc.2. Thanks for the reminder, anyway! 🙇🏻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RichText: Unable to add a custom highlight color
4 participants