Stephen A Pohl [:spohl] (OOO until 9/8) Reporter
	Description • 10 years ago

+++ This bug was initially created as a clone of Bug #870941 +++

The performance fix in bug 994861 made bug 870941 regress, especially bug 870941 comment 12. I believe that scrollbar switching is critical functionality and neither the regression in bug 994861 (or bug 987680 which caused the regression) should have higher priority. Ideally, we could find a way to fix all three issues in harmony, but we should probably go ahead and revert bug 994861 on trunk for now.

Note that this bug is also keeping us from landing the patches in bug 996848, as reftests are failing due to tiny scrollbars being displayed in the top left corner.

STR:
1. Select "Show scroll bars: Automatically..." or "When scrolling" in system preferences.
2. Open Firefox.
3. Switch system preference to "Show scroll bars: Always".

Actual:
If the page in Firefox did not have overlay scrollbars displayed (because the whole page fit in the window), tiny permanently visible scrollbars will appear in the top left corner. If overlay scrollbars were visible (because the page could be scrolled), no scrollbars will be displayed after switching the preferences.

Expected:
Scrollbar switching should be functional.

	Stephen A Pohl [:spohl] (OOO until 9/8) Reporter
	Comment 1 • 10 years ago

Timothy, do you agree that backing out bug 994861 for now is the right course of action?

	Timothy Nikkel (:tnikkel) Assignee
	Comment 2 • 10 years ago

Ah, so I'm guessing we don't reconstruct the scrollbars when the scrollbar prefs change so we need to always have the overlay scrollbar styles on mac? That's the part I missed when creating that patch.

We should be able to back out the mac changes from bug 994861 since that was only a Windows perf regression, and on Windows overlay scrollbars are conditional on the environment being metro (http://mxr.mozilla.org/mozilla-central/source/widget/windows/nsLookAndFeel.cpp#486) which I don't think it's possible to change dynamically. Patch coming up.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 4 • 10 years ago

(In reply to Timothy Nikkel (:tn) from comment #2)
> Ah, so I'm guessing we don't reconstruct the scrollbars when the scrollbar
> prefs change so we need to always have the overlay scrollbar styles on mac?

Looked into the history of this with Stephen, the key discussion starts at https://bugzilla.mozilla.org/show_bug.cgi?id=868498#c27 and continues for several comments. It appears that the ThemeChanged calls that were added there weren't enough to cause reconstruction of the scrollbars.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 5 • 10 years ago

I filed bug 1008345 for the above comment.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 6 • 10 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/73108558281d

	Avi Halachmi (:avih)
	Comment 7 • 10 years ago

(In reply to Stephen Pohl [:spohl] from comment #0)
> The performance fix in bug 994861 made bug 870941 regress, especially bug
> 870941 comment 12. I believe that scrollbar switching is critical
> functionality and neither the regression in bug 994861 (or bug 987680 which
> caused the regression) should have higher priority. Ideally, we could find a
> way to fix all three issues in harmony, but we should probably go ahead and
> revert bug 994861 on trunk for now.

It might be worth noting that when an "automatic regression bug" is filed, such as bug 994861, there's no implied expectation to automatically launch a full investigation or to make that micro-regression go away by all means.

These regressions bugs are filed first and foremost to make developers aware of them, with the hope that if something was overlooked and it could be easily fixed, then the developer may do something about it.

But if fixing the regression goes beyond the scope of the bug which triggered it, then IMO it's not necessarily the best approach touch other areas for that.

Of course, if the developer feels like s/he wants to or that there's a good reason to, sure, go ahead and touch whatever you think is worth touching, but please don't do so just because the regression bug exists, but rather because you feel it's worth touching.

See further discussion on these regression bugs at bug 990644.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 8 • 10 years ago

We didn't need to back out the Windows part of the patch that fixed the perf regression. So all is well. We got the perf fix and fixed this bug.

	Carsten Book [:Tomcat]
	Comment 9 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/73108558281d

	Timothy Nikkel (:tnikkel) Assignee
	Comment 10 • 10 years ago

Comment on attachment 8420232 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 994861
User impact if declined: switching osx scrollbars from overlay (disappearing) to classic always visible won't work
Testing completed (on m-c, etc.): this had been the status quo for quite a while before bug 994861 upset this. so it's had plenty of testing even if it just landed on m-c.
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 10 years ago

https://hg.mozilla.org/releases/mozilla-aurora/rev/026ab1c41187

	Paul Silaghi, QA [:pauly]
	Comment 12 • 10 years ago

Reproduced in Nightly 2014-05-07.
Verified fixed FF 32.0a1 (2014-05-20, Mac OS X 10.8.5.

	Paul Silaghi, QA [:pauly]
	Comment 13 • 10 years ago

Verified fixed FF 31b3, OS X 10.8.5