898783 - Have PanelUI sample the system scrollbar width only once globally, as opposed to once per window.

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Bug 861703 modified PanelUI to sample the system scrollbar width so that the panel could be sized in a way that would allow the list of items to overflow without reducing the number of columns in the panel.

We currently do that sampling when either the panel is opened the first time, or the first time customization mode is entered if the panel hasn't been opened.

But the above rules apply *per window*. They should probably apply globally, because we probably don't need to account for the case where scrollbar sizes are changing window to window.

Not sure if this is blocking, but it's certainly not high-priority, at least for now.

Comment 1

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

(I somehow doubt that this blocks shipping, however)

Comment 2

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

To fix this bug you'll need to clone the UX repository located at https://hg.mozilla.org/projects/ux/

We currently calculate this value at http://mxr.mozilla.org/projects-central/source/ux/browser/components/customizableui/content/panelUI.js#196. We can move this calculation to CustomizableUI.jsm which is located at /browser/components/customizableui/src/CustomizableUI.jsm. CustomizableUI.jsm is a JavaScript Module that is loaded only once, and so the value will only need to be calculated once.

Felix, would you like to work on this bug?

Comment 3

[Felix H. Dahlke]

Yes, I'll look into it.

Comment 4

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

Hey Felix, any update on this? Let me know if you have any questions.

Comment 5

[Felix H. Dahlke]

Sorry, the last few days have been busy, I'll actually look into it now.

Comment 6

[Felix H. Dahlke]

Attachment: bug-898783.patch

Actually looked into this today.

Seems pretty straightforward to fix, the attached patch is my first take. I only tested it on OSX though, and the Lion+ scrollbar seems to report a width of 0 anyway. I was planning to set up a Windows dev environment anyway, I'll do that and test it properly.

However, I'm wondering if this is actually a good idea. Are we sure that there is no way to change the native scrollbar width while an application is running? I don't think it's possible with GTK (requires a restart IIRC) or on OSX (I don't think it can be changed at all), but maybe on Windows? I'll test this as well once I have the Windows dev environment set up.

Comment 7

[Felix H. Dahlke]

Comment on attachment 819005 [details] [diff] [review]
bug-898783.patch

