Last Comment Bug 636564 - [10.7] add support for new scrollbar style in Mac OS X 10.7 Lion
: [10.7] add support for new scrollbar style in Mac OS X 10.7 Lion
Status: VERIFIED FIXED
[metro-mvp][parity-opera][parity-webk...
: feature
Product: Core
Classification: Components
Component: Widget: Cocoa (show other bugs)
: Trunk
: x86 Mac OS X
: P1 normal with 59 votes (vote)
: mozilla24
Assigned To: Stephen A Pohl [:spohl]
:
: Markus Stange [:mstange]
Mentors:
https://wiki.mozilla.org/Lion_Scrollb...
Depends on: 868404 869519 871215 876310 895499 920550 656990 691609 865806 868316 868396 868397 868399 868416 868420 868421 868432 868498 868648 868659 869314 870917 870941 873010 873012 873651 874486 877097 878705 881022 886160 887977 895203 896443 904561 918996
Blocks: lion-compatibility 863920 864596 770453 777610 778810 780540 880753 959025 1081072
  Show dependency treegraph
 
Reported: 2011-02-24 13:08 PST by Josh Aas
Modified: 2016-07-15 04:19 PDT (History)
99 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
affected
-
-
-
24+


Attachments
a small starting point (13.51 KB, patch)
2011-07-18 14:02 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
wip (28.76 KB, patch)
2011-07-29 02:06 PDT, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
Firefox scrollbars in Lion on a black background (244.96 KB, image/png)
2011-08-15 14:23 PDT, Bart Szyszka
no flags Details
Firefox scrollbars in Lion on a black background (w/ stuff around browser) (303.32 KB, image/png)
2011-08-15 14:32 PDT, Bart Szyszka
no flags Details
part 1, v1: handle scrollbar margins This functionality is already in trunk, so marking this patch as obsolete. (1.97 KB, patch)
2011-11-12 07:57 PST, Markus Stange [:mstange]
roc: review+
Details | Diff | Splinter Review
part 2, v1: don't add margins for collapsed scrollbars (1.41 KB, patch)
2011-11-12 08:03 PST, Markus Stange [:mstange]
roc: review+
Details | Diff | Splinter Review
part 3, v1: adjust scrollbar sizes so that horizontal and vertical scrollbars don't overlap (4.40 KB, patch)
2011-11-12 08:05 PST, Markus Stange [:mstange]
roc: review+
Details | Diff | Splinter Review
part 4, v1: respect resizer minimum size (2.04 KB, patch)
2011-11-12 08:07 PST, Markus Stange [:mstange]
roc: review+
Details | Diff | Splinter Review
part 5, v1: set minimum size on image-based resizers (721 bytes, patch)
2011-11-12 08:08 PST, Markus Stange [:mstange]
enndeakin: review+
Details | Diff | Splinter Review
part 6, v1: don't use the native resizer with overlay scrollbars because the border looks bad in that context (1.42 KB, patch)
2011-11-12 08:10 PST, Markus Stange [:mstange]
enndeakin: review+
Details | Diff | Splinter Review
part 7, v1: add overlay-scrollbars system metric that matches when the Mac OS 10.7 overlay scrollbar pref is set (14.51 KB, patch)
2011-11-12 08:14 PST, Markus Stange [:mstange]
roc: review+
jaas: review+
Details | Diff | Splinter Review
part 8, v1: add Lion scrollbar native rendering (10.48 KB, patch)
2011-11-12 08:16 PST, Markus Stange [:mstange]
jaas: review+
Details | Diff | Splinter Review
part 9, v1: add CSS for overlay scrollbars (1.03 KB, patch)
2011-11-12 08:18 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 10, v1: add nsIScrollbarHolder interface (15.42 KB, patch)
2011-11-12 08:22 PST, Markus Stange [:mstange]
roc: review+
Details | Diff | Splinter Review
part 11, v1: add ScrollbarActivity class (19.53 KB, patch)
2011-11-12 08:28 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 12, v1: report scrollbar activity from normal scroll frames (6.80 KB, patch)
2011-11-12 08:29 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 13, v1: report scrollbar activity from trees (5.08 KB, patch)
2011-11-12 08:31 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 14, v1: hide inactive scrollbars (744 bytes, patch)
2011-11-12 08:32 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 15, v1: make some failing tests pass (6.57 KB, patch)
2011-11-12 08:34 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
bundle of updated patches (14.92 KB, text/plain)
2012-04-03 16:21 PDT, Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
no flags Details
Rendering issue with scrollbars (1.04 MB, image/png)
2012-10-26 10:13 PDT, Josiah Bruner [:JosiahOne] (needinfo for responses)
no flags Details
Issues in Responsive mode (443.25 KB, image/png)
2012-10-26 11:15 PDT, Cam
no flags Details
Alt rendering issue with scrollbar (433.45 KB, image/png)
2012-10-26 11:17 PDT, Cam
no flags Details
Add GetScrollbarBox method to nsIScrollbarMediator instead of creating a new interface for it (10.42 KB, patch)
2012-11-15 07:51 PST, Markus Stange [:mstange]
no flags Details | Diff | Splinter Review
part 2, v1: don't add margins for collapsed scrollbars (1.40 KB, patch)
2013-02-06 16:24 PST, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 3, v1: adjust scrollbar sizes so that horizontal and vertical scrollbars don't overlap (3.98 KB, patch)
2013-02-06 16:25 PST, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 4, v1: respect resizer minimum size (2.78 KB, patch)
2013-02-06 16:27 PST, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 5, v1: set minimum size on image-based resizers (842 bytes, patch)
2013-02-06 16:28 PST, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 6, v1: don't use the native resizer with overlay scrollbars because the border looks bad in that context (1.41 KB, patch)
2013-02-06 16:30 PST, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 7, v1: add overlay-scrollbars system metric that matches when the Mac OS 10.7 overlay scrollbar pref is set (14.75 KB, patch)
2013-02-06 16:31 PST, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 8, v1: add Lion scrollbar native rendering (10.51 KB, patch)
2013-02-06 16:32 PST, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 10, v1: add nsIScrollbarHolder interface (14.00 KB, patch)
2013-02-06 16:34 PST, Stephen A Pohl [:spohl]
no flags Details | Diff | Splinter Review
part 11, v1: add ScrollbarActivity class (18.26 KB, patch)
2013-02-06 16:35 PST, Stephen A Pohl [:spohl]
no flags Details | Diff | Splinter Review
part 12, v1: report scrollbar activity from normal scroll frames (5.19 KB, patch)
2013-02-06 16:37 PST, Stephen A Pohl [:spohl]
no flags Details | Diff | Splinter Review
part 13, v1: report scrollbar activity from trees (5.95 KB, patch)
2013-02-06 16:38 PST, Stephen A Pohl [:spohl]
no flags Details | Diff | Splinter Review
part 15, v1: make some failing tests pass (6.58 KB, patch)
2013-02-06 16:40 PST, Stephen A Pohl [:spohl]
no flags Details | Diff | Splinter Review
Add GetScrollbarBox method to nsIScrollbarMediator instead of creating a new interface for it (10.42 KB, patch)
2013-02-06 16:57 PST, Stephen A Pohl [:spohl]
roc: feedback-
Details | Diff | Splinter Review
part 4, v1: respect resizer minimum size (17.84 KB, patch)
2013-02-22 15:07 PST, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 8, v1: add Lion scrollbar native rendering (26.13 KB, patch)
2013-02-22 15:13 PST, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 10, v1: add nsIScrollbarOwner interface (71.08 KB, patch)
2013-02-22 15:19 PST, Stephen A Pohl [:spohl]
no flags Details | Diff | Splinter Review
part 11, v1: add ScrollbarActivity class (25.74 KB, patch)
2013-02-22 15:25 PST, Stephen A Pohl [:spohl]
no flags Details | Diff | Splinter Review
part 13, v1: report scrollbar activity from trees (6.01 KB, patch)
2013-02-22 15:29 PST, Stephen A Pohl [:spohl]
no flags Details | Diff | Splinter Review
part 15, v1: make some failing tests pass (8.88 KB, patch)
2013-02-22 15:32 PST, Stephen A Pohl [:spohl]
no flags Details | Diff | Splinter Review
part 16, v1: make scrollbars wider on hover (3.43 KB, patch)
2013-02-22 15:38 PST, Stephen A Pohl [:spohl]
no flags Details | Diff | Splinter Review
part 5, v1: set minimum size on image-based resizers (818 bytes, patch)
2013-03-05 15:30 PST, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 7, v1: add overlay-scrollbars system metric that matches when the Mac OS 10.7 overlay scrollbar pref is set (14.90 KB, patch)
2013-03-05 15:34 PST, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 9, v1: add CSS for overlay scrollbars (1.01 KB, patch)
2013-03-05 15:41 PST, Stephen A Pohl [:spohl]
roc: review+
Details | Diff | Splinter Review
part 11, v1: add ScrollbarActivity class (25.96 KB, patch)
2013-03-05 15:48 PST, Stephen A Pohl [:spohl]
no flags Details | Diff | Splinter Review
part 14, v1: hide inactive scrollbars (720 bytes, patch)
2013-03-05 15:52 PST, Stephen A Pohl [:spohl]
roc: review+
Details | Diff | Splinter Review
part 16, v1: make scrollbars wider on hover (3.40 KB, patch)
2013-03-05 15:55 PST, Stephen A Pohl [:spohl]
no flags Details | Diff | Splinter Review
part 3, v1: adjust scrollbar sizes so that horizontal and vertical scrollbars don't overlap (3.98 KB, patch)
2013-04-16 15:01 PDT, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 4, v1: respect resizer minimum size (2.08 KB, patch)
2013-04-16 15:05 PDT, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 8, v1: add Lion scrollbar native rendering (11.33 KB, patch)
2013-04-16 15:34 PDT, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 10, v1: add nsIScrollbarOwner interface (11.71 KB, patch)
2013-04-16 15:41 PDT, Stephen A Pohl [:spohl]
no flags Details | Diff | Splinter Review
part 11, v1: add ScrollbarActivity class (23.83 KB, patch)
2013-04-16 15:44 PDT, Stephen A Pohl [:spohl]
roc: review+
Details | Diff | Splinter Review
part 13, v1: report scrollbar activity from trees (6.05 KB, patch)
2013-04-16 15:45 PDT, Stephen A Pohl [:spohl]
roc: review+
Details | Diff | Splinter Review
part 15, v1: make some failing tests pass (6.58 KB, patch)
2013-04-16 15:48 PDT, Stephen A Pohl [:spohl]
no flags Details | Diff | Splinter Review
part 16, v1: make scrollbars wider on hover (3.40 KB, patch)
2013-04-16 15:51 PDT, Stephen A Pohl [:spohl]
roc: review+
Details | Diff | Splinter Review
part 2, v1: don't add margins for collapsed scrollbars (1.40 KB, patch)
2013-04-16 16:19 PDT, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 10, v1: add nsIScrollbarOwner interface (11.73 KB, patch)
2013-04-17 12:10 PDT, Stephen A Pohl [:spohl]
roc: review+
Details | Diff | Splinter Review
part 15, v1: make some failing tests pass (7.50 KB, patch)
2013-04-17 12:12 PDT, Stephen A Pohl [:spohl]
roc: review+
Details | Diff | Splinter Review
part 10, v1: add nsIScrollbarOwner interface (11.73 KB, patch)
2013-04-19 15:44 PDT, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 13, v1: report scrollbar activity from trees (6.58 KB, patch)
2013-04-19 15:49 PDT, Stephen A Pohl [:spohl]
roc: review+
Details | Diff | Splinter Review
part 11, v1: add ScrollbarActivity class (24.16 KB, patch)
2013-04-19 15:57 PDT, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
part 13, v1: report scrollbar activity from trees (6.60 KB, patch)
2013-04-22 00:57 PDT, Stephen A Pohl [:spohl]
roc: review+
Details | Diff | Splinter Review
part 2, v2: ensure minimum size of 0x0 for scrollbars (1.17 KB, patch)
2013-04-22 17:56 PDT, Stephen A Pohl [:spohl]
roc: review-
Details | Diff | Splinter Review
part 2, v2-alternative: ensure minimum size of 0x0 for scrollbars (1.25 KB, patch)
2013-04-22 17:58 PDT, Stephen A Pohl [:spohl]
roc: review+
Details | Diff | Splinter Review
part 17: Skip reftests for B2G that have become invalid (1.82 KB, patch)
2013-04-23 10:39 PDT, Stephen A Pohl [:spohl]
roc: review+
Details | Diff | Splinter Review
Combined patches (68.57 KB, patch)
2013-05-01 16:32 PDT, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
small adjustment to part 7 (889 bytes, patch)
2013-05-02 07:32 PDT, Stephen A Pohl [:spohl]
ehsan: review+
Details | Diff | Splinter Review
Combined patches (84.23 KB, patch)
2013-05-02 07:53 PDT, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review
Combined patches (84.04 KB, patch)
2013-05-02 07:55 PDT, Stephen A Pohl [:spohl]
spohl.mozilla.bugs: review+
Details | Diff | Splinter Review

Description Josh Aas 2011-02-24 13:08:58 PST

    
Comment 1 Josh Aas 2011-03-19 12:19:32 PDT
Until we know we have an API that will work for us to draw the new scrollbar style we should file a bug with Apple about it. We don't want them to ship without something we can use.
Comment 2 Josh Aas 2011-04-15 19:40:35 PDT
The latest developer update to 10.7 makes this situation better and worse. It's better because we inherit the new scrollbar style - the HITheme APIs we use have been updated. It's worse because the style is different enough that our math for setting aside space for scrollbar buttons is wrong and things don't draw correctly.

Given the pending release of 10.7 and the seriousness of this bug I think this need to block macaw and Firefox 5.
Comment 3 Josh Aas 2011-04-15 19:51:19 PDT
Filed Apple bug #9294340 about the backwards compatibility implications here with the following note:

We definitely want to be able to draw the new-style scrollbars via HITheme but the currently implemented way of doing it breaks compatibility with existing and past versions of Firefox. This is a perfect time to use the "version" field of "HIThemeTrackDrawInfo". Applications that have been using version "0" would get an old-style scrollbar layout with buttons, people using version 1 could get what Apple has implemented currently - the new, button-less style.
Comment 4 Stefan Arentz [:st3fan] 2011-04-15 21:57:58 PDT
This is not just about looks. Lion also adds:

* the bounce effect, which they call 'elasticity'
* putting the scrollbars on top of the content
* auto-hiding the scrollbars
* showing scrollbars when you hover over them

To look and feel like a well behaving app on OS X Lion we will need to implement all the above.
Comment 5 Josh Aas 2011-04-16 00:17:39 PDT
I agree that we should look into that stuff but right now I'm mostly concerned about the basic look and not having things be outright broken. Also, a lot of those features seem to be turned off by default (none of the things you listed are on by default on my system).
Comment 6 Nomis101 2011-04-16 04:51:35 PDT
Hm, on my DP2 all listed things are on by default (including the reverted scroll behavior with the Magic Mouse).
Comment 7 Josh Aas 2011-04-16 10:05:32 PDT
The pref says it is hardware dependent, I don't use an Apple mouse so that might be the difference. We can test when writing patches by just turning that stuff on.
Comment 8 Daniel Veditz [:dveditz] 2011-04-18 10:51:47 PDT
At this point it doesn't look like a 4.0.x chemspill issue, minusing for mozilla-2.0. If we do chemspill for other reasons, and there's a fix already tested in FF5-aurora then we can reconsider.
Comment 9 JP Rosevear [:jpr] 2011-04-19 15:15:49 PDT
We would not hold 5 beta cut over for this but criticallity of this goes up as time progresses due to 10.7 release.  If there is a patch that is low risk, can still be nominated for approval on the 5 release train.
Comment 10 Steven Michaud [:smichaud] (Retired) 2011-05-10 15:38:52 PDT
I quite frankly hate the new Lion scrollbars, and suspect I'm not
alone.  Also, the problems with them are much worse using a file
picker than they are with browser windows -- it actually becomes
impossible to scroll the entire list of files and directories.

So I agree with Josh that we need to figure out how to keep using the
old-style scrollbars.  At some point we (probably) also want to start
supporting the new style scrollbars (if only as an option, and maybe
not the default option).  But I think we need to be very cautious
about ever completely dropping support for the old scrollbars.
Comment 11 Steven Michaud [:smichaud] (Retired) 2011-05-10 15:45:24 PDT
Google Chrome has some of the same problems with the new Lion scrollbars that we do.  But the Chrome file picker is much less badly effected than ours is.
Comment 12 Stefan Arentz [:st3fan] 2011-05-10 16:32:20 PDT
(In reply to comment #10)
> I quite frankly hate the new Lion scrollbars, and suspect I'm not
> alone.

Note that the scrollbar behaviour/style is somewhat configurable. See the Appearance preference pane: http://sa.tk/9162f75f.png

I think this appeared in the last dev preview.

I very mich do not agree that we should keep using the old scroll bars. The new style comes with Lion and that is what all apps will be using. Chrome Canary is already doing it.

We worked very hard to make Firefox look like a native app. By not going along with new style UI elements that Apple introduces we will be doing a step backward.
Comment 13 Steven Michaud [:smichaud] (Retired) 2011-05-10 16:50:46 PDT
Stefan, this can't be decided by either of us.  A lot depends on how many people share my opinion, or share yours.

But at the very least we need to support old-style scrollbars on Lion until we can get the new-style scrollbars working properly -- which I suspect won't be easy.
Comment 14 Stefan Arentz [:st3fan] 2011-05-10 17:08:03 PDT
Steven, but I can have a strong opinion about things, no? :-)

I think we have to be pragmatic. Do we want Firefox to look and behave like any other native app or not. Because when Lion comes out, those old style scroll bars will be rare. Are we willing to loose users on OS X by not following new UI paradigms that Apple introduces?
Comment 15 Steven Michaud [:smichaud] (Retired) 2011-05-10 17:25:48 PDT
> but I can have a strong opinion about things, no? :-)

