Bug 34049 – ScrollbarTheme[Chromium]Mac.mm wrongly set scrollbar view size to page step size

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Peter Kasting]

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.

[Peter Kasting]

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.

[Peter Kasting]

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

[Eric Seidel (no email)]

Seems like this would be a 2-second review for hyatt.  I don't feel qualified here.

[Dimitri Glazkov (Google)]

Hy-yatt! Hy-yatt! :)

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).

[Peter Kasting]

Created attachment 47386 [details]
Layout test rebaselines v2

Good catches, all correct.  Fixed.

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.

[Peter Kasting]

Sigh, yes.  Will fix before landing.

Didn't even think of also checking outside platform/mac when I generated the new baselines.  Sloppy.  Sorry.

[Peter Kasting]

Landed both patches in r53838.

[Simon Fraser (smfr)]

*** Bug 23819 has been marked as a duplicate of this bug. ***