102580 – [chromium] Short scrollbar has empty track rect even when buttons have no size.

Robert Flack

Reported 2012-11-16 21:00:00 PST

Short scrollbar has empty track rect even when buttons have no size.

Robert Flack

Comment 1 2012-11-16 21:01:07 PST

Created attachment 174800 [details] Patch

Robert Flack

Comment 2 2012-11-16 22:00:09 PST

Created attachment 174804 [details] Patch

WebKit Review Bot

Comment 3 2012-11-16 22:41:12 PST

Comment on attachment 174804 [details] Patch Attachment 174804 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14873397 New failing tests: fast/replaced/width100percent-textarea.html scrollbars/short-scrollbar.html

Build Bot

Comment 4 2012-11-17 14:34:10 PST

Comment on attachment 174804 [details] Patch Attachment 174804 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/14889090 New failing tests: scrollbars/short-scrollbar.html

Dana Jansens

Comment 5 2012-11-19 08:59:10 PST

Comment on attachment 174804 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=174804&action=review > LayoutTests/scrollbars/short-scrollbar.html:1 > +<div style="width:200px; height:20px; overflow:auto"> Make this proper HTML. <!DOCTYPE html> <html> <body>

Robert Flack

Comment 6 2012-11-29 11:28:41 PST

Created attachment 176768 [details] Patch

Dana Jansens

Comment 7 2012-11-29 11:32:04 PST

Comment on attachment 176768 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176768&action=review > LayoutTests/platform/chromium/scrollbars/short-scrollbar.html:5 > +On platforms with no scrollbar button images we should draw the native scrollbar track even when the scrollbar is very short. Don't make visible text in the test if you can help it. Just make the text a comment and use sized divs or something to make it scrollable.. if you can do that.

WebKit Review Bot

Comment 8 2012-11-29 15:05:11 PST

Comment on attachment 176768 [details] Patch Attachment 176768 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15061018 New failing tests: platform/chromium/scrollbars/short-scrollbar.html fast/replaced/width100percent-textarea.html

Robert Flack

Comment 9 2012-11-30 07:49:53 PST

Created attachment 176966 [details] Patch

Robert Flack

Comment 10 2012-11-30 10:37:32 PST

Created attachment 176992 [details] Patch

Dana Jansens

Comment 11 2012-11-30 10:38:55 PST

Comment on attachment 176992 [details] Patch LGTM. james?

WebKit Review Bot

Comment 12 2012-12-02 03:09:03 PST

Comment on attachment 176992 [details] Patch Attachment 176992 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15086559 New failing tests: fast/replaced/width100percent-textarea.html

Robert Flack

Comment 13 2012-12-02 17:43:13 PST

Created attachment 177165 [details] Patch

Robert Flack

Comment 14 2013-01-17 21:36:55 PST

Ping, James? Does this look alright?

James Robinson

Comment 15 2013-01-31 17:02:31 PST

Comment on attachment 177165 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177165&action=review So this is a UI feature request? Is there a link somewhere to a bug or discussion of the actual vs expected behaviors? > LayoutTests/platform/chromium/scrollbars/short-scrollbar-expected.txt:1 > +layer at (0,0) size 800x600 you don't need a layer tree dump here, just the pixels. call testRunner.dumpAsText(true) in a script block inside the test to get this output > LayoutTests/platform/chromium/scrollbars/short-scrollbar.html:4 > + I don't understand how this comment matches up with the -expected.png in this patch. I see buttons.

Robert Flack

Comment 16 2013-02-04 12:45:43 PST

