Bug 69993 - CSS2 overflow: scrollbar not visible on SELECT elements when overflow: scroll is set
Summary: CSS2 overflow: scrollbar not visible on SELECT elements when overflow: scroll...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-12 20:36 PDT by Mike Lawther
Modified: 2012-02-08 04:34 PST (History)
9 users (show)

See Also:


Attachments
Proposed Patch (19.74 KB, patch)
2011-10-17 02:57 PDT, Antaryami Pandia
no flags Details | Formatted Diff | Diff
reference screenshot (16.94 KB, image/png)
2011-10-17 23:11 PDT, Antaryami Pandia
no flags Details
Patch (31.88 KB, patch)
2011-10-18 00:21 PDT, Antaryami Pandia
no flags Details | Formatted Diff | Diff
Updated patch (35.00 KB, patch)
2011-10-18 23:34 PDT, Antaryami Pandia
ap: review-
Details | Formatted Diff | Diff
Mac screenshot (10.53 KB, image/png)
2011-10-19 12:33 PDT, Alexey Proskuryakov
no flags Details
Proposed Patch (35.92 KB, patch)
2011-11-09 00:11 PST, Antaryami Pandia
no flags Details | Formatted Diff | Diff
Patch for landing. (35.89 KB, patch)
2011-11-28 22:08 PST, Antaryami Pandia (apandia)
simon.fraser: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch. (9.42 KB, patch)
2012-02-08 02:51 PST, Antaryami Pandia (apandia)
simon.fraser: review-
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
Patch with Simon's feedback. (9.65 KB, patch)
2012-02-08 03:20 PST, Antaryami Pandia (apandia)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike Lawther 2011-10-12 20:36:36 PDT
As reported in http://crbug.com/38090:

Test code:

<form action="">
    <select id="wut" name="wut" size="8" style="overflow: scroll;">
      <option value="1">One</option>
      <option value="2">Two</option>
      <option value="3">Three</option>
      <option value="4">Four</option>
      <option value="5">Five</option>
      <option value="6">Six</option>
      <option value="7">Seven</option>
      <option value="8">Eight</option>
      <option value="9">Nine</option>
      <option value="10">Ten</option>
    </select>
  </form>

I expect a scrollbar to be displayed, as per http://www.w3.org/TR/CSS2/visufx.html#propdef-overflow.

What happens is that the content is scrollable, the scrollbar background is there, but no scrollbar is visible.

Browsers tested (all on MacOS 10.6):

WebKit Nightly r97312: no scrollbars, BAD
Chrome 16.0.906.0 canary: no scrollbars, BAD
Firefox 7.0.1: single vertical scrollbar, GOOD
Opera 11.51: single vertical scrollbar, GOOD
Comment 1 Antaryami Pandia 2011-10-17 02:57:34 PDT
Created attachment 111232 [details]
Proposed Patch

Proposed Patch.
Comment 2 Alexey Proskuryakov 2011-10-17 09:10:22 PDT
Comment on attachment 111232 [details]
Proposed Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111232&action=review

Does this fix work if overflow: scroll is inherited? E.g.

<div style="overflow:scroll">
 <select id="wut" name="wut" size="8" style="overflow: inherit;">
     <option value="1">One</option>
...

Will getComputedStyle still return overflow:scroll?

I don't know enough about this code to tell it from review, but I think that it would be helpful to add tests for both cases to the patch.

> LayoutTests/platform/mac/test_expectations.txt:34
> +// New test in https://bugs.webkit.org/show_bug.cgi?id=69993
> +BUGWK69993 : fast/forms/select-overflow-scroll-property.html = IMAGE+TEXT

