854075 - WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file content/events/src/nsEventListenerManager.cpp, line 337

Status: NEW

(Firefox :: General, defect)

Description

[Richard Newman [:rnewman]]

Logspew. Current m-c.

Comment 1

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

As the warning says, mouseenter/leave events shouldn't be used in chrome. This is chrome JS bug.

Comment 2

[Justin Dolske [:Dolske]]

Hmm, I see the following 3 uses from a search for mouseenter:

* accessible/src/jsat/TouchAdapter.jsm
  I don't know what that is or does. :/

* browser/base/content/tabbrowser.xml
  Listeners on tabs and the new-tab button. Bug 585558 just added this.

* base/content/newtab/sites.js
  New tab page.

Bug 432698 comment 58 explains a bit about the perf issue. I'm a little sad that we have this warning, since I generally want our chrome to converge on what the web is using.

I'm also curious how the depth of our chrome DOM compares to that of modern (complex) web pages, and if this is something that should be improved for both content and chrome.

Comment 3

[Justin Dolske [:Dolske]]

Attachment: DOM walker for Scratchpad

So, I was curious. Here's a little DOM tree walker for the devtools scratchpad, run with Command-L. (To show chrome you'll need devtools.chrome.enabled, then Environment -> Chrome in the scratchpad).

In my browser that's been running for a while, I get "document has 8439 nodes, max depth is 27". With a fresh start in a new profile, I get "document has 1411 nodes, max depth is 19". Although, somewhat terrifyingly, upon running a few more times without doing anything else it quickly balloons up to "document has 2647 nodes, max depth is 27". Not sure if that's a bug or if something is actually suddenly creating a ton of nodes. :/

In comparison, my Zimbra tab reports "document has 3309 nodes, max depth is 34", and this attachment submission page reports "document has 1289 nodes, max depth is 20"

Comment 4

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

