1976773 - Stop using mouseenter and mouseleave in browser-addons.js

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P1)

Description

[Andrew McCreight [:mccr8]]

When running tests in a debug build, a frequent cause of warning messages is "WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!". One test where this occurs frequently is testing/crashtest/final/1419902.html.

I added some logging to determine which places are adding these events, and two of the three most common places are a mouseenter and a mouseleave in chrome://browser/content/browser-addons.js.

I don't know about events, but if this could be changed to mouseover/mouseout that would be good.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1948260

:robwu, since you are the author of the regressor, bug 1948260, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 2

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

If there is at least one mouseenter or mouseleave event listener, EventStateManager starts dispatching them. The performance problem is, the events are fired for all ancestor elements which start (not) containing the cursor. Even though the code becomes complicated, it's better to listen to mouseover and mouseout events and check the target is in a specific target. E.g.,

let mouseIsInContainer = false;
container.addEventListener("mouseover", event => {
  if (mouseIsInContainer) {
    return;
  }
  mouseIsInContainer = true;
  // Do the things which you want to do when `mouseenter`
});
container.addEventListener("mouseout", event => {
  if (!mouseIsInContainer || container.contains(event.relatedTarget)) {
    return;
  }
  mouseIsInContainer = false;
  // Do the things which you want to do when `mouseleave`
});

Although I've not tested this script, I guess this works unless mutation may occurs during dispatching the mouse boundary events.

Comment 3

[Rob Wu [:robwu]]

I'll take a look next week.

The purpose of these events is to know whether the mouse is on the toolbar, in order to know whether it is safe to remove a button. If on the toolbar, removal is delayed until the mouse is off the toolbar. This logic ensures that the items in the toolbar do not shift while the user is hovering over the toolbar.

I'll try mouseout/mouseover. I'm also open to other ideas. E.g. if there is a cheap way to know if the mouse exited an area on the screen, that might also work (there are edge cases such as the user using a shortcut to move the window around, but if that is the trade-off in favor of better performance, it is acceptable).

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1948260

Comment 5

[Rob Wu [:robwu]]

I was considering to uplift the fix to ESR140, but I saw the same error in the logs despite my fix. Turns out that there are other uses of mouseenter/mouseleave: bug 1976772.

Comment 6

[Rob Wu [:robwu]]

Attachment: Bug 1976773 - Remove mousenter/mouseleave listeners from browser-addons.js

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/021a5b44bde4
https://hg.mozilla.org/integration/autoland/rev/1cb4f4cc66d4
Remove mousenter/mouseleave listeners from browser-addons.js r=willdurand

Comment 8

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/1cb4f4cc66d4

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:robwu, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox142 to wontfix.

For more information, please visit BugBot documentation.

Comment 10

[Rob Wu [:robwu]]

Is this serious enough to warrant an uplift to ESR140?

The fix is low-risk, but there is positive potential for (theoretical) improvements in browser UI performance, because the relevant logic is present in each browser window.

Comment 11

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

I think it's safe if we watch regression reports in a couple of releases because the change is not so risky and it does not take so long after creating the ESR branch. So, I'm not sure whether it can be uplift immediately.

Comment 12

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

Well, from the performance point of view, the fix will improve the performance when user moves cursor over the chrome UI, and that saves a lot of CPU time in the browser session.

Comment 13

[Rob Wu [:robwu]]

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #11)

I think it's safe if we watch regression reports in a couple of releases because the change is not so risky and it does not take so long after creating the ESR branch.

From the risk perspective, the patch is low risk. It is covered by unit tests. The feature dependent on the mouse events is a non-standard feature behind an opt-in (the user has to remove the Extensions Button, which is shown by default). In the worst case (if the mouse detection was completely bogus), then the button would hide as soon as the button is eligible for being hidden, instead of only when the mouse is off the toolbar.

Comment 14

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

I don't say don't uplift it immediately. If I were you, I may wait for a couple of releases to request uplift because of not urgent thing. Up to you to consider that because you believe it's completely safe. There are some benefit of this fix even in the ESR branch, so, I don't disagree with uplifting it.

Comment 15

[Rob Wu [:robwu]]

Attachment: Bug 1976773 - Remove mousenter/mouseleave listeners from browser-addons.js

Original Revision: https://phabricator.services.mozilla.com/D258434

Comment 16

[Phabricator Automation]

firefox-esr140 Uplift Approval Request

• User impact if declined: Mouse interaction with browser UI is computationally more expensive than it needs to be.
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: QA NOT needed; but if desired, open a debug build of Firefox and look at the terminal to verify that there is no "WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!" message in the terminal.
• Risk associated with taking this patch: Low
• Explanation of risk level: Changes the implementation of a new optional feature that was landed in 140, to something equivalent but with a smaller performance penalty, and also makes the listener registration conditional on when it is needed. Behavior is covered by unit tests.
• String changes made/needed: None
• Is Android affected?: no

Comment 17

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr140/rev/880c90f69d4e