1653949 - mouseout/mouseleave event doesn't work properly on the root element in Fission oop iframe

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect, P2)

Description

[Edgar Chen [:edgar]]

Attachment: test.patch

Found this in bug 1594989.

Comment 1

[Edgar Chen [:edgar]]

Attachment: Bug 1653949 - Test;

Comment 2

[Edgar Chen [:edgar]]

It happens when mouse moves from inner OOP frame to the outer OOP frame, we currently dispatch eMouseExitFromWidget event with mExitFrom = WidgetMouseEvent::eChild, we treat it as synthesized mouse, not a really exit, so we think the mouse is still over the root element of the inner OOP frame. What we should do is dispatching a really exit event while moving from inner OOP frame to the outer OOP frame, for example, mExitFrom = WidgetMouseEvent::eTopLevel, but the naming is a bit confused, I plan to introduce another type, something like eProcess, and make eTopLevel is only used in parent process for top-level widget exit.

And we would also need a mechanism to detect the 'direction' of movement, for example, inner to outer, outer to inner, or even for more complicated case: from one leaf to another leaf. And I think we could also fix bug 1646370 here.

Comment 3

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] (PTO from 07/25 to 08/02) from comment #2)

It happens when mouse moves from inner OOP frame to the outer OOP frame, we currently dispatch eMouseExitFromWidget event with mExitFrom = WidgetMouseEvent::eChild, we treat it as synthesized mouse, not a really exit, so we think the mouse is still over the root element of the inner OOP frame. What we should do is dispatching a really exit event while moving from inner OOP frame to the outer OOP frame, for example, mExitFrom = WidgetMouseEvent::eTopLevel, but the naming is a bit confused, I plan to introduce another type, something like eProcess, and make eTopLevel is only used in parent process for top-level widget exit.

And we would also need a mechanism to detect the 'direction' of movement, for example, inner to outer, outer to inner, or even for more complicated case: from one leaf to another leaf. And I think we could also fix bug 1646370 here.

Comment 4

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=f4d9a93beba74071f67286807e06e4302d4dc50d

Comment 5

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=62b57f000c274dde3f02ec404327c5e1aa2f1312

Got an intermittent time out on new added test in test verify mode.

Comment 6

[Neha Kochar [:neha]]

This is high priority for us to get fixed in M6b.

Comment 7

[Edgar Chen [:edgar]]

Attachment: Bug 1653949 - Part 2: Add ePuppet to WidgetMouseEvent::ExitFrom;

mExitFrom now contains a value only when mMessage is eMouseExitFromWidget.

• eTopLevelWidget is used only in parent process indicates if the mouse cursor
  exits from a top level widget.
• eProcessContent is used only in child process indicates if the mouse cursor
  exits from the process content rendering area.

And if mExitFrom is Nothing and mMessage is eMouseExitFromWidget, it indicates
that the mouse cursor traverses widget/process boundaries but still in
top-level widget or within the process content area.

Comment 8

[Edgar Chen [:edgar]]

Attachment: Bug 1653949 - Part 3: Dispatch MouseExitFromWidget event with proper ExitFrom type based on the "direction" of movement;

This patch also considers the nested OOP iframe case and could also fix bug 1646370.

Depends on D84748

Comment 9

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b0d29ed6d6d1258b1841bb880f8793d833c0eaad

Comment 10

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8208d543375cc745bd2522be5bd6558614ecec51

Comment 11

[Edgar Chen [:edgar]]

I spend some time to debug the test-verify timeout on the new added test, the timeout test is to test the mouseleave event would be fired properly while switching to a new tab/window, the test would do:

• call window.open() to open a new window and switch focus to it
• wait for mouseleave event.

However, window.open() will spin event loop and suppress event handling, the eMouseExitFromWidget event might be discarded if it happens during the spin event loop. We could fix this by adding eMouseExitFromWidget to delayed events. But I still got a timeout with a different case...

Comment 12

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #11)

But I still got a timeout with a different case...

Another case is somehow the mLastOverFrame is nullptr, so we don't try to notify mouseout on subframe.
I could only reproduce on try, but not locally.

Comment 13

[Edgar Chen [:edgar]]

Attachment: Bug 1653949 - Part 4: Add MouseExitFromWidget event to the delayed event queue if event handling is suppressed;

Comment 14

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #12)

Another case is somehow the mLastOverFrame is nullptr, so we don't try to notify mouseout on subframe.
I could only reproduce on try, but not locally.

The mLastOverFrame is null-ed out because of the focus call on the test iframe while starting the test, it would possibly trigger the pending restyled and clear the WeakFrame, like

