Bug 51151 – NSScrollView-based scrollbars and ScrollBarTheme-based scrollbars don't look the same

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Dimitri Glazkov (Google)]

It's an odd thing, but it appears that painting scrollbars using HIThemeDrawTrack and just relying on NSScrollView to make them doesn't produce the same result. Here's an example:

Exhibit A: http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/mac/fast/block/float/008-expected.png (uses NSScrollView to paint scrollbars)
Exhibit B: http://build.webkit.org/results/Chromium%20Mac%20Release%20(Tests)/r74045%20(3464)/fast/block/float/008-actual.png (uses ScrollBarTheme)
Exhibit C: http://build.webkit.org/results/Chromium%20Mac%20Release%20(Tests)/r74045%20(3464)/fast/block/float/008-diff.png (the diff between the two)

I spent a day trying to hack ScrollbarThemeChromiumMac.mm into submission, just trying to see what the problem is. Turns out there are lots of funny little issues:

1) The proportions of the thumb are different for each path. Not sure why, but it requires tweaking SliderTrackInfo::viewsize to get them to match. The amount seems to be some proportional value, because it varies depending on the value of max/value in ThemeTrackDrawInfo. 
2) The wavey Aqua texture on NSScrollView is off by exactly -11px compared to hand-drawn scrollbars. Again, not sure why. So I wrote an extra step to shift the thumb down, paint into a buffer, then paint back onto the track shifted upwards by the same amount.
3) Even after that, the caps on the thumb don't match! For some reason, they are also drawn differently.

To summarize, I think it might be worth pursuing other options, such as:

1) convert Mac port to hand-drawn scrollbars and rebaseline the tests;
2) cry into a pillow.

Unless someone has better ideas? :)

[Dimitri Glazkov (Google)]

BTW, I started where Avi left off a while back.

[Avi Drissman]

For those who are just joining us, I last dealt with this about a year ago when re-enabling Mac pixel tests for Chromium.

http://www.mail-archive.com/[e-mail]/msg08115.html

If we're OK with having different baselines for WebKit and Chromium, my conclusions still stand. It seems like you're trying to reconcile them, and that's going to be painful.

Right now, for the sake of a Cocoa hierarchy, the top level scrollbars (and only those) are drawn with an NSScrollView. You're asking if we can ditch those in WebKit, and that's an answer I can't provide. Are you proposing using ScrollbarThemeMac for only tests and not Safari? (I suppose that's what you mean by "hand-drawn scrollbars".)

[Avi Drissman]

BTW, the conclusion that I came to was that it simply wasn't worth the hassle to try to make an HIThemeDrawTrack scrollbar draw exactly like an NSScroller. They are two entirely different code paths and playing games is only going to leave someone in tears.

And Mac OS X doesn't cry.

[James Robinson]

In the long term, nobody will use NSScrollView for scrollbars on mac - Chromium already does not and WebKit2 will not either.  Where are the WebKit2 pixel baselines expected to go?

[Dimitri Glazkov (Google)]

(In reply to comment #4)
> In the long term, nobody will use NSScrollView for scrollbars on mac - Chromium already does not and WebKit2 will not either.  Where are the WebKit2 pixel baselines expected to go?

Adding fearless WebKit2 peeps.

[Dimitri Glazkov (Google)]

Created attachment 76774 [details]
The closest I could come to matching

I got pretty close :)

[Mihai Parparita]