(In reply to comment #15) > (From update of attachment 177165 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177165&action=review > > So this is a UI feature request? Is there a link somewhere to a bug or discussion of the actual vs expected behaviors? This isn't a feature request. This is a bug which only reproduces on chromeos as far as I know because it has non-overlay scrollbars with no up/down/left/right buttons and uses composited scrollbars. > > > LayoutTests/platform/chromium/scrollbars/short-scrollbar-expected.txt:1 > > +layer at (0,0) size 800x600 > > you don't need a layer tree dump here, just the pixels. call testRunner.dumpAsText(true) in a script block inside the test to get this output > > > LayoutTests/platform/chromium/scrollbars/short-scrollbar.html:4 > > + > > I don't understand how this comment matches up with the -expected.png in this patch. I see buttons. As far as I can tell we don't have a platform tested in webkit which reproduces the issue. It is testable if compiled for chromeos. Do you think we should try to emulate this scenario on another platform or run tests with the chromeos configuration?

James Robinson

Comment 17 2013-02-04 13:31:00 PST

(In reply to comment #16) > > > LayoutTests/platform/chromium/scrollbars/short-scrollbar-expected.txt:1 > > > +layer at (0,0) size 800x600 > > > > you don't need a layer tree dump here, just the pixels. call testRunner.dumpAsText(true) in a script block inside the test to get this output > > > > > LayoutTests/platform/chromium/scrollbars/short-scrollbar.html:4 > > > + > > > > I don't understand how this comment matches up with the -expected.png in this patch. I see buttons. > > As far as I can tell we don't have a platform tested in webkit which reproduces the issue. It is testable if compiled for chromeos. Do you think we should try to emulate this scenario on another platform or run tests with the chromeos configuration? If the scenario in question is non-overlay scrollbars without directional buttons, would the WebKit mock scrollbars suffice? I think they meet those criteria.

Robert Flack

Comment 18 2013-02-12 08:27:40 PST

Created attachment 187873 [details] Patch

Robert Flack

Comment 19 2013-02-12 08:29:48 PST

(In reply to comment #17) > (In reply to comment #16) > > > > LayoutTests/platform/chromium/scrollbars/short-scrollbar-expected.txt:1 > > > > +layer at (0,0) size 800x600 > > > > > > you don't need a layer tree dump here, just the pixels. call testRunner.dumpAsText(true) in a script block inside the test to get this output > > > > > > > LayoutTests/platform/chromium/scrollbars/short-scrollbar.html:4 > > > > + > > > > > > I don't understand how this comment matches up with the -expected.png in this patch. I see buttons. > > > > As far as I can tell we don't have a platform tested in webkit which reproduces the issue. It is testable if compiled for chromeos. Do you think we should try to emulate this scenario on another platform or run tests with the chromeos configuration? > > If the scenario in question is non-overlay scrollbars without directional buttons, would the WebKit mock scrollbars suffice? I think they meet those criteria. Unfortunately we don't call into ScrollbarThemeChromium::trackRect when drawing mock scrollbars so everything draws correctly using ScrollbarThemeMock::trackRect which returns the entire frameRect.

Robert Flack

Comment 20 2013-02-12 08:30:22 PST

(In reply to comment #15) > (From update of attachment 177165 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=177165&action=review > > So this is a UI feature request? Is there a link somewhere to a bug or discussion of the actual vs expected behaviors? > > > LayoutTests/platform/chromium/scrollbars/short-scrollbar-expected.txt:1 > > +layer at (0,0) size 800x600 > > you don't need a layer tree dump here, just the pixels. call testRunner.dumpAsText(true) in a script block inside the test to get this output Added dumpAsText and removed expected layer tree. > > > LayoutTests/platform/chromium/scrollbars/short-scrollbar.html:4 > > + > > I don't understand how this comment matches up with the -expected.png in this patch. I see buttons.

Robert Flack

Comment 21 2013-02-12 08:58:20 PST

Created attachment 187879 [details] Patch

WebKit Review Bot

Comment 22 2013-02-12 14:04:15 PST

Comment on attachment 187879 [details] Patch Attachment 187879 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/16439437 New failing tests: fast/forms/basic-textareas-quirks.html fast/overflow/overflow-x-y.html fast/parser/open-comment-in-textarea.html fast/forms/basic-textareas.html

Robert Flack

Comment 23 2013-02-21 12:16:01 PST

Created attachment 189573 [details] Patch

Robert Flack

Comment 24 2013-02-21 12:25:15 PST

I'm a little unsure how to proceed on this one. Mock scrollbars don't work to reproduce the bug as they don't use the chromium scrollbar theme code and none of the chromium platforms we test in webkit reproduce the bug as far as I can tell. Can I get advice what the best course of action here is? The options I can see: We could add a downstream image test in chromium or chromiumos to verify this case (where we do have chromiumos specific tests). We could leave this as a test which can be verified manually. Remove the test and explain why there is no test in the Changelog. Any other suggestions? I suppose we could try to expose the bug though I'm not sure how feasible modifying the platform theme for a single test is.

James Robinson

Comment 25 2013-02-21 13:12:10 PST

(In reply to comment #24) > I'm a little unsure how to proceed on this one. Mock scrollbars don't work to reproduce the bug as they don't use the chromium scrollbar theme code and none of the chromium platforms we test in webkit reproduce the bug as far as I can tell. Do you know what piece of code is different in the chromeos theme vs the linux/mac/windows theme that leads to this bug? My impression was that the theming code was substantially the same between, for instance, linux and chromeos. > > Can I get advice what the best course of action here is? The options I can see: > We could add a downstream image test in chromium or chromiumos to verify this case (where we do have chromiumos specific tests). > We could leave this as a test which can be verified manually. > Remove the test and explain why there is no test in the Changelog. > > Any other suggestions? I suppose we could try to expose the bug though I'm not sure how feasible modifying the platform theme for a single test is.

Robert Flack

Comment 26 2013-02-21 14:28:48 PST

(In reply to comment #25) > (In reply to comment #24) > > I'm a little unsure how to proceed on this one. Mock scrollbars don't work to reproduce the bug as they don't use the chromium scrollbar theme code and none of the chromium platforms we test in webkit reproduce the bug as far as I can tell. > > Do you know what piece of code is different in the chromeos theme vs the linux/mac/windows theme that leads to this bug? My impression was that the theming code was substantially the same between, for instance, linux and chromeos. It is, except that the linux theme always has scroll buttons and so doesn't draw a track rect / thumb when the scrollbar is really short, just the buttons. I believe mac uses a different trackRect function from ScrollbarThemeMac. Chromeos seems to be the only platform with no scrollbar buttons using ScrollbarThemeChromium. > > > > > Can I get advice what the best course of action here is? The options I can see: > > We could add a downstream image test in chromium or chromiumos to verify this case (where we do have chromiumos specific tests). > > We could leave this as a test which can be verified manually. > > Remove the test and explain why there is no test in the Changelog. > > > > Any other suggestions? I suppose we could try to expose the bug though I'm not sure how feasible modifying the platform theme for a single test is.

James Robinson

Comment 27 2013-02-21 14:51:01 PST

(In reply to comment #26) > (In reply to comment #25) > > (In reply to comment #24) > > > I'm a little unsure how to proceed on this one. Mock scrollbars don't work to reproduce the bug as they don't use the chromium scrollbar theme code and none of the chromium platforms we test in webkit reproduce the bug as far as I can tell. > > > > Do you know what piece of code is different in the chromeos theme vs the linux/mac/windows theme that leads to this bug? My impression was that the theming code was substantially the same between, for instance, linux and chromeos. > > It is, except that the linux theme always has scroll buttons and so doesn't draw a track rect / thumb when the scrollbar is really short, just the buttons. I believe mac uses a different trackRect function from ScrollbarThemeMac. Chromeos seems to be the only platform with no scrollbar buttons using ScrollbarThemeChromium. Where's the code that makes that determination? We don't have a ScrollbarThemeChromiumChromiumOS or anything like that. Also, why does this patch require rebaselining pixel results for chromium-linux if it's unaffected by this bug?

Robert Flack

Comment 28 2013-02-21 19:33:11 PST

(In reply to comment #27) > (In reply to comment #26) > > (In reply to comment #25) > > > (In reply to comment #24) > > > > I'm a little unsure how to proceed on this one. Mock scrollbars don't work to reproduce the bug as they don't use the chromium scrollbar theme code and none of the chromium platforms we test in webkit reproduce the bug as far as I can tell. > > > > > > Do you know what piece of code is different in the chromeos theme vs the linux/mac/windows theme that leads to this bug? My impression was that the theming code was substantially the same between, for instance, linux and chromeos. > > > > It is, except that the linux theme always has scroll buttons and so doesn't draw a track rect / thumb when the scrollbar is really short, just the buttons. I believe mac uses a different trackRect function from ScrollbarThemeMac. Chromeos seems to be the only platform with no scrollbar buttons using ScrollbarThemeChromium. > > Where's the code that makes that determination? We don't have a ScrollbarThemeChromiumChromiumOS or anything like that. The code for the scrollbar button length comes from NativeThemeAura::NativeThemeAura() in the chromium repo at ui/native_theme/native_theme_aura.cc. I believe NativeThemeBase::GetPartSize returns the size of the scrollbar buttons to webkit. > > Also, why does this patch require rebaselining pixel results for chromium-linux if it's unaffected by this bug? chromium-linux isn't affected in that we don't expect to see the scrollbar track with the buttons there however I think there can be a 1px tall scrollbar track in some of these narrow cases drawn in between the buttons.

Robert Flack

Comment 29 2013-03-04 12:22:48 PST

(In reply to comment #28) > (In reply to comment #27) > > (In reply to comment #26) > > > (In reply to comment #25) > > > > (In reply to comment #24) > > > > > I'm a little unsure how to proceed on this one. Mock scrollbars don't work to reproduce the bug as they don't use the chromium scrollbar theme code and none of the chromium platforms we test in webkit reproduce the bug as far as I can tell. > > > > > > > > Do you know what piece of code is different in the chromeos theme vs the linux/mac/windows theme that leads to this bug? My impression was that the theming code was substantially the same between, for instance, linux and chromeos. > > > > > > It is, except that the linux theme always has scroll buttons and so doesn't draw a track rect / thumb when the scrollbar is really short, just the buttons. I believe mac uses a different trackRect function from ScrollbarThemeMac. Chromeos seems to be the only platform with no scrollbar buttons using ScrollbarThemeChromium. > > > > Where's the code that makes that determination? We don't have a ScrollbarThemeChromiumChromiumOS or anything like that. > > The code for the scrollbar button length comes from NativeThemeAura::NativeThemeAura() in the chromium repo at ui/native_theme/native_theme_aura.cc. I believe NativeThemeBase::GetPartSize returns the size of the scrollbar buttons to webkit. > > > > > Also, why does this patch require rebaselining pixel results for chromium-linux if it's unaffected by this bug? > > chromium-linux isn't affected in that we don't expect to see the scrollbar track with the buttons there however I think there can be a 1px tall scrollbar track in some of these narrow cases drawn in between the buttons. Yes, I've verified that occasionally we get a 1px tall track drawing behind the buttons at some heights which results in a slight difference in the pixel shading at the corners of the buttons. Is this landable? Should I remove the 1 pixel of overlap the buttons have so the existing pixel tests don't change? Please advise, thanks!

James Robinson

Comment 30 2013-03-14 13:25:48 PDT

Comment on attachment 189573 [details] Patch Sorry for the delays. This seems all right, although I'm not super confident it'll keep working without tests to hit this case.

Robert Flack

Comment 31 2013-03-14 13:30:38 PDT

Comment on attachment 189573 [details] Patch Thanks. I could certainly test this downstream in chromium if it would be helpful in preventing this from regressing.

James Robinson

Comment 32 2013-03-14 13:32:00 PDT

Comment on attachment 189573 [details] Patch That'd be better than nothing

WebKit Review Bot

Comment 33 2013-03-14 13:52:04 PDT

Comment on attachment 189573 [details] Patch Clearing flags on attachment: 189573 Committed r145844: <http://trac.webkit.org/changeset/145844>

WebKit Review Bot

Comment 34 2013-03-14 13:52:10 PDT

All reviewed patches have been landed. Closing bug.

Attachments
Patch (2.06 KB, patch) 2012-11-16 21:01 PST, Robert Flack	no flags	Details Formatted Diff Diff
Patch (3.47 KB, patch) 2012-11-16 22:00 PST, Robert Flack	no flags	Details Formatted Diff Diff
Patch (8.90 KB, patch) 2012-11-29 11:28 PST, Robert Flack	no flags	Details Formatted Diff Diff
Patch (5.49 KB, patch) 2012-11-30 07:49 PST, Robert Flack	no flags	Details Formatted Diff Diff
Patch (5.49 KB, patch) 2012-11-30 10:37 PST, Robert Flack	no flags	Details Formatted Diff Diff
Patch (16.83 KB, patch) 2012-12-02 17:43 PST, Robert Flack	no flags	Details Formatted Diff Diff
Patch (5.25 KB, patch) 2013-02-12 08:27 PST, Robert Flack	no flags	Details Formatted Diff Diff
Patch (17.02 KB, patch) 2013-02-12 08:58 PST, Robert Flack	no flags	Details Formatted Diff Diff
Patch (316.51 KB, patch) 2013-02-21 12:16 PST, Robert Flack	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.