Use Portal-based slots for the block toolbar #16421

youknowriad · 2019-07-04T16:09:53Z

In several occasions we noticed issues with the default Slot/Fill implementation (the one that copes children from the Fill into the Slot). These issues makes it for example impossible to use A Slot inside a Fill.

The "bubblesVirtually" (Portal-based) implementation doesn't suffer from these issues. In this PR, I'm exploring the possibility to move the BlockToolbar slots into portal-based slots. The ultimate goal is that all slots would use the Portal-based implementation.

The problem though is that in several components we rely on event cascading which can be impacted by Portals. For example:

The NavigableContainer component need to catch events from all its DOM children regardless of how they are rendered.
The same issue happens in WritingFlow.

My proposal here is to move away from React event handlers into native event handlers for these particular components where the intent is to rely on the natural DOM tree bubbling.

Testing instructions

I tested for example that I can navigate the block toolbar using arrows
I tested that I can move between blocks using arrows.

(I'm not certain at this point that everything is right yet but I'd love testing and thoughts).

jorgefilipecosta

This is working really well on my tests, I did not find regressions on the post editor and on the widget screen.

The tests are failing but I think it is because we need to update them to use the new way events are set.

jorgefilipecosta · 2019-07-04T18:50:25Z

packages/block-editor/src/components/writing-flow/index.js

