1390793 - Horizontal Scrolling in RDM with touch simulation opens context menu

Status: VERIFIED FIXED

(DevTools :: Responsive Design Mode, defect, P1)

Description

[Jens]

Attachment: rdm_horizontal_scroll_open_context_menu.gif.gif

STR:
======
1. Go to https://codepen.io/nachtigall/pen/PKOwdr
2. Change View > Debug Mode to see full page
3. Enable DevTools RDM and its "touch simulation"; or just choose something like Galaxy S5
4. Horizontall scroll

See attached gif animation.


AR:
======
For whatever reasons, it opens the right-mouse-click context menu

ER:
======
I see several problem here:
1. Of course, it should not open the context menu
2. The view is not the same as if opened in native Galaxy S5 (or other device): In S5 the page content is zoomed out so that the full page is initially visible.
3. The touch event simulation is broken: I cannot long-left-click-and-drag-to-right/left for scrolling, but have to use the hardly-visible scroll bars (here Chrome is better)

Comment 1

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Thanks for filing!  I agree that's unfortunate behavior, so hopefully we can avoid triggering the menu like that.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Jens from comment #0)
> 2. The view is not the same as if opened in native Galaxy S5 (or other
> device): In S5 the page content is zoomed out so that the full page is
> initially visible.

This is most likely about <meta viewport> handling, bug 1290420 should cover this issue.

> 3. The touch event simulation is broken: I cannot
> long-left-click-and-drag-to-right/left for scrolling, but have to use the
> hardly-visible scroll bars (here Chrome is better)

That's a good idea for improvement!  Could you file a new bug about this enhancement (and have it block the new "rdm-touch" meta-bug)?

Comment 3

[Jens]

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #2)
> > 3. The touch event simulation is broken: I cannot
> > long-left-click-and-drag-to-right/left for scrolling, but have to use the
> > hardly-visible scroll bars (here Chrome is better)
> 
> That's a good idea for improvement!  Could you file a new bug about this
> enhancement (and have it block the new "rdm-touch" meta-bug)?

Done, see Bug 1405626

Comment 6

[Patrick Brosset <:pbro>]

As far as I know, this is due to this piece of code: https://searchfox.org/mozilla-central/rev/56705678f5fc363be5e0237e1686f619b0d23009/devtools/server/actors/emulation/touch-simulator.js#137

In RDM, we try to honour the ui.click_hold_context_menus.delay preference which defines how long after a click and hold should the context menu open up.
The thing is, this is a feature that should only work when ui.click_hold_context_menus is set.
So, maybe we did this on purpose in RDM when touch simulation is done, because there's no concept of right-click there. Maybe ui.click_hold_context_menus is actually true on Android by default.

In any case, this feature is implemented in platform such that if you move after the first click, the menu does not appear.
It looks like dragging the scrollbar in RDM trips that up.

Comment 7

[Micah [:mtigley] (she/her)]

Unassigning for now until actively working on this.

Comment 8

[Brad Werth [:bradwerth]]

Some analysis: With RDM off and the ui.click_hold_context_menus pref on, it appears that the Platform code consistently sends the contextmenu event even for clicks that originate on scrollbars. EventStateManager::FireContextClick() fires in almost all cases and the only scrollbar checks are for XUL scrollbars. So the event filtering is happening in JavaScript somewhere, and the RDM touch simulation is confusing it. Still looking...

Comment 9

[Brad Werth [:bradwerth]]

Okay, narrowed this down further. While iterating the listeners in EventListenerManager::HandleEventInternal, the case of click-and-hold on the body (without RDM) finds a listener keyed to eContextMenu, and so the menu appears. When that same click-and-hold is done on the scrollbar, there is no listener keyed to eContextMenu. So far, so good. That listener is a JS listener -- though I'm not sure which one.

When doing the same click-and-hold on the scrollbar with RDM, the Platform code that sends the contextmenu is never invoked. So it's all being done in the JS side, probably within touch-simulator.js. So the fix will need to be in touch-simulator.js and could take one of these forms:

1. Filter the events in JS based on the target, similar to how XUL scrollbars are filtered out in the Platform.
2. Attach the listener in such a way that it's on the body but not on the overlay scrollbars.

Since option 2 is closer to how the Platform handles it, that's what I intend to pursue.

Comment 11

[Patrick Brosset <:pbro>]

I can see 2 problems in touch-simulator.js:

• The value of the pref ui.click_hold_context_menus is not read. We should not even start the timer to show the context menu if that pref's value is false here: https://searchfox.org/mozilla-central/rev/0bcdfa99339c0512e3730903835fb6f8ed45e3c4/devtools/server/actors/emulation/touch-simulator.js#188
• And, the other thing is that, assuming the pref is true, and we do want to simulate the click_and_hold behavior, then we should not do it if the mouse moves after the initial mousedown event. We attempt to do this here: https://searchfox.org/mozilla-central/rev/0bcdfa99339c0512e3730903835fb6f8ed45e3c4/devtools/server/actors/emulation/touch-simulator.js#208-216. However that does not seem to work for scrollbars. After the initial mousedown, no mousemove events are emitted if the move happened on a scrollbar. I cannot think of a JS solution for this.

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #11)

• And, the other thing is that, assuming the pref is true, and we do want to simulate the click_and_hold behavior, then we should not do it if the mouse moves after the initial mousedown event. We attempt to do this here: https://searchfox.org/mozilla-central/rev/0bcdfa99339c0512e3730903835fb6f8ed45e3c4/devtools/server/actors/emulation/touch-simulator.js#208-216. However that does not seem to work for scrollbars. After the initial mousedown, no mousemove events are emitted if the move happened on a scrollbar. I cannot think of a JS solution for this.

If the initial mousedown event happens on a scrollbar (which we presumably can detect) then we could just bail out of the click_and_hold behavior.

Comment 13

[Patrick Brosset <:pbro>]

Attachment: Bug 1390793 - Fix the click and hold feature simulation in RDM

Comment 14

[Patrick Brosset <:pbro>]

I'm having trouble creating a test for this. The basic idea is simulating a single mousedown event, waiting for the ui.click_hold_context_menus.delay delay and then asserting whether the context menu appeared or not (various cases to test here).
However I'm just not able to figure out a way to make the menu appear after a mousedown event in the RDM test I started adding.
So for now I'll land the first patch (the fix itself) and later work on a test (or let someone more knowledgeable than me do it).

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a4d505edbc11
Fix the click and hold feature simulation in RDM r=mtigley

Comment 16

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/a4d505edbc11

Comment 17

[Patrick Brosset <:pbro>]

After spending a bit more time on this, I'm nowhere closer to getting that test working.
So for the sake of fixing this issue for people affected by it, let's mark this as fixed and let the bug be closed.

I have filed bug 1570579 for working on the test.

Comment 18

[Negritas Sergiu, Desktop QA]

Following the STR from the first comment, I managed to reproduce the issue on nightly 70.0a1 (2019-07-22).
I can confirm that the issue is fixed in 70.0b6.