Closed Bug 636564 Opened 13 years ago Closed 11 years ago

[10.7] add support for new scrollbar style in Mac OS X 10.7 Lion

Categories

(Core :: Widget: Cocoa, defect, P1)

x86
macOS
defect

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox5 - affected
firefox6 - ---
firefox19 - ---
blocking2.0 --- -
relnote-firefox --- 24+

People

(Reporter: jaas, Assigned: spohl)

References

(Depends on 6 open bugs, Blocks 2 open bugs, )

Details

(Keywords: feature, Whiteboard: [metro-mvp][parity-opera][parity-webkit][LOE:3][lion-scrollbars+])

Attachments

(1 file, 71 obsolete files)

84.04 KB, patch
spohl
: review+
Details | Diff | Splinter Review
      No description provided.
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.
Summary: [10.7] add support for overlay scrollbars in Mac OS X 10.7 Lion → [10.7] add support for new scrollbar style in Mac OS X 10.7 Lion
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.
Assignee: nobody → joshmoz
blocking2.0: --- → ?
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.
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.
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).
Hm, on my DP2 all listed things are on by default (including the reverted scroll behavior with the Magic Mouse).
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.
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.
blocking2.0: ? → -
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.
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.
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.
(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.
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.
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?
> 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.
(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.
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.
(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...
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.
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.
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.)
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).
Depends on: 656990
why not shipping two separate versions for both snow leopard and lion?
Assignee: joshmoz → nobody
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.
> 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.
(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.
Attached patch a small starting point (obsolete) — — Splinter Review
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 :)
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.
(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.
(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.
(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.
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.
Attached patch wip (obsolete) — — Splinter Review
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
Attachment #546630 - Attachment is obsolete: true
no longer tracking for 6. 6 is nearly baked and it's far too late in the game to worry about this.
Google just released a Chrome beta that includes the new scrollbar style. Maybe we can learn something from their code?
Maybe. But the remaining challenges are mostly mozilla-specific.
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.
Attachment #553262 - Attachment is obsolete: true
This is an important feature and we need an owner for this.  Who owns it?
I've made some more progress on this. Taking for now.
Assignee: nobody → mstange
Status: NEW → ASSIGNED
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
(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?
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.
(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.
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.
Any chance this could make the nightly branch before the next merge?
Does anybody have a new link to the testing build?
The patches I'm going to attach here are on top of the one in bug 691609.
Depends on: 691609
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.
Attachment #549325 - Attachment is obsolete: true
Attachment #574046 - Flags: review?(roc)
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.
Attachment #574047 - Flags: review?(roc)
Attached patch part 4, v1: respect resizer minimum size (obsolete) — — Splinter Review
The new scrollbars are going to be 11px wide, but the resizer needs 15px.
Attachment #574049 - Flags: review?(roc)
Attachment #574050 - Flags: review?(enndeakin)
Attachment #574055 - Flags: review?(joshmoz)
Attached patch part 8, v1: add Lion scrollbar native rendering (obsolete) — — Splinter Review
Attachment #574056 - Flags: review?(joshmoz)
Attached patch part 9, v1: add CSS for overlay scrollbars (obsolete) — — Splinter Review
I don't really like hardcoding the 11px here but I haven't had a better idea.
Attachment #574057 - Flags: review?(roc)
Attached patch part 10, v1: add nsIScrollbarHolder interface (obsolete) — — Splinter Review
I'd like to have the complete overlay scrollbar behavior on trees, too, not only on nsGfxScrollFrames.
Attachment #574058 - Flags: review?(roc)
Attached patch part 11, v1: add ScrollbarActivity class (obsolete) — — Splinter Review
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.
Attachment #574059 - Flags: review?(roc)
Attachment #574061 - Flags: review?(roc)
Attached patch part 14, v1: hide inactive scrollbars (obsolete) — — Splinter Review
Attachment #574062 - Flags: review?(roc)
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?
(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.
(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 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 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?
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.
Attachment #574052 - Flags: review?(enndeakin) → review+
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?
(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.)
(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?
(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.
Can you guys merge this into ux or nightly?
Attachment #574050 - Flags: review?(enndeakin) → review+
(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.
Attachment #574055 - Flags: review?(joshmoz) → review+
Attachment #574056 - Flags: review?(joshmoz) → review+
https://hg.mozilla.org/mozilla-central/rev/7f8c69ad0895
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
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?
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: mozilla11 → ---
> 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.
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.
Those patches have been waiting for review for 3 months. Is there anyone else who could review them?
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?
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.
I would like to see this feature. Keep up the good work guys :)
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 :)
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.
Attached file bundle of updated patches (obsolete) —
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()
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)
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.
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!
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.
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!
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.
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!  :-)
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?
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 :)
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?
Attachment #574057 - Flags: review?(roc)
Attachment #574058 - Flags: review?(roc)
Attachment #574059 - Flags: review?(roc)
Attachment #574060 - Flags: review?(roc)
Attachment #574061 - Flags: review?(roc)
Attachment #574062 - Flags: review?(roc)
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.
Markus, any update on this?
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
Blocks: 777610
Whiteboard: metro-beta
Whiteboard: metro-beta → [metro-mvp]
No longer blocks: 777610
Blocks: 777610
Title of this bug should be adjusted to "Lion / ML" since it affect both.
Not clear why this would ever block a release. Untracking.
No idea , why this was not yet implemented, while this is a standard since 2 OS X releases (over a year)...
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.
Whiteboard: [metro-mvp] → [metro-mvp][parity-opera][parity-webkit]
(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
Blocks: 775718
How much more work is needed here? Metro is depending on this too.
Priority: -- → P1
Whiteboard: [metro-mvp][parity-opera][parity-webkit] → [metro-mvp][parity-opera][parity-webkit][LOE:?]
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?
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.
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?
I think a bunch of the patches done for this bug have already landed, but otherwise you should probably just look at the patches.
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.
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.
Alright, thanks. I'll check out the IRC and see what I can do about the scrollbar problem.
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
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.
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.
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.
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.
Cheba, Michael, are you on 10.7? Because I, like Josiah, see problems on Bugzilla on 10.8.
No, 10.8.2
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.
Attached image Rendering issue with scrollbars (obsolete) —
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.
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.
I also saw a few artifacts when scrolling on 10.8.2 and will attach screenshots if I can.
Attached image Issues in Responsive mode (obsolete) —
The artifacts are most obvious in responsive mode.
Attached image Alt rendering issue with scrollbar (obsolete) —
This happens when scrolling if you leave your fingers down when done scrolling and then scroll again.
(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.
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.
(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.
Alright. What progress has been made since yesterday? I haven't heard anything from Markus yet.
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)
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.
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.
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.
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
Alright, great. It took me that long to import 2 patches. At least I know how now.
(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.
Attachment #682006 - Flags: feedback?(roc)
"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?
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.
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.
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 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.
Attachment #574058 - Flags: review+
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.
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?
(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.
I get an error when trying to push your patch "73ba90196d4b". That one does not work.
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.
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
The patches need to be updated to the latest version of firefox. The patches appear to be broken after the merge.
Whiteboard: [metro-mvp][parity-opera][parity-webkit][LOE:?] → [metro-mvp][parity-opera][parity-webkit][LOE:3]
thank you for your work, i'm not a coder but I always wonder, is it that hard to fix scrollbars !!??
(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.
No longer blocks: 775718
Attachment #574046 - Attachment description: part 1, v1: handle scrollbar margins → part 1, v1: handle scrollbar margins This functionality is already in trunk, so marking this patch as obsolete.
Attachment #574046 - Attachment is obsolete: true
Updated for current trunk. Carrying over r+.
Attachment #574047 - Attachment is obsolete: true
Attachment #711056 - Flags: review+
Updated for current trunk. Carrying over r+.
Attachment #711059 - Flags: review+
Attachment #574048 - Attachment is obsolete: true
Attached patch part 4, v1: respect resizer minimum size (obsolete) — — Splinter Review
Updated for current trunk. Carrying over r+.
Attachment #574049 - Attachment is obsolete: true
Attachment #711061 - Flags: review+
Updated for current trunk. Carrying over r+.
Attachment #574050 - Attachment is obsolete: true
Attachment #711063 - Flags: review+
Updated for current trunk. Carrying over r+.
Attachment #574052 - Attachment is obsolete: true
Attachment #711065 - Flags: review+
Updated for current trunk. Carrying over r+.
Attachment #574055 - Attachment is obsolete: true
Attachment #711066 - Flags: review+
Attached patch part 8, v1: add Lion scrollbar native rendering (obsolete) — — Splinter Review
Updated for current trunk. Carrying over r+.
Attachment #574056 - Attachment is obsolete: true
Attachment #711068 - Flags: review+
Attached patch part 10, v1: add nsIScrollbarHolder interface (obsolete) — — Splinter Review
Updated for current trunk. Carrying over r+.
Attachment #574058 - Attachment is obsolete: true
Attached patch part 11, v1: add ScrollbarActivity class (obsolete) — — Splinter Review
Updated for current trunk.
Attachment #574059 - Attachment is obsolete: true
Updated for current trunk.
Attachment #574060 - Attachment is obsolete: true
Updated for current trunk.
Attachment #574061 - Attachment is obsolete: true
Attached patch part 15, v1: make some failing tests pass (obsolete) — — Splinter Review
Updated for current trunk.
Attachment #574063 - Attachment is obsolete: true
Updated for current trunk. Carrying over feedback? that mstange requested from roc.
Attachment #682006 - Attachment is obsolete: true
Attachment #682006 - Flags: feedback?(roc)
Attachment #711082 - Flags: feedback?(roc)
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 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.
Attachment #711082 - Flags: feedback?(roc) → feedback-
(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.
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.
Markus, could you let me know if you'd like me to drive this from here?
Flags: needinfo?(mstange)
Yes, please! :)
Flags: needinfo?(mstange)
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.
Attached patch part 4, v1: respect resizer minimum size (obsolete) — — Splinter Review
Updated for current trunk and removed some trailing white space. Carrying over r+.
Attachment #711061 - Attachment is obsolete: true
Attachment #717355 - Flags: review+
Attached patch part 8, v1: add Lion scrollbar native rendering (obsolete) — — Splinter Review
Updated for current trunk, removed trailing white space and added rollover argument to CUIDraw call based on Markus' suggestion. Carrying over r+.
Attachment #711068 - Attachment is obsolete: true
Attachment #717363 - Flags: review+
Attached patch part 10, v1: add nsIScrollbarOwner interface (obsolete) — — Splinter Review
Renamed nsIScrollbarHolder to nsIScrollbarOwner. Removed GetEventTarget methods that were added in previous versions of this patch. Removed trailing white space.
Attachment #711069 - Attachment is obsolete: true
Attached patch part 11, v1: add ScrollbarActivity class (obsolete) — — Splinter Review
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.
Attachment #711070 - Attachment is obsolete: true
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.
Attachment #711072 - Attachment is obsolete: true
Updated for current trunk. Removed trailing white space. Slight enhancements from Markus.
Attachment #711074 - Attachment is obsolete: true
Attached patch part 15, v1: make some failing tests pass (obsolete) — — Splinter Review
Removed some trailing white space. Fixed type pointed out by Markus ("matchs" vs. "matches").
Attachment #711076 - Attachment is obsolete: true
Attached patch part 16, v1: make scrollbars wider on hover (obsolete) — — Splinter Review
Does not animate yet.
Updated for current trunk. Carrying over r+.
Attachment #711063 - Attachment is obsolete: true
Attachment #721495 - Flags: review+
Updated for current trunk. Carrying over r+.
Attachment #711066 - Attachment is obsolete: true
Attachment #721497 - Flags: review+
Attached patch part 9, v1: add CSS for overlay scrollbars (obsolete) — — Splinter Review
Updated for current trunk.
Attachment #574057 - Attachment is obsolete: true
Attached patch part 11, v1: add ScrollbarActivity class (obsolete) — — Splinter Review
Updated for current trunk.
Attachment #717368 - Attachment is obsolete: true
Attached patch part 14, v1: hide inactive scrollbars (obsolete) — — Splinter Review
Updated for current trunk.
Attachment #574062 - Attachment is obsolete: true
Attached patch part 16, v1: make scrollbars wider on hover (obsolete) — — Splinter Review
Updated for current trunk.
Attachment #717376 - Attachment is obsolete: true
Updated for current trunk, carrying over r+.
Attachment #711059 - Attachment is obsolete: true
Attachment #738203 - Flags: review+
Attached patch part 4, v1: respect resizer minimum size (obsolete) — — Splinter Review
Reverted removal of trailing white space from the previous update, since this would disturb hg blame. Carrying over r+.
Attachment #717355 - Attachment is obsolete: true
Attachment #738206 - Flags: review+
Attached patch part 8, v1: add Lion scrollbar native rendering (obsolete) — — Splinter Review
Reverted removal of trailing white space from the previous update since this would disturb hg blame. Carrying over r+.
Attachment #717363 - Attachment is obsolete: true
Attachment #738221 - Flags: review+
Attached patch part 10, v1: add nsIScrollbarOwner interface (obsolete) — — Splinter Review
Updated for current trunk and reverted removal of trailing white space from the previous update since this would disturb hg blame.
Attachment #717367 - Attachment is obsolete: true
Attached patch part 11, v1: add ScrollbarActivity class (obsolete) — — Splinter Review
Updated for current trunk.
Attachment #721507 - Attachment is obsolete: true
Updated for current trunk.
Attachment #717370 - Attachment is obsolete: true
Attached patch part 15, v1: make some failing tests pass (obsolete) — — Splinter Review
Reverted removal of trailing white space from the previous update since this would disturb hg blame.
Attachment #717372 - Attachment is obsolete: true
Attached patch part 16, v1: make scrollbars wider on hover (obsolete) — — Splinter Review
Updated for current trunk.
Attachment #721510 - Attachment is obsolete: true
Updated for current trunk, carrying over r+.
Attachment #711056 - Attachment is obsolete: true
Attachment #738249 - Flags: review+
Attached patch part 10, v1: add nsIScrollbarOwner interface (obsolete) — — Splinter Review
Updated for current trunk.
Attachment #738226 - Attachment is obsolete: true
Attached patch part 15, v1: make some failing tests pass (obsolete) — — Splinter Review
Fixed test for bug 485118.
Attachment #738235 - Attachment is obsolete: true
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.
Attachment #721504 - Flags: review?(roc)
Attachment #738667 - Flags: review?(roc)
Attachment #738228 - Flags: review?(roc)
Attachment #738231 - Flags: review?(roc)
Attachment #721509 - Flags: review?(roc)
Attachment #738668 - Flags: review?(roc)
Attachment #738237 - Flags: review?(roc)
Attachment #711082 - Attachment is obsolete: true
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 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.
Attachment #738231 - Flags: review?(roc) → review+
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
Attachment #738667 - Flags: review?(roc) → review+
Attached patch part 10, v1: add nsIScrollbarOwner interface (obsolete) — — Splinter Review
Addressed review feedback, carrying over r+.
Attachment #738667 - Attachment is obsolete: true
Attachment #739864 - Flags: review+
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.
Attachment #738231 - Attachment is obsolete: true
Attachment #739866 - Flags: review?(roc)
Blocks: 863920
Attached patch part 11, v1: add ScrollbarActivity class (obsolete) — — Splinter Review
Addressed review feedback and opened bug 863920 for CSS transitions. Carrying over r+.
Attachment #738228 - Attachment is obsolete: true
Attachment #739870 - Flags: review+
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.
Bummer. It might help if you hack the mochitest to turn it into a reduced testcase.
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*
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
Attachment #739866 - Attachment is obsolete: true
Attachment #740190 - Flags: review?(roc)
(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.
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
Attachment #738249 - Attachment is obsolete: true
Attachment #740577 - Flags: review?(roc)
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
Attachment #740578 - Flags: review?(roc)
Blocks: 864596
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 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.
Attachment #740577 - Flags: review?(roc) → review-
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. :-)
Attachment #740577 - Attachment is obsolete: true
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/?
Attachment #740916 - Flags: review?(roc)
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
Is the switch to a brighter grey for the scroll bars on dark background supposed to work or is that a separate bug?
Blocks: 865806
(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.
(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?
(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.
(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.
(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).
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.
Attached patch Combined patches (obsolete) — — Splinter Review
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.
Assignee: mstange → spohl.mozilla.bugs
Attachment #553266 - Attachment is obsolete: true
Attachment #612009 - Attachment is obsolete: true
Attachment #675608 - Attachment is obsolete: true
Attachment #675619 - Attachment is obsolete: true
Attachment #675621 - Attachment is obsolete: true
Attachment #711065 - Attachment is obsolete: true
Attachment #721495 - Attachment is obsolete: true
Attachment #721497 - Attachment is obsolete: true
Attachment #721504 - Attachment is obsolete: true
Attachment #721509 - Attachment is obsolete: true
Attachment #738203 - Attachment is obsolete: true
Attachment #738206 - Attachment is obsolete: true
Attachment #738221 - Attachment is obsolete: true
Attachment #738237 - Attachment is obsolete: true
Attachment #738668 - Attachment is obsolete: true
Attachment #739864 - Attachment is obsolete: true
Attachment #739870 - Attachment is obsolete: true
Attachment #740190 - Attachment is obsolete: true
Attachment #740578 - Attachment is obsolete: true
Attachment #740916 - Attachment is obsolete: true
Attachment #744339 - Flags: review+
Keywords: checkin-needed
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
Attached patch small adjustment to part 7 (obsolete) — — Splinter Review
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.
Attachment #744621 - Flags: review?(roc)
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!
Attachment #744621 - Flags: review?(roc) → review+
Attached patch Combined patches (obsolete) — — Splinter Review
Thanks Ehsan!

I've combined all patches in this one.
Attachment #744339 - Attachment is obsolete: true
Attachment #744621 - Attachment is obsolete: true
Attachment #744628 - Flags: review+
Attached patch Combined patches — — Splinter Review
Fixed commit message.
Attachment #744628 - Attachment is obsolete: true
Attachment #744631 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/0c513ba74137
Status: REOPENED → RESOLVED
Closed: 12 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Depends on: 868316
Depends on: 868416
Depends on: 868421
Depends on: 868432
Depends on: 868498
Depends on: 868648
Depends on: 868659
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.
(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.
Depends on: 869519
Depends on: 870917
Depends on: 871215
Depends on: 873010
Depends on: 873012
Depends on: 873651
I've filed another bug about these scrollbars resizing when the zoom level is increased.
https://bugzilla.mozilla.org/show_bug.cgi?id=873794
No longer blocks: 865806, 868420
Depends on: 865806, 868420
No longer blocks: 863920
Depends on: 863920
No longer blocks: 864596
Depends on: 864596
Blocks: 863920
No longer depends on: 863920
Blocks: 864596
No longer depends on: 864596
Whiteboard: [metro-mvp][parity-opera][parity-webkit][LOE:3] → [metro-mvp][parity-opera][parity-webkit][LOE:3][lion-scrollbars+]
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.
Depends on: 874486
Depends on: 876310
Depends on: 877097
Depends on: 878705
See Also: → 775718
Blocks: 880753
Depends on: 881022
Adding this to the sign-off criteria for Firefox 23.0b1.
Keywords: verifyme
Depends on: 886160
Depends on: 887977
Marking this verified fixed since we've not found any regressions after 4 Betas.
Status: RESOLVED → VERIFIED
Keywords: verifyme
Depends on: 895203
Depends on: 895499
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
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.
Target Milestone: mozilla23 → mozilla24
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
Depends on: 896443
(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.
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.
Flags: needinfo?(lsblakk)
Flags: needinfo?(lsblakk)
Depends on: 904561
Adding the feature keyword to be included in the new Release Tracking page.
Keywords: feature
Depends on: 918996
Depends on: 920550
Blocks: 959025
Blocks: 1450883
No longer blocks: 1450883
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: