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

Block Editor: move selection state from RichText to the store #14640

Merged
merged 3 commits into from
Apr 19, 2019

Conversation

ellatrix
Copy link
Member

@ellatrix ellatrix commented Mar 26, 2019

Description

Fixes #11616.
Blocks #11005 and #12002. Blocks making RichText a controlled component.

This is an attempt to move local RichText selection state to block-editor store.

RichText would have the following props:

  • selectionStart: the selection start offset, similar to the textarea attrubute.
  • selectionEnd: the selection end offset, similar to the textarea attrubute.
  • onSelectionChange: the function that will be called when the selection changes.

Within blocks, this state is provided by context. Nothing changes for block authors.

In the block-editor store, the selection state will now include a rich text identifier and offset in addition to the block client ID.

Why?

  • This will be needed if we ever want to read/write selection in RichText areas. Think:
  • Blocks making RichText a controlled component. This PR moves logic to set the caret position to the MERGE_BLOCKS action and removes it from RichText.

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 changed the title WIP Block Editor: move selection state from RichText to the store Mar 26, 2019
@ellatrix ellatrix mentioned this pull request Mar 26, 2019
5 tasks
@aduth aduth added the [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable label Mar 27, 2019
@ellatrix ellatrix added [Status] In Progress Tracking issues with work in progress [Package] Block editor /packages/block-editor labels Mar 28, 2019
@ellatrix ellatrix force-pushed the try/block-editor-selection-state branch 2 times, most recently from 8f68912 to d118977 Compare March 29, 2019 17:26
@ellatrix ellatrix force-pushed the try/block-editor-selection-state branch from d69e964 to a9cccaa Compare March 29, 2019 19:30
@ellatrix ellatrix force-pushed the try/block-editor-selection-state branch from d5deacd to c08c0b5 Compare March 30, 2019 17:43
@@ -97,34 +97,6 @@ const tabThroughBlockToolbar = async () => {
);
await expect( isFocusedRightAlignmentControl ).toBe( true );

// Tab to focus on the 'Bold' formatting button
Copy link
Member Author

Choose a reason for hiding this comment

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

Reason why these are no longer expected: the formatting toolbar is only available one the rich text field has selection. The buttons are useless otherwise. In master, they are useless as well, but they display because there is lingering local selection state, so the toolbar thinks that the rich text field is selected. If you select the block for the first time in master, there will also be no formatting buttons. See #13598.

};
}

// Ensures that only one RichText component can be focused.
Copy link
Member Author

Choose a reason for hiding this comment

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

This is no longer needed as the selection state is stored in block-editor and there can be no two identifiers at the same time.

@ellatrix ellatrix force-pushed the try/block-editor-selection-state branch from 2d20ef9 to 800953a Compare March 31, 2019 11:15
@ellatrix ellatrix removed the [Status] In Progress Tracking issues with work in progress label Apr 1, 2019
Copy link
Member

@gziolo gziolo left a comment

Choose a reason for hiding this comment

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

I will give it a spin and see how it works.

It might fix #7463 as well :)

@gziolo
Copy link
Member

gziolo commented Apr 1, 2019

In comparison to the master branch it works at least the same. It might work better in some cases but it's hard to tell as it's complex. I recorded a screencast with the most complicated blocks which were causing issues, like duplicated controls, in the past:

testing-rich-text-selection

It seems like the changes proposed should open possibilities to finally resolve #7463 which we tried to tackle a few times but it was extremely difficult to detect blur event when implementation of RichText was fully dependant on TinyMCE. Given that this issue exists in the master branch, I personally don't expect it to be fixed in this PR. However, I wanted to raise it as it might have some influence on how this PR is shaped in its final form. I'm leaving it totally up to you @ellatrix. Great work on the refactoring in this PR.

@aduth aduth deleted the try/block-editor-selection-state branch April 22, 2019 13:13
daniloercoli added a commit that referenced this pull request Apr 23, 2019
…rnmobile/887-History-stack-is-not-empty-on-a-fresh-start-of-the-editor

