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

Link popover: prevent mouse event propagation #11753

Merged
merged 6 commits into from Nov 15, 2018

Conversation

Projects
None yet
4 participants
@johngodley
Contributor

johngodley commented Nov 12, 2018

In a link popover if you press the mouse button down and then drag it triggers a selection event in the block list under the popover, which then results in the popover disappearing.

This is demonstrated here:

screen recording 2018-11-05 at 10 04 am

This PR prevents the mouse down event from propagating, so stopping the twitchy popover.

Fixes #11186

How has this been tested?

Tested through the included new e2e test.

Manually:

  1. Create a link in a block and trigger the link popover
  2. Click and drag in the popover. This is usually easier when the ellipsis menu has been opened
  3. Verify that the popover remains
  4. Verify that popover functionality remains identical - you can still toggle 'open in new window', edit the link, and press the confirm button
  5. Open the keyboard shortcuts modal
  6. Verify that it continues to function, and that mousedown-drag events doesnt trigger selection of blocks underneath

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
* @param {string} event Event object
*/
stopEventPropagationOutsidePopover( event ) {
event.stopPropagation();

This comment has been minimized.

@youknowriad

youknowriad Nov 12, 2018

Contributor

This seems logical, but also impactful, I'd appreciate some thoughts. @afercia @aduth

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 12, 2018

Is this still in progress? Do you mind rebasing?

@johngodley

This comment has been minimized.

Contributor

johngodley commented Nov 12, 2018

Is this still in progress? Do you mind rebasing?

Will do, and yep still in progress - I need to fix the failing test.

@aduth

This comment has been minimized.

Member

aduth commented Nov 12, 2018

I think we did similar propagation stopping outside modals under the premise they're a sort of independent document? It's not flagrantly offensive to me on glance, though I'd wish we could do so in a way which was wholesale, not picking individual events to which the behavior should apply.

@mtias mtias added this to the 4.4 milestone Nov 12, 2018

johngodley added some commits Nov 9, 2018

Prevent mouse event propagation in popover
This could propagate through to parent handlers (such as mousedown for selection) that removes the popover, causing it to disappear.
Add an isolated event container component
This is a container that prevents certain events from propagating outside of the container
Use IsolatedEventContainter on Popover
This will prevent the mousedown triggering text selection
Modal to use IsolatedEventContainer
This prevents mousedown events selecting blocks under the modal
Add e2e test to test IsolatedEventContainer
Triggers problem in #11186 and ensures link popover remains

@johngodley johngodley force-pushed the fix/link-popover-buttons branch from 895a96e to 22eadbb Nov 14, 2018

modal to be interacted with.
The current isolated events are:
- mousedown - This prevents UI interaction with other `mousedown` event handlers, such as selection

This comment has been minimized.

@youknowriad

youknowriad Nov 14, 2018

Contributor

How are we going to update this component later, will this be a breaking change on each new event handler? Can't we use a events prop instead to avoid making the decision in the component itself and leave it to the caller?

This comment has been minimized.

@johngodley

johngodley Nov 14, 2018

Contributor

I saw the component as internally containing all the events that might leak and cause problems for modal (or modal-ish) components. Passing them into the component moves that knowledge to each caller, and makes it more general purpose than planned.

I get your point about it potentially being a breaking change if a new event is added though. I don't see anything being added that would not be applicable to all users of the component, although that's easy to say without hindsight!

Maybe this is a naming thing. Would something like <IsolatedMouseContainer /> narrow the scope to specifically handle mouse events? Any new non-mouse event would then require a seperate wrapper, along with separate tests.

Happy to change either way.

This comment has been minimized.

@youknowriad

youknowriad Nov 14, 2018

Contributor

Thanks for the clarification, it's fine for me. I don't have strong opinions on naming things. you call really.

This comment has been minimized.

@aduth

aduth Nov 15, 2018

Member

I saw the component as internally containing all the events that might leak and cause problems for modal (or modal-ish) components.

At an abstract level, I like this idea. In practice, I don't see how it could possibly be implemented as described, at least not without (a) maintaining a full set of event handlers upon which to listen, and (b) providing handlers for all those events (for events like mousemove and scroll, it becomes unfeasible).

I wonder if a reasonable compromise would be to put upon the consumer to provide the events for which propagation is stopped?

I could imagine it looking similar to IgnoreNestedEvents:

https://github.com/WordPress/gutenberg/blob/master/packages/editor/src/components/ignore-nested-events/index.js

This comment has been minimized.

@johngodley

johngodley Nov 16, 2018

Contributor

I'm not sure I understand your concerns. The wrapper isn't designed to be a catch-every-handler kind of thing. IgnoredNestedEvents would seem to greatly over-complicate catching onMouseDown.

I accept the possibility that other events may be needed in the future, but I don't think this would need to break the encapsulation. If it does, it can be separated into something that is customised for that circumstance.

Certainly I can change this to be more general like IgnoreNestedEvents if you anticipate it being useful to do so.

This comment has been minimized.

@aduth

aduth Nov 16, 2018

Member

Maybe we're not on the same page as to expectations of what IsolatedEventContainer is meant to address. Could you elaborate on its purpose?

I saw the component as internally containing all the events that might leak and cause problems for modal (or modal-ish) components.

"all the events that might [...] cause problems" is a bit broad. What are those events? Couldn't any event potentially cause some problem?

This comment has been minimized.

@johngodley

johngodley Nov 16, 2018

Contributor

That's very probable! The aim was to solve the specific instance of mousedown leaking out of modalized components in a more general way than adding the event handlers to each component.

Technically yes, other events could cause problems. Rather than cause unintended side-effects with a block-everything approach I thought it best to focus only on the known and tested event.

internally containing all the events that might leak

This was only meant in the sense that yes, any event could cause a problem, somewhere and somehow, not that I was aware of them.

As I wondered above, maybe this could have been improved with more specific naming (and a clarified readme!) so it's clear that the component is only for this one case, and not a general purpose propagation blocker.

Also, after now being aware of IgnoreNestedEvents I will check the places it is used and see if the same events could cause problems with IsolatedEventContainer

This comment has been minimized.

@aduth

aduth Nov 16, 2018

Member

It was in my mind that rather than trying to plug holes as they come to surface (mousedown today, keydown tomorrow), if we could consider a modal to be a separate document / sandbox from the one in which its contained, it would be perfectly reasonable to stop all events from escaping. This is where it comes to be a "catch-every-handler kind of thing", leading to the practical concerns outlined in my first comment.†

Ignoring that, and accepting the fact we have one-off handlers, it still seems like something which is unclear when contained within a component. In using IsolatedEventContainer, how am I as a developer to know what's being isolated? Having this explicit by a prop events could make this much clearer and provide an opportunity for the developer to decide what it should handle, if they have some insight into anticipating the issues.

Conversely, there's a good chance they don't, and would benefit from some default set of handlers, closer to what's implemented now.


† As I write this, I think in my head "it's like an iframe, nothing escapes!" which then led to a crazy idea. What if it is an iframe? 😄 In fact, we have a good amount of the work done already with the Sandbox component. I expect this might not be very feasible though, at least in trying to respect styling and allowing interactions (data operations) to occur between the iframe and the surrounding page.

@youknowriad

Left a comment on the component's props but looks good otherwise.

@johngodley johngodley merged commit 2e6f051 into master Nov 15, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@johngodley johngodley deleted the fix/link-popover-buttons branch Nov 15, 2018

@johngodley johngodley referenced this pull request Nov 15, 2018

Merged

Format toolbar: prevent mousedown triggering selection #11902

0 of 4 tasks complete
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 15, 2018

The links e2e test is very unstable in Travis today (it often fails, sometimes it passes) but it was more stable yesterday.. I don't have proof as I can't reproduce locally but I wonder if it's because of this PR.

@johngodley

This comment has been minimized.

Contributor

johngodley commented Nov 15, 2018

Is it worth disabling and/or removing? I can move the test onto the modal if it's tipping the balance here

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 15, 2018

I'm not certain it's the new test actually, I think it affects the existing tests too. We should just do some tests to see if we can find the real origin of these failures.

}
// Focus on first paragraph, so the link popover will appear over the subsequent ones
await page.mouse.click( 120, 280 );

