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

gziolo · 2018-05-21T13:51:17Z

Description

With #6286 we have deprecated event proxying.

@youknowriad discovered the following:

I wonder if we should introduce an explicit onFocus prop, it seems to be something commonly used across blocks even after #5039

This PR tries to fix it.

How has this been tested?

Make sure that Image and Gallery blocks work like before and there are no warnings on JS console related to RichText component.

Types of changes

Refactoring.

Checklist:

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

youknowriad · 2018-05-21T14:56:05Z

core-blocks/image/edit.js

@@ -389,6 +363,11 @@ class ImageEdit extends Component {
 }

 export default compose( [
+	withBlockEditContext( ( { setFocusedElement } ) => {


Nice trick, do you think this should be documented or recommended?

I did the same for both BlockEdit and Plugin - it's more work to map context to props, but I like the fact that you don't need to navigate between files to discover how context is injected into a component.

See:
https://github.com/WordPress/gutenberg/blob/master/editor/components/block-edit/context.js#L24-L26

and https://github.com/WordPress/gutenberg/blob/master/plugins/components/plugin-context/index.js#L17-L19.

Well, we could even create a factory for it :D

youknowriad · 2018-05-23T09:44:11Z

editor/components/index.js

@@ -5,6 +5,7 @@ export { default as AlignmentToolbar } from './alignment-toolbar';
 export { default as BlockAlignmentToolbar } from './block-alignment-toolbar';
 export { default as BlockControls } from './block-controls';
 export { default as BlockEdit } from './block-edit';
+export { withBlockEditContext } from './block-edit/context';


I'm still on the fence here, what's the difference between making this available to block authors and passing its content as props to edit like we do for several other props :)

cc @aduth Thoughts?

We can do prop drilling for blocks, too :)
To be honest, I wasn't happy about exposing this HOC.

aduth · 2018-05-24T19:27:34Z

Should we just add default onFocus prop handling for RichText ? The intent with #6286 was more to avoid setting the expectation that we blanket forward TinyMCE events, particularly where we're inconsistent with how those events are forwarded, i.e. argument of onChange being the new value, argument of onFocus (previously) being the TinyMCE event. But it wasn't to say that we strictly don't want to support any of these props. An onFocus seems like a common use-case (obviously we depended on it already), and the implementation here seems quite complex for plugin authors to recreate on their own.

gziolo · 2018-05-27T12:18:38Z

Should we just add default onFocus prop handling for RichText ? The intent with #6286 was more to avoid setting the expectation that we blanket forward TinyMCE events, particularly where we're inconsistent with how those events are forwarded, i.e. argument of onChange being the new value, argument of onFocus (previously) being the TinyMCE event. But it wasn't to say that we strictly don't want to support any of these props. An onFocus seems like a common use-case (obviously we depended on it already), and the implementation here seems quite complex for plugin authors to recreate on their own.

I added a kind of automated default onFocus handling for RichText in #6419, which uses setFocusedElement from BlockEditContext to do the trick. We can update this implementation to handle onFocus prop, too. Related code:
https://github.com/WordPress/gutenberg/blob/master/editor/components/rich-text/index.js#L201-L205

There is a bigger issue with other components, which this PR unveiled. I'm leaning towards the solution proposed by @youknowriad to pass down setFocusedElement method as one of the props injected into edit component. It still wouldn't be super easy to use by plugin developers. In this context the only thing we need to implement is to have a way to reseet the state so we could inform the block that focus needs to be removed from RichText (when clicking on the image).

tofumatt

Curious about the a11y of switching to onClick but the code seems fine to me...

tofumatt · 2018-05-28T15:20:40Z

core-blocks/gallery/gallery-image.js

+			<figure
+				className={ className }
+				tabIndex="-1"
+				onClick={ this.props.onSelect }


Drive-by review, but does the change from onFocus in the child RichText component to the onClick here present any accessibility issues? How does keyboard navigation work after this change (I didn't see it mentioned in the manual testing being done)?

I haven't tested it with the keyboard, I will double check if it should be updated to onFocus.

Updated to use onFocus. It looks like Gallery block isn't navigable with keyboard :( Related issue: #3836.

aduth · 2018-05-29T18:47:07Z

I don't think it's been shown that we need to expose setFocusedElement to a block implementer at all. As implemented, the requirement for gallery is that we need some way to reflect that focus has left a RichText instance. In the same way we support automatic focus detection, why not should we support automatic blur detection (complications therein notwithstanding), rather than putting the impetus on the developer (particularly when they've come to expect onFocus to be managed automatically)?

youknowriad · 2018-05-29T18:50:36Z

why not should we support automatic blur detection (complications therein notwithstanding), rather than putting the impetus on the developer (particularly when they've come to expect onFocus to be managed automatically)?

The issue is that sometimes RichText loses the focus but we still want to keep it selected. (Link modal, toolbar buttons), It's probably doable but not simple because of the fact that popovers show outside the RichText's node.

aduth · 2018-05-29T18:53:58Z

The issue is that sometimes RichText loses the focus but we still want to keep it selected. (Link modal, toolbar buttons), It's probably doable but not simple because of the fact that popovers show outside the RichText's node.

I'm not sure I understand where we rely on this. Can you detail a simple set of steps to reproduce?

gziolo · 2018-05-29T18:59:47Z

The issue is that sometimes RichText loses the focus but we still want to keep it selected. (Link modal, toolbar buttons), It's probably doable but not simple because of the fact that popovers show outside the RichText's node.

I'm not sure I understand where we rely on this. Can you detail a simple set of steps to reproduce?

Let me update PR so we could discover where it could fail. I think we can make it opt-in/out if necessary.

youknowriad · 2018-05-29T19:10:33Z

I'm not sure I understand where we rely on this

When you click the "link" button in the RichText toolbar, the RichText loses the focus and it's transfered to the input in the link modal which is rendered outside the RichText which means the isSelected prop of the RichText will move to false causing the toolbar and the link modal to disappear.

I did try this a long time ago :)

gziolo · 2018-05-29T19:12:18Z

In my testing, it happens also for all controls inside block's toolbar which makes it impossible to use them :(

gziolo · 2018-05-29T19:48:21Z

Removed withBlockEditContext usage with 9da4eb4.

I will play with it a bit more tomorrow. As far as I understand, what we really need is to set onFocus on the BlockEdit which will reset focused element when focus happens outside of RichText.

youknowriad · 2018-05-30T13:10:02Z

We're approaching the 3.0 release and the proxy event handlers will be removed, should we add an explicit onFocus prop while we try to come up with a solution here?

gziolo · 2018-05-30T13:51:53Z

Yes, let’s go with explicit onFocus. I didn’t come up with anything that would handle it automatically. As you already noted, this seems like a very difficult use case to solve. I plan to play a bit more with .closest but it probably will fail as it operates on DOM nit React DOM 🙁

aduth · 2018-05-31T13:50:22Z

Note: The explicit onFocus minimal implementation was merged in #7029.

mcsf · 2018-06-26T15:45:07Z

@gziolo, given the merge of #7029, what's the status of this one?

gziolo · 2018-06-28T04:15:20Z

@mcsf, the original issue is resolved but there are more issues to solve here as explained in my comment in here #7463 (comment). Let’s close this PR for now. We can reuse this code later when we have an idea how to improve focus handling for RichText component.

gziolo added [Feature] Rich Text Related to the Rich Text component that allows developers to render a contenteditable [Type] Code Quality Issues or PRs that relate to code quality labels May 21, 2018

gziolo added this to the 3.0 milestone May 21, 2018

gziolo self-assigned this May 21, 2018

gziolo requested review from youknowriad, aduth and a team May 21, 2018 13:51

gziolo mentioned this pull request May 21, 2018

Rich Text: Deprecate event proxying #6286

Merged

gziolo added the [Feature] Blocks Overall functionality of blocks label May 21, 2018

gziolo changed the title ~~Update/core blocks on focus~~ Core blocks: Refactor Gallery and Image blocks to avoid RichText warnings May 21, 2018

youknowriad reviewed May 21, 2018

View reviewed changes

youknowriad reviewed May 23, 2018

View reviewed changes

aduth mentioned this pull request May 24, 2018

Blocks: Image and Gallery blocks depend on deprecated RichText events #6944

Closed

tofumatt reviewed May 28, 2018

View reviewed changes

gziolo added 2 commits May 29, 2018 07:42

Core blocks: Refactor Gallery to avoid RichText warning

626392f

Core blocks: Refactor Image to avoid RichText warnings

6dc4397

gziolo force-pushed the update/core-blocks-on-focus branch from 38f7978 to 6dc4397 Compare May 29, 2018 10:46

Editor: Set only focus for the RichText component

17603b2

gziolo force-pushed the update/core-blocks-on-focus branch from 9da4eb4 to 17603b2 Compare May 29, 2018 19:42

aduth mentioned this pull request May 30, 2018

Blocks: Remove onFocus from core blocks' RichText usage #7029

Merged

gziolo modified the milestones: 3.0, 3.1 May 31, 2018

gziolo added the [Status] In Progress Tracking issues with work in progress label May 31, 2018

gziolo changed the title ~~Core blocks: Refactor Gallery and Image blocks to avoid RichText warnings~~ [WIP] Core blocks: Refactor Gallery and Image blocks to avoid RichText warnings May 31, 2018

gziolo modified the milestones: 3.1, 3.2 Jun 19, 2018

grappler mentioned this pull request Jun 22, 2018

Toolbar showing for TextControl when RichText is not selected #7463

Closed

gziolo closed this Jun 28, 2018

gziolo deleted the update/core-blocks-on-focus branch June 28, 2018 04:15

gziolo mentioned this pull request Oct 24, 2018

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

Closed

gziolo mentioned this pull request Nov 12, 2018

RichText: own undo signalling #10650

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

gziolo commented May 21, 2018

youknowriad May 21, 2018

gziolo May 23, 2018

gziolo May 23, 2018

gziolo May 23, 2018

youknowriad May 23, 2018

youknowriad May 23, 2018

gziolo May 23, 2018 •

edited

Loading

aduth commented May 24, 2018 •

edited

Loading

gziolo commented May 27, 2018

tofumatt left a comment

tofumatt May 28, 2018

gziolo May 29, 2018

gziolo May 29, 2018 •

edited

Loading

aduth commented May 29, 2018 •

edited

Loading

youknowriad commented May 29, 2018

aduth commented May 29, 2018

gziolo commented May 29, 2018

youknowriad commented May 29, 2018

gziolo commented May 29, 2018 •

edited

Loading

gziolo commented May 29, 2018

youknowriad commented May 30, 2018

gziolo commented May 30, 2018

aduth commented May 31, 2018

mcsf commented Jun 26, 2018

gziolo commented Jun 28, 2018

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

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

Conversation

gziolo commented May 21, 2018

Description

How has this been tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo May 23, 2018 • edited Loading

Choose a reason for hiding this comment

aduth commented May 24, 2018 • edited Loading

gziolo commented May 27, 2018

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo May 29, 2018 • edited Loading

Choose a reason for hiding this comment

aduth commented May 29, 2018 • edited Loading

youknowriad commented May 29, 2018

aduth commented May 29, 2018

gziolo commented May 29, 2018

youknowriad commented May 29, 2018

gziolo commented May 29, 2018 • edited Loading

gziolo commented May 29, 2018

youknowriad commented May 30, 2018

gziolo commented May 30, 2018

aduth commented May 31, 2018

mcsf commented Jun 26, 2018

gziolo commented Jun 28, 2018

gziolo May 23, 2018 •

edited

Loading

aduth commented May 24, 2018 •

edited

Loading

gziolo May 29, 2018 •

edited

Loading

aduth commented May 29, 2018 •

edited

Loading

gziolo commented May 29, 2018 •

edited

Loading