Bug 69993 – CSS2 overflow: scrollbar not visible on SELECT elements when overflow: scroll is set

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Mike Lawther]

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

[Antaryami Pandia]

Created attachment 111232 [details]
Proposed Patch

Proposed Patch.

[Alexey Proskuryakov]

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?

[Antaryami Pandia]

(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.

[Antaryami Pandia]

Created attachment 111387 [details]
reference screenshot

reference screenshot

[Alexey Proskuryakov]

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?

[Antaryami Pandia]

(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.

[Antaryami Pandia]

Created attachment 111399 [details]
Patch

Upadated patch with Alexey's feedback.

[Alexey Proskuryakov]

> 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?

[Antaryami Pandia]

(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.

[Alexey Proskuryakov]

> 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.

[Antaryami Pandia]

(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.

[Antaryami Pandia]

Created attachment 111567 [details]
Updated patch

Added layout test for getcomputedstyle.

[Alexey Proskuryakov]

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).

[Alexey Proskuryakov]

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.

[Antaryami Pandia]

(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.

[Alexey Proskuryakov]

> 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.

[Antaryami Pandia]

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.

[Antaryami Pandia]

Hi Alexey, please provide some feedback.

[Alexey Proskuryakov]

This patch looks right to me. As mentioned before, I do not feel sufficiently qualified to review rendering code.

[Antaryami Pandia]

Hi Eric, Can you please review the patch?

[Eric Seidel (no email)]

Hyatt is really your best reviewer.  This is a tiny patch, perhaps we can convince him to take a quick look.

[Antaryami Pandia]

Hi Hyatt,please review the patch.

[Antaryami Pandia (apandia)]

Created attachment 116894 [details]
Patch for landing.

Patch with updated source.

[Antaryami Pandia]

Comment on attachment 114216 [details]
Proposed Patch

marking obsolute as added a new patch with updated source.

[Eric Seidel (no email)]

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.

[Antaryami Pandia (apandia)]

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.

[Antaryami Pandia (apandia)]

@Simon, please review the patch.

[WebKit Review Bot]

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

[Antaryami Pandia (apandia)]

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.

[Simon Fraser (smfr)]

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.

[Antaryami Pandia (apandia)]

Created attachment 126043 [details]
Patch with Simon's feedback.

Thanks for the quick reply.
Uploaded a patch with modified changelog.

[WebKit Review Bot]

Comment on attachment 126043 [details]
Patch with Simon's feedback.

Clearing flags on attachment: 126043

Committed r107072: <http://trac.webkit.org/changeset/107072>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.