task 2020-08-15T23:04:17.535Z] 23:04:17 INFO - GECKO(2453) | #01: mozilla::PresShell::ClearFrameRefs(nsIFrame*) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x46b3148]
[task 2020-08-15T23:04:17.543Z] 23:04:17 INFO - GECKO(2453) | #02: nsIFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x4875faf]
[task 2020-08-15T23:04:17.544Z] 23:04:17 INFO - GECKO(2453) | #03: nsSubDocumentFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x4901d9c]
[task 2020-08-15T23:04:17.544Z] 23:04:17 INFO - GECKO(2453) | #04: nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsIFrame*, nsFrameList*, mozilla::layout::PostFrameDestroyData&) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x48de139]
[task 2020-08-15T23:04:17.544Z] 23:04:17 INFO - GECKO(2453) | #05: nsBlockFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x47b8f28]
[task 2020-08-15T23:04:17.547Z] 23:04:17 INFO - GECKO(2453) | #06: nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsIFrame*, nsFrameList*, mozilla::layout::PostFrameDestroyData&) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x48de139]
[task 2020-08-15T23:04:17.548Z] 23:04:17 INFO - GECKO(2453) | #07: nsBlockFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x47b8f28]
[task 2020-08-15T23:04:17.558Z] 23:04:17 INFO - GECKO(2453) | #08: nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsIFrame*, nsFrameList*, mozilla::layout::PostFrameDestroyData&) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x48de139]
[task 2020-08-15T23:04:17.558Z] 23:04:17 INFO - GECKO(2453) | #09: nsBlockFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x47b8f28]
[task 2020-08-15T23:04:17.558Z] 23:04:17 INFO - GECKO(2453) | #10: nsBlockFrame::DoRemoveFrameInternal(nsIFrame*, unsigned int, mozilla::layout::PostFrameDestroyData&) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x47d64f1]
[task 2020-08-15T23:04:17.565Z] 23:04:17 INFO - GECKO(2453) | #11: nsBlockFrame::RemoveFrame(mozilla::layout::FrameChildListID, nsIFrame*) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x47d4a24]
[task 2020-08-15T23:04:17.565Z] 23:04:17 INFO - GECKO(2453) | #12: nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x4717eff]
[task 2020-08-15T23:04:17.565Z] 23:04:17 INFO - GECKO(2453) | #13: nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x470f125]
[task 2020-08-15T23:04:17.572Z] 23:04:17 INFO - GECKO(2453) | #14: mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x46d8e95]
[task 2020-08-15T23:04:17.573Z] 23:04:17 INFO - GECKO(2453) | #15: mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x46dff9a]
[task 2020-08-15T23:04:17.573Z] 23:04:17 INFO - GECKO(2453) | #16: mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x46b8037]
[task 2020-08-15T23:04:17.577Z] 23:04:17 INFO - GECKO(2453) | #17: mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x1b16f5c]
[task 2020-08-15T23:04:17.578Z] 23:04:17 INFO - GECKO(2453) | #18: nsFocusManager::FlushAndCheckIfFocusable(mozilla::dom::Element*, unsigned int) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x1c71d33]
[task 2020-08-15T23:04:17.578Z] 23:04:17 INFO - GECKO(2453) | #19: nsFocusManager::SetFocusInner(mozilla::dom::Element*, int, bool, bool) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x1c6f6be]
[task 2020-08-15T23:04:17.587Z] 23:04:17 INFO - GECKO(2453) | #20: nsFocusManager::SetFocus(mozilla::dom::Element*, unsigned int) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x1c71b4a]
[task 2020-08-15T23:04:17.587Z] 23:04:17 INFO - GECKO(2453) | #21: mozilla::dom::Element::Focus(mozilla::dom::FocusOptions const&, mozilla::dom::CallerType, mozilla::ErrorResult&) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x1b3d5e3]
[task 2020-08-15T23:04:17.587Z] 23:04:17 INFO - GECKO(2453) | #22: mozilla::dom::HTMLElement_Binding::focus(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&) [/Users/cltbld/tasks/task_1597525354/build/application/Firefox NightlyDebug.app/Contents/MacOS/XUL + 0x2de059b]

This seems a timing issue and the try run has a higher chance to hit it. One possible solution is to introduce a way to clear synthesized mouse status after finishing the test.

Comment 15

[Edgar Chen [:edgar]]

Attachment: Bug 1653949 - Part 5: Add tests;

Depends on D86953

Comment 16

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #14)

One possible solution is to introduce a way to clear synthesized mouse status after finishing the test.

Unfortunately, I could not make it work successfully.
I did some experiments on Mac, the code in cocoa widget doesn't support synthesizing mouse exit event, and I also tried to do a quick hack, but no luck to make it work. And on Windows, there seems no Exit event that we can simulate, https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-mouse_event. I didn't try Linux.

I decide to skip test-verify for now and bug 1659940 is filed as a follow-up.

Comment 18

[Jens Stutte [:jstutte]]

