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

Use Portal-based slots for the block toolbar #16421

Merged
merged 10 commits into from Jul 25, 2019

Conversation

@youknowriad
Copy link
Contributor

commented Jul 4, 2019

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
Copy link
Member

left a comment

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.

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

This comment has been minimized.

Copy link
@jorgefilipecosta

jorgefilipecosta Jul 4, 2019

Member

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?

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 4, 2019

Author Contributor

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 youknowriad requested review from nerrad and ntwb as code owners Jul 5, 2019

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2019

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 added some commits Jul 5, 2019


// 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 );

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 5, 2019

Author Contributor

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

youknowriad added some commits Jul 5, 2019

@youknowriad youknowriad force-pushed the try/use-native-event-handlers branch from a277380 to d2db635 Jul 5, 2019

@aduth
Copy link
Member

left a comment

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.

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2019

This seems ready for a final review.


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

This comment has been minimized.

Copy link
@gziolo

gziolo Jul 18, 2019

Member

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

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

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

This comment has been minimized.

Copy link
@gziolo

gziolo Jul 18, 2019

Member

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

This comment has been minimized.

Copy link
@youknowriad

youknowriad Jul 18, 2019

Author Contributor

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

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

This comment has been minimized.

Copy link
Member

commented Jul 18, 2019

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

@youknowriad

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2019

@gziolo it should be fixed with the last commit.

@gziolo

gziolo approved these changes Jul 25, 2019

Copy link
Member

left a comment

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.

@youknowriad youknowriad merged commit bed7e0c into master Jul 25, 2019

1 of 2 checks passed

Filter merged Filter merged
Details
Travis CI - Pull Request Build Passed
Details

@youknowriad youknowriad deleted the try/use-native-event-handlers branch Jul 25, 2019

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

sbardian added a commit to sbardian/gutenberg that referenced this pull request Jul 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.