Bug 34049

Summary: ScrollbarTheme[Chromium]Mac.mm wrongly set scrollbar view size to page step size
Product: WebKit Reporter: Peter Kasting <pkasting>
Component: WebKit Misc.Assignee: Peter Kasting <pkasting>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, eric, hyatt, john, mitz
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Attachments:
Description Flags
patch v1 (no layout test updates)
mitz: review+
Layout test rebaselines
none
Layout test rebaselines v2 mitz: review+

Description Peter Kasting 2010-01-24 00:07:29 PST
In ScrollbarTheme[Chromium]Mac::paint(), we have the following line:

trackInfo.trackInfo.scrollbar.viewsize = scrollbar->pageStep();

There is no documentation on what this member represents, other than header comments, and the ones in Appearance.h are not too enlightening: "The current view range size".

I believe this is supposed to be the size of the visible area, and thus should be using scrollbar->visibleSize().  Using pageStep() meant that when bug 32595 changed how far we travel when scrolling by page, the scroll thumb changed size, which is unexpected.

Changing this to use visibleSize() will necessitate updating a number of layout test pixel baselines.
Comment 1 Peter Kasting 2010-01-25 10:49:47 PST
Created attachment 47358 [details]
patch v1 (no layout test updates)

Here's the actual fix.  This doesn't change any test baselines.  Note that the affected tests already don't match their baselines today.
Comment 2 Peter Kasting 2010-01-25 18:02:13 PST
Created attachment 47380 [details]
Layout test rebaselines

Here are the layout test rebaselines to go with the patch.

There are also a few other tests I should mention:
* The following tests have a pixel diff showing scroll thumb length changes, but are actually caused by some other pre-existing change, not my patch:
  * fast/block/margin-collapse/103.html
  * fast/inline/long-wrapped-line.html
  * fast/multicol/float-multicol.html
* The following tests are affected by my change, but also already have other pixel diffs, so I won't rebaseline them to avoid papering over other regressions:
  * compositing/layers-inside-overflow-scroll.html
  * compositing/self-painting-layers.html
  * css1/font_properties/font_family.html
  * fast/box-shadow/basic-shadows.html
  * fast/forms/textarea-scrollbar.html
Comment 3 Eric Seidel (no email) 2010-01-25 18:37:23 PST
Seems like this would be a 2-second review for hyatt.  I don't feel qualified here.
Comment 4 Dimitri Glazkov (Google) 2010-01-25 18:40:35 PST
Hy-yatt! Hy-yatt! :)
Comment 5 mitz 2010-01-25 20:42:39 PST
Comment on attachment 47380 [details]
Layout test rebaselines

    platform/mac/fast/overflow/line-clamp-expected.png
    platform/mac-leopard/compositing/overflow/overflow-positioning-expected.png
and
    svg/custom/text-zoom-expected.png

appear to be unrelated to your patch (the latter looks no different from what’s already checked in, I couldn’t discern any difference in the former either).
Comment 6 Peter Kasting 2010-01-25 21:22:12 PST
Created attachment 47386 [details]
Layout test rebaselines v2

Good catches, all correct.  Fixed.
Comment 7 mitz 2010-01-25 21:24:30 PST
Comment on attachment 47386 [details]
Layout test rebaselines v2

> +        * fast/repaint/inline-overflow-expected.png:
> +        * fast/repaint/inline-relative-positioned-expected.png:
> +        * fast/repaint/table-cell-overflow-expected.png:
> +        * fast/repaint/trailing-floats-root-line-box-overflow-expected.png:

These too.
Comment 8 Peter Kasting 2010-01-25 21:35:51 PST
Sigh, yes.  Will fix before landing.

Didn't even think of also checking outside platform/mac when I generated the new baselines.  Sloppy.  Sorry.
Comment 9 Peter Kasting 2010-01-25 21:58:12 PST
Landed both patches in r53838.
Comment 10 Simon Fraser (smfr) 2011-03-28 15:23:40 PDT
*** Bug 23819 has been marked as a duplicate of this bug. ***