># HG changeset patch
># Parent 8f5711ebd0f8cd49580eeb9287963bd96cef9878
>diff -r 8f5711ebd0f8 -r 51ebaa9dfa92 browser/components/customizableui/content/panelUI.js
>--- a/browser/components/customizableui/content/panelUI.js	Wed Oct 02 20:41:14 2013 +0200
>+++ b/browser/components/customizableui/content/panelUI.js	Fri Oct 18 16:17:04 2013 +0200
>@@ -4,8 +4,6 @@
> 
> XPCOMUtils.defineLazyModuleGetter(this, "CustomizableUI",
>                                   "resource:///modules/CustomizableUI.jsm");
>-XPCOMUtils.defineLazyModuleGetter(this, "Promise",
>-                                  "resource://gre/modules/Promise.jsm");
> /**
>  * Maintains the state and dispatches events for the main menu panel.
>  */
>@@ -178,9 +176,10 @@
>       if (!this._scrollWidth) {
>         // In order to properly center the contents of the panel, while ensuring
>         // that we have enough space on either side to show a scrollbar, we have to
>-        // do a bit of hackery. In particular, we sample the system scrollbar width,
>-        // and then use that to calculate a new width for the scroller.
>-        this._scrollWidth = (yield this._sampleScrollbarWidth()) + "px";
>+        // do a bit of hackery. In particular, we calculate a new width for the
>+        // scroller, based on the system scrollbar width.
>+        this._scrollWidth =
>+          (yield CustomizableUI.getSystemScrollbarWidth()) + "px";
>         let cstyle = window.getComputedStyle(this.scroller);
>         let widthStr = cstyle.width;
>         // Get the calculated padding on the left and right sides of
>@@ -371,35 +370,5 @@
> 
>   _onHelpViewHide: function(aEvent) {
>     this.removeEventListener("command", PanelUI.onCommandHandler);
>-  },
>-
>-  _sampleScrollbarWidth: function() {
>-    let deferred = Promise.defer();
>-    let hwin = Services.appShell.hiddenDOMWindow;
>-    let hdoc = hwin.document.documentElement;
>-    let iframe = hwin.document.createElementNS("http://www.w3.org/1999/xhtml",
>-                                               "html:iframe");
>-    iframe.setAttribute("srcdoc", '<body style="overflow-y: scroll"></body>');
>-    hdoc.appendChild(iframe);
>-
>-    let cwindow = iframe.contentWindow;
>-    let utils = cwindow.QueryInterface(Ci.nsIInterfaceRequestor)
>-                       .getInterface(Ci.nsIDOMWindowUtils);
>-
>-    cwindow.addEventListener("load", function onLoad(aEvent) {
>-      cwindow.removeEventListener("load", onLoad);
>-      let sbWidth = {};
>-      try {
>-        utils.getScrollbarSize(true, sbWidth, {});
>-      } catch(e) {
>-        Components.utils.reportError("Could not sample scrollbar size: " + e +
>-                                     " -- " + e.stack);
>-        sbWidth.value = 0;
>-      }
>-      deferred.resolve(sbWidth.value);
>-      iframe.remove();
>-    });
>-
>-    return deferred.promise;
>   }
> };
>diff -r 8f5711ebd0f8 -r 51ebaa9dfa92 browser/components/customizableui/src/CustomizableUI.jsm
>--- a/browser/components/customizableui/src/CustomizableUI.jsm	Wed Oct 02 20:41:14 2013 +0200
>+++ b/browser/components/customizableui/src/CustomizableUI.jsm	Fri Oct 18 16:17:04 2013 +0200
>@@ -18,6 +18,8 @@
>   "resource://gre/modules/DeferredTask.jsm");
> XPCOMUtils.defineLazyModuleGetter(this, "PrivateBrowsingUtils",
>   "resource://gre/modules/PrivateBrowsingUtils.jsm");
>+XPCOMUtils.defineLazyModuleGetter(this, "Promise",
>+                                  "resource://gre/modules/Promise.jsm");
> XPCOMUtils.defineLazyGetter(this, "gWidgetsBundle", function() {
>   const kUrl = "chrome://browser/locale/customizableui/customizableWidgets.properties";
>   return Services.strings.createBundle(kUrl);
>@@ -120,6 +122,8 @@
> let gSingleWrapperCache = new WeakMap();
> let gListeners = new Set();
> 
>+let gSystemScrollbarWidth = null;
>+
> let gModuleName = "[CustomizableUI]";
> #include logging.js
> 
>@@ -1938,7 +1942,52 @@
>     }
> 
>     return true;
>-  }
>+  },
>+
>+  getSystemScrollbarWidth: function() {
>+    let deferred = Promise.defer();
>+
>+    if (gSystemScrollbarWidth !== null) {
>+      deferred.resolve(gSystemScrollbarWidth);
>+      return deferred.promise;
>+    }
>+
>+    this._sampleSystemScrollbarWidth().then(function(systemScrollbarWidth) {
>+      gSystemScrollbarWidth = systemScrollbarWidth;
>+      deferred.resolve(gSystemScrollbarWidth);
>+    });
>+    return deferred.promise;
>+  },
>+
>+  _sampleSystemScrollbarWidth: function() {
>+    let deferred = Promise.defer();
>+    let hwin = Services.appShell.hiddenDOMWindow;
>+    let hdoc = hwin.document.documentElement;
>+    let iframe = hwin.document.createElementNS("http://www.w3.org/1999/xhtml",
>+                                               "html:iframe");
>+    iframe.setAttribute("srcdoc", '<body style="overflow-y: scroll"></body>');
>+    hdoc.appendChild(iframe);
>+
>+    let cwindow = iframe.contentWindow;
>+    let utils = cwindow.QueryInterface(Ci.nsIInterfaceRequestor)
>+                       .getInterface(Ci.nsIDOMWindowUtils);
>+
>+    cwindow.addEventListener("load", function onLoad(aEvent) {
>+      cwindow.removeEventListener("load", onLoad);
>+      let sbWidth = {};
>+      try {
>+        utils.getScrollbarSize(true, sbWidth, {});
>+      } catch(e) {
>+        Components.utils.reportError("Could not sample scrollbar size: " + e +
>+                                     " -- " + e.stack);
>+        sbWidth.value = 0;
>+      }
>+      deferred.resolve(sbWidth.value);
>+      iframe.remove();
>+    });
>+
>+    return deferred.promise;
>+  },
> };
> Object.freeze(CustomizableUIInternal);
> 
>@@ -2072,6 +2121,9 @@
>   },
>   onWidgetDrag: function(aWidgetId, aArea) {
>     CustomizableUIInternal.notifyListeners("onWidgetDrag", aWidgetId, aArea);
>+  },
>+  getSystemScrollbarWidth: function() {
>+    return CustomizableUIInternal.getSystemScrollbarWidth();
>   }
> };
> Object.freeze(this.CustomizableUI);

Comment 8

[Felix H. Dahlke]

Attachment: bug-898783.patch

Never mind that last commend, tried to fix a typo inline. Still have to come to terms with BugZilla :)

Here's a new patch with the typo fixed.

Comment 9

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

Comment on attachment 819008 [details] [diff] [review]
bug-898783.patch

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

We should put this in a separate JSM since it's not dependent on the other CustomizableUI functionality. Overall looks good though.

Also, you can disable the floating scrollbars on OSX by changing one of the mouse/scrollbar properties in OSX's Settings.

Comment 10

[Felix H. Dahlke]

