868498 - Lion-style scrollbars do not change when System Preferences' "Show scroll bars" setting changes until browser restart

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Chris Peterson [:cpeterson]]

STR:
1. Quit Firefox
2. In System Preferences > General, set "Show scroll bars" to "When scrolling".
3. Restart Firefox
4. See that Firefox's scrollbars only appear when scrolling, as expected.
5. In System Preferences > General, set "Show scroll bars" to "Always".
6. See that Firefox's scrollbars still only appear when scrolling.
7. Restart Firefox
8. See that Firefox's scrollbars are always visible, as expected.

Google Chrome's scrollbars change style dynamically as the System Preferences change.

Comment 1

[Stephen A Pohl [:spohl]]

I believe there is a track record for requiring a restart after making settings changes. I imagine it has to do with efficiency: reading out settings once and remembering them is more efficient than reading them out repeatedly. If things are different in this case, I'd be happy to hear about it.

Comment 2

[Josiah Bruner [:jsbruner]]

In *this* specific case, when a change is made in the Settings Application, I believe we should behave like other applications. For in-browser changes I suppose forcing a restart for a change to take affect is fine. However we are trying to mimic a standard OS element here, therefore it should behave like one.

Now, if it would quite drastically affect performance for running a check every so often, then go ahead and demand a restart. However if it does not create a performance hit much/at all, then I do believe we should be consistent with the rest of the OS.

Just my two cents.

Comment 3

[Chris Peterson [:cpeterson]]

I assume there is a Cocoa API for listening to System Preference changes because the applications I tested (Chrome, Safari, Terminal, and LimeChat) all respond to the "Show scroll bars" setting changes immediately.

Comment 4

[Stefan [:stefanh]]

We do obey instantly to the aqua/graphite change (in the general system settings) and I guess it ought to be possible to do something similar here?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

These preference change automatically when you insert or remove a mouse. Currently we stop drawing all together when you remove a mouse.

Comment 7

[(no longer active)]

Attachment: Screencast

This is a screencast of three different behaviors, on one page (bugzilla) we show the old style scrollbars, one another page (github) which has been open for a while we show nothing, and there's some corruption on top of the page, with a tiny unfunctional scrollbar rendered on the top left, and on another page we show the old style scrollbars which fade out when not scrolling.

This is with a Bluetooth mouse attached.

Comment 8

[Stephen A Pohl [:spohl]]

I meant to reply here earlier: We actually detect the preference change in System Preferences in widget/cocoa/nsChildView.mm, where we add an observer for "NSPreferredScrollerStyleDidChangeNotification". What we don't handle yet is replacing the lion-style scrollbars with the old-style ones and vice-versa. We probably have to tear down all scrollbars and replace them with the newly chosen style, but where to do this isn't clear to me yet.

The observer for the "NSPreferredScrollerStyleDidChangeNotification" notification will trigger a call to nsNativeThemeCocoa::ThemeChanged():
http://mxr.mozilla.org/mozilla-central/source/widget/cocoa/nsNativeThemeCocoa.mm#2935

However, that's just returning a result of NS_OK. The comment in nsNativeThemeCocoa::ThemeChanged() says:
   // This is unimplemented because we don't care if gecko changes its theme
   // and Mac OS X doesn't have themes.

I'm guessing the proper thing to do would be to implement this for scrollbar style changes. But how to get a hold of all the scrollbars, tear them down and generate new ones from there isn't clear to me yet.

Comment 10

[Robert Strong (they/them - no direct email)]

Stephen tells me that these can also lead to a crash

Comment 11

[Stephen A Pohl [:spohl]]

There seem to be three steps that are necessary to fix this bug:
1. Trigger a reflow when system preferences change.
2. Correctly destroy or create ScrollbarActivity objects when the preferences change, including registering/unregistering from the RefreshDriver. ScrollbarActivity objects are only used in the overlay-scrollbar case.
3. Recalculate the scrollbar widths and margins when the preferences change.

