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

Components: Refactor SlotFill #19242

Merged
merged 31 commits into from Feb 26, 2020
Merged

Components: Refactor SlotFill #19242

merged 31 commits into from Feb 26, 2020

Conversation

@diegohaz
Copy link
Member

diegohaz commented Dec 19, 2019

Description

This PR is part of #18619.

The problem

There are several problems with the current slot-fill implementation. For example, the context value never changes because it's a class property whose value is never updated during the SlotFillProvider lifecycle:

this.contextValue = {
registerSlot: this.registerSlot,
unregisterSlot: this.unregisterSlot,
registerFill: this.registerFill,
unregisterFill: this.unregisterFill,
getSlot: this.getSlot,
getFills: this.getFills,
hasFills: this.hasFills,
subscribe: this.subscribe,
};

Because of that, both the Slot and the Fill components will never re-render when some action is done in the context (for example, when calling registerSlot or registerFill). Workarounds like forceUpdate() have been introduced to the code to overcome this:

// Sometimes the fills are registered after the initial render of slot
// But before the registerSlot call, we need to rerender the slot
this.forceUpdateSlot( name );
// If a new instance of a slot is being mounted while another with the
// same name exists, force its update _after_ the new slot has been
// assigned into the instance, such that its own rendering of children
// will be empty (the new Slot will subsume all fills for this name).
if ( previousSlot ) {
previousSlot.forceUpdate();
}

Also, the Fill component won't re-render when new fillProps are passed to the Slot component (probably, a new forceUpdate call should be placed here to work around this issue):

// If a function is passed as a child, provide it with the fillProps.
if ( isFunction( children ) ) {
children = children( slot.props.fillProps );
}

This solution

This is a rewrite of the slot-fill module using React Hooks and exposing only the Portal API (activated by using the bubblesVirtually prop today). This greatly simplifies the code without workarounds.

The component should behave exactly the same as when using the bubblesVirtually prop today, except that it will properly re-render when fillProps are updated.

The idea is to gradually migrate the codebase to use this while this is experimental and then replace the current slot-fill with this one if the experiment succeeds. This would also validate whether the current usage of slots without bubblesVirtually could use it or not.

How has this been tested?

This was tested using Storybook and unit tests.

Types of changes

New feature

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR. .
@diegohaz diegohaz mentioned this pull request Dec 19, 2019
3 of 6 tasks complete
@diegohaz

This comment has been minimized.

Copy link
Member Author

diegohaz commented Feb 23, 2020

So, it turns out that both the current code on master that uses bubblesVirtually and this PR don't cover the case where fills could be mounted asynchronously in different order.

I tried different approaches to solve that:

Re-mount the portal when its position in the DOM has to be corrected

This can be achieved by passing a different key to the portal:

createPortal( element, container, key );
// or
createPortal( <Fragment key={ key }>{ element }</Fragment>, container );

To identify when the key should be changed, I tried different methods:

  • Render an empty <span /> in place on the Fill component and use Node.compareDocumentPosition to determine where to add the fill in the array. We can compare its position with the other fills' spans in the DOM. This is what we use on Reakit so we can add items to an array in the same order they're rendered in the DOM.

  • Only change the key when fills that were already registered tried to register themselves again, which would indicate a new render cycle. If the new fills array were different from the last cycle, that was because a fill has been added or removed.

  • A simpler solution: change key whenever the fills array changes. It resulted in several re-mounts.

According to the tests, all of those methods successfully kept fills in the expected order. But in all those cases the block toolbar behaved quite weirdly:

Feb-22-2020 22-21-51

This clearly had something to do with the portal re-mounting, but I couldn't figure out how to mitigate that. Maybe it's something specific to how block toolbars respond to mouse over and out? Tried more variations, but I was spending too much time on this key strategy. So I decided to research more and try other things.

Instead of moving the portal itself, put it inside a <span> and work only on that

const [ span ] = useState( () => createElement( 'span' ) );
// it'll be re-appended on every render so it'll always be in the correct position 
useEffect( () => {
  slotNode.appendChild( span );
  return () => span.remove();
} );
// createPortal on `span` instead of `slotNode`
return createPortal( children, span );

This is based on a comment by Dan Abramov. The issue they're trying to solve there is not the same, but this was the solution that, after a few tweaks, had the best result. As long as you keep the reference to the container element, you can move it freely and the portal element will not need to be recreated.

Still, this presented a lot of problems: fill elements now would have a <span> wrapping them, which broke the styling. Adding flex: inherit and other css rules to the span worked around that. But, for some reason, now you could have two popovers open at the same time within the block toolbar. And some e2e tests were failing.

