1979940 - Expand on hover does a lot of unnecessary work

Status: RESOLVED FIXED

(Firefox :: Sidebar, defect, P1)

Description

[Emilio Cobos Álvarez [:emilio]]

Noticed when looking at the review comments of bug 1979548, but is unrelated / reproduces without that.

diff --git a/browser/components/sidebar/browser-sidebar.js b/browser/components/sidebar/browser-sidebar.js
index 15354d48fbeb..7c434d6e81e6 100644
--- a/browser/components/sidebar/browser-sidebar.js
+++ b/browser/components/sidebar/browser-sidebar.js
@@ -1086,6 +1086,8 @@ var SidebarController = {
   },

   async _animateSidebarMain() {
+    dump(`_animateSidebarMain()\n`);
+    dump(new Error().stack + "\n");
     let tabbox = document.getElementById("tabbrowser-tabbox");
     let animatingElements;
     let expandOnHoverEnabled = document.documentElement.hasAttribute(
@@ -2180,6 +2182,8 @@ var SidebarController = {
   },

   onMouseLeave() {
+    dump(`onMouseLeave()\n`);
+    dump(new Error().stack + "\n");
     this.mouseEnterTask.disarm();
     const contentArea = document.getElementById("tabbrowser-tabbox");
     this._box.toggleAttribute("sidebar-launcher-hovered", false);
@@ -2192,6 +2196,8 @@ var SidebarController = {
   },

   onMouseEnter() {
+    dump(`onMouseEnter()\n`);
+    dump(new Error().stack + "\n");
     this.mouseEnterTask = new DeferredTask(
       () => {
         this.debouncedMouseEnter();

It seems that when the launcher is expanded, we try to animate / expand it again once every second roughly? That seems unnecessary if the launcher is already expanded. Looks like addListener will happily callListener(), reentering in the process:

...
_animateSidebarMain()
_animateSidebarMain@chrome://browser/content/sidebar/browser-sidebar.js:1090:10
debouncedMouseEnter@chrome://browser/content/sidebar/browser-sidebar.js:2179:12
onMouseEnter/this.mouseEnterTask<@chrome://browser/content/sidebar/browser-sidebar.js:2203:14
#runTask@resource://gre/modules/DeferredTask.sys.mjs:356:25
#timerCallback/<@resource://gre/modules/DeferredTask.sys.mjs:327:28
#timerCallback@resource://gre/modules/DeferredTask.sys.mjs:346:9
#startTimer/callback/<@resource://gre/modules/DeferredTask.sys.mjs:192:32

onMouseEnter()
onMouseEnter@chrome://browser/content/sidebar/browser-sidebar.js:2200:10
_callListener@chrome://browser/content/browser.js:4881:18
addListener@chrome://browser/content/browser.js:4841:10
_removeHoverStateBlocker@chrome://browser/content/sidebar/browser-sidebar.js:1410:23
_animateSidebarMain@chrome://browser/content/sidebar/browser-sidebar.js:1249:18

_animateSidebarMain()
_animateSidebarMain@chrome://browser/content/sidebar/browser-sidebar.js:1090:10
debouncedMouseEnter@chrome://browser/content/sidebar/browser-sidebar.js:2179:12
onMouseEnter/this.mouseEnterTask<@chrome://browser/content/sidebar/browser-sidebar.js:2203:14
#runTask@resource://gre/modules/DeferredTask.sys.mjs:356:25
#timerCallback/<@resource://gre/modules/DeferredTask.sys.mjs:327:28
#timerCallback@resource://gre/modules/DeferredTask.sys.mjs:346:9
#startTimer/callback/<@resource://gre/modules/DeferredTask.sys.mjs:192:32

onMouseEnter()
onMouseEnter@chrome://browser/content/sidebar/browser-sidebar.js:2200:10
_callListener@chrome://browser/content/browser.js:4881:18
addListener@chrome://browser/content/browser.js:4841:10
_removeHoverStateBlocker@chrome://browser/content/sidebar/browser-sidebar.js:1410:23
_animateSidebarMain@chrome://browser/content/sidebar/browser-sidebar.js:1249:18

_animateSidebarMain()
_animateSidebarMain@chrome://browser/content/sidebar/browser-sidebar.js:1090:10
debouncedMouseEnter@chrome://browser/content/sidebar/browser-sidebar.js:2179:12
onMouseEnter/this.mouseEnterTask<@chrome://browser/content/sidebar/browser-sidebar.js:2203:14
#runTask@resource://gre/modules/DeferredTask.sys.mjs:356:25
#timerCallback/<@resource://gre/modules/DeferredTask.sys.mjs:327:28
#timerCallback@resource://gre/modules/DeferredTask.sys.mjs:346:9
#startTimer/callback/<@resource://gre/modules/DeferredTask.sys.mjs:192:32

onMouseEnter()
onMouseEnter@chrome://browser/content/sidebar/browser-sidebar.js:2200:10
_callListener@chrome://browser/content/browser.js:4881:18
addListener@chrome://browser/content/browser.js:4841:10
_removeHoverStateBlocker@chrome://browser/content/sidebar/browser-sidebar.js:1410:23
_animateSidebarMain@chrome://browser/content/sidebar/browser-sidebar.js:1249:18

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1979940 - Avoid unnecessary work for expand-on-hover if already expanded / collapsed. r=kcochrane,#sidebar-reviewers

Comment 2

[BugBot [:suhaib / :marco]]

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

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/842812fef42c
https://hg.mozilla.org/integration/autoland/rev/12bd21faed15
Avoid unnecessary work for expand-on-hover if already expanded / collapsed. r=kcochrane,sidebar-reviewers

Comment 4

[[e-mail]]

https://hg.mozilla.org/mozilla-central/rev/12bd21faed15

Comment 5

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:emilio, 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 6

[Emilio Cobos Álvarez [:emilio]]

(In reply to BugBot [:suhaib / :marco/ :calixte] from comment #5)

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

Kelly wdyt?

Comment 7

[Kelly Cochrane [:kcochrane]]

Seems low risk, so I say go for it.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 9503655 [details]
Bug 1979940 - Avoid unnecessary work for expand-on-hover if already expanded / collapsed. r=kcochrane,#sidebar-reviewers

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Fixes performance regression with expanded sidebar.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: none
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Relatively simple fix
• String changes made/needed: none
• Is Android affected?: No

Comment 9

[Dianna Smith [:diannaS]]

Comment on attachment 9503655 [details]
Bug 1979940 - Avoid unnecessary work for expand-on-hover if already expanded / collapsed. r=kcochrane,#sidebar-reviewers

Approved for 142.0b6

Comment 10

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/b99159baa2e5
https://hg.mozilla.org/releases/mozilla-beta/rev/5727589b958b