Step 3, the way we calculate widths/margins for scrollbars, is in the process of being changed in bug 868416 and I'll focus on that step first.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You probably want to call nsPresContext::RebuildAllStyleData(NS_STYLE_HINT_FRAMECHANGE). That basically rebuilds the world.

Comment 13

[Stephen A Pohl [:spohl]]

Attachment: Wip

Posting my work in progress.

Calling nsPresContext::RebuildAllStyleData(NS_STYLE_HINT_FRAMECHANGE) resulted in assertions because we're seemingly not supposed to reconstruct the root of the frame tree. However, nsPresShell::ReconstructFrames() seems to properly trigger a reflow.

During reflow we detect a switch between scrollbar styles and properly destroy or instantiate mScrollbarActivity. I'll probably break this out into a method in nsGfxScrollFrameInner for the final patch.

This patch also illustrates the importance of dynamically recalculating our scrollbar margins. These are intentionally hardcoded here because I'll address this in bug 868416.

This patch doesn't work yet during resizing. It also doesn't work when switching from regular to overlay scrollbars if the page was first laid out with regular scrollbars (only if the page was first laid out with overlay scrollbars). This is intentional, since both of these issues will be resolved by the patch for bug 868416.

Comment 14

[Markus Stange [:mstange]]

Hmm. Can't we detect the CSS margin change on the scrollbar frame and use that to trigger reflows in the right places? Style data should already be rebuilt properly because the negative scrollbar margin is inside a "@media all and (-moz-overlay-scrollbars) {...}" which is (hopefully) properly invalidated on the pref change: -[ChildView systemMetricsChanged] -> nsBaseWidget::NotifyThemeChanged -> nsPresContext::ThemeChanged() -> nsPresContext::ThemeChangedInternal() -> nsPresContext::MediaFeatureValuesChanged

I haven't done any debugging, but my first guess would be that the code which triggers reflows from CSS margin changes doesn't do so for native anonymous content, or at least doesn't propagate the "reflow necessary" flag outside to the scrollframe.

Comment 15

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange from comment #14)
> Hmm. Can't we detect the CSS margin change on the scrollbar frame and use
> that to trigger reflows in the right places? Style data should already be
> rebuilt properly because the negative scrollbar margin is inside a "@media
> all and (-moz-overlay-scrollbars) {...}" which is (hopefully) properly
> invalidated on the pref change: -[ChildView systemMetricsChanged] ->
> nsBaseWidget::NotifyThemeChanged -> nsPresContext::ThemeChanged() ->
> nsPresContext::ThemeChangedInternal() ->
> nsPresContext::MediaFeatureValuesChanged
> 

The style data is not being rebuilt in this case. I'll do some debugging and figure out why not.

Comment 16

[Jesse Ruderman]

