Gutenberg: Consider to find a solid way to avoid events conflicts with nested components

[afercia]

DOM events (especially keyboard events since a single keypress has many different events that fire) when used on multiple and _nested_ components may lead to unexpected behaviors. For example, while working on the DropDown menu currently used in the table block:

at first, I couldn't realize why pressing Escape when the menu is open closed the menu and also made the whole toolbar disappear. Turns out the VisualEditorBlock component is using a few events including keydown and keyup.

At the same time, the menu is using keydown and it's correctly preventing its propagation. However, when pressing _and releasing_ Escape, that's also a keyup event that propagates to the block, making the toolbar disappear.

With nested components, this kind of behaviors might happen in other scenarios too, leading to unexpected behaviors and time spent in debugging.

There are solutions, but not so ideal because ideally any component should be "isolated" and work outside of the box without having the need to have some knowledge about other components.

See also https://wordpress.slack.com/archives/C02QB2JS7/p1500643535380467.

Trying to find a solid high-level solution for the future would be great.

[afercia]

I understand this issue is too generic. I’d rather consider to mention this as a best practice in the documentation for plugin authors.

[jeffpaul]

_This ticket was mentioned in Slack in #core-editor by jeffpaul. View the logs._

[noisysocks]

I've definitely noticed that bugs due to a missing event.stopPropagation() keep popping up.

It would be nice to have a generic solution to this. I don't think we can use portals because having a component render outside of its parent's DOM will break styling in a lot of cases.

Maybe a component that stops its children from propagating key and form events would suffice?

function stopEventPropagation( event ) {
    event.stopPropagation();
}

function EventBoundary( { children } ) {
    return (
        <div
            // Key events
            onKeyDown={ stopEventPropagation }
            onKeyPress={ stopEventPropagation }
            onKeyUp={ stopEventPropagation }

            // Form events
            onChange={ stopEventPropagation }
            onInput={ stopEventPropagation }
            onInvalid={ stopEventPropagation }
            onSubmit={ stopEventPropagation }
        >
            { children }
        </div>
    );
}

// Usage:

<EventBoundary>
    <input type="text" placeholder="Go ahead, press arrow keys in me" />
</EventBoundary>

Alternatively, we could try to make <ObserveTyping> smarter in some cases. For example, we might not trigger startTyping() if the event target is nested within a <Toolbar> or <InspectorControl>.

I recall @aduth worked on something to do with ignoring descendent events which might also help here.

[aduth]

This is what the IgnoreNestedEvents component is meant to achieve. But it's also a very challenging problem, with subtleties like those discovered at https://github.com/WordPress/gutenberg/pull/5658#issuecomment-375307696 and #5289.

[designsimply]

I couldn't realize why pressing Escape when the menu is open closed the menu and also made the whole toolbar disappear.

I tested just now with Firefox 61.0.1 on macOS 10.13.6 and found that the escape key correctly closes toolbar dropdown without closing the toolbar. (24s)

With nested components, this kind of behaviors might happen in other scenarios too, leading to unexpected behaviors and time spent in debugging.

@afercia I also tested a table block nested inside a columns block and didn't see the problem there either. I am going to close this issue because it seems resolved based on my testing today. Please do let me know if you can think of additional scenarios to test and I'll re-open. 🙂

[afercia]

@designsimply agree. It's my opinion that we'll see more errors like these ones though. At the very least there should be a recommendation to always stop events propagation. This should be clearly explained in the documentation. As noted above, a built-in solution would be nice.