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
Created attachment 111232 [details] Proposed Patch Proposed Patch.
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?
(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.
Created attachment 111387 [details] reference screenshot reference screenshot
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?
(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.
Created attachment 111399 [details] Patch Upadated patch with Alexey's feedback.
> 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?
(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.
> 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.
(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.
Created attachment 111567 [details] Updated patch Added layout test for getcomputedstyle.
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 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.
(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.
> 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.
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.
Hi Alexey, please provide some feedback.
This patch looks right to me. As mentioned before, I do not feel sufficiently qualified to review rendering code.
Hi Eric, Can you please review the patch?
Hyatt is really your best reviewer. This is a tiny patch, perhaps we can convince him to take a quick look.
Hi Hyatt,please review the patch.
Created attachment 116894 [details] Patch for landing. Patch with updated source.
Comment on attachment 114216 [details] Proposed Patch marking obsolute as added a new patch with updated source.
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.
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.
@Simon, please review the patch.
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
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 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.
Created attachment 126043 [details] Patch with Simon's feedback. Thanks for the quick reply. Uploaded a patch with modified changelog.
Comment on attachment 126043 [details] Patch with Simon's feedback. Clearing flags on attachment: 126043 Committed r107072: <http://trac.webkit.org/changeset/107072>
All reviewed patches have been landed. Closing bug.