For automated testing (regression tests and fuzzing), it would be nice to have a way to force Firefox to switch between scrollbar modes.  (One that doesn't involve changing a system setting or inserting and removing a USB mouse.)

Comment 17

[Jesse Ruderman]

Attachment: a scrollable page

Comment 18

[Stephen A Pohl [:spohl]]

(In reply to Jesse Ruderman from comment #16)
> For automated testing (regression tests and fuzzing), it would be nice to
> have a way to force Firefox to switch between scrollbar modes.  (One that
> doesn't involve changing a system setting or inserting and removing a USB
> mouse.)

We do have a pref for overlay scrollbars ("ui.useOverlayScrollbars", see bug 868396), but the challenge will be to trigger the same reflow (not reload) of the page that a change of system settings does. A resize might do the trick and could simultaneously test bug 870941 (which seems to occur when a page is being resized).

Comment 19

[Stephen A Pohl [:spohl]]

Attachment: Patch

This also fixes bug 870941.

Comment 20

[Stephen A Pohl [:spohl]]

Attachment: Patch

This patch ensures that we behave correctly should the frame (and therefore the ScrollbarActivity object) be destroyed while we set visibility properties.

Comment 21

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

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

::: layout/generic/ScrollbarActivity.cpp
@@ +82,5 @@
> +  SetVisibility(GetHorizontalScrollbar(), true);
> +  if (!weakFrame.IsAlive()) {
> +    return;
> +  }
> +  SetVisibility(GetVerticalScrollbar(), true);

Can you explain why we need to do this?

::: layout/generic/nsGfxScrollFrame.cpp
@@ +973,5 @@
> +{
> +  bool didSwitch = false;
> +
> +  // Check if we switched between scrollbar styles.
> +  if (mScrollbarActivity &&

Since you're reconstructing frames, it's unclear to me why you need to do this. All the nsGfxScrollFrames are being destroyed and recreated ... right?

::: widget/cocoa/nsChildView.mm
@@ +2672,5 @@
>      mGeckoChild->NotifyThemeChanged();
> +    nsIWidgetListener* listener = mGeckoChild->GetWidgetListener();
> +    if (listener) {
> +      listener->GetPresShell()->ReconstructFrames();
> +    }

Can we avoid doing this for all theme changes? It's expensive.

Comment 22

[Stephen A Pohl [:spohl]]

Attachment: Patch

This patch only reconstructs frames when the scrollbar system preference changes.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> ::: layout/generic/ScrollbarActivity.cpp
> @@ +82,5 @@
> > +  SetVisibility(GetHorizontalScrollbar(), true);
> > +  if (!weakFrame.IsAlive()) {
> > +    return;
> > +  }
> > +  SetVisibility(GetVerticalScrollbar(), true);
> 
> Can you explain why we need to do this?

Bug 877097 comment 3 is what prompted me to do this. It looks like the frame
can get destroyed when setting properties and thus the ScrollbarActivity object.

> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +973,5 @@
> > +{
> > +  bool didSwitch = false;
> > +
> > +  // Check if we switched between scrollbar styles.
> > +  if (mScrollbarActivity &&
> 
> Since you're reconstructing frames, it's unclear to me why you need to do
> this. All the nsGfxScrollFrames are being destroyed and recreated ... right?
> 

I don't see the nsGfxScrollFrames getting destroyed and/or recreated. But the frames are getting reflown and that's why we can still perform this check to see when the scrollbar style has changed. I chose ReconstructFrames because it does not require a target frame to initiate the reflow. When we detect the change in scrollbar system preference (scrollbarSystemMetricChanged in nsChildView.mm) we don't have a target frame readily available. All the other methods such as nsIPresShell::FrameNeedsReflow that I'm aware of require a target frame to initiate reflows.

Maybe there is a better way to initiate a reflow from scrollbarSystemMetricChanged?

Comment 23

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Stephen Pohl [:spohl] from comment #22)
> I don't see the nsGfxScrollFrames getting destroyed and/or recreated. But
> the frames are getting reflown and that's why we can still perform this
> check to see when the scrollbar style has changed. I chose ReconstructFrames
> because it does not require a target frame to initiate the reflow. When we
> detect the change in scrollbar system preference
> (scrollbarSystemMetricChanged in nsChildView.mm) we don't have a target
> frame readily available. All the other methods such as
> nsIPresShell::FrameNeedsReflow that I'm aware of require a target frame to
> initiate reflows.

Ah, ReconstructFrames reconstructs everything except the root scrollframe. Ho hum, that's annoying.

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

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

::: layout/generic/ScrollbarActivity.cpp
@@ +51,5 @@
> +  SetVisibility(GetHorizontalScrollbar(), true);
> +  if (!weakFrame.IsAlive()) {
> +    return;
> +  }
> +  SetVisibility(GetVerticalScrollbar(), true);

Can you explain why we need to restore scrollbar visibility here when we didn't before?

In fact why are we messing with the visibility? Shouldn't we just call SetIsActive(true)?

::: layout/generic/nsGfxScrollFrame.cpp
@@ +983,5 @@
> +  else if (!mScrollbarActivity &&
> +           LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars) != 0) {
> +    didSwitch = true;
> +    mScrollbarActivity = new ScrollbarActivity(do_QueryFrame(mOuter));
> +  }

I think you could unconditionally destroy the old mScrollbarActivity and create a new one if needed.

@@ +988,5 @@
> +
> +  if (didSwitch) {
> +    if (mVScrollbarBox) {
> +      mVScrollbarBox->MarkIntrinsicWidthsDirty();
> +      nsIFrame* child = mVScrollbarBox->GetChildBox();

Here I think you can unconditionally call nsIPresShell::FrameNeedsReflow(this, eStyleChange, NS_FRAME_DIRTY).

Comment 25

[Stephen A Pohl [:spohl]]

Thanks for your feedback, roc!

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #24)
> ::: layout/generic/ScrollbarActivity.cpp
> @@ +51,5 @@
> > +  SetVisibility(GetHorizontalScrollbar(), true);
> > +  if (!weakFrame.IsAlive()) {
> > +    return;
> > +  }
> > +  SetVisibility(GetVerticalScrollbar(), true);
> 
> Can you explain why we need to restore scrollbar visibility here when we
> didn't before?
> 
> In fact why are we messing with the visibility? Shouldn't we just call
> SetIsActive(true)?
> 

We did this before via nativescrollbar.css. However, the visibility wouldn't work dynamically in the case of a switch in scrollbar style. If we switched from old-style to overlay-style the property specified in nativescrollbar.css wouldn't be applied and the scrollbar would switch to full opacity and visibility after having faded away. If we had overlay scrollbars and switched to the old style the scrollbars would only show a white background until the page was either refreshed or the page resized.

Instead of trying to dynamically change the CSS rules that apply to a scrollbar I thought I would do the same as with the margins (bug 868416): take the visibility property out of the CSS and apply them when appropriate in C++. Doing this in ScrollbarActivity.cpp makes sense because we only ever have a ScrollbarActivity object when overlay scrollbars are in use and we only have to set visibility to 'hidden' in the overlay case.

> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +983,5 @@
> > +  else if (!mScrollbarActivity &&
> > +           LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars) != 0) {
> > +    didSwitch = true;
> > +    mScrollbarActivity = new ScrollbarActivity(do_QueryFrame(mOuter));
> > +  }
> 
> I think you could unconditionally destroy the old mScrollbarActivity and
> create a new one if needed.

I may be misunderstanding, but mScrollbarActivity is the way we detect the scrollbar style that's currently in use by Gecko: mScrollbarActivity is non-null when overlay scrollbars are in use, it is null when the old style is in use. We compare this against the current system preference (which may be different from Gecko now) by checking LookAndFeel::eIntID_UseOverlayScrollbars to see if there was a change in preference. If no change was detected mScrollbarActivity should be left untouched.

> @@ +988,5 @@
> > +
> > +  if (didSwitch) {
> > +    if (mVScrollbarBox) {
> > +      mVScrollbarBox->MarkIntrinsicWidthsDirty();
> > +      nsIFrame* child = mVScrollbarBox->GetChildBox();
> 
> Here I think you can unconditionally call
> nsIPresShell::FrameNeedsReflow(this, eStyleChange, NS_FRAME_DIRTY).

This ends up with a bunch of assertions (and ultimately a crash) because we're trying to mark frames dirty during reflow:
###!!! ASSERTION: can't mark frame dirty during reflow: '!mIsReflowing', file /Users/Stephen/Documents/mozilla-central/layout/base/nsPresShell.cpp, line 2461

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Stephen Pohl [:spohl] from comment #25)
> We did this before via nativescrollbar.css. However, the visibility wouldn't
> work dynamically in the case of a switch in scrollbar style. If we switched
> from old-style to overlay-style the property specified in
> nativescrollbar.css wouldn't be applied and the scrollbar would switch to
> full opacity and visibility after having faded away. If we had overlay
> scrollbars and switched to the old style the scrollbars would only show a
> white background until the page was either refreshed or the page resized.

I see. I think instead we should figure out why the style system isn't taking care of this automatically.

> > @@ +988,5 @@
> > > +
> > > +  if (didSwitch) {
> > > +    if (mVScrollbarBox) {
> > > +      mVScrollbarBox->MarkIntrinsicWidthsDirty();
> > > +      nsIFrame* child = mVScrollbarBox->GetChildBox();
> > 
> > Here I think you can unconditionally call
> > nsIPresShell::FrameNeedsReflow(this, eStyleChange, NS_FRAME_DIRTY).
> 
> This ends up with a bunch of assertions (and ultimately a crash) because
> we're trying to mark frames dirty during reflow:
> ###!!! ASSERTION: can't mark frame dirty during reflow: '!mIsReflowing',
> file /Users/Stephen/Documents/mozilla-central/layout/base/nsPresShell.cpp,
> line 2461

OK. It could be done in a post-reflow callback instead. But let's fix the style system issue and then see what else needs to be done.

Comment 27

[Stephen A Pohl [:spohl]]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #26)
> (In reply to Stephen Pohl [:spohl] from comment #25)
> > We did this before via nativescrollbar.css. However, the visibility wouldn't
> > work dynamically in the case of a switch in scrollbar style. If we switched
> > from old-style to overlay-style the property specified in
> > nativescrollbar.css wouldn't be applied and the scrollbar would switch to
> > full opacity and visibility after having faded away. If we had overlay
> > scrollbars and switched to the old style the scrollbars would only show a
> > white background until the page was either refreshed or the page resized.
> 
> I see. I think instead we should figure out why the style system isn't
> taking care of this automatically.

From looking at this again it seems that comment 13 and comment 23 are relevant here: In order for the style data to be rebuilt for the scrollbar we need to call nsPresContext::RebuildAllStyleData(NS_STYLE_HINT_FRAMECHANGE) which we can't do for root scrollframes, and ReconstructFrames reconstructs everything except the root scrollframe.

roc, could you tell me why rebuilding style data here is the preferred way to go? I thought that rebuilding style data was expensive and if we could be explicit about the scrollbar's visibility from ScrollbarActivity we would favor that approach. I may just be missing some context or history here. Thanks!

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We don't have to do a RebuildAllStyleData just to get media queries to reevaluate or 'visibility' style data to change.

Comment 29

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Maybe dbaron can tell us how to get media queries to reevaluate...

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Something that changes whether media queries might match should call:

  presContext->MediaFeatureValuesChanged(eRebuildStyleIfNeeded);

on each pres context in which the media queries might have changed.  (I suspect we have some code somewhere for iterating all pres contexts, which seems like it's needed for this sort of case.  I wonder if we have a generic "system metric changed" code somewhere... I don't remember any right now?)

Comment 31

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Though currently there's a codepath from nsBaseWidget::ThemeChanged to nsPresContext::ThemeChangedInternal, which takes a *slightly* more destructive codepath.

It might not be a bad idea to differentiate between the different sorts of things described in the comment at the end of nsPresContext::ThemeChangedInternal, though these sorts of setting changes are probably rare enough that it's not worthwhile.

I think you should probably find a way to reuse that codepath.

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sorry, the first method name in comment 31 should have been nsBaseWidget::NotifyThemeChanged

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Though, if the notification you get from the system is global rather than per-widget, it might make more sense to do change the global to per-window conversion at a layer closer to the pres context, which would mean using a different codepath.

Comment 34

[Stephen A Pohl [:spohl]]

Attachment: Patch

Thanks roc and dbaron!

This patch calls nsPresContext::ThemeChanged. As a result, we no longer have to call MarkIntrinsicWidthsDirty nor do we have to set visibility explicitly.

This patch still moves the values for position and z-index out of "@media all and (-moz-overlay-scrollbars)" in nativescrollbars.css because we would otherwise run into the assertions and misbehavior described in bug 870941 comment 12 and bug 870941 comment 14. The behavior for both old-style and overlay style scrollbars is as expected with this change. With the many other scrollbar bugs waiting for my attention I'm hoping that this is acceptable, or could be addressed in a follow-up bug. I couldn't figure out why this was happening yet.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

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

This patch is much better. However, the z-index rule move is still a problem.

I think we can fix https://bugzilla.mozilla.org/show_bug.cgi?id=870941#c12. I'll give you a patch to try.

::: layout/generic/nsGfxScrollFrame.cpp
@@ +980,5 @@
> +  }
> +  else if (!mScrollbarActivity &&
> +           LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars) != 0) {
> +    mScrollbarActivity = new ScrollbarActivity(do_QueryFrame(mOuter));
> +    mOuter->PresContext()->ThemeChanged();

Why not just issue the themechanged call unconditionally from scrollbarSystemMetricChanged?

::: toolkit/themes/osx/global/nativescrollbars.css
@@ +10,5 @@
>    -moz-binding: url(chrome://global/content/bindings/scrollbar.xml#scrollbar);
>    cursor: default;
>    background-color: white;
> +  position: relative;
> +  z-index: 2147483647;

This is going to change the rendering of a scrollable element with an abs-pos child, in the regular-scrollbars case. The child will be covered by the scrollbars now, whereas before (and on non-Mac platforms) the child would cover the scrollbars. This is a change we probably shouldn't make.

Comment 36

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: possible fix for the assertion issue

Try this!

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: better assertion fix

Comment 38

[Stephen A Pohl [:spohl]]

Attachment: Patch

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #35)
> This patch is much better. However, the z-index rule move is still a problem.
> 
> I think we can fix https://bugzilla.mozilla.org/show_bug.cgi?id=870941#c12.
> I'll give you a patch to try.

Thank you, roc! Your patch worked great and saved me a lot of time trying to figure this out.
 
> ::: layout/generic/nsGfxScrollFrame.cpp
> @@ +980,5 @@
> > +  }
> > +  else if (!mScrollbarActivity &&
> > +           LookAndFeel::GetInt(LookAndFeel::eIntID_UseOverlayScrollbars) != 0) {
> > +    mScrollbarActivity = new ScrollbarActivity(do_QueryFrame(mOuter));
> > +    mOuter->PresContext()->ThemeChanged();
> 
> Why not just issue the themechanged call unconditionally from
> scrollbarSystemMetricChanged?

We don't have easy access to the nsPresContexts of the scroll frames in scrollbarSystemMetricChanged. listener->GetPresShell()->GetPresContext()->ThemeChanged() doesn't work because the nsPresContexts that are returned aren't the ones for the scroll frames. With the current approach we trigger a reflow of all frames by calling ReconstructFrames and we then call ThemeChanged during reflow of the scroll frames.

> ::: toolkit/themes/osx/global/nativescrollbars.css
> @@ +10,5 @@
> >    -moz-binding: url(chrome://global/content/bindings/scrollbar.xml#scrollbar);
> >    cursor: default;
> >    background-color: white;
> > +  position: relative;
> > +  z-index: 2147483647;
> 
> This is going to change the rendering of a scrollable element with an
> abs-pos child, in the regular-scrollbars case. The child will be covered by
> the scrollbars now, whereas before (and on non-Mac platforms) the child
> would cover the scrollbars. This is a change we probably shouldn't make.

Thank you for the explanation, this makes sense. I reverted the changes to nativescrollbars.css in this patch.

Comment 39

[Stephen A Pohl [:spohl]]

Comment on attachment 773816 [details] [diff] [review]
better assertion fix

Works great for me!

Comment 40

[Stephen A Pohl [:spohl]]

Comment on attachment 773816 [details] [diff] [review]
better assertion fix

Reassigning, since matspal is on PTO.

It'd be good if we could land the patches in this bug as soon as possible to see whether or not it has a positive effect on bug 877097.

Comment 41

[Cameron McCormack (:heycam)]

Comment on attachment 773816 [details] [diff] [review]
better assertion fix

This seems OK; from my searching through layout/{generic,base}/ I can't see any places where we'd need to reframe based on position changing between static and relative.

Comment 42

[Stephen A Pohl [:spohl]]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0ba09885dab
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8bf739addfa

Comment 43

[Stephen A Pohl [:spohl]]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 636564
User impact if declined: Plugging in a mouse, connecting an external monitor or changing the scrollbar system preference will force a user to restart the browser or refresh all pages in the browser before the new style of scrollbars displays correctly.
Testing completed (on m-c, etc.): Extensive testing locally.
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 44

[Stephen A Pohl [:spohl]]

Comment on attachment 773816 [details] [diff] [review]
better assertion fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 636564
User impact if declined: Plugging in a mouse, connecting an external monitor or changing the scrollbar system preference and then resizing the browser window would cause scrollbar artifacts and the scrollbars don't appear in their right place anymore.
Testing completed (on m-c, etc.): Extensive testing locally.
Risk to taking this patch (and alternatives if risky): low
String or IDL/UUID changes made by this patch: none

Comment 45

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a0ba09885dab
https://hg.mozilla.org/mozilla-central/rev/e8bf739addfa

Comment 46

[Frank Wein [:mcsmurf]]

It looks like this patch caused a regression regarding website rendering, see Bug 894931 for more details, Comment 3 in that bug points out that this bug here regressed it (I tested mozilla-inbound builds to make sure).

Comment 47

[Stephen A Pohl [:spohl]]

I just determined locally that this is caused by attachment 773816 [details] [diff] [review]. I'll request checkin on these patches and ask for roc's feedback in bug 894931 because right now, I don't believe that the regression is worse than bug 870941 (which we'd run into without the assertion fix).

Requesting checkin for aurora and beta.

Comment 48

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/69cef757d7aa
https://hg.mozilla.org/releases/mozilla-aurora/rev/4304cf04f1e8

https://hg.mozilla.org/releases/mozilla-beta/rev/2cc1ab17c0de
https://hg.mozilla.org/releases/mozilla-beta/rev/98604d9f3c1f

Comment 49

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Stephen Pohl [:spohl] from comment #47)
> I just determined locally that this is caused by attachment 773816 [details] [diff] [review]
> [diff] [review]. I'll request checkin on these patches and ask for roc's
> feedback in bug 894931 because right now, I don't believe that the
> regression is worse than bug 870941 (which we'd run into without the
> assertion fix).
> 
> Requesting checkin for aurora and beta.

No, bug 894931 is absolutely unacceptable in aurora and beta, and probably shouldn't even appear in a nightly (we could get deluged with bug reports).  Can you back them out today?


bug 895181 comment 4 suggests a way to make the optimization roc was trying to make in the assertion patch here, but I think it should probably go in a separate bug for clarity.

Comment 50

[Stephen A Pohl [:spohl]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #49)
> No, bug 894931 is absolutely unacceptable in aurora and beta, and probably
> shouldn't even appear in a nightly (we could get deluged with bug reports). 
> Can you back them out today?
> 

Backout is occurring as part of bug 894931. Inbound was closed tonight and I couldn't watch the m-c tree. Will try again shortly.

> 
> bug 895181 comment 4 suggests a way to make the optimization roc was trying
> to make in the assertion patch here, but I think it should probably go in a
> separate bug for clarity.

I reopened bug 870941 to track the work on the assertion fix. It wasn't so much an optimization that we tried to apply here but to work around the issues mentioned in bug 870941 comment 12 and bug 870941 comment 14.

Comment 51

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: scrollbar zorder fix

Stephen, this patch might avoid the need to set position:relative and z-index in the style rules.

It won't always put the scrollbars on top. Positioned content could be displayed over the scrollbars. Then again, that's possible with the style rules you had. I'm not sure how "on top" these scrollbars need to be.

Comment 52

[Stephen A Pohl [:spohl]]

This patch works great! The risk seems to be very limited to overlay scrollbars only. If you don't mind, I'll attach your patch to bug 870941 because it's really fixing that bug there. I'll then go ahead and attach a second patch that removes position:relative and z-index from nativescrollbars.css.

Comment 53

[Stephen A Pohl [:spohl]]

I forgot to mention: in some cases we would probably want to have positioned content appear over the scrollbars. Bugs have already been filed, such as bug 868404.

Comment 54

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out the second patch in:
https://hg.mozilla.org/mozilla-central/rev/625e6c220d30
https://hg.mozilla.org/releases/mozilla-aurora/rev/668caad64c7a
https://hg.mozilla.org/releases/mozilla-beta/rev/e4f6d58191b9

If this breaks the tree in any way, please back out the other patch from bug 868498.

Comment 55

[Ed Morley [:emorley]]

Does this bug need reopening, given we've just had a partial backout?

Comment 56

[Stephen A Pohl [:spohl]]

(In reply to Ed Morley [:edmorley UTC+1] from comment #55)
> Does this bug need reopening, given we've just had a partial backout?

We don't need to reopen this bug because the patch that got backed out was really fixing bug 870941. I've reopened bug 870941 to track progress on a better fix and to separate the two issues.

Comment 57

[Ed Morley [:emorley]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #54)
> Backed out the second patch in:

This is causing conflicts on merge of inbound->m-c, I'm presuming due to it being backed out independantly on both ().

If we double-land on two repos, please can we use transplant or similar, so that Mercurial resolves automagically... thank you! :-)

Comment 58

[Ed Morley [:emorley]]

In fact the backouts weren't identical:
https://hg.mozilla.org/mozilla-central/rev/625e6c220d30
https://hg.mozilla.org/integration/mozilla-inbound/rev/f713a828fe36

Please can someone merge m-c -> inbound and resolve the differences.

Comment 59

[Stephen A Pohl [:spohl]]

Sorry about that. We should just back out f713a828fe36 from inbound.

Comment 60

[Ed Morley [:emorley]]

Sure, done :-)

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/cc580895795b

Now merged m-c -> inbound & we'll wait until that's green before merging back.

Comment 61

[Cornel Ionce [:noni] [Hubs QA]]

I am still able to reproduce this issue on Firefox 23 beta 9 (build ID: 20130725195523).

No matter what I set from System Preferences > General, the scroll bars are always shown.

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.7; rv:23.0) Gecko/20100101 Firefox/23.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:23.0) Gecko/20100101 Firefox/23.0

Comment 62

[Stephen A Pohl [:spohl]]

Overlay scrollbars have been disabled in Firefox 23 (bug 895203), so I don't think you were able to reproduce the issue described in this bug. This bug is specific about switching between scrollbar styles, which requires overlay scrollbars to be enabled. They are still enabled in FF24 and up.

Comment 63

[Cornel Ionce [:noni] [Hubs QA]]

I have checked in latest Aurora and Nightly and indeed, it works.
As I mentioned in comment 61, I changed the settings for the scroll bar and nothing happened (on beta). 
Sorry, my mistake here, got confused by it's fixed status for FF 23.

Should we mark this as disabled for FF 23?

Comment 64

[Stephen A Pohl [:spohl]]

Yes, I think it'd be correct to mark this as disabled for FF23 per bug 895203. Thanks, Cornel!

Comment 65

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:24.0) Gecko/20100101 Firefox/24.0

Verified as fixed on Firefox 24 Beta 8 (build ID:20130902131354).

Comment 66

[Cornel Ionce [:noni] [Hubs QA]]

Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:25.0) Gecko/20100101 Firefox/25.0

Verified as fixed on Firefox Aurora (build ID: 20130910004002).