34049 2010-01-24 00:07:29 -0800 ScrollbarTheme[Chromium]Mac.mm wrongly set scrollbar view size to page step size 2011-03-28 15:23:40 -0700 1 1 1 Unclassified WebKit WebKit Misc. 528+ (Nightly build) PC OS X 10.5 RESOLVED FIXED P2 Normal --- 1 pkasting pkasting dglazkov eric hyatt john mitz oldest_to_newest 184051 0 pkasting 2010-01-24 00:07:29 -0800 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. 184368 1 47358 pkasting 2010-01-25 10:49:47 -0800 Created attachment 47358 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. 184461 2 47380 pkasting 2010-01-25 18:02:13 -0800 Created attachment 47380 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 184466 3 eric 2010-01-25 18:37:23 -0800 Seems like this would be a 2-second review for hyatt. I don't feel qualified here. 184467 4 dglazkov 2010-01-25 18:40:35 -0800 Hy-yatt! Hy-yatt! :) 184480 5 47380 mitz 2010-01-25 20:42:39 -0800 Comment on attachment 47380 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). 184491 6 47386 pkasting 2010-01-25 21:22:12 -0800 Created attachment 47386 Layout test rebaselines v2 Good catches, all correct. Fixed. 184492 7 47386 mitz 2010-01-25 21:24:30 -0800 Comment on attachment 47386 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. 184497 8 pkasting 2010-01-25 21:35:51 -0800 Sigh, yes. Will fix before landing. Didn't even think of also checking outside platform/mac when I generated the new baselines. Sloppy. Sorry. 184499 9 pkasting 2010-01-25 21:58:12 -0800 Landed both patches in r53838. 374831 10 simon.fraser 2011-03-28 15:23:40 -0700 *** Bug 23819 has been marked as a duplicate of this bug. *** 47358 2010-01-25 10:49:47 -0800 2010-01-25 20:31:55 -0800 patch v1 (no layout test updates) patch text/plain 2140 pkasting SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiA1MzgwNikKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTUgQEAKKzIwMTAtMDEtMjUgIFBldGVyIEthc3RpbmcgIDxwa2FzdGluZ0Bnb29n bGUuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAg IE1hYyBzY3JvbGxiYXIgdGh1bWJzIHdlcmUgZHJhd24gd2l0aCB0aGUgd3Jvbmcgc2l6ZS4KKyAg ICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTM0MDQ5CisKKyAg ICAgICAgKiBwbGF0Zm9ybS9jaHJvbWl1bS9TY3JvbGxiYXJUaGVtZUNocm9taXVtTWFjLm1tOgor ICAgICAgICAoV2ViQ29yZTo6U2Nyb2xsYmFyVGhlbWVDaHJvbWl1bU1hYzo6cGFpbnQpOgorICAg ICAgICAqIHBsYXRmb3JtL21hYy9TY3JvbGxiYXJUaGVtZU1hYy5tbToKKyAgICAgICAgKFdlYkNv cmU6OlNjcm9sbGJhclRoZW1lTWFjOjpwYWludCk6CisKIDIwMTAtMDEtMjUgIFBhdmVsIEZlbGRt YW4gIDxwZmVsZG1hbkBjaHJvbWl1bS5vcmc+CiAKICAgICAgICAgUmV2aWV3ZWQgYnkgVGltb3Ro eSBIYXRjaGVyLgpJbmRleDogV2ViQ29yZS9wbGF0Zm9ybS9jaHJvbWl1bS9TY3JvbGxiYXJUaGVt ZUNocm9taXVtTWFjLm1tCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvcGxhdGZvcm0vY2hyb21pdW0v U2Nyb2xsYmFyVGhlbWVDaHJvbWl1bU1hYy5tbQkocmV2aXNpb24gNTM4MDYpCisrKyBXZWJDb3Jl L3BsYXRmb3JtL2Nocm9taXVtL1Njcm9sbGJhclRoZW1lQ2hyb21pdW1NYWMubW0JKHdvcmtpbmcg Y29weSkKQEAgLTM3NSw3ICszNzUsNyBAQCBib29sIFNjcm9sbGJhclRoZW1lQ2hyb21pdW1NYWM6 OnBhaW50KFNjCiAgICAgdHJhY2tJbmZvLm1pbiA9IDA7CiAgICAgdHJhY2tJbmZvLm1heCA9IHNj cm9sbGJhci0+bWF4aW11bSgpOwogICAgIHRyYWNrSW5mby52YWx1ZSA9IHNjcm9sbGJhci0+Y3Vy cmVudFBvcygpOwotICAgIHRyYWNrSW5mby50cmFja0luZm8uc2Nyb2xsYmFyLnZpZXdzaXplID0g c2Nyb2xsYmFyLT5wYWdlU3RlcCgpOworICAgIHRyYWNrSW5mby50cmFja0luZm8uc2Nyb2xsYmFy LnZpZXdzaXplID0gc2Nyb2xsYmFyLT52aXNpYmxlU2l6ZSgpOwogICAgIHRyYWNrSW5mby5hdHRy aWJ1dGVzID0gMDsKICAgICBpZiAoc2Nyb2xsYmFyLT5vcmllbnRhdGlvbigpID09IEhvcml6b250 YWxTY3JvbGxiYXIpCiAgICAgICAgIHRyYWNrSW5mby5hdHRyaWJ1dGVzIHw9IGtUaGVtZVRyYWNr SG9yaXpvbnRhbDsKSW5kZXg6IFdlYkNvcmUvcGxhdGZvcm0vbWFjL1Njcm9sbGJhclRoZW1lTWFj Lm1tCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvcGxhdGZvcm0vbWFjL1Njcm9sbGJhclRoZW1lTWFj Lm1tCShyZXZpc2lvbiA1MzgwNikKKysrIFdlYkNvcmUvcGxhdGZvcm0vbWFjL1Njcm9sbGJhclRo ZW1lTWFjLm1tCSh3b3JraW5nIGNvcHkpCkBAIC0zNjIsNyArMzYyLDcgQEAgYm9vbCBTY3JvbGxi YXJUaGVtZU1hYzo6cGFpbnQoU2Nyb2xsYmFyKgogICAgIHRyYWNrSW5mby5taW4gPSAwOwogICAg IHRyYWNrSW5mby5tYXggPSBzY3JvbGxiYXItPm1heGltdW0oKTsKICAgICB0cmFja0luZm8udmFs dWUgPSBzY3JvbGxiYXItPmN1cnJlbnRQb3MoKTsKLSAgICB0cmFja0luZm8udHJhY2tJbmZvLnNj cm9sbGJhci52aWV3c2l6ZSA9IHNjcm9sbGJhci0+cGFnZVN0ZXAoKTsKKyAgICB0cmFja0luZm8u dHJhY2tJbmZvLnNjcm9sbGJhci52aWV3c2l6ZSA9IHNjcm9sbGJhci0+dmlzaWJsZVNpemUoKTsK ICAgICB0cmFja0luZm8uYXR0cmlidXRlcyA9IDA7CiAgICAgaWYgKHNjcm9sbGJhci0+b3JpZW50 YXRpb24oKSA9PSBIb3Jpem9udGFsU2Nyb2xsYmFyKQogICAgICAgICB0cmFja0luZm8uYXR0cmli dXRlcyB8PSBrVGhlbWVUcmFja0hvcml6b250YWw7Cg== 47380 2010-01-25 18:02:13 -0800 2010-01-25 21:22:12 -0800 Layout test rebaselines patch text/plain 0 pkasting 47386 2010-01-25 21:22:12 -0800 2010-01-25 21:24:30 -0800 Layout test rebaselines v2 patch text/plain 0 pkasting