(In reply to Justin Dolske [:Dolske] from comment #2)
>  I'm a little sad
> that we have this warning, since I generally want our chrome to converge on
> what the web is using.
Well, perf critical web pages should not use mouseenter/leave events.
They are handy events in some cases, but mouseover/out work too.

Comment 5

[Tim Taubert [:ttaubert] (inactive)]

Attachment: use mouseover for newtab page's spec connect

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 728949 [details] [diff] [review]
use mouseover for newtab page's spec connect

Review of attachment 728949 [details] [diff] [review]:
-----------------------------------------------------------------

File separate bugs for the other cases that Justin mentioned or fix them in this same bug?

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Jared Wein [:jaws] (Vacation 3/30 to 4/7) from comment #6)
> File separate bugs for the other cases that Justin mentioned or fix them in
> this same bug?

I think we can fix them here as well.

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

CC'ing some more people that worked on bug 585558.

Comment 10

[Adam]

Attachment: Switch to using mouseover and mouseout for setting positional hover attributes.

Output below is "{event} {target.nodeName} ({originalTarget.nodeName})"

Output of hovering a tab label and leaving using enter and leave:
> mouseenter tab (tab)
> mouseenter tab (xul:stack)
> mouseenter tab (xul:hbox)
> mouseenter tab (xul:label)
> [now hovered]
> mouseleave tab (xul:label)
> mouseleave tab (xul:hbox)
> mouseleave tab (xul:stack)
> mouseleave tab (tab)

Output of hovering a tab label and leaving using over and out:
> mouseover tab (tab)
> mouseout tab (tab)
> mouseover tab (xul:label)
> [now hovered]
> mouseout tab (xul:label)
> mouseover tab (tab)
> mouseout tab (tab)

For the favicon it's 8 events and 10 events, and the close button 8 events and 6 events (enter and leave vs. over and out).  

I believe my reasoning for using enter and leave was to avoid the mix of contrary events and that the depth of the tab subtrees here are limited.  

Maybe it's a moderate win to switch?  Relevant tests pass when using over and out.

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 12

[Dão Gottwald [:dao]]

(In reply to Adam [:hobophobe] from comment #10)
> Output below is "{event} {target.nodeName} ({originalTarget.nodeName})"

> Output of hovering a tab label and leaving using over and out:
> > mouseover tab (tab)
> > mouseout tab (tab)
> > mouseover tab (xul:label)
> > [now hovered]
> > mouseout tab (xul:label)
> > mouseover tab (tab)
> > mouseout tab (tab)

This seems broken, as we're using phase="target", which is supposed to mean that "the handler will only fire if event.originalTarget is the same as the target to which the handler is attached" (https://developer.mozilla.org/en-US/docs/XBL/XBL_1.0_Reference/Event_Handlers).

Comment 13

[Adam]

https://mxr.mozilla.org/mozilla-central/source/content/xbl/src/nsXBLEventHandler.cpp#34 shows it checks if the event phase is 'at target', which is https://mxr.mozilla.org/mozilla-central/source/content/events/src/nsDOMEvent.cpp#400 (in our case, when |currentTarget| matches |target|).  

Expanding on the output above, the currentTarget (as well as the explicitOriginalTarget, but that's not important for the event firing) is the tab in all cases (both sets of over and out, enter and leave).  

So either the code or that doc is wrong.  https://developer.mozilla.org/en-US/docs/DOM/event/Comparison_of_Event_Targets says that the difference between the originalTarget and explicitOriginalTarget is that the latter won't contain anonymous content if a retargeting occurs.  So this is a retargeting issue.

https://developer.mozilla.org/en-US/docs/XBL/XBL_1.0_Reference/Anonymous_Content#Mouseover_and_Mouseout_Events

Quoting:  

> Mouseover and mouseout events are retargeted if the mouse genuinely enters or exits the bound element (in addition to entering or exiting some anonymous content). If, however, the user has simply moved the mouse from one anonymous element to another, without entering or exiting the bound element itself, then the event is stopped.

If that's correct (rather than the code), we should not be receiving these extra retargeted events on over/out at all.  That's again either a bug in the retargeting code (haven't looked it up yet to see where retargeting happens) or the doc.

Comment 14

[Dão Gottwald [:dao]]

bz, could you please shed some light on the questions raised in comment 13?

Comment 15

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

Those questions are very much for me ;)

(Don't even think about explicitOriginalTarget. It shouldn't be used unless really really needed)

So XBL1 documentation doesn't reflect the reality here. We prevent mouseover/out propagation only
when crossing native-anonymous / chrome-only-anonymous boundary.
http://mxr.mozilla.org/mozilla-central/source/content/base/src/FragmentOrElement.cpp#713
XBL used in chrome creates just normal anonymous content.

Comment 16

[Dão Gottwald [:dao]]

Comment on attachment 729902 [details] [diff] [review]
Switch to using mouseover and mouseout for setting positional hover attributes.

So I think we need to not handle the events if orginialTarget != target (and we can drop phase="target"). Does this make sense?

Comment 17

[Adam]

Attachment: Merge with the other mouseover/mouseout handlers

(In reply to Dão Gottwald [:dao] from comment #16)
> So I think we need to not handle the events if orginialTarget != target (and
> we can drop phase="target"). Does this make sense?

I tested along that and similar lines for a bit, only to learn the hard way that we do not want to ignore any of the events.  Counterintuitive, I know.

Bug 432698, comment 58:
> [...]
> But depending on css styling and how fast you move mouse etc. only the deepest element in the
> subtrees may get mouseout/over.

If we ignore those content events, the tests fail and the tabs can have inconsistent attributes by manually moving the mouse fast across them.  But we can't exclude the non-content events, because we may not be over descendants.

There may be a way to ignore the right events at the right times, but the complexity would be worse than responding to all the events.

This patch drops the |phase| attribute and merges the handlers with the other mouseover/mouseout handlers.

Comment 18

[Adam]

Attachment: Ready for checkin

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=c845cb475d98

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0653b625b6bb

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0653b625b6bb

Comment 21

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Even though a11y code still has the event listeners, I don't see the warnings with my debug build. Shouldn't this marked as FIXED and file a new bug for the a11y code?

Comment 22

[Magnus Melin [:mkmelin]]

Still get the warning in debug builds.
Looks like there's a few cases left: https://searchfox.org/mozilla-central/search?q=mouseleave&path=toolkit&case=true&regexp=false