Scrollbar switching on OSX is broken

VERIFIED FIXED in Firefox 31

Status

()

defect
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: spohl, Assigned: tnikkel)

Tracking

({regression})

unspecified
mozilla32
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox31+ verified, firefox32 verified)

Details

(Whiteboard: [lion-scrollbars+])

Attachments

(1 attachment)

+++ 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.
Timothy, do you agree that backing out bug 994861 for now is the right course of action?
Flags: needinfo?(tnikkel)
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.
Assignee: nobody → tnikkel
Flags: needinfo?(tnikkel)
Posted patch patchSplinter Review
Attachment #8420232 - Flags: review?(roc)
(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.
I filed bug 1008345 for the above comment.
(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.
Blocks: 990644
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.
No longer blocks: 990644
https://hg.mozilla.org/mozilla-central/rev/73108558281d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
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
Attachment #8420232 - Flags: approval-mozilla-aurora?
Attachment #8420232 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Keywords: verifyme
Reproduced in Nightly 2014-05-07.
Verified fixed FF 32.0a1 (2014-05-20, Mac OS X 10.8.5.
Status: RESOLVED → VERIFIED
Verified fixed FF 31b3, OS X 10.8.5
You need to log in before you can comment on or make changes to this bug.