Bug 34049 - ScrollbarTheme[Chromium]Mac.mm wrongly set scrollbar view size to page step size
Summary: ScrollbarTheme[Chromium]Mac.mm wrongly set scrollbar view size to page step size
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Peter Kasting
URL:
Keywords:
: 23819 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-01-24 00:07 PST by Peter Kasting
Modified: 2011-03-28 15:23 PDT (History)
5 users (show)

See Also:


Attachments
patch v1 (no layout test updates) (2.09 KB, patch)
2010-01-25 10:49 PST, Peter Kasting
mitz: review+
Details | Formatted Diff | Diff
Layout test rebaselines (deleted)
2010-01-25 18:02 PST, Peter Kasting
no flags Details | Formatted Diff | Diff
Layout test rebaselines v2 (deleted)
2010-01-25 21:22 PST, Peter Kasting
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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. ***