1300203 - [e10s] Tool-tip of the page title covers the context menu on touch enabled laptop

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect, P3)

Description

[Abe - QA (:Abe_LV)]

Attachment: tooltipCoversMenu.png

[Pre-Requisites] 
touch screen laptop/surface pro
Use Nightly channel

about:config settings:
javascript.options.asyncstack; false
browser.tabs.remote.force-enable; true
layers.async-pan-zoom.enabled; true 

about:support shows
Multiprocess Windows 1/1 (Enabled by user)
about:support Asynchronous Pan/Zoom wheel input enabled; touch input enabled

[Steps to Reproduce]
1.Navigate to the above url (or any webpage having title can be used)
2.Using your finger press and hold on the tab until the context menu pops up. 

[Actual Results]
The part of the context menu is covered by title tooltip.  

[Expected Results]
Context menu should be displayed clearly.

[Note]
This only happens when e10s is enabled.

--
Version  51.0a1
Build ID  20160902030222
Update Channel  nightly
User Agent  Mozilla/5.0 (Windows NT 10.0; WOW64; rv:51.0) Gecko/20100101 Firefox/51.0

Comment 1

[Kartikaya Gupta (email:[e-mail])]

I'll look at this soonish.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

This is actually really annoying - it looks like Windows is moving the mouse cursor after the user does a long press, and so we fire a mousemove event after the contextmenu event, and that triggers the tooltip. The sequence of events is like so:

- touchstart
- touchend
- contextmenu
- mousemove

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Seems like the mousemove event is actually triggered by the appearance of the contextmenu popup. On pages where I preventDefault the contextmenu event or where it just does text selection, that mousemove event never appears. This "new widget" mousemove event is supposed to be prevented by the code at [1] but it doesn't because the previous mousemove is thrown away earlier as it is touch-induced. Basically, if we update sLastMouseMovePoint even for touch-induced mousemove events, then this bug goes away because that last mousemove event is discarded.

[1] http://searchfox.org/mozilla-central/rev/5bdd2876aeb9dc97dc619ab3e067e86083dad023/widget/windows/nsWindow.cpp#4083

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1300203 - Ensure that sLastMouseMovePoint is updated even for touch-induced WM_MOUSEMOVE messages.

Review commit: https://reviewboard.mozilla.org/r/81708/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/81708/

Comment 5

[Kartikaya Gupta (email:[e-mail])]

To clarify a bit better, the sequence of events before this patch was like so:

WM_TOUCH for touchdown
WM_TOUCH for touchup
WM_MOUSEMOVE (touch source)
WM_RBUTTONDOWN (touch source)
WM_RBUTTONUP (touch source)
WM_CONTEXTMENU (touch source)
APZ fires contextmenu
context menu is displayed
WM_MOUSEMOVE (mouse source)

All of the "touch source" events were getting discarded in nsWindow.cpp since we have touch support in APZ enabled. So the first WM_MOUSEMOVE never got around to updating sLastMouseMovePoint, and the second WM_MOUSEMOVE would get dispatched to web content and trigger the tooltip.

With the patch, the first WM_MOUSEMOVE *does* still update sLastMouseMovePoint, and so the second WM_MOUSEMOVE is consumed in the widget code and no tooltip is triggered.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 8795762 [details]
Bug 1300203 - Ensure that sLastMouseMovePoint is updated even for touch-induced WM_MOUSEMOVE messages.

https://reviewboard.mozilla.org/r/81708/#review80760

::: widget/windows/nsWindow.cpp:4021
(Diff revision 1)
> +  LayoutDeviceIntPoint mpScreen = eventPoint + WidgetToScreenOffset();
> +
> +  // Suppress mouse moves caused by widget creation. Make sure to do this early
> +  // so that we update sLastMouseMovePoint even for touch-induced mousemove events.
> +  if (aEventMessage == eMouseMove) {
> +    if ((sLastMouseMovePoint.x == mpScreen.x) && (sLastMouseMovePoint.y == mpScreen.y))

nit - brace this if

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5049535533e
Ensure that sLastMouseMovePoint is updated even for touch-induced WM_MOUSEMOVE messages. r=jimm

Comment 8

[Carsten Book [:Tomcat]]

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