Closed
Bug 928722
Opened 12 years ago
Closed 11 years ago
Resample the system scrollbar width when the UI theme is changed or mouse plugged in
Categories
(Firefox :: Toolbars and Customization, defect)
Firefox
Toolbars and Customization
Tracking
()
RESOLVED
FIXED
Firefox 30
People
(Reporter: fhd, Assigned: jaws)
References
(Blocks 1 open bug)
Details
(Whiteboard: [Australis:P3])
Attachments
(1 file, 4 obsolete files)
3.65 KB,
patch
|
jaws
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Updated•12 years ago
|
Component: Untriaged → Toolbars and Customization
OS: Mac OS X → All
Hardware: x86 → All
Whiteboard: [Australis:M?][Australis:P5]
Version: unspecified → Trunk
Reporter | ||
Comment 1•12 years ago
|
||
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.
Reporter | ||
Updated•12 years ago
|
Summary: Resample the system scrollbar width when the UI themes is changed → Resample the system scrollbar width when the UI theme is changed
Comment 2•12 years ago
|
||
(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.
Blocks: australis-cust
Comment 3•12 years ago
|
||
(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.
Depends on: 898783
Reporter | ||
Comment 4•12 years ago
|
||
Sounds good, although a bit hacky. I'll give that a go once bug 898783 lands.
Assignee | ||
Comment 5•12 years ago
|
||
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 6•12 years ago
|
||
(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
Assignee | ||
Comment 7•12 years ago
|
||
(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•12 years ago
|
||
Isn't this already possible with window.matchMedia("(-moz-overlay-scrollbars)").addListener?
Comment 9•12 years ago
|
||
(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?
Assignee | ||
Comment 10•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
(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...
Reporter | ||
Comment 13•12 years ago
|
||
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.
Attachment #822703 -
Flags: feedback?(gijskruitbosch+bugs)
Reporter | ||
Comment 14•12 years ago
|
||
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•12 years ago
|
||
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.
Attachment #822703 -
Flags: feedback?(gijskruitbosch+bugs)
Reporter | ||
Comment 16•12 years ago
|
||
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.
Attachment #822703 -
Attachment is obsolete: true
Reporter | ||
Comment 17•12 years ago
|
||
(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•12 years ago
|
||
(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. :-)
Assignee | ||
Comment 19•12 years ago
|
||
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•12 years ago
|
||
(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. :-(
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → fhd
Status: NEW → ASSIGNED
Whiteboard: [Australis:M?][Australis:P5] → [Australis:M?][Australis:P-]
Comment 21•12 years ago
|
||
(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•12 years ago
|
||
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?
Updated•12 years ago
|
Whiteboard: [Australis:M?][Australis:P-] → [Australis:M?][Australis:P4]
Reporter | ||
Comment 24•12 years ago
|
||
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•11 years ago
|
||
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.
Summary: Resample the system scrollbar width when the UI theme is changed → Resample the system scrollbar width when the UI theme is changed or mouse plugged in
Whiteboard: [Australis:M?][Australis:P4] → [Australis:P3]
Assignee | ||
Comment 26•11 years ago
|
||
I've got a patch for this that I will upload tomorrow.
Assignee: fhd → jaws
Assignee | ||
Comment 27•11 years ago
|
||
Need to test on OSX.
Attachment #823056 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Attachment #8386778 -
Flags: review?(mconley)
Comment 28•11 years ago
|
||
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!
Attachment #8386778 -
Flags: review?(mconley) → review+
Comment 29•11 years ago
|
||
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.
Attachment #8386778 -
Flags: review+ → review-
Assignee | ||
Comment 30•11 years ago
|
||
Attachment #8386778 -
Attachment is obsolete: true
Attachment #8386879 -
Flags: review?(mconley)
Updated•11 years ago
|
Attachment #8386879 -
Flags: review?(mconley) → review+
Comment 31•11 years ago
|
||
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.
Attachment #8386879 -
Attachment is obsolete: true
Attachment #8386931 -
Flags: review?(jaws)
Assignee | ||
Updated•11 years ago
|
Attachment #8386931 -
Flags: review?(jaws) → review+
Assignee | ||
Comment 32•11 years ago
|
||
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3][fixed-in-fx-team] → [Australis:P3]
Target Milestone: --- → Firefox 30
Assignee | ||
Comment 34•11 years ago
|
||
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
Attachment #8386931 -
Flags: approval-mozilla-aurora?
Updated•11 years ago
|
status-firefox29:
--- → affected
status-firefox30:
--- → fixed
Updated•11 years ago
|
Attachment #8386931 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 35•11 years ago
|
||
Comment 36•11 years ago
|
||
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?
Flags: needinfo?(fhd)
Updated•11 years ago
|
QA Whiteboard: [qa-]
Reporter | ||
Comment 37•11 years ago
|
||
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.
Flags: needinfo?(fhd)
You need to log in
before you can comment on or make changes to this bug.
Description
•