Do all platforms besides Gtk fail the test, or do they actually pass, and need separate results? If it's the former, does this patch make behavior worse?
Comment 3 Antaryami Pandia 2011-10-17 23:09:15 PDT
(In reply to comment #2)
> Does this fix work if overflow: scroll is inherited? E.g.
> 
> <div style="overflow:scroll">
>  <select id="wut" name="wut" size="8" style="overflow: inherit;">
>      <option value="1">One</option>
 
Yes, it fixes the inherited case also.

> 
> Will getComputedStyle still return overflow:scroll?

Since we are changing the style itself, it should return overflow:hidden.

> I don't know enough about this code to tell it from review, but I think that it would be helpful to add tests for both cases to the patch.

Ok. I will add both the test cases. 

> > LayoutTests/platform/mac/test_expectations.txt:34
> > +// New test in https://bugs.webkit.org/show_bug.cgi?id=69993
> > +BUGWK69993 : fast/forms/select-overflow-scroll-property.html = IMAGE+TEXT
> 
> Do all platforms besides Gtk fail the test, or do they actually pass, and need separate results? If it's the former, does this patch make behavior worse?

Yes. The issue can be seen on other platform also. I verified the issue on chromium and safari(on windows). I didn't have a mac set-up but I think w.r.t. this issue, the behavior will be same across all platforms.

Attached is a screenshot of GtkWebkit for reference.

As can be seen the attached screenshot patch changes the visual display, hence I have added the pixel tests.
Comment 4 Antaryami Pandia 2011-10-17 23:11:41 PDT
Created attachment 111387 [details]
reference screenshot

reference screenshot
Comment 5 Alexey Proskuryakov 2011-10-17 23:21:39 PDT
I don't think that skipping tests that just need platform results in the preferred approach. We may not notice that for a long time if the tests are skipped.

> Since we are changing the style itself, it should return overflow:hidden.

That seems wrong. Does any other browser behave this way?

In fact, did you find out why scrollbars appear in this case? Are <select> bounds calculated incorrectly perhaps?
Comment 6 Antaryami Pandia 2011-10-17 23:43:23 PDT
(In reply to comment #5)
> I don't think that skipping tests that just need platform results in the preferred approach. We may not notice that for a long time if the tests are skipped.

Ok. I will remove the test from skipped files.

> > Since we are changing the style itself, it should return overflow:hidden.
> 
> That seems wrong. Does any other browser behave this way?

Yes, you are right.I check on firefox and it has the computed style for overflow as visible.I will make the changes accordingly.

> In fact, did you find out why scrollbars appear in this case? Are <select> bounds calculated incorrectly perhaps?

The scrollbar appears because the listbox renderer handles the scenerio.Code can be found at RenderListBox::paintObject and RenderListBox::paintScrollbar.
After the change the behavior is as par with firefox and IE.Both displays only vertical scrollbar.
Comment 7 Antaryami Pandia 2011-10-18 00:21:55 PDT
Created attachment 111399 [details]
Patch

Upadated patch with Alexey's feedback.
Comment 8 Alexey Proskuryakov 2011-10-18 08:58:01 PDT
> The scrollbar appears because the listbox renderer handles the scenerio.

I'm asking about overflow scrollbars. Why are those painted?

> I think that it would be helpful to add tests for both cases to the patch.

What is the reason not to do that?
Comment 9 Antaryami Pandia 2011-10-18 22:06:08 PDT
(In reply to comment #8)
> > The scrollbar appears because the listbox renderer handles the scenerio.
> 
> I'm asking about overflow scrollbars. Why are those painted?

For overflow:scroll, currently webkit always places the horizontal and vertical scrollbar.Related code can be found at RenderBlock::layoutBlock.

> > I think that it would be helpful to add tests for both cases to the patch.
> 
> What is the reason not to do that?

I have added test cases for overflow:scroll (select-overflow-scroll.html) and inherited case (select-overflow-scroll-inherited.html) and theirs pixel tests.
Were you talking about the overflow:scroll and inherited cases.Please let me know if I have misunderstood.
Comment 10 Alexey Proskuryakov 2011-10-18 22:22:19 PDT
> For overflow:scroll, currently webkit always places the horizontal and vertical scrollbar.Related code can be found at RenderBlock::layoutBlock.

Yes, but why make an exception just for <select>? What you see is that overflow:scroll scrollbars are positioned so that they obscure <select>'s own scrollbar, which likely happens because its bounds are computed incorrectly.

If you think that it's incorrect to always draw scrollbars for overflow:scroll, you could attack that issue in general.

> I have added test cases for overflow:scroll (select-overflow-scroll.html) and inherited case (select-overflow-scroll-inherited.html) and theirs pixel tests.
> Were you talking about the overflow:scroll and inherited cases.Please let me know if I have misunderstood.

I didn't notice that you added another test, but there is still no test for getComputedStyle(). It should be tested, and it should work the same way it does in other browsers.
Comment 11 Antaryami Pandia 2011-10-18 22:44:26 PDT
(In reply to comment #10)
> > For overflow:scroll, currently webkit always places the horizontal and vertical scrollbar.Related code can be found at RenderBlock::layoutBlock.
> 
> Yes, but why make an exception just for <select>? What you see is that overflow:scroll scrollbars are positioned so that they obscure <select>'s own scrollbar, which likely happens because its bounds are computed incorrectly.
> 
> If you think that it's incorrect to always draw scrollbars for overflow:scroll, you could attack that issue in general.

Firefox and IE shows only vertical scrollbar for list box (<select> with size) with overflow:scroll.I think they also overlook the overflow:scroll property for this case. With this change webkit matches the other browser's behavior.
Also there is piece of of code present in CSSStyleSelector::adjustRenderStyle which always set menulist elements overflow as visible.

    // Menulists should have visible overflow
    if (style->appearance() == MenulistPart) {
        style->setOverflowX(OVISIBLE);
        style->setOverflowY(OVISIBLE);
    }

> > I have added test cases for overflow:scroll (select-overflow-scroll.html) and inherited case (select-overflow-scroll-inherited.html) and theirs pixel tests.
> > Were you talking about the overflow:scroll and inherited cases.Please let me know if I have misunderstood.
> 
> I didn't notice that you added another test, but there is still no test for getComputedStyle(). It should be tested, and it should work the same way it does in other browsers.

OK.I will add a test case for getComputedStyle. I checked the computed style (using firebug-inspect element) and the elements overflow can be seen as visisble, which matches with this change.
Comment 12 Antaryami Pandia 2011-10-18 23:34:29 PDT
Created attachment 111567 [details]
Updated patch

Added layout test for getcomputedstyle.
Comment 13 Alexey Proskuryakov 2011-10-19 12:33:51 PDT
Created attachment 111658 [details]
Mac screenshot

I think that some of the confusion comes from how Gtk render theme renders these scrollbars. These are completely separate from regular <select> scrollbar - on Mac, they even have a different size.

Also, this issue doesn't happen at all with OS X Lion style overlay scrollbars (the screenshot I'm attaching is from Lion, but with legacy scrollbars enabled).
Comment 14 Alexey Proskuryakov 2011-10-19 12:38:45 PDT
Comment on attachment 111567 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=111567&action=review

> LayoutTests/fast/css/getComputedStyle/computed-style-select-overflow-expected.txt:6
> +PASS computedOverflowStyle('scroll', 'overflow-x') is 'visible'
> +PASS computedOverflowStyle('scroll', 'overflow-y') is 'visible'
> +PASS computedInheritedOverflowStyle('inherit', 'overflow-x') is 'visible'
> +PASS computedInheritedOverflowStyle('inherit', 'overflow-y') is 'visible'

Firefox 7.0.1 says FAIL on these tests for me, matching current WebKit.

FAIL computedOverflowStyle('scroll', 'overflow-x') should be visible. Was scroll.
FAIL computedOverflowStyle('scroll', 'overflow-y') should be visible. Was scroll.
FAIL computedInheritedOverflowStyle('inherit', 'overflow-x') should be visible. Was scroll.
FAIL computedInheritedOverflowStyle('inherit', 'overflow-y') should be visible. Was scroll.

Consequently, I do not think that this is the right way to approach this problem.
Comment 15 Antaryami Pandia 2011-10-19 21:21:34 PDT
(In reply to comment #14)
> (From update of attachment 111567 [details])
> Firefox 7.0.1 says FAIL on these tests for me, matching current WebKit.
> 
> FAIL computedOverflowStyle('scroll', 'overflow-x') should be visible. Was scroll.
> FAIL computedOverflowStyle('scroll', 'overflow-y') should be visible. Was scroll.
> FAIL computedInheritedOverflowStyle('inherit', 'overflow-x') should be visible. Was scroll.
> FAIL computedInheritedOverflowStyle('inherit', 'overflow-y') should be visible. Was scroll.
> 
> Consequently, I do not think that this is the right way to approach this problem.

Thanks for the feedback.I will do some more investigation.
Also it will be helpful if you can provide some info how to test these layout tests on firefox.
Comment 16 Alexey Proskuryakov 2011-10-19 21:59:45 PDT
> Also it will be helpful if you can provide some info how to test these layout tests on firefox.

I just dragged the file from Finder into an empty Firefox window.
Comment 17 Antaryami Pandia 2011-11-09 00:11:00 PST
Created attachment 114216 [details]
Proposed Patch

As per my understanding when an element has overflow:scroll, both horizontal and vertical scrollbar are rendered.And this is true for non-replaced elements.But listbox is a replaced element, and for a replaced element, the rendering of its internals is not defined by CSS.

Since listbox renderer handles the scrolling, can we make an exception for listbox and don't set it's scrollbar.
This will also helps in avoiding drawing of scrollbar twice.
Comment 18 Antaryami Pandia 2011-11-17 05:15:08 PST
Hi Alexey, please provide some feedback.
Comment 19 Alexey Proskuryakov 2011-11-17 08:55:50 PST
This patch looks right to me. As mentioned before, I do not feel sufficiently qualified to review rendering code.
Comment 20 Antaryami Pandia 2011-11-20 22:31:35 PST
Hi Eric, Can you please review the patch?
Comment 21 Eric Seidel (no email) 2011-11-20 23:06:12 PST
Hyatt is really your best reviewer.  This is a tiny patch, perhaps we can convince him to take a quick look.
Comment 22 Antaryami Pandia 2011-11-22 02:12:47 PST
Hi Hyatt,please review the patch.
Comment 23 Antaryami Pandia (apandia) 2011-11-28 22:08:18 PST
Created attachment 116894 [details]
Patch for landing.

Patch with updated source.
Comment 24 Antaryami Pandia 2011-11-28 22:14:44 PST
Comment on attachment 114216 [details]
Proposed Patch

marking obsolute as added a new patch with updated source.
Comment 25 Eric Seidel (no email) 2011-12-19 10:49:10 PST
Comment on attachment 114216 [details]
Proposed Patch

Cleared Simon Fraser's review+ from obsolete attachment 114216 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 26 Antaryami Pandia (apandia) 2011-12-19 22:20:04 PST
Hi Simon, can you please check the patch once. The patch is the same as the earlier one but with updated source to avoid merge conflicts.
Comment 27 Antaryami Pandia (apandia) 2012-02-08 01:30:45 PST
@Simon, please review the patch.
Comment 28 WebKit Review Bot 2012-02-08 01:57:10 PST
Comment on attachment 116894 [details]
Patch for landing.

Rejecting attachment 116894 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
erflow.html
patching file LayoutTests/fast/forms/select-overflow-scroll-inherited.html
patching file LayoutTests/fast/forms/select-overflow-scroll.html
patching file LayoutTests/platform/gtk/fast/forms/select-overflow-scroll-expected.txt
patching file LayoutTests/platform/gtk/fast/forms/select-overflow-scroll-inherited-expected.txt

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Simon Fras..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue/

Full output: http://queues.webkit.org/results/11462333
Comment 29 Antaryami Pandia (apandia) 2012-02-08 02:51:53 PST
Created attachment 126039 [details]
Patch.

Uploading the patch (with latest source) again as the svn-merge has been failed.
Hoping I will be get lucky third time and the patch will land on the trunk.
Comment 30 Simon Fraser (smfr) 2012-02-08 03:04:43 PST
Comment on attachment 126039 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=126039&action=review

> Source/WebCore/ChangeLog:7
> +        https://bugs.webkit.org/show_bug.cgi?id=69993
> +
> +        Reviewed by NOBODY (OOPS!).
> +

Actually, this needs some description of what the problem was and how you fixed it.
Comment 31 Antaryami Pandia (apandia) 2012-02-08 03:20:09 PST
Created attachment 126043 [details]
Patch with Simon's feedback.

Thanks for the quick reply.
Uploaded a patch with modified changelog.
Comment 32 WebKit Review Bot 2012-02-08 04:34:40 PST
Comment on attachment 126043 [details]
Patch with Simon's feedback.

Clearing flags on attachment: 126043

Committed r107072: <http://trac.webkit.org/changeset/107072>
Comment 33 WebKit Review Bot 2012-02-08 04:34:46 PST
All reviewed patches have been landed.  Closing bug.