Conclusion

I ended up reverting those attempts to fix the order issue. It looks like, at least for the modules that are currently using bubblesVirtually, they don't need this kind of precision?

It took so many workarounds for this to vaguely work that I think we should, in the future, re-consider the bubblesVirtually approach altogether. Maybe using React Portal wasn't a good idea? After all, it's not even supported by React Native.

This PR is already an improvement over the current implementation on master. It properly updates fills when fillProps change and gets rid of the Consumer. So I'm still in favor of landing it as is.

@youknowriad

This comment has been minimized.

Copy link
Contributor

youknowriad commented Feb 24, 2020

So I do think we need this precision at some point but I agree that the fact that it's present in master makes it less important to fix in this particular PR.

Copy link
Contributor

youknowriad left a comment

This is looking great, I'd appreciate a follow-up to switch all slots to the new implementation and officially deprecate/remove the old implementation.

I'd also appreciate @aduth thoughts here (even after merge as he's AFK).

@gziolo

This comment has been minimized.

Copy link
Member

gziolo commented Feb 24, 2020

There is one blocker to resolve:

Screen Shot 2020-02-24 at 13 23 10

When you enable the top toolbar option in the More Menu, most of the controls are missing. Once you refresh the page everything goes back to normal. It must be something with the setting change.

Master

Screen Shot 2020-02-24 at 13 40 06

@@ -30,6 +30,9 @@ const BlockInspector = ( {
selectedBlockName,
showNoBlockSelectedMessage = true,
} ) => {
const slot = useSlot( InspectorAdvancedControls.slotName );

This comment has been minimized.

Copy link
@gziolo

gziolo Feb 24, 2020

Member

Yes, let's keep __experimentalUseSlot experimental until we come up with a better way to handle it. It seems fine for the time being.

diegohaz added 2 commits Feb 25, 2020
@diegohaz

This comment has been minimized.

Copy link
Member Author

diegohaz commented Feb 25, 2020

Good catch @gziolo! Thanks for the review! ❤️

So, I found the problem and fixed it. This is something that should be covered by the current tests (should subsume another slot by the same name). But react-test-renderer doesn't support portals (facebook/react#11565), so even on master that is not being covered.

I'll propose in a follow-up PR a refactored set of tests for this module using react-testing-library (using the dependency directly, without the @wordpress/test-utils package proposed in #18855).

@diegohaz diegohaz mentioned this pull request Feb 25, 2020
6 of 6 tasks complete
@diegohaz

This comment has been minimized.

Copy link
Member Author

diegohaz commented Feb 25, 2020

Follow-up PR with improved tests: #20428

@gziolo
gziolo approved these changes Feb 25, 2020
Copy link
Member

gziolo left a comment

I can confirm it fixes the issue I raised yesterday. Awesome work on fixing it. I believe it closes more existing issues. I will try to identify them :)

Let's ship it 🚢

@diegohaz diegohaz merged commit e8f4f37 into master Feb 26, 2020
3 checks passed
3 checks passed
build
Details
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@github-actions github-actions bot added this to the Gutenberg 7.7 milestone Feb 26, 2020
@diegohaz diegohaz deleted the update/slot-fill branch Mar 2, 2020
@aduth

This comment has been minimized.

Copy link
Member

aduth commented Mar 2, 2020

This looks really cool! I'd like to circle back and dive into the specific changes, but at least on a cursory glance, the implementation looks solid.

Would you happen to have a sense whether this will help unblock #14845 / #17355 ? I noticed some earlier comments at #17355 (comment) , but those were prior to some additional changes that came later in this pull request.

@draganescu

This comment has been minimized.

Copy link
Contributor

draganescu commented Mar 27, 2020

Advised by @aduth I tried reverting this PR and now this #18560 new editor crash is fixed. So this PR apparently did something that crashes the editor with the console message:

TypeError: "slot is undefined"

when doing this:

gif-reusable-crash

const { [ name ]: slot, ...nextSlots } = prevSlots;
// Make sure we're not unregistering a slot registered by another element
// See https://github.com/WordPress/gutenberg/pull/19242#issuecomment-590295412
if ( slot.ref === ref ) {
Comment on lines +29 to +32

This comment has been minimized.

Copy link
@aduth

aduth Mar 27, 2020

Member

Should it be safe to assume slot is defined here?

In debugging the issue mentioned by @draganescu at #18560 (comment) , the exact error occurs at this line of code:

image

It's not yet clear to me if the bug is in how the slot came to be undefined in the first place, or if we should be guarded in how we expect slot to be available at this line of code.

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.