928722 - Resample the system scrollbar width when the UI theme is changed or mouse plugged in

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Felix H. Dahlke]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20100101 Firefox/24.0 (Beta/Release)
Build ID: 20130910160258

Comment 1

[Felix H. Dahlke]

For bug 861703, panels calculate their width using the system scrollbar size, among others. However, the scrollbar size is not sampled every time a panel opens, so the width can become incorrect.

Reproducing this on Windows 7:
1. Open the main menu panel, adding items until a scrollbar is visible
2. Change the system UI theme to Classic
3. Open the main menu panel again - the scrollbar is barely visible, the window is not wide enough

Reproducing this on Mac OS X 10.8:
1. Go to System Preferences -> General and set "Show scrollbars" to "When scrolling"
2. Open the main menu panel, adding items until the items become scrollable
3. Set "Show scrollbars" to "Always"
4. Open the main menu panel again - the scrollbar overlaps the items on the right

I'd be interested in working on this, but I'm not sure how to best tackle it. Resampling the scrollbar width every time a panel is opened seems like the easiest way.

Comment 2

[:Gijs (he/him)]

(In reply to Felix H. Dahlke from comment #1)
> I'd be interested in working on this, but I'm not sure how to best tackle
> it. Resampling the scrollbar width every time a panel is opened seems like
> the easiest way.

Unfortunately that's also expensive, so I don't really think it's a good idea. To the extent that I'd actually rather WONTFIX this than fix it in that way. There are already various other bugs when people change the UI theme while the browser is running (e.g. the size of the window controls (min/max/close icons)) on both regular Firefox and UX.

Comment 3

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #2)
> (In reply to Felix H. Dahlke from comment #1)
> > I'd be interested in working on this, but I'm not sure how to best tackle
> > it. Resampling the scrollbar width every time a panel is opened seems like
> > the easiest way.
> 
> Unfortunately that's also expensive, so I don't really think it's a good
> idea. To the extent that I'd actually rather WONTFIX this than fix it in
> that way. There are already various other bugs when people change the UI
> theme while the browser is running (e.g. the size of the window controls
> (min/max/close icons)) on both regular Firefox and UX.

Instead, once bug 898783 is fixed, we could store the last UI Theme (on Windows) and the last "overlayed scrollbar?" value (on Mac). These can be queried using media queries on the hidden window's document element, which I *think* shouldn't trigger a reflow. In particular,

-moz-overlay-scrollbars: 1

will match on OSX if scrollbars are overlayed. We could cache the result, and compare with the current state.

On Windows, when we have to compute something, we could store the value of the document element's getComputedStyle().mozWindowsTheme, and check with matchesMedia whether the theme has changed compared to what we stored before. (Note: this won't catch when people change the specific scrollbar width within e.g. the classic theme. If we wanted to fix that, we should implement an observer notification for the theme change in widget's native windows implementation)

This is all working around the fact that apparently neither the mac or windows implementation of these features broadcast anything like observer service notifications. That'd actually be better, but more work.

Comment 4

[Felix H. Dahlke]

Sounds good, although a bit hacky. I'll give that a go once bug 898783 lands.

Comment 5

[(parental leave 10/24-mid-February) Jared Wein [:jaws] (please needinfo? me)]

The native look and feel code really needs to emit an event when it notices this system setting change (it doesn't currently), and the JSM can listen for the event and update accordingly.

Spohl knows about this area of the code, and may be able to help when he has time.

Comment 6

[Stephen A Pohl [:spohl]]

(In reply to Jared Wein [:jaws] from comment #5)
> Spohl knows about this area of the code, and may be able to help when he has
> time.

I don't have a complete solution ready to go, but a change in scrollbar styles on OSX is detected during reflow in nsGfxScrollFrameInner::HandleScrollbarStyleSwitching() [1]. It should be possible to send an appropriate event from there.

I could look into this myself, but I would need to know:
1. What type of event should be sent?
2. I'm not very familiar with dispatching JS events from C++. Is there a good precedent/example that could get me started?

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsGfxScrollFrame.cpp#1001

Comment 7

[(parental leave 10/24-mid-February) Jared Wein [:jaws] (please needinfo? me)]

(In reply to Stephen Pohl [:spohl] from comment #6)
> (In reply to Jared Wein [:jaws] from comment #5)
> > Spohl knows about this area of the code, and may be able to help when he has
> > time.
> 
> I don't have a complete solution ready to go, but a change in scrollbar
> styles on OSX is detected during reflow in
> nsGfxScrollFrameInner::HandleScrollbarStyleSwitching() [1]. It should be
> possible to send an appropriate event from there.
> 
> I could look into this myself, but I would need to know:
> 1. What type of event should be sent?

A chrome event, not sure about the name.

> 2. I'm not very familiar with dispatching JS events from C++. Is there a
> good precedent/example that could get me started?

See http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsContentUtils.h#1001 and http://mxr.mozilla.org/mozilla-central/ident?i=DispatchChromeEvent

Comment 8

[Markus Stange [:mstange]]

Isn't this already possible with window.matchMedia("(-moz-overlay-scrollbars)").addListener?

Comment 9

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #7)
> (In reply to Stephen Pohl [:spohl] from comment #6)
> > (In reply to Jared Wein [:jaws] from comment #5)
> > > Spohl knows about this area of the code, and may be able to help when he has
> > > time.
> > 
> > I don't have a complete solution ready to go, but a change in scrollbar
> > styles on OSX is detected during reflow in
> > nsGfxScrollFrameInner::HandleScrollbarStyleSwitching() [1]. It should be
> > possible to send an appropriate event from there.
> > 
> > I could look into this myself, but I would need to know:
> > 1. What type of event should be sent?
> 
> A chrome event, not sure about the name.
> 
> > 2. I'm not very familiar with dispatching JS events from C++. Is there a
> > good precedent/example that could get me started?
> 
> See
> http://mxr.mozilla.org/mozilla-central/source/content/base/public/
> nsContentUtils.h#1001 and
> http://mxr.mozilla.org/mozilla-central/ident?i=DispatchChromeEvent

Why not use an observer notification?

Comment 10

[(parental leave 10/24-mid-February) Jared Wein [:jaws] (please needinfo? me)]

(In reply to Markus Stange [:mstange] from comment #8)
> Isn't this already possible with
> window.matchMedia("(-moz-overlay-scrollbars)").addListener?

If this will work then perfect!

Comment 11

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #10)
> (In reply to Markus Stange [:mstange] from comment #8)
> > Isn't this already possible with
> > window.matchMedia("(-moz-overlay-scrollbars)").addListener?
> 
> If this will work then perfect!

It'll be problematic after bug 898783 because the JS doesn't have consistent DOM access.

The theme/scrollbar changes affect the entire application, not just a single window, so firing an observer notification seems to me like something that'd make sense... We could do the same on Windows.

Comment 12

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #11)
> It'll be problematic after bug 898783 because the JS doesn't have consistent
> DOM access.

Egh. I meant, the JS*M* won't have...

Comment 13

[Felix H. Dahlke]

Attachment: Resample the system scroll bar width after it's shown or hidden

Looked into this a bit. Markus' suggestion, window.matchMedia("(-moz-overlay-scrollbars)").addListener fixes this on OSX, I've attached a patch with just this change. Hope you don't mind that I requested feedback from you Gijs, let me know if you think someone else should review this. Also, you said that the JSM doesn't have consistent DOM access, do you think passing in the window object from the XUL script is going to suffice?

Anyway, as expected, this does not fix the problem on Windows. So that means we'll still need some sort of event when the Windows UI theme changes, right? (I cannot get Firefox to compile on Linux, but I have a feeling we'll have the same issue there.)

How about we rename this bug and discuss the OSX fix here, and then tackle the theme changing issue in a new one? Seems to be a non-issue on OSX, since it's not possible to change the UI theme there.

Comment 14

[Felix H. Dahlke]

Note that this is an experimental change, just something that works. I'm seeing some issues with this:

1. It shouldn't keep adding listeners, a single one might suffice
2. I'd rather add the listener on initialisation, but I'm not sure how to get access to a window object there
3. If it's proven to have no effect on Windows and Linux, we might want to add the listener only on OSX

Comment 15

[:Gijs (he/him)]

Comment on attachment 822703 [details] [diff] [review]
Resample the system scroll bar width after it's shown or hidden

(In reply to Felix H. Dahlke from comment #13)
> Created attachment 822703 [details] [diff] [review]
> Resample the system scroll bar width after it's shown or hidden

This looks to be an OS.File patch, not the one you meant to attach...

Speculative feedback below. :-)

> Looked into this a bit. Markus' suggestion,
> window.matchMedia("(-moz-overlay-scrollbars)").addListener fixes this on
> OSX, I've attached a patch with just this change. Hope you don't mind that I
> requested feedback from you Gijs, let me know if you think someone else
> should review this. Also, you said that the JSM doesn't have consistent DOM
> access, do you think passing in the window object from the XUL script is
> going to suffice?

It's fine to ask me for feedback/review. However, unfortunately I don't think this is the right approach. Basically, if you add a listener in the JSM to the window, that means you're either keeping a reference to the window, or if you're not, you at least are going to be sad if the window which was passed in goes away and another window takes its place, and you no longer get a notification and then your code doesn't behave as expected.

I really think the relevant bits of native theme land need to send out observer notifications or equivalent. The weird thing is that I *think* we get individual notifications for each window of changes to the Windows UI Theme - even though, to the best of my knowledge, the theme can't actually be different between several windows. Not sure if we should then send out multiple notifications with the window as its subject, as it'd be a problem if the notification raced with the native implementation of a particular window actually having updated its CSS stuff or whatnot.

> How about we rename this bug and discuss the OSX fix here, and then tackle
> the theme changing issue in a new one? Seems to be a non-issue on OSX, since
> it's not possible to change the UI theme there.

I'm OK with this, but IMO we still need a better approach (again, without having seen your patch, so perhaps you've thought of something I've missed!). If you're not comfortable writing the C++/ObjC for this, I or someone else can probably help with those bits.

Comment 16

[Felix H. Dahlke]

Attachment: Resample the system scroll bar width after it's shown or hidden (the correct patch this time)

Oops, here is the correct patch. But I didn't mark it for feedback or review, since we probably can't go with this approach.

Comment 17

[Felix H. Dahlke]

(In reply to :Gijs Kruitbosch from comment #15)
> This looks to be an OS.File patch, not the one you meant to attach...
> 
> Speculative feedback below. :-)

Oops, you're right, that was the wrong one :) Attached the real one for reference, but your speculative feedback was pretty spot on.

> > How about we rename this bug and discuss the OSX fix here, and then tackle
> > the theme changing issue in a new one? Seems to be a non-issue on OSX, since
> > it's not possible to change the UI theme there.
> 
> I'm OK with this, but IMO we still need a better approach (again, without
> having seen your patch, so perhaps you've thought of something I've
> missed!). If you're not comfortable writing the C++/ObjC for this, I or
> someone else can probably help with those bits.

Well, if we're not sure that the OSX fix will be completely different from the Windows/Linux fix, let's keep it in one bug for now I suppose. I'll look into the native stuff in a bit.

Comment 18

[:Gijs (he/him)]

(In reply to Felix H. Dahlke from comment #17)
> Well, if we're not sure that the OSX fix will be completely different from
> the Windows/Linux fix, let's keep it in one bug for now I suppose. I'll look
> into the native stuff in a bit.

Awesome! Note that if we're going to have to patch widget, you're going to need different reviewers for those bits. :-)

Comment 19

[(parental leave 10/24-mid-February) Jared Wein [:jaws] (please needinfo? me)]

Why not just add the listener to the hidden window, since we don't have to worry about it going away while the application is running, and the JSM should be able to retrieve it. For example, see http://mxr.mozilla.org/mozilla-central/source/toolkit/identity/Sandbox.jsm#80.

Comment 20

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #19)
> Why not just add the listener to the hidden window, since we don't have to
> worry about it going away while the application is running, and the JSM
> should be able to retrieve it. For example, see
> http://mxr.mozilla.org/mozilla-central/source/toolkit/identity/Sandbox.
> jsm#80.

We could, however AIUI from IRC a couple of days ago, we're trying to get rid of the hidden window. :-(

Comment 21

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to :Gijs Kruitbosch from comment #20)
> We could, however AIUI from IRC a couple of days ago, we're trying to get
> rid of the hidden window. :-(

Yep. Boy is that a long road...

If we can avoid adding additional dependencies on the hidden window, that would be great.

Comment 23

[Mike Conley (:mconley) (:⚙️) (Wayyyy behind on needinfos)]

So I think we should reconsider our P-'ing of this bug. A common scenario (at least for Mozilla employees who use Macbooks) is to get to their desk, drop their Macbook on a mount, and plug in a USB Mouse / Keyboard.

The USB mouse causes OS X to show scrollbars all the time. If Firefox had been opened without the USB Mouse plugged in, and scrollbars had been initially hidden, the result upon subsequent opens of the menu panel look like this: http://cl.ly/image/1P0R1v2B3i3e

That's not a great experience. :/ I think we should bump the priority on this one. Any concerns about that?

Comment 24

[Felix H. Dahlke]

Sorry, I've been missing in action here, I'll get back on it next week. What implication does P4 have, should it be done by a certain date?

Comment 25

[Justin Dolske [:Dolske]]

I think the OS X issue here specifically makes this a P3, whereas the theme-change portion is lower. If the same fix takes care of both, great, but if a simpler OSX-only fix exists, we should split up the bug.

Comment 26

[(parental leave 10/24-mid-February) Jared Wein [:jaws] (please needinfo? me)]

I've got a patch for this that I will upload tomorrow.

Comment 27

[(parental leave 10/24-mid-February) Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Need to test on OSX.

Comment 28

[Mike Conley (:mconley) (:⚙️) (Wayyyy behind on needinfos)]

Comment on attachment 8386778 [details] [diff] [review]
Patch

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

I'm good with this. At first, I thought maybe ScrollbarSampler.jsm should take care of listening for system changes, like -moz-overlay-scrollbars, instead of creating more coupling between panelUI.js and ScrollbarSampler.jsm... but now that I hear that ScrollbarSampler.jsm's HiddenDOMWindow dependency is on the kill list, and that we'll eventually be passing a window, this makes more sense to me.

So, in sum, I'm good with this. Thanks Jared!

Comment 29

[Mike Conley (:mconley) (:⚙️) (Wayyyy behind on needinfos)]

Comment on attachment 8386778 [details] [diff] [review]
Patch

Whoops - it turns out I'm getting this in the console:

JavaScript error: chrome://browser/content/customizableui/panelUI.js, line 455: this._calculateScrollbarWidth is not a function

The top theory is that the matchMedia listener has a different sense of what "this" is.

Comment 30

[(parental leave 10/24-mid-February) Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch v2

Comment 31

[Mike Conley (:mconley) (:⚙️) (Wayyyy behind on needinfos)]

Attachment: Patch v3

So it turns out this was not as complicated as we thought it'd be. We just invalidate the scrollWidth cached value now when we notice that moz-overlay-scrollbars has changed.

Comment 32

[(parental leave 10/24-mid-February) Jared Wein [:jaws] (please needinfo? me)]

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

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 34

[(parental leave 10/24-mid-February) Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8386931 [details] [diff] [review]
Patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 898783
User impact if declined: if a user plugs in their mouse, the width of the panel may need to change, but we won't change it and items can be clipped or there could be large blank space
Testing completed (on m-c, etc.): locally, just landed on m-c
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 35

[Mike Conley (:mconley) (:⚙️) (Wayyyy behind on needinfos)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/45108d8e8987

Comment 36

[[PTO until 18 Nov] Bogdan Maris [:bogdan_maris], Release Desktop QA]

I am not able to reproduce this issue on Windows 7 64bit and Mac OS X 10.8.5 using a old nightly thus I can`t verify if the issue is fixed or not. Felix can you please verify that the issue is fixed?

Comment 37

[Felix H. Dahlke]

I can still reproduce it as described above in Windows 7.

On OS X 10.8.5, the behaviour is quite weird now: When I open the panel, three columns of widgets are shown. Then, after a short delay, the scrollbar appears, the content jumps left a bit, and only two columns are shown. When I restart Nightly, three columns are shown, but there's still a short delay before the scrollbar appears and the content jumps left a bit.