Bug 102580 - [chromium] Short scrollbar has empty track rect even when buttons have no size.
Summary: [chromium] Short scrollbar has empty track rect even when buttons have no size.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Robert Flack
URL:
Keywords:
Depends on:
Blocks: 112384
  Show dependency treegraph
 
Reported: 2012-11-16 21:00 PST by Robert Flack
Modified: 2013-03-14 14:58 PDT (History)
5 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Robert Flack 2012-11-16 21:00:00 PST
Short scrollbar has empty track rect even when buttons have no size.
Comment 1 Robert Flack 2012-11-16 21:01:07 PST
Created attachment 174800 [details]
Patch
Comment 2 Robert Flack 2012-11-16 22:00:09 PST
Created attachment 174804 [details]
Patch
Comment 3 WebKit Review Bot 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
Comment 4 Build Bot 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
Comment 5 Dana Jansens 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>
Comment 6 Robert Flack 2012-11-29 11:28:41 PST
Created attachment 176768 [details]
Patch
Comment 7 Dana Jansens 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.
Comment 8 WebKit Review Bot 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
Comment 9 Robert Flack 2012-11-30 07:49:53 PST
Created attachment 176966 [details]
Patch
Comment 10 Robert Flack 2012-11-30 10:37:32 PST
Created attachment 176992 [details]
Patch
Comment 11 Dana Jansens 2012-11-30 10:38:55 PST
Comment on attachment 176992 [details]
Patch

LGTM.

james?
Comment 12 WebKit Review Bot 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
Comment 13 Robert Flack 2012-12-02 17:43:13 PST
Created attachment 177165 [details]
Patch
Comment 14 Robert Flack 2013-01-17 21:36:55 PST
Ping, James? Does this look alright?
Comment 15 James Robinson 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
> +<!-- On platforms with no scrollbar button images we should draw the native scrollbar track even when the scrollbar is very short. -->

I don't understand how this comment matches up with the -expected.png in this patch.  I see buttons.
Comment 16 Robert Flack 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
> > +<!-- On platforms with no scrollbar button images we should draw the native scrollbar track even when the scrollbar is very short. -->
> 
> 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?
Comment 17 James Robinson 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
> > > +<!-- On platforms with no scrollbar button images we should draw the native scrollbar track even when the scrollbar is very short. -->
> > 
> > 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.
Comment 18 Robert Flack 2013-02-12 08:27:40 PST
Created attachment 187873 [details]
Patch
Comment 19 Robert Flack 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
> > > > +<!-- On platforms with no scrollbar button images we should draw the native scrollbar track even when the scrollbar is very short. -->
> > > 
> > > 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.
Comment 20 Robert Flack 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
> > +<!-- On platforms with no scrollbar button images we should draw the native scrollbar track even when the scrollbar is very short. -->
> 
> I don't understand how this comment matches up with the -expected.png in this patch.  I see buttons.
Comment 21 Robert Flack 2013-02-12 08:58:20 PST
Created attachment 187879 [details]
Patch
Comment 22 WebKit Review Bot 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
Comment 23 Robert Flack 2013-02-21 12:16:01 PST
Created attachment 189573 [details]
Patch
Comment 24 Robert Flack 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.
Comment 25 James Robinson 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.
Comment 26 Robert Flack 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.
Comment 27 James Robinson 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?
Comment 28 Robert Flack 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.
Comment 29 Robert Flack 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!
Comment 30 James Robinson 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.
Comment 31 Robert Flack 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.
Comment 32 James Robinson 2013-03-14 13:32:00 PDT
Comment on attachment 189573 [details]
Patch

That'd be better than nothing
Comment 33 WebKit Review Bot 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>
Comment 34 WebKit Review Bot 2013-03-14 13:52:10 PDT
All reviewed patches have been landed.  Closing bug.