|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>|
|Severity:||Normal||CC:||dglazkov, eric, hyatt, john, mitz|
|Version:||528+ (Nightly build)|
|OS:||OS X 10.5|
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.