(In reply to Jared Wein [:jaws] from comment #9)
> We should put this in a separate JSM since it's not dependent on the other
> CustomizableUI functionality.

Got something in mind? Or did you mean a new one like ScrollbarSampler.jsm?

> Also, you can disable the floating scrollbars on OSX by changing one of the
> mouse/scrollbar properties in OSX's Settings.

Ah, thanks. Seems like my changes are fine then.

However, I noticed that the panel width does not change if this setting is changed while Firefox is running. Steps to reproduce:

1. Ensure "Show scroll bars" is set to "When scrolling"
2. Open the main panel (ensuring there's not enough vertical space to show all elements)
3. Set "Show scroll bars" to "Always"
4. Open the main panel again, the static scroll bar will be on top of the panel elements, obscuring a small part of the rightmost elements. The panel layout is fortunately not messed up though

I can reproduce it with and without my patch applied, so it seems like a separate issue. From what I see, without my patch applied, it should sample the scrollbar width whenever a panel is opened, so I'm a bit puzzled that it doesn't work even then. Material for a follow-up, I presume?

Comment 11

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

(In reply to Felix H. Dahlke from comment #10)
> (In reply to Jared Wein [:jaws] from comment #9)
> > We should put this in a separate JSM since it's not dependent on the other
> > CustomizableUI functionality.
> 
> Got something in mind? Or did you mean a new one like ScrollbarSampler.jsm?

A new one like ScrollbarSampler.jsm.

> > Also, you can disable the floating scrollbars on OSX by changing one of the
> > mouse/scrollbar properties in OSX's Settings.
> 
> Ah, thanks. Seems like my changes are fine then.
> 
> However, I noticed that the panel width does not change if this setting is
> changed while Firefox is running. Steps to reproduce:
> 
> 1. Ensure "Show scroll bars" is set to "When scrolling"
> 2. Open the main panel (ensuring there's not enough vertical space to show
> all elements)
> 3. Set "Show scroll bars" to "Always"
> 4. Open the main panel again, the static scroll bar will be on top of the
> panel elements, obscuring a small part of the rightmost elements. The panel
> layout is fortunately not messed up though
> 
> I can reproduce it with and without my patch applied, so it seems like a
> separate issue. From what I see, without my patch applied, it should sample
> the scrollbar width whenever a panel is opened, so I'm a bit puzzled that it
> doesn't work even then. Material for a follow-up, I presume?

This is because even before your patch we cache the value once it's computed (per window). With your patch we will still cache the value but the cached value will be used for all windows. So we would need to invalidate the cache if this setting changes, unfortunately there isn't an event yet that will notify us if the scrollbar style changes (we can file a bug for it).

Comment 12

[Felix H. Dahlke]

Attachment: bug-898783.patch

Here's a new patch, I moved the scrollbar sampling and width caching code to a new module as discussed. I can't say I'm happy with the name (ScrollbarSampler.jsm), but couldn't really come up with anything better. Suggestions welcome.

Comment 13

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

Comment on attachment 819253 [details] [diff] [review]
bug-898783.patch

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

Please upload a patch with 8 lines of context. In your hgrc file, make sure you have the following:

[diff]
git = 1
showfunc = 1
unified = 8

Comment 14

[Felix H. Dahlke]

Attachment: bug-898783.patch

Sorry, missed that part. Created a new patch that should comply with the conventions.

Comment 15

[Felix H. Dahlke]

Attachment: bug-898783.patch

Uploaded another patch, this one actually applies on the latest HEAD without conflicts.

I also got around to testing this on Windows. My modifications work fine, but as I feared, changing the UI theme messes things up. I filed bug 928722 for this.

Comment 16

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

Comment on attachment 819541 [details] [diff] [review]
bug-898783.patch

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

I'm not keen on the name, but I don't have a better name in mind so we can roll with this for now and change it in another bug when someone else has a better name.

Please make the following two changes and retest, and then request review one more time.

::: browser/components/customizableui/src/ScrollbarSampler.jsm
@@ +49,5 @@
> +      let sbWidth = {};
> +      try {
> +        utils.getScrollbarSize(true, sbWidth, {});
> +      } catch(e) {
> +        Components.utils.reportError("Could not sample scrollbar size: " + e +

While moving this code, we can change this to use Cu.reportError instead of spelling out "Components.utils.". I know you didn't introduce this, but now is a good time to fix it.

::: browser/components/customizableui/src/moz.build
@@ +8,5 @@
>      'CustomizableUI.jsm',
>      'CustomizableWidgets.jsm',
>      'CustomizeMode.jsm',
>      'PanelWideWidgetTracker.jsm',
> +    'ScrollbarSampler.jsm',

EXTRA_PP_JS_MODULES is for modules that require preprocessing. Since ScrollbarSampler.jsm doens't use the preprocessor, it can be moved to a new EXTRA_JS_MODULES = [] array (declared prior to the EXTRA_PP_JS_MODULES array).

Comment 17

[Felix H. Dahlke]

Attachment: bug-898783.patch

Here's a new patch, addressed all comments.

Comment 18

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

Comment on attachment 820842 [details] [diff] [review]
bug-898783.patch

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

Looks good! Thank you for the patch, I'll try to find another bug that you can help with.

Comment 19

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

https://hg.mozilla.org/projects/ux/rev/26afb7771f04

Comment 20

[:Gijs (he/him)]

https://hg.mozilla.org/mozilla-central/rev/26afb7771f04