(In reply to Edgar Chen [:edgar] from comment #16)

(In reply to Edgar Chen [:edgar] from comment #14)

One possible solution is to introduce a way to clear synthesized mouse status after finishing the test.

Unfortunately, I could not make it work successfully.
I did some experiments on Mac, the code in cocoa widget doesn't support synthesizing mouse exit event, and I also tried to do a quick hack, but no luck to make it work. And on Windows, there seems no Exit event that we can simulate, https://docs.microsoft.com/en-us/windows/win32/api/winuser/nf-winuser-mouse_event. I didn't try Linux.

I decide to skip test-verify for now and bug 1659940 is filed as a follow-up.

Just to be clear: this is "just" a problem with the tests on Mac and Windows, not with the patch itself?

Comment 19

[Edgar Chen [:edgar]]

(In reply to Jens Stutte [:jstutte] (REO for FF 81) from comment #18)

Just to be clear: this is "just" a problem with the tests on Mac and Windows, not with the patch itself?

This is a potential issue on the long-existing code (not related to the patch), the newly added test hit it on try (did not see this locally).

Comment 20

[Jens Stutte [:jstutte]]

(In reply to Edgar Chen [:edgar] from comment #19)

(In reply to Jens Stutte [:jstutte] (REO for FF 81) from comment #18)

Just to be clear: this is "just" a problem with the tests on Mac and Windows, not with the patch itself?

This is a potential issue on the long-existing code (not related to the patch), the newly added test hit it on try (did not see this locally).

I see, thanks. Should we file a different bug for it then?

Comment 21

[Edgar Chen [:edgar]]

(In reply to Jens Stutte [:jstutte] (REO for FF 81) from comment #20)

I see, thanks. Should we file a different bug for it then?

Yes, had filed bug 1659940 for that.

Comment 22

[Edgar Chen [:edgar]]

Attachment: Bug 1653949 - Part 1: Change mExitFrom in WidgetMouseEvent to be Maybe<ExitFrom>;

mExitFrom now contains a value only when mMessage is eMouseExitFromWidget

Comment 23

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=1183a612c34f27be19cdf42ea7a1fa0802644348

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/390dd2c04cd4
Part 1: Change mExitFrom in WidgetMouseEvent to be Maybe<ExitFrom>; r=smaug
https://hg.mozilla.org/integration/autoland/rev/a87ac2a7db70
Part 2: Add ePuppet to WidgetMouseEvent::ExitFrom; r=smaug
https://hg.mozilla.org/integration/autoland/rev/03e65cbd2a11
Part 3: Dispatch MouseExitFromWidget event with proper ExitFrom type based on the "direction" of movement; r=smaug
https://hg.mozilla.org/integration/autoland/rev/31cb90ef998a
Part 4: Add MouseExitFromWidget event to the delayed event queue if event handling is suppressed; r=smaug
https://hg.mozilla.org/integration/autoland/rev/ad7c35ab2a40
Part 5: Add tests; r=smaug

Comment 25

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/230586563fe7
Backed out 5 changesets for assertion failures on EventStateManager.cpp. CLOSED TREE

Comment 26

[Cosmin Sabou [:CosminS]]

Backed out for assertion failures on EventStateManager.cpp.

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&revision=ad7c35ab2a4097cf1099e7a0cd64c1ce0c99b88b&selectedTaskRun=FqZXOtJdRF2dNpLdsVEeVg.0

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=314199358&repo=autoland

Backout link: https://hg.mozilla.org/integration/autoland/rev/230586563fe7

Comment 27

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=6c517c85dcdf74b8977170cea44df604677b42ff

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aadba4e6432e
Part 1: Change mExitFrom in WidgetMouseEvent to be Maybe<ExitFrom>; r=smaug
https://hg.mozilla.org/integration/autoland/rev/0621a2181232
Part 2: Add ePuppet to WidgetMouseEvent::ExitFrom; r=smaug
https://hg.mozilla.org/integration/autoland/rev/a5ea6e6a60b5
Part 3: Dispatch MouseExitFromWidget event with proper ExitFrom type based on the "direction" of movement; r=smaug
https://hg.mozilla.org/integration/autoland/rev/5f7c12fa235a
Part 4: Add MouseExitFromWidget event to the delayed event queue if event handling is suppressed; r=smaug
https://hg.mozilla.org/integration/autoland/rev/8053557b9f59
Part 5: Add tests; r=smaug

Comment 29

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/aadba4e6432e
https://hg.mozilla.org/mozilla-central/rev/0621a2181232
https://hg.mozilla.org/mozilla-central/rev/a5ea6e6a60b5
https://hg.mozilla.org/mozilla-central/rev/5f7c12fa235a
https://hg.mozilla.org/mozilla-central/rev/8053557b9f59