I can, too :-)

I don't normally have strong opinions on user-interface issues.  But
this is a rare exception.  I truly think that the new scroll-bars, as
they're currently implemented (in all the DPs up to the current one,
which is build 11A444d) are a disaster -- maybe not on the order of
"New Coke", but not that far off, either.

I won't go into detail (though I could), because a user-interface
discussion isn't really relevant to this bug, and would make it much
harder to read.

Even if I loved them, though, the problem remains that they seriously
break things in current versions of Firefox -- particularly the file
picker.  And I strongly suspect these problems won't be easy to fix.
Having old-fashioned scrollbars is better than having badly broken
ones.  Surely we can agree on that.

> Because when Lion comes out, those old style scroll bars will be
> rare.

I wouldn't be so sure.  Though a lot depends on how many others hate
the new scrollbars.  And whether Apple changes them before Lion's
release to make them less hateful to people like me.
Comment 16 d 2011-05-11 04:09:38 PDT
(In reply to comment #15)
> I won't go into detail (though I could), because a user-interface
> discussion isn't really relevant to this bug, and would make it much
> harder to read.

For those of us without 10.7, an un-biased description (and then one with your opinions included, perhaps) may help us decide, though.
Comment 17 Josh Aas 2011-05-11 05:57:42 PDT
Apple is looking into my bug report and changing the situation with every seed. Let's not start a general debate on Apple's 10.7 scrollbar choices in public until things settle down or someone is actually writing code for this. It's just going to result in a lot of clutter.

We should probably make the potential temporary solution of fixing our existing scrollbars a separate bug.
Comment 18 Nomis101 2011-05-11 10:15:40 PDT
(In reply to comment #14)
> Do we want Firefox to look and behave like
> any other native app or not.
Not any, iTunes have different scrollbars, different to 10.6 and different to 10.7. Why ever.
(But I also think FF should look like a native app)

(In reply to comment #15)
> I truly think that the new scroll-bars, as
> they're currently implemented (in all the DPs up to the current one,
> which is build 11A444d) are a disaster -- 
I agree that the current 10.7 scrollbars are a disaster. But is it sure, that this are the final 10.7 scrollbars? 10.7 is only a DP. Sometimes this scrollbars seems to me like a placeholder for a scrollbar...
Comment 19 Steven Michaud [:smichaud] (Retired) 2011-05-13 13:10:21 PDT
Even when the new style Lion scrollbars don't autohide, Firefox
doesn't display them properly -- for example, there's a blank space
where the arrow keys would normally appear.  I've spun this problem
off into bug 656990.

I've posted a patch there, and a tryserver build should be available
in several hours.
Comment 20 Steven Michaud [:smichaud] (Retired) 2011-05-13 13:29:24 PDT
The most serious problem with Lion's new scrollbars that I've observed
(which happens in filepickers), turns out not to be specific to
Firefox.  Also, it only happens when the new scrollbars are (globally)
set to autohide.

Currently Lion's scrollbars don't autohide in Firefox browser windows.
The reason appears to be that when you use Appearance Manager calls
(specifially HITheme calls) to manage scrollbars (as we now do on OS
X), there isn't (yet) a way to make them autohide.  Josh is (I think)
trying to get Apple to support autohiding of the scrollbars via
HITheme calls.

This *isn't* true for the filepickers used by Firefox, or any other
app.  Whether or not a filepicker's scrollbars autohide is governed by
a global preference in the Appearance panel -- "Show scroll bars".

Here's a brief description of the problem:

It's possible for the lowest item in a filepicker to be obscured by a
horizontal scrollbar, and therefore unselectable ... at least until a
timer fires and the horizontal scrollbar disappears (autohides).  But
if you don't know to wait several seconds for the horizontal scrollbar
to disappear, you think the filepicker is simply broken.

Since this isn't Firefox's problem, and since the user has a way to
fix it (choose "Show scroll bars" "Always"), it doesn't seem that we
need to do anything about it.
Comment 21 Steven Michaud [:smichaud] (Retired) 2011-05-13 13:49:39 PDT
For the record, here's how to find out programmatically what the
current setting is for "Show scroll bars":

The "key" is "AppleShowScrollBars".

The possible "values" are:

"Automatic"     (== "Automatically based on input device", the default)
"WhenScrolling" (== "When scrolling")
"Always"        (== "Always")

(This is true as of the current DP, which is build 11A444d.)
Comment 22 Steven Michaud [:smichaud] (Retired) 2011-05-16 12:35:01 PDT
Someone please tell me if I'm wrong, but it looks like Apple's latest
Lion DP (build 11A459e) has made a significant change in how scrollbar
autohiding works:

As of build 11A459e, the following two features are (globally) off by
default if any kind of traditional mouse (including Apple's Mighty
Mouse) is connected to your computer (that is, if you've chosen the
default setting of "Show scroll bars" "Automatically based on input
device"):

* putting the scrollbars on top of the content
* auto-hiding the scrollbars

They're still both on by default if you're running a laptop with no
mouse attached (if you're only using the built-in trackpad).
Comment 23 Evan Ren 2011-06-10 17:51:09 PDT
why not shipping two separate versions for both snow leopard and lion?
Comment 24 Asa Dotzler [:asa] 2011-07-17 22:29:03 PDT
Steven, we are tracking this for Firefox 6 (in the middle of Beta) and I'm gonna guess that it's too late to get something of a fix here. Am I wrong? 

Limi, Marcia, what's the experience like on the latest Lion? Limi, what kind of priority do you think this is. I'm just not on Mac enough to know.
Comment 25 Steven Michaud [:smichaud] (Retired) 2011-07-18 08:07:59 PDT
> Steven, we are tracking this for Firefox 6 (in the middle of Beta)
> and I'm gonna guess that it's too late to get something of a fix
> here. Am I wrong?

I assume you're talking about scrollbar autohiding.  If so, you're
correct -- there's no prospect of getting support for this into FF 6.

The problem is that the HITheme API (which we use to draw scrollbars,
buttons and the like) doesn't currently support autohiding, and we
don't know when (or even if) it will do so.

It'd be difficult to change from the HITheme API, because it allows us
to have the OS draw to graphics contexts (in effect to buffers),
rather than directly to the display.  It's conceivable the same goal
can be achieved with a different API (perhaps with the addition of
some judicious hacking).  But anything like this will take time to
investigate and implement.
Comment 26 Markus Stange [:mstange] 2011-07-18 13:45:48 PDT
(In reply to bug 672050 comment #4)
> Markus, do you think CoreUI might be a viable option for drawing
> scrollbars (instead of the HITheme API we're currently using)?

Could be. You can find out: Attach gdb to a native app that uses these scrollbars, set a breakpoint in CUIDraw and see whether it's hit when you cause a repaint of the scrollbar (for example by moving your mouse over it).
"po $rdx" should give you the widget drawing settings, see bug 668195 comment 0.

> And might this allow us to support autohiding the scrollbars on OS X 10.7
> (which the HITheme API currently doesn't support)?

It might. Maybe the drawing dictionary has a field for the opacity or for the elapsed time.

In any case, if CoreUI doesn't give us the right API, we can always draw the scrollbar manually; it's just a rounded rectangle.

As for the actual fade-out effect: OS X can't help us there. We have to handle it all inside Gecko. Only we know whether the mouse is over the scrollbar or whether the scroll position has changed recently. And the fade-out animation needs to be done using traditional methods, i.e. by periodically redrawing the scrollbar and measuring the elapsed time (from which the opacity is calculated).

And the hardest part of the whole thing is putting the scrollbar on top of the content.
Comment 27 Markus Stange [:mstange] 2011-07-18 14:02:56 PDT
Created attachment 546630 [details] [diff] [review]
a small starting point

Here's what I've managed to come up with today. This patch is nothing more than a starting point and needs to be cleaned up considerably.

Brief explanation:
 - Putting scrollbars on top of content happens in nativescrollbars.css using
   negative margins. In order for that to work, we need to return true for
   eMetric_ScrollbarsCanOverlapContent in nsLookAndFeel.mm, and we need to make
   a change to nsGfxScrollFrame.cpp so that content can not overlap the
   scrollbar. That change mirrors the one made in bug 631337 for the resizer.
   We also can no longer mark scrollbars as opaque in GetWidgetTransparency.
 - nsScrollbarFrame::mLastActivity holds a timestamp that we can use to
   determine whether to draw the scrollbar or not. We reset that timestamp when
   the scrollbar's curpos attribute changes, or when it's drawn while hovered.
 - Fading out the scrollbar happens by basing the drawing opacity on the elapsed
   time since nsScrollbarFrame::mLastActivity. We get it to animate by calling
   QueueAnimatedContentForRefresh on every draw until it's invisible.
 - The rendering is done manually (it's just a translucent black rectangle
   for now) until we know whether CoreUI can do it.

Problems with this patch:
 - No checks for Lion or "AppleShowScrollBars" pref
 - No fade-in animation (is there one for native scrollbars?)
 - Web page layout doesn't ignore scrollbars; it still makes room for them as
   long as it can (example: 14px right margin on google.com)
 - Textarea resizer in the wrong position
 - Dependency inversion: nsNativeThemeCocoa.mm includes nsScrollbarFrame.h,
   but widget/ shouldn't depend on layout/

Unfortunately that's all I have time for for now. Steven, feel free to pick this up :)
Comment 28 Steven Michaud [:smichaud] (Retired) 2011-07-18 14:19:22 PDT
Thanks, Markus!

Your work takes us a lot closer to the goal than we were.  But of course we still have a long way to go.

I'll get to this when I can (unless someone else beats me to it).  But most of my time is spent putting out fires, and I generally don't have time for longer-term projects.
Comment 29 Stefan Arentz [:st3fan] 2011-07-18 14:34:33 PDT
(In reply to comment #27)

>  - No fade-in animation (is there one for native scrollbars?)

Isn't it possible to do this with CoreAnimation? I have done similar things (in pure Cocoa apps, so it might be different) where you simply hide a control like a button in a Core Animation block to get a nice fade out.
Comment 30 Markus Stange [:mstange] 2011-07-19 12:00:23 PDT
(In reply to comment #29)
> (In reply to comment #27)
> 
> >  - No fade-in animation (is there one for native scrollbars?)

What I meant here is that I only implemented the fade-*out* animation, not yet the fade-*in* animation.

> Isn't it possible to do this with CoreAnimation?

Well, "possible" isn't really the problem. It's possible with the current approach, too, it just needs a little more code, which I haven't written yet.

We can't use CoreAnimation in Firefox because we've taken over too much of the rendering stack. Our OpenGL layer compositor and our CSS transition / animation implementation are basically a cross-platform replacement for CoreAnimation.

In fact, using CSS transitions for the scrollbar animation wouldn't be such a bad idea. I've tried it briefly and it doesn't work out-of-the-box (maybe because the scrollbar element is native anonymous content?), but it can probably made to work.
Comment 31 Markus Stange [:mstange] 2011-07-20 16:37:17 PDT
(In reply to comment #27)
>  - No fade-in animation (is there one for native scrollbars?)

The answer is: No, native scrollbars appear instantly when scrolling happens. Only their disappearance is animated.
Comment 32 Philipp Kewisch [:Fallen] 2011-07-27 09:29:49 PDT
I'm not sure if these properties go together, but in Adium I see an auto-hide scrollbar on the users list that is more narrow than the standard scrollbar. Both of these properties would be great for Calendar/Lightning.
Comment 33 Markus Stange [:mstange] 2011-07-29 02:06:27 PDT
Created attachment 549325 [details] [diff] [review]
wip

This one is a bit better. Here's a test build:
https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mstange@themasta.com-d728455128c0/try-macosx64/firefox-8.0a1.en-US.mac.dmg

This patch fixes the metrics, gets the correct look by using CoreUI (widget type kCUIWidgetOverlayScrollBar) and uses the right animation times.

(In reply to comment #27)
> Problems with this patch:
>  - No checks for Lion or "AppleShowScrollBars" pref

Added nsLookAndFeel::UseOverlayScrollbars(), eMetric_ScrollbarsOverlayContent, :-moz-system-metric(overlay-scrollbars) and @media (-moz-overlay-scrollbars).

>  - Web page layout doesn't ignore scrollbars; it still makes room for them as
>    long as it can (example: 14px right margin on google.com)

Fixed by making nsHTMLScrollFrame::ReflowScrolledFrame use GetScrollbarMetrics instead of GetPrefSize because the former also applies margins.

>  - Textarea resizer in the wrong position
>  - Dependency inversion: nsNativeThemeCocoa.mm includes nsScrollbarFrame.h,
>    but widget/ shouldn't depend on layout/

still unfixed

Also needs fixing:
 - non-root scroll area scrollbars can still be covered by web page elements
   (e.g. in the right pane on twitter)
 - if a scroll area needs both scrollbars, they should always be visible at
   the same time
 - scrollbars shouldn't fade away while the mouse is moving inside the scroll
   area, or while the mouse is still in scroll mode (see -[NSEvent phase])
 - invisible scrollbars shouldn't be hoverable
 - once hovered, the scrollbar track should be visible until the scrollbars
   fade away or until the other scrollbar of the scroll area is hovered
 - if there's only one scrollbar it shouldn't reserve any scroll corner space
   at the end
 - dark backgrounds should induce bright scrollbars
 - scrollbars should flash after pageload, window activation status change and
   tab switching
 - dynamic toggling of the system pref doesn't relayout scrollbars
 - animations should be implemented with CSS transitions
Comment 34 Asa Dotzler [:asa] 2011-08-01 14:55:11 PDT
no longer tracking for 6. 6 is nearly baked and it's far too late in the game to worry about this.
Comment 35 Stefan Arentz [:st3fan] 2011-08-12 18:57:11 PDT
Google just released a Chrome beta that includes the new scrollbar style. Maybe we can learn something from their code?
Comment 36 Markus Stange [:mstange] 2011-08-12 23:17:39 PDT
Maybe. But the remaining challenges are mostly mozilla-specific.
Comment 37 Bart Szyszka 2011-08-15 14:23:05 PDT
Created attachment 553262 [details]
Firefox scrollbars in Lion on a black background

Until a new scrollbar style is implemented for Lion, is there any chance some logic could be added to switch to the darker version of the scrollbar for pages with dark backgrounds? If you look at the attached screenshot, the scrollbar is very hard to "read" on a black background since there's such a stronger contrast between the page and the entire scrollbar vs. the scroll tracker and the scrollbar's gutter.
Comment 38 Bart Szyszka 2011-08-15 14:32:19 PDT
Created attachment 553266 [details]
Firefox scrollbars in Lion on a black background (w/ stuff around browser)
Comment 39 Damon Sicore (:damons) 2011-09-05 18:16:37 PDT
This is an important feature and we need an owner for this.  Who owns it?
Comment 40 Markus Stange [:mstange] 2011-09-07 14:30:13 PDT
I've made some more progress on this. Taking for now.
Comment 41 Markus Stange [:mstange] 2011-09-08 14:35:34 PDT
Here's a new build:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mstange@themasta.com-9c3818aa4fa1/try-macosx64/firefox-9.0a1.en-US.mac.dmg

Fixes these bugs:

(In reply to Markus Stange from comment #33)
>  - if a scroll area needs both scrollbars, they should always be visible at
>    the same time
>  - scrollbars shouldn't fade away while the mouse is moving inside the scroll
>    area, or while the mouse is still in scroll mode (see -[NSEvent phase])
(2nd part of this is still unfixed)
>  - invisible scrollbars shouldn't be hoverable
>  - once hovered, the scrollbar track should be visible until the scrollbars
>    fade away or until the other scrollbar of the scroll area is hovered
>  - if there's only one scrollbar it shouldn't reserve any scroll corner space
>    at the end

>  - animations should be implemented with CSS transitions

WIP patches are here: http://hg.mozilla.org/try/pushloghtml?changeset=9c3818aa4fa1
Comment 42 Alex Limi (:limi) — Firefox UX Team 2011-09-08 15:06:50 PDT
(In reply to Markus Stange from comment #41)
> Here's a new build

So pretty! Any chance we can land this on the UX branch soon, so we get wider usage? Or is it close enough that we're comfortable just landing it in Nightly?
Comment 43 Xan 2011-10-19 01:17:48 PDT
Is this landing in Nightly soon? I tested out a build with the new scrollbar style with the two finger animation and it seemed fine to me.
Comment 44 Jordan 2011-10-19 15:09:37 PDT
(In reply to Xan from comment #43)
> Is this landing in Nightly soon? I tested out a build with the new scrollbar
> style with the two finger animation and it seemed fine to me.

The new scrollbars seem fine to me as well.  I really hope they show up soon.
Comment 45 Alex Limi (:limi) — Firefox UX Team 2011-10-20 16:17:39 PDT
Yeah, seems like we should land this in Nightly or UX branch, it seems robust enough from testing for a few days — any other bugs will likely be found after it gets a bigger audience to run it in their main browser.
Comment 46 Philipp Kewisch [:Fallen] 2011-11-01 11:40:42 PDT
Any chance this could make the nightly branch before the next merge?
Comment 47 Michael 2011-11-08 08:06:51 PST
Does anybody have a new link to the testing build?
Comment 48 Markus Stange [:mstange] 2011-11-12 07:51:06 PST
The patches I'm going to attach here are on top of the one in bug 691609.
Comment 49 Markus Stange [:mstange] 2011-11-12 07:57:01 PST
Created attachment 574046 [details] [diff] [review]
part 1, v1: handle scrollbar margins

This functionality is already in trunk, so marking this patch as obsolete.

Overlay scrollbars are going to have negative margins so that they don't take up any space. Scroll frame content layout needs to take these margins into account when determining the size of the content area. GetScrollbarMetrics handles them.
Comment 50 Markus Stange [:mstange] 2011-11-12 08:03:38 PST
Created attachment 574047 [details] [diff] [review]
part 2, v1: don't add margins for collapsed scrollbars

I'm not sure when I hit this problem, but it's possible that scrollbars have visibility:collapsed (by inheritance, I think), and then negative margins on zero-sized scrollbars cause the inner size of the scrolled frame to be bigger than the outer size and we get assertions.
Comment 51 Markus Stange [:mstange] 2011-11-12 08:05:41 PST
Created attachment 574048 [details] [diff] [review]
part 3, v1: adjust scrollbar sizes so that horizontal and vertical scrollbars don't overlap
Comment 52 Markus Stange [:mstange] 2011-11-12 08:07:13 PST
Created attachment 574049 [details] [diff] [review]
part 4, v1: respect resizer minimum size

The new scrollbars are going to be 11px wide, but the resizer needs 15px.
Comment 53 Markus Stange [:mstange] 2011-11-12 08:08:27 PST
Created attachment 574050 [details] [diff] [review]
part 5, v1: set minimum size on image-based resizers
Comment 54 Markus Stange [:mstange] 2011-11-12 08:10:39 PST
Created attachment 574052 [details] [diff] [review]
part 6, v1: don't use the native resizer with overlay scrollbars because the border looks bad in that context
Comment 55 Markus Stange [:mstange] 2011-11-12 08:14:27 PST
Created attachment 574055 [details] [diff] [review]
part 7, v1: add overlay-scrollbars system metric that matches when the Mac OS 10.7 overlay scrollbar pref is set
Comment 56 Markus Stange [:mstange] 2011-11-12 08:16:26 PST
Created attachment 574056 [details] [diff] [review]
part 8, v1: add Lion scrollbar native rendering
Comment 57 Markus Stange [:mstange] 2011-11-12 08:18:28 PST
Created attachment 574057 [details] [diff] [review]
part 9, v1: add CSS for overlay scrollbars

I don't really like hardcoding the 11px here but I haven't had a better idea.
Comment 58 Markus Stange [:mstange] 2011-11-12 08:22:19 PST
Created attachment 574058 [details] [diff] [review]
part 10, v1: add nsIScrollbarHolder interface

I'd like to have the complete overlay scrollbar behavior on trees, too, not only on nsGfxScrollFrames.
Comment 59 Markus Stange [:mstange] 2011-11-12 08:28:23 PST
Created attachment 574059 [details] [diff] [review]
part 11, v1: add ScrollbarActivity class

There's a long comment in ScrollbarActivity.h which describes what I'm doing here.

I'm not using CSS transitions for the fade out animation because scrollbars inherit from the :-moz-viewport and :-moz-viewport-scroll style contexts and thus are blocked from CSS transitions.
Comment 60 Markus Stange [:mstange] 2011-11-12 08:29:50 PST
Created attachment 574060 [details] [diff] [review]
part 12, v1: report scrollbar activity from normal scroll frames
Comment 61 Markus Stange [:mstange] 2011-11-12 08:31:00 PST
Created attachment 574061 [details] [diff] [review]
part 13, v1: report scrollbar activity from trees
Comment 62 Markus Stange [:mstange] 2011-11-12 08:32:31 PST
Created attachment 574062 [details] [diff] [review]
part 14, v1: hide inactive scrollbars
Comment 63 Markus Stange [:mstange] 2011-11-12 08:34:09 PST
Created attachment 574063 [details] [diff] [review]
part 15, v1: make some failing tests pass
Comment 64 Markus Stange [:mstange] 2011-11-12 08:40:23 PST
This gets us most of the way. Still to do:
 - fix dynamic pref toggling
 - scrollbars shouldn't fade away while the finger is still on the touchpad
 - dark backgrounds should induce bright scrollbars
 - scrollbars should flash after pageload, window activation status change and
   tab switching

For the last point, what's a good way to find all visible scrollbar holders inside a frame? Should I recurse over the frames, or should I construct a display list and look at the display list items?
Comment 65 Philipp Kewisch [:Fallen] 2011-11-13 03:30:16 PST
(In reply to Markus Stange from comment #64)
>  - scrollbars should flash after pageload, window activation status change
> and tab switching
Is this really needed? I don't see this happening in other apps (Terminal, Adium) and given how Calendar would be using this, I think it would be quite distracting: all 35 month day boxes would flash after the window is activated.
Comment 66 Markus Stange [:mstange] 2011-11-13 13:11:47 PST
(In reply to Markus Stange from comment #64)
> For the last point, what's a good way to find all visible scrollbar holders
> inside a frame? Should I recurse over the frames, or should I construct a
> display list and look at the display list items?

Okay, using a display list probably won't work because the scrollbars might be invisible and the scroll frame transparent, so it wouldn't create any display list items at all.

(In reply to Philipp Kewisch [:Fallen] from comment #65)
> (In reply to Markus Stange from comment #64)
> >  - scrollbars should flash after pageload, window activation status change
> > and tab switching
> Is this really needed? I don't see this happening in other apps (Terminal,
> Adium)

I'll test again. Maybe there's something special about those scroll areas that flash.
Comment 67 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-13 14:53:13 PST
Comment on attachment 574057 [details] [diff] [review]
part 9, v1: add CSS for overlay scrollbars

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

::: toolkit/themes/pinstripe/global/nativescrollbars.css
@@ +54,5 @@
> +@media all and (-moz-overlay-scrollbars) {
> +  scrollbar {
> +    position: relative;
> +    z-index: 2147483647;
> +  }

Seems like this will mess with naked XUL <scrollbar> elements. Can we avoid that? Or don't we care?
Comment 68 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-13 14:56:23 PST
Comment on attachment 574058 [details] [diff] [review]
part 10, v1: add nsIScrollbarHolder interface

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

::: layout/generic/nsIScrollbarHolder.h
@@ +60,5 @@
> +  /**
> +   * Obtain the frame for the horizontal or vertical scrollbar, or null
> +   * if there is no such box.
> +   */
> +  virtual nsIFrame* GetScrollbarBox(bool aVertical) = 0;

Any reason to not just add these to nsIScrollbarMediator?
Comment 69 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-13 15:31:20 PST
dbaron says adding transition support for anonymous scrollbar elements probably is quite easy. How much of the code here would be saved by doing that?

Using transitions would mean that when we do async compositing, the fade-out could eventually be handled by the layer compositor and would be smooth even if the main thread blocks.
Comment 70 Neil Deakin 2011-11-14 11:36:16 PST
Comment on attachment 574050 [details] [diff] [review]
part 5, v1: set minimum size on image-based resizers

> resizer {
>   -moz-appearance: resizer;
>   background: url("chrome://global/skin/icons/resizer.png") no-repeat;
>   cursor: se-resize;
>+  min-width: 15px;
>   width: 15px;
>+  min-height: 15px;
>   height: 15px;

It looks ok but can you explain why you need it?
Comment 71 Markus Stange [:mstange] 2011-11-14 13:22:58 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #67)
> Comment on attachment 574057 [details] [diff] [review] [diff] [details] [review]
> part 9, v1: add CSS for overlay scrollbars
> 
> Review of attachment 574057 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/themes/pinstripe/global/nativescrollbars.css
> @@ +54,5 @@
> > +@media all and (-moz-overlay-scrollbars) {
> > +  scrollbar {
> > +    position: relative;
> > +    z-index: 2147483647;
> > +  }
> 
> Seems like this will mess with naked XUL <scrollbar> elements.

By making them be on top of everything else? Not sure that's much of a problem; I'd expect the negative margin to create bigger problems...

> Can we avoid that? Or don't we care?

I don't know, do we? :-)
We could have trees and nsGfxScrollFrames set a special class on their scrollbars and only target those here. Not sure if it's worth it.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #68)
> Comment on attachment 574058 [details] [diff] [review] [diff] [details] [review]
> part 10, v1: add nsIScrollbarHolder interface
> 
> Review of attachment 574058 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/generic/nsIScrollbarHolder.h
> @@ +60,5 @@
> > +  /**
> > +   * Obtain the frame for the horizontal or vertical scrollbar, or null
> > +   * if there is no such box.
> > +   */
> > +  virtual nsIFrame* GetScrollbarBox(bool aVertical) = 0;
> 
> Any reason to not just add these to nsIScrollbarMediator?

The biggest reason is that nsGfxScrollFrame doesn't implement nsIScrollbarMediator, and that I was too lazy to find out whether it should and what that would break.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #69)
> dbaron says adding transition support for anonymous scrollbar elements
> probably is quite easy. How much of the code here would be saved by doing
> that?

A little. But we can't make ScrollbarActivity completely oblivious to the animation because we still want the fading-out state to behave differently than the invisible state. Specifically, we want mouse moves to be ignored during the invisible state, but during the fade out animation mouse moves should reset the animation and make scrollbars completely visible again. So we'd probably have to listen for a transitionend event, or introduce another timer.

> Using transitions would mean that when we do async compositing, the fade-out
> could eventually be handled by the layer compositor and would be smooth even
> if the main thread blocks.

Good point.

(In reply to Neil Deakin from comment #70)
> Comment on attachment 574050 [details] [diff] [review] [diff] [details] [review]
> part 5, v1: set minimum size on image-based resizers
> 
> > resizer {
> >   -moz-appearance: resizer;
> >   background: url("chrome://global/skin/icons/resizer.png") no-repeat;
> >   cursor: se-resize;
> >+  min-width: 15px;
> >   width: 15px;
> >+  min-height: 15px;
> >   height: 15px;
> 
> It looks ok but can you explain why you need it?

Sure. At the moment only those resizers that we draw with native theming have a minimum size (from nsNativeThemeCocoa::GetMinimumWidgetSize); those where we disable native theming don't have one. That means that they get sized to the sizes of the scrollbars, and the new scrollbars are only 11px thick, so the resizer would be 11x11 and the image would be cropped.
Also, if the image-based resizer has a different minimum size than the native resizer, we don't correctly deal with the size change when the sudden appearance of a scrollbar switches the resizer style. (We won't have that problem here because part 6 makes us always use the image resizer, but I don't want to rely on that.)
Comment 72 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-15 03:04:54 PST
(In reply to Markus Stange from comment #71)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #67)
> > Comment on attachment 574057 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]
> > part 9, v1: add CSS for overlay scrollbars
> > 
> > Review of attachment 574057 [details] [diff] [review] [diff] [details] [review] [diff] [details] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: toolkit/themes/pinstripe/global/nativescrollbars.css
> > @@ +54,5 @@
> > > +@media all and (-moz-overlay-scrollbars) {
> > > +  scrollbar {
> > > +    position: relative;
> > > +    z-index: 2147483647;
> > > +  }
> > 
> > Seems like this will mess with naked XUL <scrollbar> elements.
> 
> By making them be on top of everything else? Not sure that's much of a
> problem; I'd expect the negative margin to create bigger problems...

Good point.

> > Can we avoid that? Or don't we care?
> 
> I don't know, do we? :-)
> We could have trees and nsGfxScrollFrames set a special class on their
> scrollbars and only target those here. Not sure if it's worth it.

I'm worried that extensions might use a naked <scrollbar> element for something and break here.

> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #68)
> > Any reason to not just add these to nsIScrollbarMediator?
> 
> The biggest reason is that nsGfxScrollFrame doesn't implement
> nsIScrollbarMediator, and that I was too lazy to find out whether it should
> and what that would break.

OK. It's not really clear what the difference between nsIScrollbarMediator and nsIScrollbarHolder is, and that's not good.

+  /**
+   * Obtain an event target that receives mouse events over the scrolled area
+   * and over the scrollbars. This will usually be an ancestor of the frame
+   * that the scrollbars are attached to.

It's not clear what this is used for.

> A little. But we can't make ScrollbarActivity completely oblivious to the
> animation because we still want the fading-out state to behave differently
> than the invisible state. Specifically, we want mouse moves to be ignored
> during the invisible state, but during the fade out animation mouse moves
> should reset the animation and make scrollbars completely visible again. So
> we'd probably have to listen for a transitionend event, or introduce another
> timer.

Would listening for the transitionend event be bad?
Comment 73 Markus Stange [:mstange] 2011-11-15 05:04:56 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #72)
> > > Can we avoid that? Or don't we care?
> > 
> > I don't know, do we? :-)
> > We could have trees and nsGfxScrollFrames set a special class on their
> > scrollbars and only target those here. Not sure if it's worth it.
> 
> I'm worried that extensions might use a naked <scrollbar> element for
> something and break here.

Okay, I'll look into this.

> +  /**
> +   * Obtain an event target that receives mouse events over the scrolled
> area
> +   * and over the scrollbars. This will usually be an ancestor of the frame
> +   * that the scrollbars are attached to.
> 
> It's not clear what this is used for.

ScrollbarActivity needs it to track mouse moves over the scroll area which should keep scrollbars visible. Do you know a simpler way of doing that?

> > A little. But we can't make ScrollbarActivity completely oblivious to the
> > animation because we still want the fading-out state to behave differently
> > than the invisible state. Specifically, we want mouse moves to be ignored
> > during the invisible state, but during the fade out animation mouse moves
> > should reset the animation and make scrollbars completely visible again. So
> > we'd probably have to listen for a transitionend event, or introduce another
> > timer.
> 
> Would listening for the transitionend event be bad?

No, it's just a factor that might not make using transitions that much simpler.
In any case we don't want this transitionend event to bubble to the surrounding page. But dbaron probably has a plan for that.
Comment 74 Lars Olofsson 2011-11-15 05:17:09 PST
Can you guys merge this into ux or nightly?
Comment 75 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-11-15 15:37:04 PST
(In reply to Markus Stange from comment #73)
> ScrollbarActivity needs it to track mouse moves over the scroll area which
> should keep scrollbars visible. Do you know a simpler way of doing that?

No, but why don't you just QueryFrame to the nsIFrame and then use GetContent()?

I think we should probably merge nsIScrollbarHolder and nsIScrollbarMediator into nsIScrollbarOwner. I can't see a reason to keep them separate.
Comment 76 Ed Morley [:emorley] 2011-12-20 09:01:16 PST
https://hg.mozilla.org/mozilla-central/rev/7f8c69ad0895
Comment 77 Anthony Ricaud (:rik) 2011-12-21 02:21:42 PST
I am on Nightly 12.0a1 2011-12-20. This is built from f890732ebaa3 which should include 7f8c69ad0895. But I'm not seeing the new scrollbar style. Is it really fixed?
Comment 78 Mounir Lamouri (:mounir) 2011-12-21 02:29:50 PST
I believe most of the patches didn't land (and haven't been reviewed).

Guys, when landing a patch in inbound, if you don't want the bug to be fixed, you should mention it in the whiteboard. See the inbound tree rules:
https://wiki.mozilla.org/Tree_Rules/Inbound#Please_do_the_following_after_pushing_to_inbound
Comment 79 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2011-12-27 06:47:14 PST
> I believe most of the patches didn't land (and haven't been reviewed).
> 
> Guys, when landing a patch in inbound, if you don't want the bug to be
> fixed, you should mention it in the whiteboard. See the inbound tree rules:
> https://wiki.mozilla.org/Tree_Rules/
> Inbound#Please_do_the_following_after_pushing_to_inbound

That's a mistake on my side.

The bug that has landed is bug 691609. But the patch in the other bug was coming from this one and the bug number has not been updated in the comment. tbh I haven't seen it.
Comment 80 Xan 2012-03-04 19:12:15 PST
It would be awesome if scrollbar patch was merged to UX/Nightly. We would have more feedback about bugs and such. 

When is this scheduled to be updated? Mountain Lion is on the horizon and Chrome already has most Lion features.
Comment 81 Anthony Ricaud (:rik) 2012-03-18 16:49:07 PDT
Those patches have been waiting for review for 3 months. Is there anyone else who could review them?
Comment 82 Patrick Kelleher 2012-03-18 22:08:46 PDT
Gents, I hate to add noise to the thread, and I know this isn't critical, but it's this has been alive for a year. Any news?
Comment 83 Markus Stange [:mstange] 2012-03-19 00:04:08 PDT
Sorry, I've been busy with other things. The ball is in my court, not in roc's; I need to address his review comments. I'll start today.
Comment 84 Ryan Watson [:w0ts0n] 2012-03-25 15:44:14 PDT
I would like to see this feature. Keep up the good work guys :)
Comment 85 Alex 2012-04-01 11:00:19 PDT
In the meantime, I've created a Greasemonkey User Script that will at least show the new scrollbar style. See http://en.beriechil.de/?s=fx#GM

It's not perfect;
- you'll have to hide the real scrollbars, there's a link to a Stylish style that does that.
- you can't click and drag the scrollbars
- it only displays vertical scrollbars, though I could implement horizontal ones
- it's not visible on very dark backgrounds because I didn't get the border to work right
- in some cases it will run out of the bottom of the window if you scroll down all the way.
Though the main problem I've run into is that it looks so realistic, I've found myself trying to drag it already even though I just wrote it. That could be a problem.

@ Mozilla Team: hope you'll get that to work soon. Apart from that, keep up the good work :)
Comment 86 Stefan Arentz [:st3fan] 2012-04-02 07:14:27 PDT
Hey just a note that the Lion scrollbar style has changed a bit in Mountain Lion.

So it starts the usual size when you use two finger scroll but as soon as you hover over it it becomes probably twice as wide.

I think they discovered that many people had problems to grab the scrollbar in such a small area.
Comment 87 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2012-04-03 16:21:34 PDT
Created attachment 612009 [details]
bundle of updated patches

I updated the patches & pushed to the UX branch as a combined patch (for easy backout there) - https://hg.mozilla.org/projects/ux/rev/7e93aefab66c - so for those aching to play with it, give that a whirl.

Apart from some bitrot, the only other change needed (afaict) was in part 2 (attachment 574047 [details] [diff] [review]).
> aBox->IsCollapsed(aState)
Bug 524925 got rid of the param, so it should just be
> aBox->IsCollapsed()
Comment 88 Anthony Ricaud (:rik) 2012-04-04 12:00:28 PDT
Thanks for letting us test this.

From initial testing:
- When focusing a window or switching tabs, there is no "flash of scrollbars".
- The fade-out transition seems faster than OS scrollbars.
- On dark backgrounds, this is not turning into a white-ish scrollbar (I used http://daringfireball.net/ to test this)
Comment 89 Nomis101 2012-04-06 12:19:25 PDT
Because I've couldn't find the FF build with this patch (I've tried the "latest-ux" build, but that doesn't work a expected), I've made me an own (TB) build with your combined patch. The fade-out transition works (but a bit faster, as noted above). But I noticed the following, normally I've set the scrollbar in system preferences to show everytime. On OS X it than darkens a bit if I hover over it with the mouse, it doesn't darken for me with this patch.
Comment 90 scottgrigsby 2012-05-18 22:28:47 PDT
Hi all.  For some very obscure reason that I have been absolutely unable to figure out, the new Lion-style scrollbars don't show up for me at all.  I'm running Mac OS X (Lion) v.10.7.4.  It doesn't matter which version of Firefox I use -- 12.0, 13.0b4, or Aurora -- I ALWAYS get the old-style scroll bars.  I've tried a clean install of Firefox.  I've tried clean, new profiles.  I've deleted /User/<me>/Library/Application Support/Firefox/ altogether and started over with a new installation of Firefox.  And yet, I always get the old-style scrollbars.  Why would that be?  I'm happy to help debug in any way I can.  Thanks!
Comment 91 Xan 2012-05-19 15:29:27 PDT
Maybe you have the scroll bars to always show? Go to System Preferences by clicking on the apple on the menu and going to "General". In the "show scroll bars" option, make sure that the "always" option is unchecked. Keep it on "automatically based on input device" setting.
Comment 92 scottgrigsby 2012-05-19 21:09:25 PDT
Thank you for the suggestion!  Alas, but that's not it:  I have "Automatically based on input device" selected.  And I'm getting the Lion-style fade-in/out scrollbars in all my other applications.  Thanks for trying to help!
Comment 93 matthias koplenig 2012-05-19 22:56:39 PDT
as mentioned in comment 86 the patch landed on the ux-branch of nightly. therefore you have to get on of these builds to see them.
Comment 94 scottgrigsby 2012-05-20 10:34:27 PDT
Aha!  Thank you!  I don't know why I misunderstood -- I thought it had already been rolled into the regular releases.  Anyway, I'm now running the latest ux (15.0a1) and there they are!  I suck at coding, but am happy to help test your fine work.  Thanks, everyone!  :-)
Comment 95 scottgrigsby 2012-05-20 21:37:53 PDT
Hey, that's not right.  My new Mac Mini (running 10.7.4) and my gf's Macbook Air (running Lion) show the new-style scrollbars.  And they're just running regular Firefox 12.0.  Why doesn't my MacBook Pro (running Lion 10.7.4) have them?
Comment 96 Jared Wein [:jaws] (please needinfo? me) 2012-06-19 12:21:05 PDT
This patch was backed out of the UX Nightly branch due to code churn. Please update the patch and reland if you would like it to remain on the UX branch.

Has there been any movement with getting this patch reviewed & landing? UX branch shouldn't be the final stop for UI features :)
Comment 97 Asa Dotzler [:asa] 2012-06-24 12:20:42 PDT
Roc, this is a year overdue Mac fix. Will you be able to get to this in time for it to make the 16 uplift?
Comment 98 Markus Stange [:mstange] 2012-06-24 12:27:37 PDT
There are review comments to address. I don't have the time to address them at the moment. I might have a little time for it in about a month, but somebody else is welcome to take over working on this in the meantime.
Comment 99 Paul Rouget [:paul] 2012-07-23 17:18:29 PDT
Markus, any update on this?
Comment 100 asaisthe7 2012-07-28 09:56:42 PDT
It appears this was mad to work in UX for lion, but with upgrade to ML it breaks the scroll bars again, as well as the smooth back and forward pull
Comment 101 jenthe.huysmans 2012-10-12 17:08:44 PDT
Title of this bug should be adjusted to "Lion / ML" since it affect both.
Comment 102 Alex Keybl [:akeybl] 2012-10-16 09:16:56 PDT
Not clear why this would ever block a release. Untracking.
Comment 103 Mieszko Ślusarczyk 2012-10-19 03:56:09 PDT
No idea , why this was not yet implemented, while this is a standard since 2 OS X releases (over a year)...
Comment 104 jenthe.huysmans 2012-10-19 08:10:30 PDT
I completly agree with Mieszko. While I can easily imagine that this bug won't have top priority, I am a bit frustrated that Firefox still can't adapt to the OS X gui because of that stupid scrollbar. It seems like there is no progress.

Chrome has it. Opera has it.
Comment 105 Asa Dotzler [:asa] 2012-10-19 09:11:44 PDT
(In reply to Mieszko Ślusarczyk from comment #103)
> No idea , why this was not yet implemented, while this is a standard since 2
> OS X releases (over a year)...

It's not implemented yet because we don't have the Mac developer resources to cover all of our Mac system integration bugs and features. If you're a Mac developer and you'd like to step up to help out with this, it sure would be appreciated. If not, then please refrain from advocacy comments in Bugzilla which only make tracking issues more difficult. https://bugzilla.mozilla.org/page.cgi?id=etiquette.html
Comment 106 Mark Finkle (:mfinkle) (use needinfo?) 2012-10-25 13:38:56 PDT
How much more work is needed here? Metro is depending on this too.
Comment 107 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-10-26 06:15:22 PDT
What exactly needs to be done in order for the scrollbars to work properly? I am an Objective-C developer, so I may be able to help. Is the scrollbar problem directly related to the Mozilla engine, or is part of the Firefox application?
Comment 108 Dirkjan Ochtman (:djc) 2012-10-26 06:23:35 PDT
Josiah: it would be awesome if you could take a stab at fixing this! I think the scrollbar problem is related to the Mozilla engine. You could probably get a good feeling for what needs to be done by browsing through the patches. A good first step would probably be trying to unbitrot them so you have something that applies to the current tree. A great next step would be to address review comments. From a quick reading, look at comment 64 through to comment 75 and maybe comment 87.
Comment 109 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-10-26 06:27:59 PDT
Alright, I'll give it a shot. I will have to download the Mozilla source though. For some reason I don't have it anymore. Also, is there any attempt in the current engine to accomplish this, or are the patches the only code that has been done concerning this?
Comment 110 Dirkjan Ochtman (:djc) 2012-10-26 06:33:59 PDT
I think a bunch of the patches done for this bug have already landed, but otherwise you should probably just look at the patches.
Comment 111 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-10-26 06:36:24 PDT
Okay, I'm cloning mozilla-central now. Is that what I should use? I'm going through Mercurial. I am probably not going to push the changes back to a server yet, but I want to make sure that is the correct directory. Last time I think I obtained it from the tarballs.
Comment 112 Dirkjan Ochtman (:djc) 2012-10-26 06:40:28 PDT
That's right. If you want more assistance, it might help if you join IRC. Most developers hang out in #developers on irc.mozilla.org.
Comment 113 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-10-26 06:42:10 PDT
Alright, thanks. I'll check out the IRC and see what I can do about the scrollbar problem.
Comment 114 Markus Stange [:mstange] 2012-10-26 06:56:26 PDT
I'm almost finished updating the patches. I'll attach them here when I'm done.

Josiah: Thanks for the initiative! The remaining hard parts are not in the Objective C part of the code, unfortunately, but in figuring out how the Mozilla interfaces nsIScrollbarMediator, nsIScrollableFrame and the new nsIScrollbarHolder interact. I have some notes on this which I'll post shortly.

The good news is that I have some time for this bug now.

The area I could use the most help in is testing. I'll especially need somebody to test on 10.7 because I only have 10.8 nowadays, and I think the metrics of rendered scrollbars are slightly different between the two.

A rough test build will probably appear here in two or three hours: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mstange@themasta.com-883789d18883/try-macosx64/firefox-19.0a1.en-US.mac.dmg

(In reply to Mark Finkle (:mfinkle) from comment #106)
> How much more work is needed here? Metro is depending on this too.

 - Figure out whether we can and should get rid of nsIScrollbarMediator
 - Update the patches
 - Fix rendering metrics on 10.7 and 10.8

The following things can probably wait until the first part has landed:

 - Implement the new onhover width enlargement on 10.8
 - Survive system pref toggling
 - Keeps scrollbars visible while fingers touch the touchpad
 - Flash on pageload
 - Flash on window focusing
 - Bright scrollbars on dark backgrounds
Comment 115 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-10-26 07:00:16 PDT
Markus Stange: Yeah, I figured that out after the source downloaded, so I'm glad to hear the you are going to start working on this. I would be glad to test the changes, but I do run 10.8.
Comment 116 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-10-26 08:15:41 PDT
I see you have the test build up. It seems to work pretty good, but you forgot one thing that needs to be implemented, which is the "elastic" effect. Also, the scrollbar has rendering problems on Bugzilla, but not other sites.
Comment 117 Cheba 2012-10-26 09:07:55 PDT
Looks fantastic.

I haven't noticed any rendering problems on Bugzilla.

Another thing missing is that on ML scrollbar becomes wider (higher for horizontal scrollbars) on hover.
Comment 118 Michael 2012-10-26 09:47:14 PDT
I also don't have any bugs with it here on Bugzilla.  Thanks for your hard work on it!  Can't wait until it finally makes it's way into the main build.
Comment 119 Markus Stange [:mstange] 2012-10-26 09:55:31 PDT
Cheba, Michael, are you on 10.7? Because I, like Josiah, see problems on Bugzilla on 10.8.
Comment 120 Michael 2012-10-26 09:56:27 PDT
No, 10.8.2
Comment 121 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-10-26 10:12:34 PDT
While scrolling down, you will see that the very bottom of the scrollbar seems to erase itself and then a little later reappear. That by itself is no major problem, however, when you hit the bottom of the page and then try to scroll up, there are some major rendering issues. I will post a screenshot in a second.
Comment 122 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-10-26 10:13:30 PDT
Created attachment 675608 [details]
Rendering issue with scrollbars
Comment 123 Ben Perkins 2012-10-26 10:21:35 PDT
Great! Really glad we're getting progress made on this! I downloaded your build and had quite a few issues with it, but it's a great start. I'm on 10.7 still and I'm more than happy to do any testing you need. For me, it worked well to start, but after a minute or so, pages either wouldn't scroll at all or were very choppy. Also, the scrollbar would no longer auto-hide. The scrolling issues only applied to the scroll wheel though, I could still grab it to scroll. Also, not sure if this is related or not, but all of my tabs were off by one. If I was on the first tab, the contents of the second tab would be showing, even though the correct title and URL were showing. Probably not related to your patch though.
Comment 124 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-10-26 10:25:18 PDT
There are no lag issues or auto-hide issues that I have seen yet on 10.8, so it would indeed appear as though 10.7 and 10.8 do render things differently. Hopefully this feature can be added to Firefox 19 when it goes live.
Comment 125 Cam 2012-10-26 11:13:32 PDT
I also saw a few artifacts when scrolling on 10.8.2 and will attach screenshots if I can.
Comment 126 Cam 2012-10-26 11:15:08 PDT
Created attachment 675619 [details]
Issues in Responsive mode

The artifacts are most obvious in responsive mode.
Comment 127 Cam 2012-10-26 11:17:14 PDT
Created attachment 675621 [details]
Alt rendering issue with scrollbar

This happens when scrolling if you leave your fingers down when done scrolling and then scroll again.
Comment 128 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-10-26 11:20:36 PDT
(In reply to Cam from comment #127)
> Created attachment 675621 [details]
> Alt rendering issue with scrollbar
> 
> This happens when scrolling if you leave your fingers down when done
> scrolling and then scroll again.

Actually that is not true. I have the same problem even if I completely move my fingers off the trackpad and wait for the scrollbar to disappear. Then when I start scrolling again, the bug occurs. 

The odd thing here is that this only occurs while scrolling up. The issue is very minor while scrolling down.
Comment 129 Cheba 2012-10-26 11:30:04 PDT
I'm on 10.8.2. I'm pretty sure I haven't seen this on my first run. Though, on the second run I see the problem.

I did some experiments. This probably has something to do with the size of the thumb. Problem appears when it gets small. For example, problem appears for document 19306+ px high in a viewport 698px high. But if I expand the viewport (or shrink the document) it starts rendering without artifacts.
Comment 130 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-10-26 11:33:39 PDT
(In reply to Cheba from comment #129)
> I'm on 10.8.2. I'm pretty sure I haven't seen this on my first run. Though,
> on the second run I see the problem.
> 
> I did some experiments. This probably has something to do with the size of
> the thumb. Problem appears when it gets small. For example, problem appears
> for document 19306+ px high in a viewport 698px high. But if I expand the
> viewport (or shrink the document) it starts rendering without artifacts.

Yep. You're right. As the viewport gets larger, there comes a point where the error no longer occurs.
Comment 131 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-10-27 15:02:24 PDT
Alright. What progress has been made since yesterday? I haven't heard anything from Markus yet.
Comment 132 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-11-05 05:45:32 PST
I believe this bug has to do with the size of the scrollbar itself. The scrollbar appears smaller because the amount of content on a webpage is making the scrollbar that way. When you zoom out on a page, therefore causing the scrollbar to grow, the issue stops.

Therefore, the problem is related to when the scrollbar size is small. (When I say small I mean height of the black bar)
Comment 133 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-11-15 06:24:25 PST
I would like to try to continue development on this myself. However, I am pretty new at the Firefox project, and therefore am not really sure how to work the patches. Where I work, when patches are worked a little differently. If somebody could explain how I add these patches to my source I would be very grateful. Or, if you simply want to attach a .zip file containing all the source files that have to do with this, that would be even better. Since at the moment, this is all I am working on, I would appreciate just having the source files so I can copy/paste it into the original source. That would save me a lot of time, but if it is a lot of work to do that, then just explaining how to work the patches would be great.
Comment 134 Josh Matthews [:jdm] (on vacation until Dec 5) 2012-11-15 06:28:51 PST
You should import the patches into a Mercurial Queue: https://developer.mozilla.org/en-US/docs/Mercurial_Queues. That page can be a bit intimidating, but think of queues as providing temporary commits that can be pushed, popped, and modified. You'll want the qimport command: |hg qimport https://bugzilla.mozilla.org/attachment.cgi?id=574046 -n part1 --push| to import the first, for example. There's more information about using mercurial correctly at https://developer.mozilla.org/en-US/docs/Mercurial_FAQ.
Comment 135 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-11-15 06:37:18 PST
Does that require me getting the mozilla source with Mercurial? I obtained the source using hg pull. I'll go look at the page though. 

The team I work with is very small. Therefore, patches aren't usually done very automatically like these are. They are usually just a .rtf file with a comment at the top with the file they belong to. Then, new code is marked in green, and removed code is marked in red.

So, it might take a little while to get used to this. But if I have problems I'll ask them on #developers. Thanks for your quick reply.
Comment 136 Markus Stange [:mstange] 2012-11-15 07:28:57 PST
I've started a new tryserver build at https://tbpl.mozilla.org/?tree=Try&rev=9a3c370114ae
You can pull the updated patches from there; I'm not going to attach them to this bug until they're more polished.
The finished build will appear here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/mstange@themasta.com-9a3c370114ae/try-macosx64/firefox-19.0a1.en-US.mac.dmg

I've fixed the drawing problem with small scrollbars, it was caused by an insufficient minimum height. I've also added the animated scrollbar growing on hover which was added in 10.8.

Remaining todo list:
 - Restore existing ScrollbarActivity functionality from bug 778810 (which I've
   overwritten) and make the old pref LookAndFeel::eIntID_ShowHideScrollbars work
   again.
 - Figure out which part of the animation could be done using CSS transitions, and
   figure out why the opacity fade out CSS transition from bug 778810 works in B2G
   but not for me
 - Code cleanup
Comment 137 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-11-15 07:33:42 PST
Alright, great. It took me that long to import 2 patches. At least I know how now.
Comment 138 Markus Stange [:mstange] 2012-11-15 07:51:10 PST
Created attachment 682006 [details] [diff] [review]
Add GetScrollbarBox method to nsIScrollbarMediator instead of creating a new interface for it

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #72)
> > > ::: layout/generic/nsIScrollbarHolder.h
> > > @@ +60,5 @@
> > > > +  /**
> > > > +   * Obtain the frame for the horizontal or vertical scrollbar, or null
> > > > +   * if there is no such box.
> > > > +   */
> > > > +  virtual nsIFrame* GetScrollbarBox(bool aVertical) = 0;
> > > Any reason to not just add these to nsIScrollbarMediator?
> > 
> > The biggest reason is that nsGfxScrollFrame doesn't implement
> > nsIScrollbarMediator, and that I was too lazy to find out whether it should
> > and what that would break.
> 
> OK. It's not really clear what the difference between nsIScrollbarMediator
> and nsIScrollbarHolder is, and that's not good.

Here's a patch that uses nsIScrollbarMediator instead of adding nsIScrollbarHolder.
I'm not sure which is the better approach.
The goal is to have an interface which offers a GetScrollbarBox method for ScrollbarActivity to call, which is implemented both by nsIScrollableFrames (nsHTMLScrollFrame / nsXULScrollFrame) and by XUL trees. (I no longer need the GetEventTarget method that the earlier patch added.)
nsIScrollableFrame didn't implement nsIScrollbarMediator before, so now that it does, I had to stub out the implementation of the three methods it offers. Also, nsListBoxBodyFrame already implements nsIScrollbarMediator, but ScrollbarActivity will never deal with nsListBoxBodyFrames, so GetScrollbarBox doesn't need to be implemented by nsListBoxBodyFrame.
nsListBoxBodyFrame is used for XUL list boxes. They use a nsGfxScrollFrame for their scrolling, so that's what ScrollbarActivity will see.

So using a new interface for the GetScrollbarBox method on nsIScrollableFrames and nsTreeBodyFrame instead of reusing nsIScrollbarMediator would have the advantage that we don't need to stub out unnecessary method implementations in several places. Do you think that's reason enough for nsIScrollbarHolder to exist?

One thing that I haven't implemented yet, but should, is the scrollbar flashing on tab or window activation. For that, something from the outside will probably need to call a FlashScrollbars() method on the right frames, and nsIScrollbarHolder would be a good interface to add such a method to.
Comment 139 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-11-15 07:57:17 PST
"One thing that I haven't implemented yet, but should, is the scrollbar flashing on tab or window activation."

What? I don't know what your talking about. On your previous build, the scrollbar does appear when a frame is loaded, why does it need to flash. I am not aware that any OS X app flashes the scrollbar on window activation. Perhaps I am missing something?
Comment 140 Markus Stange [:mstange] 2012-11-15 08:05:53 PST
Huh, you're right, that no longer happens. I thought it did. Was that changed between 10.7 and 10.8? Safari still flashes scrollbars when switching tabs, though.
Comment 141 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-11-15 08:09:15 PST
That's true. That could be implemented. It's not really a big deal though. What does need to be added is that the scrollbar should change it's color to white on a black background. At the moment it does not, though I'm not sure if your latest patches addressed that issue yet. 

Also, I am not sure if this is part of the scrollbar problem, but when you press the home key that takes you to the top of the page, it does not "scroll" you over to it, it just jumps. That is also not consistent with Safari and other OS X apps.
Comment 142 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-11-15 08:57:31 PST
By the way, I have created a Google Doc File with a list of ToDos. Anyone can edit it, so feel free to add/remove things.

https://docs.google.com/document/d/1mdTpJuvuwAGN4BZ5SXX6vihNYrZke6xmwB0CiX1avxs/edit
Comment 143 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-11-15 10:17:12 PST
Comment on attachment 574058 [details] [diff] [review]
part 10, v1: add nsIScrollbarHolder interface

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

::: layout/generic/nsIScrollbarHolder.h
@@ +60,5 @@
> +  /**
> +   * Obtain the frame for the horizontal or vertical scrollbar, or null
> +   * if there is no such box.
> +   */
> +  virtual nsIFrame* GetScrollbarBox(bool aVertical) = 0;

Alright, you convinced me that this is the right way to go.

But I think nsIScrollbarOwner is a better name. Let's use that.
Comment 144 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-11-15 12:20:23 PST
What patches must I apply in order to catch up to where you are Markus? Do I only need to import the patches from tryserver, or do I also needs the patches on Bugzilla? If so, which ones. I tried to use the first few from here, but ran into errors.
Comment 145 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-11-15 12:58:06 PST
For example, when I try to push the first patch on Bugzilla here, I get it rejected. Here are the contents of the .rej file:


--- nsGfxScrollFrame.cpp
+++ nsGfxScrollFrame.cpp
@@ -498,28 +498,30 @@ nsHTMLScrollFrame::ReflowScrolledFrame(S
   nscoord computedMinHeight = aState->mReflowState.mComputedMinHeight;
   nscoord computedMaxHeight = aState->mReflowState.mComputedMaxHeight;
   if (!ShouldPropagateComputedHeightToScrolledContent()) {
     computedHeight = NS_UNCONSTRAINEDSIZE;
     computedMinHeight = 0;
     computedMaxHeight = NS_UNCONSTRAINEDSIZE;
   }
   if (aAssumeHScroll) {
-    nsSize hScrollbarPrefSize = 
-      mInner.mHScrollbarBox->GetPrefSize(const_cast<nsBoxLayoutState&>(aState->mBoxState));
+    nsSize hScrollbarPrefSize;
+    GetScrollbarMetrics(aState->mBoxState, mInner.mHScrollbarBox,
+                        nsnull, &hScrollbarPrefSize, false);
     if (computedHeight != NS_UNCONSTRAINEDSIZE)
       computedHeight = NS_MAX(0, computedHeight - hScrollbarPrefSize.height);
     computedMinHeight = NS_MAX(0, computedMinHeight - hScrollbarPrefSize.height);
     if (computedMaxHeight != NS_UNCONSTRAINEDSIZE)
       computedMaxHeight = NS_MAX(0, computedMaxHeight - hScrollbarPrefSize.height);
   }
 
   if (aAssumeVScroll) {
-    nsSize vScrollbarPrefSize = 
-      mInner.mVScrollbarBox->GetPrefSize(const_cast<nsBoxLayoutState&>(aState->mBoxState));
+    nsSize vScrollbarPrefSize;
+    GetScrollbarMetrics(aState->mBoxState, mInner.mVScrollbarBox,
+                        &vScrollbarPrefSize, nsnull, true);
     availWidth = NS_MAX(0, availWidth - vScrollbarPrefSize.width);
   }
 
   nsPresContext* presContext = PresContext();
 
   // Pass false for aInit so we can pass in the correct padding.
   nsHTMLReflowState kidReflowState(presContext, aState->mReflowState,
                                    mInner.mScrolledFrame,


Earlier, I thought I got that too work. Why does it not anymore?
Comment 146 Markus Stange [:mstange] 2012-11-15 15:42:52 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #143)
> But I think nsIScrollbarOwner is a better name. Let's use that.

Will do.

(In reply to Josiah from comment #144)
> What patches must I apply in order to catch up to where you are Markus?

Only those from the tryserver, for example like this:
hg qimport https://hg.mozilla.org/try/raw-rev/f31cc89fc292 && hg qpush
hg qimport https://hg.mozilla.org/try/raw-rev/ace6c6019d48 && hg qpush
etc.
Comment 147 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-11-15 15:51:28 PST
I get an error when trying to push your patch "73ba90196d4b". That one does not work.
Comment 148 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-11-19 06:00:59 PST
How long do you guys estimate it will take to get this pushed to the Nightly? I was hoping this could get added to Firefox 20, as Nightly releases will be starting sometime today. That sill gives us quite bit of time, but then the amount of testers would be boosted up.
Comment 149 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-11-20 09:07:44 PST
While waiting for my previous comment to be answered, I would also like to ask someone to vouch for me.

 https://mozillians.org/en-US/u/JosiahOne
Comment 150 Josiah Bruner [:JosiahOne] (needinfo for responses) 2012-11-27 14:38:20 PST
The patches need to be updated to the latest version of firefox. The patches appear to be broken after the merge.
Comment 151 AbdulElah 2013-01-08 13:25:15 PST
thank you for your work, i'm not a coder but I always wonder, is it that hard to fix scrollbars !!??
Comment 152 Josiah Bruner [:JosiahOne] (needinfo for responses) 2013-01-08 13:27:30 PST
(In reply to AbdulElah from comment #151)
> thank you for your work, i'm not a coder but I always wonder, is it that
> hard to fix scrollbars !!??

You would think not. But in this case it is quite difficult. The Gecko engine uses it's own scrollbars, not the default OS X API scrollview. Essentially, we can not rely on Apple's code. We must re-write it ourselves.
Comment 153 Stephen A Pohl [:spohl] 2013-02-06 16:24:02 PST
Created attachment 711056 [details] [diff] [review]
part 2, v1: don't add margins for collapsed scrollbars

Updated for current trunk. Carrying over r+.
Comment 154 Stephen A Pohl [:spohl] 2013-02-06 16:25:39 PST
Created attachment 711059 [details] [diff] [review]
part 3, v1: adjust scrollbar sizes so that horizontal and vertical scrollbars don't overlap

Updated for current trunk. Carrying over r+.
Comment 155 Stephen A Pohl [:spohl] 2013-02-06 16:27:39 PST
Created attachment 711061 [details] [diff] [review]
part 4, v1: respect resizer minimum size

Updated for current trunk. Carrying over r+.
Comment 156 Stephen A Pohl [:spohl] 2013-02-06 16:28:47 PST
Created attachment 711063 [details] [diff] [review]
part 5, v1: set minimum size on image-based resizers

Updated for current trunk. Carrying over r+.
Comment 157 Stephen A Pohl [:spohl] 2013-02-06 16:30:01 PST
Created attachment 711065 [details] [diff] [review]
part 6, v1: don't use the native resizer with overlay scrollbars because the border looks bad in that context

Updated for current trunk. Carrying over r+.
Comment 158 Stephen A Pohl [:spohl] 2013-02-06 16:31:23 PST
Created attachment 711066 [details] [diff] [review]
part 7, v1: add overlay-scrollbars system metric that matches when the Mac OS 10.7 overlay scrollbar pref is set

Updated for current trunk. Carrying over r+.
Comment 159 Stephen A Pohl [:spohl] 2013-02-06 16:32:36 PST
Created attachment 711068 [details] [diff] [review]
part 8, v1: add Lion scrollbar native rendering

Updated for current trunk. Carrying over r+.
Comment 160 Stephen A Pohl [:spohl] 2013-02-06 16:34:12 PST
Created attachment 711069 [details] [diff] [review]
part 10, v1: add nsIScrollbarHolder interface

Updated for current trunk. Carrying over r+.
Comment 161 Stephen A Pohl [:spohl] 2013-02-06 16:35:59 PST
Created attachment 711070 [details] [diff] [review]
part 11, v1: add ScrollbarActivity class

Updated for current trunk.
Comment 162 Stephen A Pohl [:spohl] 2013-02-06 16:37:27 PST
Created attachment 711072 [details] [diff] [review]
part 12, v1: report scrollbar activity from normal scroll frames

Updated for current trunk.
Comment 163 Stephen A Pohl [:spohl] 2013-02-06 16:38:44 PST
Created attachment 711074 [details] [diff] [review]
part 13, v1: report scrollbar activity from trees

Updated for current trunk.
Comment 164 Stephen A Pohl [:spohl] 2013-02-06 16:40:00 PST
Created attachment 711076 [details] [diff] [review]
part 15, v1: make some failing tests pass

Updated for current trunk.
Comment 165 Stephen A Pohl [:spohl] 2013-02-06 16:57:04 PST
Created attachment 711082 [details] [diff] [review]
Add GetScrollbarBox method to nsIScrollbarMediator instead of creating a new interface for it

Updated for current trunk. Carrying over feedback? that mstange requested from roc.
Comment 166 Stephen A Pohl [:spohl] 2013-02-06 16:57:46 PST
I updated the patches and maintained their flags (if any), since no functionality was added or changed.

Part 1 is no longer necessary, since the functionality is already in the current trunk. Only part 2 to part 15 still apply.
Comment 167 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-02-06 17:22:44 PST
Comment on attachment 711082 [details] [diff] [review]
Add GetScrollbarBox method to nsIScrollbarMediator instead of creating a new interface for it

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

Markus convinced me that this patch is not the way to go.
Comment 168 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-02-06 17:23:39 PST
(In reply to Stephen Pohl [:spohl] from comment #160)
> Created attachment 711069 [details] [diff] [review]
> part 10, v1: add nsIScrollbarHolder interface
> 
> Updated for current trunk. Carrying over r+.

I requested that the interface be named nsIScrollbarOwner. Don't forget to address that (and any other review comments)! Thanks.
Comment 169 Josiah Bruner [:JosiahOne] (needinfo for responses) 2013-02-07 06:38:36 PST
Unless the patch: 

"Add GetScrollbarBox method to nsIScrollbarMediator instead of creating a new interface for it"

Fixes it (I have not applied that one), then the drawing issue is still a problem.
Comment 170 Stephen A Pohl [:spohl] 2013-02-07 10:21:59 PST
Markus, could you let me know if you'd like me to drive this from here?
Comment 171 Markus Stange [:mstange] 2013-02-08 05:34:54 PST
Yes, please! :)
Comment 172 Frank Yan (:fryn) 2013-02-20 14:52:21 PST
Overall, I really want the new scrollbar style, but I've been experiencing a usability problem with the new style:

We don't use the "native" drop-down list style for <select> and <option> elements (combo boxes). Instead we use a more plain-looking list with regular scrollbars, which become overlay scrollbars in the new style, but the overlay scrollbars do not appear when the list is opened, so there is no overflow indication in the list until there is an attempt to scroll the list.

This is mitigated somewhat for <select> elements with a size attribute with value greater than 1 by briefly displaying the overlay scrollbars upon page load. Perhaps, we could do the same for <select> elements with no size attribute, or we could switch to a more native-looking drop-down list for <select> elements, like we do for font selection in the preferences dialog.

To be clear, I don't think this issue should block the landing of the new scrollbars.
Comment 173 Stephen A Pohl [:spohl] 2013-02-22 15:07:27 PST
Created attachment 717355 [details] [diff] [review]
part 4, v1: respect resizer minimum size

Updated for current trunk and removed some trailing white space. Carrying over r+.
Comment 174 Stephen A Pohl [:spohl] 2013-02-22 15:13:42 PST
Created attachment 717363 [details] [diff] [review]
part 8, v1: add Lion scrollbar native rendering

Updated for current trunk, removed trailing white space and added rollover argument to CUIDraw call based on Markus' suggestion. Carrying over r+.
Comment 175 Stephen A Pohl [:spohl] 2013-02-22 15:19:23 PST
Created attachment 717367 [details] [diff] [review]
part 10, v1: add nsIScrollbarOwner interface

Renamed nsIScrollbarHolder to nsIScrollbarOwner. Removed GetEventTarget methods that were added in previous versions of this patch. Removed trailing white space.
Comment 176 Stephen A Pohl [:spohl] 2013-02-22 15:25:22 PST
Created attachment 717368 [details] [diff] [review]
part 11, v1: add ScrollbarActivity class

Updated for current trunk and incorporated the following enhancements from Markus:
1. Use do_QueryFrame instead of GetEventTarget.
2. Some refactoring.
3. Made ~ScrollbarActivity virtual.
4. Use uint32_t instead of PRUint32.
Comment 177 Stephen A Pohl [:spohl] 2013-02-22 15:27:03 PST
Comment on attachment 711072 [details] [diff] [review]
part 12, v1: report scrollbar activity from normal scroll frames

This patch has become obsolete with the latest patches.
Comment 178 Stephen A Pohl [:spohl] 2013-02-22 15:29:48 PST
Created attachment 717370 [details] [diff] [review]
part 13, v1: report scrollbar activity from trees

Updated for current trunk. Removed trailing white space. Slight enhancements from Markus.
Comment 179 Stephen A Pohl [:spohl] 2013-02-22 15:32:18 PST
Created attachment 717372 [details] [diff] [review]
part 15, v1: make some failing tests pass

Removed some trailing white space. Fixed type pointed out by Markus ("matchs" vs. "matches").
Comment 180 Stephen A Pohl [:spohl] 2013-02-22 15:38:18 PST
Created attachment 717376 [details] [diff] [review]
part 16, v1: make scrollbars wider on hover

Does not animate yet.
Comment 181 Stephen A Pohl [:spohl] 2013-03-05 15:30:01 PST
Created attachment 721495 [details] [diff] [review]
part 5, v1: set minimum size on image-based resizers

Updated for current trunk. Carrying over r+.
Comment 182 Stephen A Pohl [:spohl] 2013-03-05 15:34:33 PST
Created attachment 721497 [details] [diff] [review]
part 7, v1: add overlay-scrollbars system metric that matches when the Mac OS 10.7 overlay scrollbar pref is set

Updated for current trunk. Carrying over r+.
Comment 183 Stephen A Pohl [:spohl] 2013-03-05 15:41:29 PST
Created attachment 721504 [details] [diff] [review]
part 9, v1: add CSS for overlay scrollbars

Updated for current trunk.
Comment 184 Stephen A Pohl [:spohl] 2013-03-05 15:48:33 PST
Created attachment 721507 [details] [diff] [review]
part 11, v1: add ScrollbarActivity class

Updated for current trunk.
Comment 185 Stephen A Pohl [:spohl] 2013-03-05 15:52:26 PST
Created attachment 721509 [details] [diff] [review]
part 14, v1: hide inactive scrollbars

Updated for current trunk.
Comment 186 Stephen A Pohl [:spohl] 2013-03-05 15:55:07 PST
Created attachment 721510 [details] [diff] [review]
part 16, v1: make scrollbars wider on hover

Updated for current trunk.
Comment 187 Stephen A Pohl [:spohl] 2013-04-16 15:01:15 PDT
Created attachment 738203 [details] [diff] [review]
part 3, v1: adjust scrollbar sizes so that horizontal and vertical scrollbars don't overlap

Updated for current trunk, carrying over r+.
Comment 188 Stephen A Pohl [:spohl] 2013-04-16 15:05:06 PDT
Created attachment 738206 [details] [diff] [review]
part 4, v1: respect resizer minimum size

Reverted removal of trailing white space from the previous update, since this would disturb hg blame. Carrying over r+.
Comment 189 Stephen A Pohl [:spohl] 2013-04-16 15:34:15 PDT
Created attachment 738221 [details] [diff] [review]
part 8, v1: add Lion scrollbar native rendering

Reverted removal of trailing white space from the previous update since this would disturb hg blame. Carrying over r+.
Comment 190 Stephen A Pohl [:spohl] 2013-04-16 15:41:29 PDT
Created attachment 738226 [details] [diff] [review]
part 10, v1: add nsIScrollbarOwner interface

Updated for current trunk and reverted removal of trailing white space from the previous update since this would disturb hg blame.
Comment 191 Stephen A Pohl [:spohl] 2013-04-16 15:44:05 PDT
Created attachment 738228 [details] [diff] [review]
part 11, v1: add ScrollbarActivity class

Updated for current trunk.
Comment 192 Stephen A Pohl [:spohl] 2013-04-16 15:45:42 PDT
Created attachment 738231 [details] [diff] [review]
part 13, v1: report scrollbar activity from trees

Updated for current trunk.
Comment 193 Stephen A Pohl [:spohl] 2013-04-16 15:48:06 PDT
Created attachment 738235 [details] [diff] [review]
part 15, v1: make some failing tests pass

Reverted removal of trailing white space from the previous update since this would disturb hg blame.
Comment 194 Stephen A Pohl [:spohl] 2013-04-16 15:51:32 PDT
Created attachment 738237 [details] [diff] [review]
part 16, v1: make scrollbars wider on hover

Updated for current trunk.
Comment 195 Stephen A Pohl [:spohl] 2013-04-16 16:19:17 PDT
Created attachment 738249 [details] [diff] [review]
part 2, v1: don't add margins for collapsed scrollbars

Updated for current trunk, carrying over r+.
Comment 196 Stephen A Pohl [:spohl] 2013-04-17 12:10:13 PDT
Created attachment 738667 [details] [diff] [review]
part 10, v1: add nsIScrollbarOwner interface

Updated for current trunk.
Comment 197 Stephen A Pohl [:spohl] 2013-04-17 12:12:04 PDT
Created attachment 738668 [details] [diff] [review]
part 15, v1: make some failing tests pass

Fixed test for bug 485118.
Comment 198 Stephen A Pohl [:spohl] 2013-04-18 09:08:44 PDT
While I'm working through the last failure on try (https://tbpl.mozilla.org/?tree=Try&rev=9fc6be3cf171) I'm going to ask for reviews on the remaining patches. Robert, please feel free to redirect reviews if necessary.
Comment 199 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-04-18 15:28:43 PDT
Comment on attachment 738228 [details] [diff] [review]
part 11, v1: add ScrollbarActivity class

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

I would really like to see CSS transitions used here. I know it's a bit more work, but we are very close to turning on off-main-thread-compositing on Mac by default. So if we're using CSS transitions, the scrollbar fades will be very efficient and smooth.

However, I'm OK with landing this as-is and using CSS transitions as followup work, because it may involve some style system trickiness and it's not worth holding this up.

::: layout/generic/nsGfxScrollFrame.h
@@ +282,5 @@
>    nsIFrame* mScrollCornerBox;
>    nsIFrame* mResizerBox;
>    nsContainerFrame* mOuter;
>    nsRefPtr<AsyncScroll> mAsyncScroll;
> +  nsCOMPtr<mozilla::layout::ScrollbarActivity> mScrollbarActivity;

Please add a typedef mozilla::layout::ScrollbarActivity ScrollbarActivity; to nsGfxScrollFrameInner so we don't have this prefix here.
Comment 200 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-04-18 18:58:54 PDT
Comment on attachment 738231 [details] [diff] [review]
part 13, v1: report scrollbar activity from trees

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

::: layout/xul/tree/nsTreeBodyFrame.cpp
@@ +868,5 @@
>    }
> +
> +  if (weakFrame.IsAlive() && mScrollbarActivity) {
> +    mScrollbarActivity->ActivityOccurred();
> +  }

Instead of using nsWeakFrame, grab a reference to mScrollbarActivity into a local nsCOMPtr before the SetAttrs and don't refer to mScrollbarActivity again.

::: layout/xul/tree/nsTreeBodyFrame.h
@@ +539,5 @@
>    Slots* mSlots;
>  
>    nsRevocableEventPtr<ScrollEvent> mScrollEvent;
>  
> +  nsCOMPtr<mozilla::layout::ScrollbarActivity> mScrollbarActivity;

Add a typedef to avoid the prefix here.
Comment 201 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-04-18 19:03:06 PDT
Comment on attachment 738667 [details] [diff] [review]
part 10, v1: add nsIScrollbarOwner interface

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

::: layout/generic/nsFrameIdList.h
@@ +58,1 @@
>  FRAME_ID(nsIScrollbarMediator)

These are out of order

::: layout/generic/nsIScrollbarOwner.h
@@ +22,5 @@
> +   * Obtain the frame for the horizontal or vertical scrollbar, or null
> +   * if there is no such box.
> +   */
> +  virtual nsIFrame* GetScrollbarBox(bool aVertical) = 0;
> +

Remove blank line
Comment 202 Stephen A Pohl [:spohl] 2013-04-19 15:44:50 PDT
Created attachment 739864 [details] [diff] [review]
part 10, v1: add nsIScrollbarOwner interface

Addressed review feedback, carrying over r+.
Comment 203 Stephen A Pohl [:spohl] 2013-04-19 15:49:29 PDT
Created attachment 739866 [details] [diff] [review]
part 13, v1: report scrollbar activity from trees

Addressed review feedback. Setting to r? just to be safe and have the part verified about grabbing a reference to mScrollbarActivity into a local nsCOMPtr.
Comment 204 Stephen A Pohl [:spohl] 2013-04-19 15:57:22 PDT
Created attachment 739870 [details] [diff] [review]
part 11, v1: add ScrollbarActivity class

Addressed review feedback and opened bug 863920 for CSS transitions. Carrying over r+.
Comment 205 Stephen A Pohl [:spohl] 2013-04-19 16:08:10 PDT
Thank you roc for the very fast turnaround on these reviews! I believe I've addressed all the review feedback.

I'm still looking into the last failure on try:
https://tbpl.mozilla.org/?tree=Try&rev=f95592fbe8a7

I added a few print statements to the build (search for "assert") which can be seen in the log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=21979132&tree=Try&full=1#error0

This revealed that mScrollPort.XMost() and mScrollPort.YMost() is off by precisely 60 in a total of 9 scenarios, generating 9 asserts. These are treated as test failures.

The value 60 happens to be exactly the number of app units per CSS pixel:
http://mxr.mozilla.org/mozilla-central/source/gfx/src/nsDeviceContext.h#54

Unfortunately, I have yet to find where this is going south. I can't reproduce locally, using the same tests and others, with HIDPI on/off etc.
Comment 206 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-04-20 04:39:22 PDT
Bummer. It might help if you hack the mochitest to turn it into a reduced testcase.
Comment 207 Stephen A Pohl [:spohl] 2013-04-20 12:17:16 PDT
Trying to debug this, I've added more logging, which suddenly made it pass on 10.8:
https://tbpl.mozilla.org/?tree=Try&rev=2357ee01d420

Adding even more logging made it pass on 10.7 as well:
https://tbpl.mozilla.org/?tree=Try&rev=152add0de0de

*scratches head*
Comment 208 Stephen A Pohl [:spohl] 2013-04-22 00:57:59 PDT
Created attachment 740190 [details] [diff] [review]
part 13, v1: report scrollbar activity from trees

I'd like to go back to using nsWeakFrame in nsTreeBodyFrame::UpdateScrollbars because when the frame is destroyed, we actually call Destroy() on mScrollbarActivity (rather than just releasing the object). The nsWeakFrame allows us to check whether or not the frame is still alive before interacting with mScrollbarActivity.

Using an nsCOMPtr crashed in quite a number of cases:
https://tbpl.mozilla.org/?tree=Try&rev=c9e7602a5af6

I've kicked off a try run with this patch and it looks green so far:
https://tbpl.mozilla.org/?tree=Try&rev=c4e42e34010e
Comment 209 Stephen A Pohl [:spohl] 2013-04-22 17:52:21 PDT
(In reply to Stephen Pohl [:spohl] from comment #207)
> Trying to debug this, I've added more logging, which suddenly made it pass
> on 10.8:
> https://tbpl.mozilla.org/?tree=Try&rev=2357ee01d420
> 
> Adding even more logging made it pass on 10.7 as well:
> https://tbpl.mozilla.org/?tree=Try&rev=152add0de0de
> 
> *scratches head*

Turns out that these runs generated the same assertions (see logs), they just didn't appear as failures on tbpl.

Markus added a patch (part 2) in comment 50 to avoid similar assertions when scrollbars are collapsed. However, the assertions don't seem to be constrained to collapsed scrollbars. GetScrollbarMetrics adds margins to *aMin and *aPref. In some scenarios, this can cause the width and/or the height property to drop below 0 and ultimately generate the assertions in nsGfxScrollFrameInner::LayoutScrollbars that we've been seeing.

I've investigated a total of four avenues to fix this and I'll post two patches shortly that seem the most acceptable to me. For completeness, I'd like to give a quick summary of the possible solutions:

1. Remove the assertions in nsGfxScrollFrameInner::LayoutScrollbars and clamp the values for r.width and r.height to 0 instead. I didn't like this option since we probably should have clamped the values earlier if they aren't 'valid' by the time we get here.

2. In nsStyleMargin::RecalcData(), change this line:
     mCachedMargin.Side(side) = CalcCoord(mMargin.Get(side), nullptr, 0);
to
     mCachedMargin.Side(side) = std::max(CalcCoord(mMargin.Get(side), nullptr, 0), 0);
similar to the way it is done in other places in nsStyleStruct.cpp. Based on a try build however, it seems like values < 0 are tolerated or even expect in some scenarios, so this doesn't seem like a proper fix:
https://tbpl.mozilla.org/?tree=Try&rev=3f18330538f8

3. Set *aMin and/or *aPref to nsSize() in GetScrollbarMetrics if either height or width for these objects is < 0.

4. Only clamp the actual properties (width/height) of aMin and aPref to 0 that would otherwise be < 0 in GetScrollbarMetrics.

I'm going to post the patches for approach 3 and 4. Robert, please let me know if these are acceptable and which one would be preferable.
Comment 210 Stephen A Pohl [:spohl] 2013-04-22 17:56:45 PDT
Created attachment 740577 [details] [diff] [review]
part 2, v2: ensure minimum size of 0x0 for scrollbars

Approach 3 from comment 209: Set *aMin and/or *aPref to nsSize() in GetScrollbarMetrics if either height or width for these objects is < 0.

Try run with this patch (and all other patches) looks green so far:
https://tbpl.mozilla.org/?tree=Try&rev=951fb7bcb6c1
Comment 211 Stephen A Pohl [:spohl] 2013-04-22 17:58:46 PDT
Created attachment 740578 [details] [diff] [review]
part 2, v2-alternative: ensure minimum size of 0x0 for scrollbars

Approach 4 from comment 209: Only clamp the actual properties (width/height) of aMin and aPref to 0 that would otherwise be < 0 in GetScrollbarMetrics.

Try run with this patch (and all other patches) looks green so far:
https://tbpl.mozilla.org/?tree=Try&rev=35c1b14bd3fe
Comment 212 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-04-22 18:28:02 PDT
I actually think nsBox::AddMargin should clamp the width and height to a minimum of 0. However, I don't think I should make you do that :-).
Comment 213 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-04-22 18:28:42 PDT
Comment on attachment 740577 [details] [diff] [review]
part 2, v2: ensure minimum size of 0x0 for scrollbars

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

This wouldn't be the right approach.
Comment 214 Stephen A Pohl [:spohl] 2013-04-22 18:42:02 PDT
Thanks Robert for the quick reviews!

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #212)
> I actually think nsBox::AddMargin should clamp the width and height to a
> minimum of 0. However, I don't think I should make you do that :-).

I thought so too and approach 2 was an attempt at that, but it would require a lot of fixing up in our tree and changing of test cases (especially ones that expect those assertions to fire). I'm glad you see it the way you do. :-)
Comment 215 Stephen A Pohl [:spohl] 2013-04-23 10:39:50 PDT
Created attachment 740916 [details] [diff] [review]
part 17: Skip reftests for B2G that have become invalid

This patch skips a total of 6 reftests on B2G that have started to fail with the patches in this bug. For some reason, B2G seems to share some scrollbar code with OSX. The tests used to compare where scrollbars are being rendered with ltr/rtl support. The tests used to expect the rendered pages to look differently. Since we're now hiding the scrollbars unless the user is actively scrolling, the rendered pages look identical and the tests fail.

Page rendered without patches applied, ltr: http://tinyurl.com/d9n3m72
Page rendered without patches applied, rtl: http://tinyurl.com/bqu7gwl
Page rendered with patches applied: http://tinyurl.com/cv6tt5v

Since the code is being shared with OSX, I'm assuming that we expect the scrollbars to behave identically and that it is safe to skip these tests. Asking this question on #b2g and #gaia didn't reveal anything to the contrary.

Try run with all patches applied is green:
https://tbpl.mozilla.org/?tree=Try&rev=eb16349f8d7b

roc, are you able to review this since the file lives under /layout/?
Comment 216 Stephen A Pohl [:spohl] 2013-04-25 11:35:34 PDT
Once part 17 is reviewed I believe this is ready to be checked in. There are known limitations that are filed as bugs.

If you'd like to take a try build for a spin with the patches in this bug, here is the latest one:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/spohl@mozilla.com-35c1b14bd3fe/try-macosx64/firefox-23.0a1.en-US.mac.dmg
Comment 217 beingalink 2013-04-25 12:04:44 PDT
Is the switch to a brighter grey for the scroll bars on dark background supposed to work or is that a separate bug?
Comment 218 beingalink 2013-04-25 12:07:23 PDT
See this site as an example: http://www.klack.de/fernsehprogramm/2015-im-tv/0/free.html
Comment 219 Stephen A Pohl [:spohl] 2013-04-25 12:11:26 PDT
(In reply to beingalink from comment #217)
> Is the switch to a brighter grey for the scroll bars on dark background
> supposed to work or is that a separate bug?

It was supposed to be a separate bug, but it wasn't filed yet. Thanks for the reminder.
Comment 220 David Baron :dbaron: ⌚️UTC-8 2013-04-25 13:32:41 PDT
(In reply to Stephen Pohl [:spohl] from comment #215)
> This patch skips a total of 6 reftests on B2G that have started to fail with
> the patches in this bug. For some reason, B2G seems to share some scrollbar
> code with OSX. The tests used to compare where scrollbars are being rendered
> with ltr/rtl support. The tests used to expect the rendered pages to look
> differently. Since we're now hiding the scrollbars unless the user is
> actively scrolling, the rendered pages look identical and the tests fail.

Is this the desired behavior for B2G?
Comment 221 Stephen A Pohl [:spohl] 2013-04-25 13:43:25 PDT
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #220)
> (In reply to Stephen Pohl [:spohl] from comment #215)
> 
> Is this the desired behavior for B2G?

The reasoning was (from comment 215):
> Since the code is being shared with OSX, I'm assuming that we expect the
> scrollbars to behave identically and that it is safe to skip these tests.
> Asking this question on #b2g and #gaia didn't reveal anything to the
> contrary.

If there is someone who could confirm for sure, it'd be great if you could need-info? this person to this bug.

As an aside: it's interesting that the majority of the tests for the same bug are already being skipped on B2G. The 6 specific tests that I was going to disable are expected to fail on Android as well.
Comment 222 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-04-28 23:12:50 PDT
(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #220)
> Is this the desired behavior for B2G?

B2G has auto-disappearing scrollbars like OSX does. I don't know how these tests were ever supposed to work with B2G. We should just disable them on B2G.
Comment 223 Stephen A Pohl [:spohl] 2013-04-29 09:44:24 PDT
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #222)
> We should just disable them on B2G.

Great! This should be addressed in patch 'part 17' (comment 215).
Comment 224 Stephen A Pohl [:spohl] 2013-05-01 10:05:52 PDT
I believe we're a bit late in the current cycle to land this. Unless there's opposition to this idea, I'll update the patches and get them ready to land once the new cycle has started.
Comment 225 Stephen A Pohl [:spohl] 2013-05-01 16:32:34 PDT
Created attachment 744339 [details] [diff] [review]
Combined patches

This patch combines all individual parts and is updated for current trunk. All individual parts have been r+'d, so marking this patch as r+.

After some discussion, we've decided to land this now to get more feedback. If the overlay scrollbars need to be turned off, there are currently two ways to do so:
1. Under System Preferences > General, select "Show scroll bars: Always".
2. Change nsLookAndFeel::UseOverlayScrollbars to always return false.
Comment 226 Ryan VanderMeulen [:RyanVM] 2013-05-02 05:08:41 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1840b15583fd
Comment 227 Ryan VanderMeulen [:RyanVM] 2013-05-02 05:59:55 PDT
Backed out for build bustage. If this is a needs-clobber issue, be sure to update CLOBBER in the root directory accordingly.
https://hg.mozilla.org/integration/mozilla-inbound/rev/42cf263214c3

https://tbpl.mozilla.org/php/getParsedLog.php?id=22498536&tree=Mozilla-Inbound
Comment 228 Stephen A Pohl [:spohl] 2013-05-02 07:32:06 PDT
Created attachment 744621 [details] [diff] [review]
small adjustment to part 7

enum IntID in LookAndFeel.h has this comment:

  // When modifying this list, also modify nsXPLookAndFeel::sIntPrefs
  // in widget/xpwidgts/nsXPLookAndFeel.cpp.

This wasn't done in part 7. This patch is modifying the list in nsXPLookAndFeel.cpp.
Comment 229 :Ehsan Akhgari 2013-05-02 07:45:03 PDT
Comment on attachment 744621 [details] [diff] [review]
small adjustment to part 7

roc is away, but I've done this stuff before, so I claim that I can review this!
Comment 230 Stephen A Pohl [:spohl] 2013-05-02 07:53:28 PDT
Created attachment 744628 [details] [diff] [review]
Combined patches

Thanks Ehsan!

I've combined all patches in this one.
Comment 231 Stephen A Pohl [:spohl] 2013-05-02 07:55:53 PDT
Created attachment 744631 [details] [diff] [review]
Combined patches

Fixed commit message.
Comment 233 Ryan VanderMeulen [:RyanVM] 2013-05-02 18:45:37 PDT
https://hg.mozilla.org/mozilla-central/rev/0c513ba74137
Comment 234 Szabolcs Horvát 2013-05-03 19:52:30 PDT
I tried this out in today's nightly, and it works nicely.  There's one tiny difference from other Mac programs that I noticed: simply placing two fingers on the trackpad (but not moving them at all) reveals the scrollbar in other programs (e.g. Safari).  This does not happen in Firefox.
Comment 235 Cameron McCormack (:heycam) 2013-05-03 20:06:38 PDT
(In reply to Szabolcs Horvát from comment #234)
> I tried this out in today's nightly, and it works nicely.  There's one tiny
> difference from other Mac programs that I noticed: simply placing two
> fingers on the trackpad (but not moving them at all) reveals the scrollbar
> in other programs (e.g. Safari).  This does not happen in Firefox.

I filed bug 868648 for that.
Comment 236 Ben Perkins 2013-05-18 15:46:43 PDT
I've filed another bug about these scrollbars resizing when the zoom level is increased.
https://bugzilla.mozilla.org/show_bug.cgi?id=873794
Comment 237 Lukas Blakk [:lsblakk] use ?needinfo 2013-05-21 11:19:12 PDT
This has been noted in the Aurora 23 release notes:

http://www.mozilla.org/en-US/firefox/23.0a2/auroranotes/

If you would like to make any changes or have questions/concerns please contact me directly.
Comment 238 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-06-17 15:38:50 PDT
Adding this to the sign-off criteria for Firefox 23.0b1.
Comment 239 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-07-10 16:51:13 PDT
Marking this verified fixed since we've not found any regressions after 4 Betas.
Comment 240 miltonbecker 2013-07-20 17:29:45 PDT
Hello there,
There has been a regression in the latest beta build. The old scrollbar is back. I'm running Firefox 23 - Gecko build ID is: 20130718163513 - on OS X 10.8.4.
Thank you,
Milton
Comment 241 Robert Strong [:rstrong] (use needinfo to contact me) 2013-07-20 17:34:04 PDT
Milton, the new scrollbars has been disabled on Beta for Firefox 23 due to a serious regression. It is now scheduled to be released in Firefox 24 as long as there are no serious regressions.
Comment 242 Mehmet 2013-07-21 15:29:36 PDT
Not sure if this is a know issue, but at www.apple.com the scrollbar do not appear when the window is not maximized.

Steps to reproduce:

1. run latest Nightly
2. open a small window
3. go to www.apple.com
4. scroll

--> Result: No scrollbar
Comment 243 Stephen A Pohl [:spohl] 2013-07-22 06:17:28 PDT
(In reply to Mehmet Sahin from comment #242)
> Not sure if this is a know issue, but at www.apple.com the scrollbar do not
> appear when the window is not maximized.

This wasn't a known issue, thanks for reporting. The preferred way of reporting these issues is by opening new bugs. I went ahead and opened bug 896443 for you.
Comment 244 :Ehsan Akhgari 2013-08-06 07:48:13 PDT
This is mentioned in the release notes for 23: <https://www.mozilla.org/en-US/firefox/23.0/releasenotes/>.  We should remove it there and add it to the 24 release notes.
Comment 245 Alex Keybl [:akeybl] 2013-09-02 12:47:08 PDT
Adding the feature keyword to be included in the new Release Tracking page.

Note You need to log in before you can comment on or make changes to this bug.