H(In reply to comment #3)
> BTW, the conclusion that I came to was that it simply wasn't worth the hassle to try to make an HIThemeDrawTrack scrollbar draw exactly like an NSScroller. They are two entirely different code paths and playing games is only going to leave someone in tears.

Can we just use NSScroller directly (instantiate one, use setKnobProportion, etc, and then have it draw itself)?

[Avi Drissman]

(In reply to comment #7)
> Can we just use NSScroller directly (instantiate one, use setKnobProportion, etc, and then have it draw itself)?

I suppose we could. But if we're dropping NSScrollViews altogether and moving towards only ScrollbarThemeMac ones, I don't see the advantage of dropping the current implementation of ScrollbarThemeMac for a new one based on Cocoa widgets.

[Mihai Parparita]

(In reply to comment #8)
> I suppose we could. But if we're dropping NSScrollViews altogether and moving towards only ScrollbarThemeMac ones, I don't see the advantage of dropping the current implementation of ScrollbarThemeMac for a new one based on Cocoa widgets.

Will HIThemeDrawTrack be around indefinitely? It's part of the "legacy" Appearance Manager API.

[Avi Drissman]

(In reply to comment #9)
> Will HIThemeDrawTrack be around indefinitely? It's part of the "legacy" Appearance Manager API.

HIThemeDrawTrack isn't Appearance Manager, it's HITheme. When Carbon was killed for 64-bits, people at WWDC asked explicitly about HITheme and using it to draw appearance primitives, and Apple said that it would stick around. For now, I'm not worried about its future.

[Mihai Parparita]

Created attachment 76930 [details]
Patch

[Mihai Parparita]

The attached patch seems to produce identical results to the native NSScrollView scrollbars. It has a bunch of FIXMEs, but if this seems like a reasonable approach, I can look into getting it landed. The biggest hack it in is the use of FakeActiveWindow and overriding the private _hasActiveControls NSWindow method to control (in)active appearance. Is there any way to improve on that?

[Mihai Parparita]

Avi and I chatted about this, and he feels like the right thing to do is to ge the Safari/Mac port (or at least its DRT) off of NSScrollView for the root scrollable area, so that it matches the other ports.

However, from talking to James, I realized that that means a mass rebaseline (in platform/mac and platform/mac-leopard) of all layout tests that have scrollbars in them. The whole point of Dimitri's and my exploration in this area is to get Chromium/Mac to match Safari/Mac so that we can use the existing baselines. If we're going to be doing a mass rebaseline anyway, we might as well just maintain platform/chromium-mac baselines with our different scrollbar rendering (since we're already down that path -- we started with ~400 scrollbar-related failures in platform/chromium/test_expectations.txt and are now down to 229).

Avi, how about I limit the scope of this patch to only use NSScroller when running under the Chromium DRT _and_ we're rendering the top-level scrollbar? That way any brittleness will be isolated. I'm also open to any suggestions that you might have to lessen that (e.g. can we get at the real NSWindow that we're in, instead of needing FakeActiveWindow?)

[Avi Drissman]

(In reply to comment #13)
> Avi, how about I limit the scope of this patch to only use NSScroller when running under the Chromium DRT _and_ we're rendering the top-level scrollbar? That way any brittleness will be isolated.

Yes, that would likely work, though I'm not thrilled at the code flow (if we're in DRT and we're at the top level then run this entirely different code path). BTW, how do you plan to pipe in that state so that the scrollbar code would know?

> I'm also open to any suggestions that you might have to lessen that (e.g. can we get at the real NSWindow that we're in, instead of needing FakeActiveWindow?)

Um, the window that the renderer's output is drawn into is in a different process. No, you may not get at it.

[Mihai Parparita]

(In reply to comment #14)
> (In reply to comment #13)
> > Avi, how about I limit the scope of this patch to only use NSScroller when running under the Chromium DRT _and_ we're rendering the top-level scrollbar? That way any brittleness will be isolated.
> 
> Yes, that would likely work, though I'm not thrilled at the code flow (if we're in DRT and we're at the top level then run this entirely different code path). BTW, how do you plan to pipe in that state so that the scrollbar code would know?

I'd use a similar mechanism to the Windows port, which does something similar. It uses DRT-specific widget rendering code to get consistency between Win7, Vista and XP (see comments in http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/chromium/WebThemeEngineDRT.h#L31). Specifically:

ScrollbarThemeChromiumWin::paintTrackPiece (http://trac.webkit.org/browser/trunk/WebCore/platform/chromium/ScrollbarThemeChromiumWin.cpp#L107) calls:
ChromiumBridge::paintScrollbarTrack (http://trac.webkit.org/browser/trunk/WebKit/chromium/src/ChromiumBridge.cpp#L742) which calls:
WebThemeEngine:: paintScrollbarTrack (defined at http://trac.webkit.org/browser/trunk/WebKit/chromium/public/win/WebThemeEngine.h#L67)

WebThemeEngine:: paintScrollbarTrack  has multiple implementations:
http://www.google.com/codesearch?hl=en&vert=chromium&lr=&q=engine.*::paintscrollbartrack&sbtn=Search

The one in WebThemeEngineDRT::paintScrollbarTrack is pulled in via a .gyp condition: http://trac.webkit.org/browser/trunk/Tools/DumpRenderTree/DumpRenderTree.gypi#L61

Linux has its own WebThemeEngine definition/implementation (added pretty recently: http://trac.webkit.org/changeset/69311), Mac will join the club.

[Avi Drissman]

OK. I have no doubts that you're capable of doing it. I only have opinions left.

1. Yes, we can have two different scrollbar drawing implementations. It may solve a real problem. That doesn't mean it doesn't give me a bad feeling and scream "backward-looking". My gut says that fixing the top-level scrollbar != other scrollbar problem for Safari and rebaselining everything is a real solution and that this is just a hack.

2. You're doing WebThemeEngine for the Mac? Wasn't jam working on that? There is no way to draw the separate pieces of a scrollbar on the Mac (arrows, track, etc) with HITheme. Almost certainly not with the public API of NSScroller either.

Avi

[Mihai Parparita]

(In reply to comment #16)
> 1. Yes, we can have two different scrollbar drawing implementations. It may solve a real problem. That doesn't mean it doesn't give me a bad feeling and scream "backward-looking". My gut says that fixing the top-level scrollbar != other scrollbar problem for Safari and rebaselining everything is a real solution and that this is just a hack.

Yes, this is a hack, but it seems preferable to either not looking at the results of hundreds of tests or maintaining parallel baselines for them.

> 2. You're doing WebThemeEngine for the Mac? Wasn't jam working on that? 

Not sure, adding him to the bug.

> There is no way to draw the separate pieces of a scrollbar on the Mac (arrows, track, etc) with HITheme. Almost certainly not with the public API of NSScroller either.

The only part that's actually different is the thumb, so that's the only part we'd need to move into WebThemeEngineMac. We already draw that separately with HITheme:

http://trac.webkit.org/browser/trunk/WebCore/platform/chromium/ScrollbarThemeChromiumMac.mm#L450

And NSScroller has drawKnob: http://developer.apple.com/library/mac/#documentation/Cocoa/Reference/ApplicationKit/Classes/NSScroller_Class/Reference/Reference.html#//apple_ref/occ/instm/NSScroller/drawKnob

[Avi Drissman]

So kinda like we have today, where the Lin and Win WebThemeEngines are totally different?

Can we put a TODO in this so that someday when we drop using the NSScrollView for the outer scroller we'll remember to drop these weird dual code paths?

[Mihai Parparita]

I've landed all the necessary code changes to fix this, all that remains is to flip the USE_WEB_THEME_ENGINE_TO_PAINT_THUMB #define in ScrollbarThemeChromiumMac. I'll try to do that in the next couple of days, while I'm still gardening, so that the inevitable mass rebaseline is less disruptive.

[Mihai Parparita]

Committed r74892: <http://trac.webkit.org/changeset/74892>

[Mihai Parparita]

Keeping this open until rebaselines are done.

[WebKit Review Bot]

http://trac.webkit.org/changeset/74892 might have broken Leopard Intel Debug (Tests)

[Mihai Parparita]

Marking this as fixed since I'm done with rebaselines. Bug 52414 is still open about a small remaining mac vs. chromium-mac scrollbar difference.

[Eric Seidel (no email)]

Did we ever file a radar about this API mismatch?

[Mihai Parparita]

(In reply to comment #24)
> Did we ever file a radar about this API mismatch?

Filed rdar://8881862 (http://openradar.appspot.com/8881862)

[Eric Seidel (no email)]

Ideally you should mention this (or any other WebKit/Chromium) bugs in your Radar.  I've poked relevant people @ Apple.

[Mihai Parparita]

(In reply to comment #26)
> Ideally you should mention this (or any other WebKit/Chromium) bugs in your Radar.  I've poked relevant people @ Apple.

Done.

[Mihai Parparita]

Curiouser and curiouser: http://trac.webkit.org/changeset/76292

[Eric Seidel (no email)]

You should probably mention that commit and the associated Radar in your Radar if you haven't already.

[Avi Drissman]

(In reply to comment #28)
> Curiouser and curiouser: http://trac.webkit.org/changeset/76292

Sigh. Because all we needed was more opaque SPI. :(

[Eric Seidel (no email)]

I suspect the opaque API may be related to the need for the chromium hack and mihai's radar.  We just need to get the right Apple people talkign to one another so we can remove our hack and they can remove their opaque API. :)