This comment has been minimized.

@youknowriad

youknowriad Nov 15, 2018

Contributor

This test assumes things are in given position, I think it would have been better to rely on the actual position of things (See boundingBox)

This comment has been minimized.

@aduth

aduth Nov 15, 2018

Member

Or, since it may be complex, alternative means to navigate to the intended block:

  • Shift-Tab?
  • Arrow Up?
  • Block navigator?
}
// Focus on first paragraph, so the link popover will appear over the subsequent ones
await page.mouse.click( 120, 280 );

This comment has been minimized.

@aduth

aduth Nov 15, 2018

Member

Or, since it may be complex, alternative means to navigate to the intended block:

  • Shift-Tab?
  • Arrow Up?
  • Block navigator?
await page.click( 'button[aria-label="Link Settings"]' );
// Move mouse over the 'open in new tab' section, then click and drag
await page.mouse.move( 50, 330 );

This comment has been minimized.

@aduth

aduth Nov 15, 2018

Member

This as well is incredibly fragile, and will most certainly break in the future with simple stylistic updates.

This comment has been minimized.

@johngodley

johngodley Nov 16, 2018

Contributor

Ok, understood. I copied existing tests where absolute positions are used, and assumed this was the accepted way.

@johngodley johngodley referenced this pull request Nov 16, 2018

Merged

Remove absolute positions in link popover e2e test #11968

0 of 4 tasks complete

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

Link popover: prevent mouse event propagation (WordPress#11753)
* Prevent mouse event propagation in popover

This could propagate through to parent handlers (such as mousedown for selection) that removes the popover, causing it to disappear.

* Add e2e test to test IsolatedEventContainer

Triggers problem in WordPress#11186 and ensures link popover remains

* Update doc manifest with IsolatedEventContainer
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment