Blocks: Add automatic handling of focus for RichText component by gziolo · Pull Request #6419 · WordPress/gutenberg

gziolo · 2018-04-25T13:55:55Z

Description

Closes #6213.

Follow up for #5029 where we introduced new context API from React 16.3 to apply isSelected prop to a few components used with BlockEdit.

This PR explores this idea further to make it possible to enforce that there can only be one RichText component focused at the same time. Historically it was manually enforced by using local state which stored which component is focused and prevented others to be selected at the same time.

I proposed the following

Yes, multiple RichText is quite difficult to handle and maintain. I will look into it next week. Maybe it wouldn't be that hard to automatically handle active state in a way where only one RichText can be active per block at a time.

@jorgefilipecosta also proposed together with @jayshenk the following:

I think we can have only one active RichText for the whole post. Maybe we can just use a component id and store id of RichText that is active, it would simplify the logic of some blocks.

I started small by using BlockEdit scoped solution to aovid dealing with the global store. I didn't see an immediate need to use wp.data to solve the issue we had here. We might want to revisit this later, but at the moment it seems like using the context API limits the number of operation React would have to perform where the focused element changes. I might be wrong here though.

How has this been tested?

Made sure it works without any changes for all blocks and for those updated in particular:

Pullquote
Quote
Text columns

Screenshots

Types of changes

Refactoring, no changes in the UX or UI.

Next steps

We use a similar technique that uses withState to change focus for Gallery and Image blocks, but they are more complex so I excluded them from this PR. However we might look into exposing a general purpose helper that would allow to wrap components and inject this behavior to them.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.

gziolo · 2018-04-26T10:21:21Z

@zgordon @Shelob9 - this PR would solve the issue we had at WC Miami when you wanted to provide onFocus and isSelected to the RichText. Unlimited RichText components inside your block with automatically handled focus for you :)

youknowriad · 2018-04-27T06:15:29Z

Should we add a deprecation message about the isSelected prop in RichText components?

We might, but as soon as we remove all occurrences in core blocks. Otherwise, we would flood the console with the deprecation messages. We should also make sure there aren't any cases when this possibility would still make sense.

I love having focus handled automatically by default. I'd like to be able to optionally control it myself by setting that prop using my own logic.

youknowriad

I like it but there might be a small bug on initial mount.

Try this:

Insert a paragraph block
Using the autocompleter switch to the text columns block
Type and move the mouse
Notice the toolbar is not shown even if the text columns has the focus.

gziolo · 2018-04-27T14:15:22Z

@youknowriad, great catch. I think I found a solution which solves the issue you described: f0edd3c. Let me know if you have some ideas how to improve this code. Maybe there is a way to avoid introducing another method in the BlockEdit context, which doesn't require passing down boolean param.

gziolo · 2018-04-30T14:51:58Z

Rebased with master.

