Closed Bug 1300203 Opened 3 years ago Closed 3 years ago

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

Categories

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

51 Branch
x86_64
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
e10s - ---
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- fixed

People

(Reporter: Abe_LV, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted][nightly only])

Attachments

(2 files)

Attached image 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
tracking-e10s: --- → ?
Component: Tabbed Browser → Panning and Zooming
Product: Firefox → Core
Priority: -- → P3
Whiteboard: [gfx-noted][nightly only]
I'll look at this soonish.
Assignee: nobody → bugmail
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
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
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 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
Attachment #8795762 - Flags: review?(jmathies) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5049535533e
Ensure that sLastMouseMovePoint is updated even for touch-induced WM_MOUSEMOVE messages. r=jimm
https://hg.mozilla.org/mozilla-central/rev/e5049535533e
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.