@@ -355,8 +365,6 @@ class WritingFlow extends Component {
 			<div className="editor-writing-flow block-editor-writing-flow">
 				<div
 					ref={ this.bindContainer }
-					onKeyDown={ this.onKeyDown }


I'm probably missing something, but why change the approach where we pass onKeyDown to the element directly during the render to an approach where we use addEventListener? I guess it is to not use the react events which bubble in a different way, should we add a small comment?

yes, when we use Portals, we have parts of the toolbars that are rendered in a completely different React Tree which means events from these components won't be caught. But for WritingFlow and NavigableContainer, the behavior is tied to the DOM tree and not the React tree. so instead of using React Events, I'm relying on Native events instead.

youknowriad · 2019-07-05T09:24:52Z

So it turns out we have two hidden bugs that this PR highlights and fixes:

The fact that withContextEdit doesn't work properly on VerticalAlignmentToolbar (and I supposed other toolbar components) creates a wrong isCollapsed value.
We have some edge cases in Slots that use bubblesVirtually causing the fills to not render sometimes. (This is fixed by the newly introduced useSlot hook)

youknowriad · 2019-07-05T10:47:33Z

packages/rich-text/src/component/index.js

+
+			// This uses a native event listener because WritingFlow uses native event listener as well
+			// Event bubbling won't work properly otherwise between the two components.
+			this.editableRef.addEventListener( 'keydown', this.onKeyDown );


It looks like this change broke the "splitting/merging". When you merge blocks, the caret is not put at the right position.

I'm not certain how this works in the recent versions, I might need some help @ellatrix

packages/block-editor/src/components/navigable-toolbar/index.js

aduth

I ran through some of the previous incompatibilities which were reported last time we tried similar (#3132, #3082, #3075). I also searched broadly for toolbar controls, where I had some expectation that interactions like click+drag / arrow keys might trigger some ancestor handling unintentionally (as described in these issues). Generally it seems to work pretty well here.

packages/block-editor/src/components/block-toolbar/index.js

packages/components/src/slot-fill/context.js

packages/block-editor/src/components/navigable-toolbar/index.js

youknowriad · 2019-07-15T11:13:01Z

This seems ready for a final review.

packages/block-editor/src/components/block-toolbar/index.js

packages/block-editor/src/components/block-toolbar/style.scss

gziolo · 2019-07-18T10:28:39Z

packages/components/src/slot-fill/slot.js


 		if ( bubblesVirtually ) {
-			return <div ref={ this.bindNode } />;
+			return <div ref={ this.bindNode } className={ className } />;


This needs a mention in Slot docs about new param available.

gziolo · 2019-07-18T10:34:37Z

packages/components/src/slot-fill/fill.js

@@ -69,10 +63,9 @@ function FillComponent( { name, getSlot, children, registerFill, unregisterFill

 const Fill = ( props ) => (
 	<Consumer>
-		{ ( { getSlot, registerFill, unregisterFill } ) => (
+		{ ( { registerFill, unregisterFill } ) => (


We can probably refactor it further in a follow-up PR to avoid using Consumer at all and use useSlot here as well.

yeah, I also thought about adding a useFills (similar to useSlot and simplify the Slot component implementation using it)

Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>

gziolo · 2019-07-18T10:47:00Z

I'm observing some issues when clicking on the block toolbar, it randomly selects the previous block.

youknowriad · 2019-07-18T11:27:51Z

@gziolo it should be fixed with the last commit.

gziolo

Code changes look great and I can confirm that the last fix solves the issue I experienced in the past.

Let's make sure that new className prop added to Slot is documented as noted in #16421 (comment) before this PR gets merged.

Use Portal-based slots for the block toolbar

9611c44

youknowriad added Framework Issues related to broader framework topics, especially as it relates to javascript [Type] Code Quality Issues or PRs that relate to code quality labels Jul 4, 2019

youknowriad requested review from aduth, ajitbohra, chrisvanpatten, ellatrix, gziolo, jorgefilipecosta and talldan as code owners July 4, 2019 16:09

youknowriad self-assigned this Jul 4, 2019

jorgefilipecosta reviewed Jul 4, 2019

View reviewed changes

Fix Slot&Fill bubblesVirtually

7caa658

youknowriad requested review from nerrad and ntwb as code owners July 5, 2019 09:22

youknowriad added 2 commits July 5, 2019 10:58

Fix unit tests

d805c7e

Fix e2e tests

f86a3f6

youknowriad requested review from daniloercoli, etoledom and SergioEstevao as code owners July 5, 2019 10:34

youknowriad commented Jul 5, 2019

View reviewed changes

ellatrix reviewed Jul 5, 2019

View reviewed changes

packages/block-editor/src/components/navigable-toolbar/index.js Outdated Show resolved Hide resolved

youknowriad added 2 commits July 5, 2019 12:03

Fix typo

468dbd5

Revert WritingFlow and RichText changes

d2db635

youknowriad force-pushed the try/use-native-event-handlers branch from a277380 to d2db635 Compare July 5, 2019 11:13

aduth reviewed Jul 9, 2019

View reviewed changes

packages/block-editor/src/components/block-toolbar/index.js Outdated Show resolved Hide resolved

packages/components/src/slot-fill/context.js Show resolved Hide resolved

packages/block-editor/src/components/navigable-toolbar/index.js Show resolved Hide resolved

Adding clarifying comments

aecc8c4

gziolo reviewed Jul 18, 2019

View reviewed changes

Fix typo

ca8caef

Co-Authored-By: Grzegorz (Greg) Ziółkowski <grzegorz@gziolo.pl>

Fix multi-selection triggered when clicking the block toolbar

c906079

gziolo approved these changes Jul 25, 2019

View reviewed changes

Document the className prop

9c62e64

youknowriad merged commit bed7e0c into master Jul 25, 2019

youknowriad deleted the try/use-native-event-handlers branch July 25, 2019 15:25

youknowriad added this to the Gutenberg 6.2 milestone Jul 26, 2019

youknowriad mentioned this pull request Jul 30, 2019

Update the Inspector slots to use the bubblesVirtually slots #16807

Merged

swissspidy mentioned this pull request Oct 15, 2019

Image alignment not functional ampproject/amp-wp#3504

Closed

gziolo mentioned this pull request Nov 5, 2019

Components: Make Toolbar accessible #17875

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use Portal-based slots for the block toolbar #16421

Use Portal-based slots for the block toolbar #16421

youknowriad commented Jul 4, 2019

jorgefilipecosta left a comment

jorgefilipecosta Jul 4, 2019

youknowriad Jul 4, 2019

youknowriad commented Jul 5, 2019

youknowriad Jul 5, 2019

aduth left a comment

youknowriad commented Jul 15, 2019

gziolo Jul 18, 2019

gziolo Jul 18, 2019

youknowriad Jul 18, 2019

gziolo commented Jul 18, 2019

youknowriad commented Jul 18, 2019

gziolo left a comment

Use Portal-based slots for the block toolbar #16421

Use Portal-based slots for the block toolbar #16421

Conversation

youknowriad commented Jul 4, 2019

jorgefilipecosta left a comment

Choose a reason for hiding this comment

jorgefilipecosta Jul 4, 2019

Choose a reason for hiding this comment

youknowriad Jul 4, 2019

Choose a reason for hiding this comment

youknowriad commented Jul 5, 2019

youknowriad Jul 5, 2019

Choose a reason for hiding this comment

aduth left a comment

Choose a reason for hiding this comment

youknowriad commented Jul 15, 2019

gziolo Jul 18, 2019

Choose a reason for hiding this comment

gziolo Jul 18, 2019

Choose a reason for hiding this comment

youknowriad Jul 18, 2019

Choose a reason for hiding this comment

gziolo commented Jul 18, 2019

youknowriad commented Jul 18, 2019

gziolo left a comment

Choose a reason for hiding this comment