1003934 - [e10s] Tooltips persist when moving mouse out of content through edge overlapping element

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Steven MacLeod [:smacleod]]

To reproduce:

a) Open an e10s window
b) Load a page where a large element with a tooltip can be scrolled so it is cut off at edge of the content (I used the image at http://www.exocomics.com/325)
c) Move the mouse over the element with a tooltip, and then out of the content area (the mouse must leave the content area where the element meets the edge)
d) A tooltip will be displayed and will persist even through operations such as switching tabs.

I was able to reproduce this on OS X and mconley reproduced on Windows 7.

Comment 1

[Steven MacLeod [:smacleod]]

The underlying problem here might also relate to Bug 1003943

Comment 2

[Jim Mathies [:jimm]]

(In reply to Steven MacLeod [:smacleod] from comment #0)
> To reproduce:
> 
> a) Open an e10s window
> b) Load a page where a large element with a tooltip can be scrolled so it is
> cut off at edge of the content (I used the image at
> http://www.exocomics.com/325)
> c) Move the mouse over the element with a tooltip, and then out of the
> content area (the mouse must leave the content area where the element meets
> the edge)
> d) A tooltip will be displayed and will persist even through operations such
> as switching tabs.
> 
> I was able to reproduce this on OS X and mconley reproduced on Windows 7.

I'm not having any luck reproducing this on the test case cited here. Steve, are you still seeing this?

Comment 3

[Steven MacLeod [:smacleod]]

(In reply to Jim Mathies [:jimm] from comment #2)
> (In reply to Steven MacLeod [:smacleod] from comment #0)
> > To reproduce:
> > 
> > a) Open an e10s window
> > b) Load a page where a large element with a tooltip can be scrolled so it is
> > cut off at edge of the content (I used the image at
> > http://www.exocomics.com/325)
> > c) Move the mouse over the element with a tooltip, and then out of the
> > content area (the mouse must leave the content area where the element meets
> > the edge)
> > d) A tooltip will be displayed and will persist even through operations such
> > as switching tabs.
> > 
> > I was able to reproduce this on OS X and mconley reproduced on Windows 7.
> 
> I'm not having any luck reproducing this on the test case cited here. Steve,
> are you still seeing this?

Mike and I can reproduce on latest nightlies. He's going to post a short video demoing how to reproduce.

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Here we go:

http://www.screencast.com/t/xShwrym7

Comment 5

[Jim Mathies [:jimm]]

(In reply to Mike Conley (:mconley) from comment #4)
> Here we go:
> 
> http://www.screencast.com/t/xShwrym7

Thanks, that helped. I can confirm still present on win as well.

Comment 6

[Jim Mathies [:jimm]]

I'm able to reproduce this by hovering over the bookmarks toolbar, implying mouse input is getting sent to the content process despite chrome being under the mouse. This seems like a larger issue than just hiding a tooltip at the right time.

Comment 7

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #6)
> I'm able to reproduce this by hovering over the bookmarks toolbar, implying
> mouse input is getting sent to the content process despite chrome being
> under the mouse. This seems like a larger issue than just hiding a tooltip
> at the right time.

Actually this isn't as bad as I thought.. if you drag up from the image, you get the delayed tooltip, but if you drag up from outside the image and over you don't.

Comment 8

[Jim Mathies [:jimm]]

Attachment: testcase image

Comment 9

[Jim Mathies [:jimm]]

Attachment: image tooltip testcase

Comment 10

[Jim Mathies [:jimm]]

Attachment: wip patch

Comment 11

[Jim Mathies [:jimm]]

Attachment: forward NS_MOUSE_EXIT_SYNTH to content patch v.1

Once this forwards over, it triggers a mouseout event for the content frame, which shuts down the pending tooltip display event in ChromeTooltipListener located in nsDocShellTreeOwner.cpp.

Good STR to reproduce:

1) opent etest case able
2) scroll the image up so the top of the image is under the bookmarks toolbar.
3) move the mouse from the image over empty chrome in the toolbar.
4) hold the mouse there for a sec.

resut: the image tooltip will display shortly over chrome.
with this patch: the tooltip doesn't display.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

patch generated with -P would be easier to review.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8426219 [details] [diff] [review]
forward NS_MOUSE_EXIT_SYNTH to content patch v.1

I don't understand this setup.
Why would we dispatch EXIT_SYNTH but not ENTER_SYNTH?

But I agree we should dispatch an event to the child process.
When the iframe in the parent gets EXIT_SYNTH, child should probably get
NS_MOUSE_EXIT. That way child would end up creating right kinds of events.

Comment 14

[Jim Mathies [:jimm]]