* 'master' of https://github.com/WordPress/gutenberg:
  'string' misspelled as 'srting' (#15118)
  [Mobile] Improving accessibility on Post title (#15106)
  Fix 13776: Format is already registered to handle class name (#15072)
  Add wpDataSelect WordPress end 2 end test util (#15052)
  Block Editor: move selection state from RichText to the store (#14640)
  chore: Fix: Lint error that makes unit tests (and CI tests) fail. (#15073)
  Set ownProps.onFocus when context.onFocus is undefined. (#15069)
@pinarol
Copy link
Contributor

pinarol commented Apr 23, 2019

This seems to have a crashing regression effect on mobile side: wordpress-mobile/gutenberg-mobile#921

@ellatrix
Copy link
Member Author

@pinarol Selection state has been moved to the block-editor store. The mobile version should probably also set it there so the merge function can make use of it.

@pinarol
Copy link
Contributor

pinarol commented Apr 24, 2019

Thanks for the info @ellatrix
cc @hypest

case 'TOGGLE_SELECTION':
return {
...state,
...BLOCK_SELECTION_INITIAL_STATE,
Copy link
Member

Choose a reason for hiding this comment

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

This change has the effect of deselecting the current block when toggling selection. Is that intended?

  1. Insert an image block
  2. Assign an image from the media library
  3. Resize the image using the resizers

Expected: Block remains selected
Actual: Block is unselected

Copy link
Contributor

Choose a reason for hiding this comment

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

I think I noticed something similar with the Spacer block as well when using the resize handle.

@youknowriad youknowriad added this to the 5.6 (Gutenberg) milestone May 13, 2019
Tug added a commit that referenced this pull request May 15, 2019
Upgrade RichText on mobile native to use the latest changes from the web, mainly those made in #14640

It also removes the customization we made to the rich-text library since writing in multiple formats at once is now stable on the web.

* Adding `onSelectionChange` from store to RichText

* First experiment fixing selection change handling in rich text

* Update readme change postcommit

* Fix sequence of events on split and selection change

* Remove commented onTextUpdate call on enter pressed

* Do not reset activeFormats when onSelectionChange is emitted while typing

* Update value from props

* Pinpoint conditions for the case where onSelectionChange is emitted without changing selection

* Improve isTyping detection

* Fix applying link format

* Do not try to update text from the paste handler

* onChange should update the state

* Selection change events always fire after text already changed

* Fix issue where the list block was not being selected on focus.

* Add isSelected prop from context to RichText

* Fix move caret to the end when merging.
This avoid moving the caret to the end when two blocks, both with content, are merged.

* Caret position from props, don't force to end

* Fix lint errors

* WIP: more fixes

* WIP: Fix list external Enter key on Android

* No stray onSelectionChanged events from AztecAndroid

* Only block html elements will be trimmed by AztecAndroid

* Mirror caret state on text change

* Fix issue on iOS where the format buttons were not working properly.
Pressing on the buttons didn't change the format of the selected text.

* Restore iOS autoscroll while editing.

* Cleanup

* Try a new approach to detect if the selection change event is manual or not

* Fix iOS issue with formats, where pressing the format buttons sometimes won't change the format of the selected text
@phpbits
Copy link
Contributor

phpbits commented May 24, 2019

@ellatrix Would you mind sending me to the right direction on RichText documentation? I need to fix the selection after onChange( applyFormat( record, { type: 'strong' }, startIndex, endIndex ) )

I'm using same code here :

record = applyFormat( record, { type: 'code' }, startIndex, endIndex );
which works perfectly but the selection is being reset to the start of paragraph for some reasons. I can't figure it out, been trying to on last couple of days. Here's the screenshot of my work so far:

Screen Capture on 2019-05-24 at 17-14-32

Thank you very much!

@@ -1042,6 +1068,18 @@ Returns an action object used in signalling that the caret has entered formatted

Returns an action object used in signalling that the user caret has exited formatted text.

### selectionChange
Copy link
Member

Choose a reason for hiding this comment

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

To maximize consistency with other action names in the pattern of "verbNoun" / "actSubject" (insertBlock, updateSettings, clearSelectedBlock), in retrospect I think this should have been named as changeSelection. Has this landed in a stable WordPress release?

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] Block editor /packages/block-editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove local RichText state in favor of global selection state
7 participants