aduth · 2018-04-30T17:49:06Z

 		return {
-			isBlockSelected: isSelected,
+			isSelected: context.isSelected && context.focusedElement === ownProps.instanceId,
+			onSetup: () => {


Nit: Why arrow function rather than function property shorthand?

onSetup() { // ... }

Good question, I don’t have an answer 😜 I’ll update.

Updated with 6f4a136

aduth · 2018-04-30T17:50:07Z

 		return {
-			isBlockSelected: isSelected,
+			isSelected: context.isSelected && context.focusedElement === ownProps.instanceId,
+			onSetup: () => {


When does onSetup get called? When the user first focuses into the field? I would have thought it'd be at each's initialization, though it doesn't seem like we should want to set the focused element at all until the user actually focuses within (i.e. only onFocus).

OnSetup gets called when RichText gets mounted (initial render of the editor or when new block gets added). OnFocus is triggered only when a block gets focus but not on the initial render. That’s why we need to maintain two methods.

But I don't understand why we need to set the focused element merely by fact the RichText has initialized.

aduth · 2018-04-30T17:50:35Z

 		return {
-			isBlockSelected: isSelected,
+			isSelected: context.isSelected && context.focusedElement === ownProps.instanceId,
+			onSetup: () => {


Is this going to incur a lot of unnecessary renders due to the new function references being passed each time (bypassing any pure component behavior)?

There are also multiple props present that change frequently causing rerenders. I tried to add some optimizations to avoid creating new references, but it wasn’t helpful at all. I didn’t notice any improvements

"If you can't beat them, join them" ? 😆

It would be great to take a closer look at all updates triggered by RichText and better understand how and why it works this way.

gziolo · 2018-05-02T11:31:52Z

I rebased with master again and also added a small change in 6f4a136.

mcsf

Nice work.

gziolo · 2018-05-02T13:19:43Z

@mcsf thanks for review. I want to investigate if we can use only onFocus set on the div element which wraps TinyMCE. Props to @aduth for suggesting this solution 👍

gziolo · 2018-05-04T11:07:21Z

@aduth I think I found a way to simplify this implementation using onFocus attached to div as you suggested in one of the comments. It seems to work exactly the same but in addition we no longer create new functions on every re-render :)

aduth · 2018-05-04T14:21:57Z

+		}
+		// When explicitly set as selected or has onFocus prop provided,
+		// use value stored in the context instead.
+		if ( ownProps.isSelected === true || ownProps.onFocus ) {


With the onFocus prop effectively deprecated by #6286 / #6352, should we remove it from consideration here?

Yes, it makes sense 👍

aduth · 2018-05-04T14:23:00Z

-	edit: withState( {
-		editable: 'content',
-	} )( ( { attributes, setAttributes, isSelected, className, editable, setState } ) => {
+	edit: ( { attributes, setAttributes, isSelected, className } ) => {


Nit: Fewer characters as function property shorthand:

edit: ( { attributes, setAttributes, isSelected, className } ) => {

edit( { attributes, setAttributes, isSelected, className } ) {

Will do on Monday 👍

Also applied to Quote

… blocks

gziolo added [Feature] Blocks Overall functionality of blocks [Status] In Progress Tracking issues with work in progress labels Apr 25, 2018

gziolo self-assigned this Apr 25, 2018

gziolo force-pushed the update/rich-text-focus branch from 57280ac to 0e50419 Compare April 25, 2018 16:18

gziolo requested a review from mcsf April 25, 2018 16:21

gziolo force-pushed the update/rich-text-focus branch from 0e50419 to c5a10a8 Compare April 26, 2018 09:52

gziolo removed the [Status] In Progress Tracking issues with work in progress label Apr 26, 2018

gziolo changed the title ~~[WIP] Blocks: Add automatic handling of focus for RichText component~~ Blocks: Add automatic handling of focus for RichText component Apr 26, 2018

gziolo requested review from aduth, jorgefilipecosta and youknowriad April 26, 2018 10:19

gziolo added this to the 2.8 milestone Apr 26, 2018

gziolo added the [Type] Enhancement A suggestion for improvement. label Apr 26, 2018

youknowriad reviewed Apr 27, 2018

View reviewed changes

gziolo changed the base branch from update/block-edit-context-name to master April 30, 2018 14:17

gziolo force-pushed the update/rich-text-focus branch from f0edd3c to e80d22f Compare April 30, 2018 14:48

gziolo mentioned this pull request Apr 30, 2018

Toolbar controls of block persist after selecting different one #6213

Closed

aduth reviewed Apr 30, 2018

View reviewed changes

gziolo force-pushed the update/rich-text-focus branch from e80d22f to 6f4a136 Compare May 2, 2018 11:30

mcsf approved these changes May 2, 2018

View reviewed changes

aduth modified the milestones: 2.8, 2.9 May 3, 2018

gziolo force-pushed the update/rich-text-focus branch from 6f4a136 to c9f68d3 Compare May 4, 2018 11:03

aduth reviewed May 4, 2018

View reviewed changes

gziolo added 6 commits May 7, 2018 09:37

Blocks: Add automatic handling of focus for RichText component

77c9548

Core blocks: Add automatic handling for focus for Quote and Pullquote…

0b26e7c

… blocks

Blocks: Show controls for newly created block

e0304b1

Blocks: Use method shorthand instead of arrow functions

c01ec93

Blocks: Use onFocus on div to set focused element

052b92c

Blocks: Address review feedback

8fb655e

gziolo force-pushed the update/rich-text-focus branch from c9f68d3 to 8fb655e Compare May 7, 2018 07:38

Core Blocks: Remove obsolete isSelected usage from Spacer block

bdb3a53

gziolo merged commit ead8dc5 into master May 7, 2018

gziolo deleted the update/rich-text-focus branch May 7, 2018 08:01

This was referenced May 11, 2018

Editor: Introduce name prop for RichText to handle changes automatically #6705

Closed

Multiple RichText issues #6740

Closed

RichText issues #6747

Closed

gziolo mentioned this pull request May 27, 2018

[WIP] Core blocks: Refactor Gallery and Image blocks to avoid RichText warnings #6871

Closed

4 tasks

aduth mentioned this pull request Jun 6, 2018

Block List: Remove createInnerBlockList utility / context #7192

Merged

aduth mentioned this pull request Sep 10, 2018

Introduce a way to manage focus on the RichText editor #9740

Closed

ellatrix mentioned this pull request Nov 1, 2018

RichText: Revise onSplit prop #11005

Closed

4 tasks

pharmarin mentioned this pull request Jan 17, 2019

Remove isSelected from RichText render nk-crew/lazy-blocks#36

Merged

ellatrix mentioned this pull request Apr 1, 2019

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

Merged

5 tasks

Conversation

gziolo commented Apr 25, 2018 • edited by ianbelanger79 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Screenshots

Types of changes

Next steps

Checklist:

Uh oh!

gziolo commented Apr 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

gziolo commented Apr 27, 2018

Uh oh!

gziolo commented Apr 30, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gziolo commented May 2, 2018

Uh oh!

mcsf left a comment

Choose a reason for hiding this comment

Uh oh!

gziolo commented May 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gziolo commented May 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gziolo commented Apr 25, 2018 •

edited by ianbelanger79

Loading

gziolo commented Apr 26, 2018 •

edited

Loading

gziolo commented May 2, 2018 •

edited

Loading