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

RichText: unify active formats, 'selectedFormat' and 'placeholderFormat' #14411

Merged

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Mar 13, 2019

Description

Fixes #11743.
Fixes #14120.
Fixes issue where emoji would be destroyed when inserting it right after a format element:

emoji

This is an attempt to unify active formats (determines which UI is displayed as active), selectedFormat (used for format boundaries) and formatPlaceholder (used for format shortcuts on collapsed selected).

The replacement is an activeFormats key which holds information about which formats are active and can be used to apply to an insertion.

  • In the middle of a piece of (formatted) text, this holds the same formats as the one stored on the index.
  • When at the edge of a format, this can be the formats before or after the caret, depending on the user selection.
  • The user can also change this array by using the shortcuts cmd+B etc. when there is a collapsed selection.

How has this been tested?

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

@ellatrix ellatrix added this to the 5.3 (Gutenberg) milestone Mar 14, 2019
@youknowriad youknowriad removed this from the 5.3 (Gutenberg) milestone Mar 18, 2019
@ellatrix ellatrix force-pushed the try/unify-active-formats-selected-format-placeholder branch from 6df04e1 to 41b30f8 Compare March 19, 2019 13:41
@ellatrix ellatrix added this to the 5.4 (Gutenberg) milestone Mar 19, 2019
@ellatrix ellatrix added [Type] Bug An existing feature does not function as intended [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] Rich text /packages/rich-text labels Mar 19, 2019
@ellatrix ellatrix force-pushed the try/unify-active-formats-selected-format-placeholder branch from 41b30f8 to 2ea70db Compare March 21, 2019 21:06
@ellatrix
Copy link
Member Author

Rebased and added another e2e test.

@ellatrix
Copy link
Member Author

ellatrix commented Apr 1, 2019

@noisysocks I would love this to be in Gutenberg 5.4.

@ellatrix
Copy link
Member Author

ellatrix commented Apr 1, 2019

(It should go in the next WordPress 5.2 Beta.)

@noisysocks
Copy link
Member

noisysocks commented Apr 1, 2019

I can cherry-pick it for GB 5.4 tomorrow. To have it in WP 5.2 we'll need to cherry-pick it onto wp/5.2 and release packages cc. @youknowriad.

Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

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

All of the following issues appear to be present in master as well, but are related to the active format behavior, so I'll leave it to your discretion whether you want to address here or separately:

  • I still see multiple active formats highlighted when navigating from one block to another. (screencast)
  • When creating a new prompt and immediately pressing Cmd+B, the "Bold" toolbar button does not become active. Proceeding to type, however, will yield bolded text.
  • Perhaps related to the prior point, when within a bolded or italic text which occurs at the end of a paragraph, pressing Cmd+B or Cmd+I to deactivate that format similarly does not toggle the toolbar button active state, but does in-fact deactivate the format.
  • When a selection extends from non-formatted text into formatted text (e.g. Bold), pressing Cmd+B one time will remove all formats. Subsequent presses, however, do nothing. This one I actually observed mistakenly in master, and appears fixed as of this branch, possibly the same as Cannot Add 2nd Inline Format When Type (Nested Bold & Italic While Typing) #11743 . (screencast)

packages/rich-text/src/index.js Show resolved Hide resolved
packages/rich-text/src/normalise-formats.js Show resolved Hide resolved
packages/rich-text/src/normalise-formats.js Show resolved Hide resolved
packages/rich-text/src/get-active-formats.js Show resolved Hide resolved
@ellatrix
Copy link
Member Author

ellatrix commented Apr 2, 2019

I still see multiple active formats highlighted when navigating from one block to another.

This is a separate issue: we currently have lingering selection state after blurring element. I can quickly resolve this here by adding :focus to the boundary selected used to style the element.

I'll also have a look into the other points.

@ellatrix
Copy link
Member Author

ellatrix commented Apr 2, 2019

When creating a new prompt and immediately pressing Cmd+B, the "Bold" toolbar button does not become active. Proceeding to type, however, will yield bolded text.

This one I cannot reproduce:

bold

@aduth
Copy link
Member

aduth commented Apr 2, 2019

When creating a new prompt and immediately pressing Cmd+B, the "Bold" toolbar button does not become active. Proceeding to type, however, will yield bolded text.

This one I cannot reproduce:

I was seeing it in new (empty) paragraphs specifically (with "Top Toolbar" setting). Trying again right now though, I'm not able to reproduce it anymore 🤷‍♂️

@ellatrix ellatrix force-pushed the try/unify-active-formats-selected-format-placeholder branch from 2ca178c to 4e7888d Compare April 2, 2019 21:09
@noisysocks noisysocks merged commit 5d6527d into master Apr 3, 2019
@noisysocks noisysocks deleted the try/unify-active-formats-selected-format-placeholder branch April 3, 2019 00:13
noisysocks pushed a commit that referenced this pull request Apr 3, 2019
…at' (#14411)

* RichText: unify active formats, 'selectedFormat' and 'placeholderFormat'

* Add extra e2e test

* Only should boundary style when focused

* Update docs
@ellatrix
Copy link
Member Author

ellatrix commented Apr 3, 2019

Thanks for the review and merge!

ellatrix added a commit that referenced this pull request Apr 3, 2019
…at' (#14411)

* RichText: unify active formats, 'selectedFormat' and 'placeholderFormat'

* Add extra e2e test

* Only should boundary style when focused

* Update docs
ellatrix added a commit that referenced this pull request Apr 4, 2019
* RichText: improve format boundary style (#14519)

* RichText: improve format boundary style

* rgb => rgba

* Paste: check plain text for gutenberg content (#14536)

* Make ClipboardButton inside a block work correctly in Safari (#7106)

* Make ClipboardButton inside a block work in Safari

* Update changelogs

* Block Editor: Update "Next" to "Unreleased" per guidelines

https://github.com/WordPress/gutenberg/blob/master/packages/README.md#maintaining-changelogs

* Input Interaction: always expand single line selection vertically (#14487)

* Input Interaction: always expand single line selection vertically

* Add e2e test

* Use MenuItem instead of IconButton (#14569)

* Remove id, infoid, label and aria-describedby from MenuItem (#14423)

* Preformatted: save line breaks as characters (#14653)

* Preformatted: save line breaks as characters

* Update e2e test

* Remove negative toolbar position rules from full-aligned blocks. (#14669)

* Fix issue with double scrollbar in Fullscreen Mode (#14677)

This PR fixes an issue where the sidebar would have two scrollbars when in fullscreen mode.

* Fix WordPress embed block resolution (#14658)

* Retry failing embeds with trailing slash (#14705)

* Fix embedding Twitter URLs with a trailing slash (Closes #12664)

* Fix race condition for WordPress URLs that end in slashes, add test

* API Fetch: Fix error on empty OPTIONS preload data (#14714)

* Input Interaction: better horizontal edge detection (#14462)

* Input Interaction: better horizontal edge detection

* Correct BR ranges

* Add e2e test

* Increase buffer for Firefox

* Clean up

* Merge isEdge logic

* Fix typo

* Address feedback

* Build docs

* Fix memize option key typo (#14750)

* RichText: unify active formats, 'selectedFormat' and 'placeholderFormat' (#14411)

* RichText: unify active formats, 'selectedFormat' and 'placeholderFormat'

* Add extra e2e test

* Only should boundary style when focused

* Update docs

* Try to trigger tests with Travis

* Restore Travis config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Package] Rich text /packages/rich-text [Type] Bug An existing feature does not function as intended
Projects
None yet
4 participants