(In reply to Olli Pettay [:smaug] from comment #13)
> Comment on attachment 8426219 [details] [diff] [review]
> forward NS_MOUSE_EXIT_SYNTH to content patch v.1
> 
> I don't understand this setup.
> Why would we dispatch EXIT_SYNTH but not ENTER_SYNTH?

enter events are generated by the child process in mouse move handling their generation is self contained.

http://mxr.mozilla.org/mozilla-central/source/dom/events/EventStateManager.cpp#3750

> 
> But I agree we should dispatch an event to the child process.
> When the iframe in the parent gets EXIT_SYNTH, child should probably get
> NS_MOUSE_EXIT. That way child would end up creating right kinds of events.

I'm not sure what you're getting at here.. NS_MOUSE_EXIT_SYNTH generates the proper dom events in the child.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

local (running in the current process) EventStateManager should generate NS_MOUSE_EXIT_SYNTH events.
widget level code deals with non-synth.

Comment 16

[Jim Mathies [:jimm]]

Attachment: mouse event tracking testcase

Comment 17

[Jim Mathies [:jimm]]

Attachment: forward NS_MOUSE_EXIT to remote content patch v.2

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8426395 [details] [diff] [review]
forward NS_MOUSE_EXIT to remote content patch v.2

We need to still dispatch mouseout (NS_MOUSE_EXIT_SYNTH)  to the xul:browser, and then also dispatch NS_MOUSE_EXIT to the child process.

Comment 19

[Jim Mathies [:jimm]]

Attachment: wip

Comment 20

[Jim Mathies [:jimm]]

Attachment: patch v.3

try push:
https://tbpl.mozilla.org/?tree=Try&rev=b91d86574c33

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8426954 [details] [diff] [review]
patch v.3

>+  mCurrentTargetContent = nullptr;
>+
>+  if (!aTargetContent || !aMouseEvent) {
Why null check for aMouseEvent. The old code doesn't seem to have it.

>+  if (!mPresContext) {
>+    mCurrentTargetContent = nullptr;
>+    mCurrentTarget = previousTarget;
>+    return nullptr;
>+  }
I would just keep the following code inside
if (mPresContext) so that we don't need to have
>+    mCurrentTargetContent = nullptr;
>+    mCurrentTarget = previousTarget;
twice.

>+  // If we are leaving remote content, dispatch a mouse exit event to the
>+  // remote frame.
>+  if (IsRemoteTarget(aTargetContent)) {
>+    // For remote content, send a normal widget mouse exit event.
>+    uint32_t message = aMessage;
>+    if (message == NS_MOUSE_EXIT_SYNTH) {
>+      message = NS_MOUSE_EXIT;
>     }
So you end up sending all the mouse events to child, not only mouse_exit.
if (message == NS_MOUSE_EXIT_SYNTH && IsRemoteTarget(aTargetContent)) {
  message = NS_MOUSE_EXIT;
  ...

Comment 22

[Jim Mathies [:jimm]]

(In reply to Olli Pettay [:smaug] from comment #21)
> >+  // If we are leaving remote content, dispatch a mouse exit event to the
> >+  // remote frame.
> >+  if (IsRemoteTarget(aTargetContent)) {
> >+    // For remote content, send a normal widget mouse exit event.
> >+    uint32_t message = aMessage;
> >+    if (message == NS_MOUSE_EXIT_SYNTH) {
> >+      message = NS_MOUSE_EXIT;
> >     }
> So you end up sending all the mouse events to child, not only mouse_exit.
> if (message == NS_MOUSE_EXIT_SYNTH && IsRemoteTarget(aTargetContent)) {
>   message = NS_MOUSE_EXIT;
>   ...

Well, we would only send what DispatchMouseOrPointerEvent sends and what CrossProcessSafeEvent allows, which is only NS_MOUSE_EXIT currently. But I can go ahead and update per your comment.

Comment 23

[Jim Mathies [:jimm]]

(In reply to Olli Pettay [:smaug] from comment #21)
> Comment on attachment 8426954 [details] [diff] [review]
> patch v.3
> 
> >+  mCurrentTargetContent = nullptr;
> >+
> >+  if (!aTargetContent || !aMouseEvent) {
> Why null check for aMouseEvent. The old code doesn't seem to have it.

Seemed like a safe thing to do, we dref aMouseEvent in CreateMouseOrPointerWidgetEvent. Are you sure you want to take this out?

Comment 24

[Jim Mathies [:jimm]]

Attachment: patch v.4

- null event pointer checks removed
- code wrapped in if (mPresContext) {..}
- special cased NS_MOUSE_EXIT_SYNTH

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8427166 [details] [diff] [review]
patch v.4

> EventStateManager::DispatchMouseOrPointerEvent(WidgetMouseEvent* aMouseEvent,
>                                                uint32_t aMessage,
>                                                nsIContent* aTargetContent,
>                                                nsIContent* aRelatedContent)
> {
>   // http://dvcs.w3.org/hg/webevents/raw-file/default/mouse-lock.html#methods
>-  // "[When the mouse is locked on an element...e]vents that require the concept
>+  // "[When the mouse is locked on an element...events that require the concept
No need for this, or remove also [

Comment 27

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/fx-team/rev/4073b066636a

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/4073b066636a

Comment 29

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Setting for verification in 34 since there is not enough time to investigate this at the moment.

Comment 30

[:Felipe Gomes (needinfo for replies!)]

Target Milestone is used as the reference field for us to know when a fix first landed, so it's usually only changed if there has been a backout and the fix relanded in a later cycle. Can you use a different strategy for your tracking?

Comment 31

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

(In reply to :Felipe Gomes from comment #30)
> Target Milestone is used as the reference field for us to know when a fix
> first landed, so it's usually only changed if there has been a backout and
> the fix relanded in a later cycle. Can you use a different strategy for your
> tracking?

I thought since e10s is currently tracked in 34 this would be ok (since we also need to verify it in 34). Since it's important for you to not change the Target Milestone in this case, I guess setting the status-firefox34 to fixed would be an option for us... however, since we actually got some help and we have some time at the moment, we'll just verify it in both 32 & 34 and get this out of the way.

Comment 32

[Camelia Badau [:cbadau], Desktop Test Engineering]

I was able to reproduce the initial issue on Nightly 32.0a1 (2014-04-30). 

This is now verified fixed on Firefox 32 Beta 9 (BuildID: 20140822024446) and latest Nightly 34.0a1 (builID: 20140824030210).

Comment 33

[Camelia Badau [:cbadau], Desktop Test Engineering]

I forgot to mention that the issue was verified on Windows 7 32bit, Ubuntu 13.10 32bit and Mac OSX 10.9.4.