Applies to latest Safari, Chrome, and Webkit Nightly on OS X with automatic scrollbars enabled (default setting on OS X). In those browsers, with padding applied to the select[multiple], the fancy OS X scrollbar is completely hidden. See http://jsbin.com/vojij/3/edit for details. Per the demo link, only by resetting the padding-right to zero can you see the scrollbar. Re-enabling the always visible scrollbars presents no problem though, and Firefox exhibits no problem in this matter.
Relevant bug for Chromium: https://code.google.com/p/chromium/issues/detail?id=342208.
Original Bootstrap issue report: https://github.com/twbs/bootstrap/issues/12536
Still happening with Safari 8.0 (10600.1.25) on OS X Yosemite.
Created attachment 243054 [details] Screenshot of the bug Somewhat more obvious example: http://jsbin.com/fekipo/2/edit?html,output Also added screenshot. Note the difference between the 2 scrollbars that the green arrows are pointing to.
Have also filed this as: <rdar://problem/19208483>
Does this bug impact a common web site or app?
This bug affects any website using Bootstrap 3 and <select multiple>. Try out the <select multiple> example on http://getbootstrap.com/css/#forms-controls in Safari; the scrollbar is completely invisible. This bug is also listed on http://getbootstrap.com/browser-bugs/ According to http://blog.meanpath.com/4-7m-sites-using-bootstrap-vs-334k-on-foundation/ , as of 2014-02-28, 4.7 million websites (= 1.79% of all websites) used some version of Bootstrap. Bootstrap is also the most-starred project on GitHub (75,336 stars; see https://github.com/search?q=stars%3a%3E1&s=stars&type=Repositories ).
Still reproduces.
(In reply to comment #8) > Still reproduces. Reproducible on Safari as of r197784 (Mar/8/2016). Taking a look ..
(In reply to comment #9) > (In reply to comment #8) > > Still reproduces. > > Reproducible on Safari as of r197784 (Mar/8/2016). Taking a look .. Just started to work on this bug today. Quick update: What I could realize right way is that the scrollbar keeps being positioned at the same spot (vertical line), no matter whether a 'padding-right' is set or not. I believe this is right. Problem is that when 'padding-right' is set to the <select>, a specific "layer" from right towards the left is bigger, covering partially or totally the overlay scrollbar.
I figured that the the following change locally fixes the problem: diff --git a/Source/WebCore/rendering/RenderListBox.cpp b/Source/WebCore/rendering/RenderListBox.cpp index 73c1f34..344ebd4 100644 --- a/Source/WebCore/rendering/RenderListBox.cpp +++ b/Source/WebCore/rendering/RenderListBox.cpp @@ -730,7 +730,8 @@ bool RenderListBox::nodeAtPoint(const HitTestRequest& request, HitTestResult& re LayoutRect RenderListBox::controlClipRect(const LayoutPoint& additionalOffset) const { - LayoutRect clipRect = contentBoxRect(); + // Clip to the padding box to at least give content the extra padding space, and not clip out overlay scrollbars if any.. + LayoutRect clipRect(paddingBoxRect()); if (verticalScrollbarIsOnLeft() && (!layer() || !layer()->verticalScrollbarIsOnLeft())) clipRect.move(m_vBar->occupiedWidth(), 0); clipRect.moveBy(additionalOffset); .. however, I am still checking the implications of such change. Basically, issue has started in https://trac.webkit.org/changeset/19037/trunk/WebCore/rendering/RenderListBox.cpp (9 years ago!), when r19037 excluded "padding" from the RenderListBox::controlClipRect. Adding Sam (author).
Created attachment 275687 [details] Patch v1.
Comment on attachment 275687 [details] Patch v1. Revoking review for now. Plan is to extend our test infra to allow us to more easily test such changes.
Comment on attachment 275687 [details] Patch v1. View in context: https://bugs.webkit.org/attachment.cgi?id=275687&action=review > Source/WebCore/ChangeLog:28 > + No new tests as there is no machinery in place to test overlay scrollbars. Does window.internals.setUsesOverlayScrollbars() not work? > Source/WebCore/rendering/RenderListBox.cpp:736 > + LayoutRect clipRect = hasOverlayScrollbars() ? paddingBoxRect() : contentBoxRect(); The second patch you link to explicitly states "items should not spill into the padding box." I don't know the original reason why (because I'm not the author) but I would imagine it is there to preserve consistency between WebKit list boxes and existing platform controls. This "spilling" is testable by making an .html file with the following contents: <select multiple="multiple" style="font: 50px Zapfino; padding-right: 50px; padding-left: 50px;"> <option>faQ'</option> <option>a</option> <option>a</option> <option>a</option> <option>a</option> </select> <div style="font: 50px Zapfino; position: absolute; left: 400px; top: 100px; background: red;">f</div> Notice how the "f" in the list box is clipped so that it does not intrude into the padding, and therefore does not look like the "f" outside the list box. Now, either one of two things is true: - This behavior is unnecessary. In this situation, we should simply always use the paddingBoxRect(), and never use the contentBoxRect. - We really do need to preserve this behavior. In this case, we need to come up with some other way to both clip the contents of the <option>s but also allow for the scrollbar to be painted. I'm not sure which of those options we should do. Hopefully Sam can shed more light on the issue. But, neither of those approaches are the one take in this patch, so r-.
(In reply to comment #14) > Comment on attachment 275687 [details] > Patch v1. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=275687&action=review > > > Source/WebCore/ChangeLog:28 > > + No new tests as there is no machinery in place to test overlay scrollbars. > > Does window.internals.setUsesOverlayScrollbars() not work? Spoke to Simon briefly about it on IRC. We agreed on getting on introducing a capability to Internals thats allows overlay scrollbars to be forcibly hidden, narrow, wide. This way we could get a ref mismatch test to verify overlay scrollbar is painted correctly. > > Source/WebCore/rendering/RenderListBox.cpp:736 > > + LayoutRect clipRect = hasOverlayScrollbars() ? paddingBoxRect() : contentBoxRect(); > > The second patch you link to explicitly states "items should not spill into > the padding box." I don't know the original reason why (because I'm not the > author) but I would imagine it is there to preserve consistency between > WebKit list boxes and existing platform controls. Key point here seems to be: overlay scrollbars are allow to get paint on the padding area. It should not happen in case of always-on scrollbar. This is whats patch tries to preserve. > This "spilling" is testable by making an .html file with the following > contents: > > <select multiple="multiple" style="font: 50px Zapfino; padding-right: 50px; > padding-left: 50px;"> > <option>faQ'</option> > <option>a</option> > <option>a</option> > <option>a</option> > <option>a</option> > </select> > <div style="font: 50px Zapfino; position: absolute; left: 400px; top: 100px; > background: red;">f</div> > > Notice how the "f" in the list box is clipped so that it does not intrude > into the padding, and therefore does not look like the "f" outside the list > box. > > Now, either one of two things is true: > - This behavior is unnecessary. In this situation, we should simply always > use the paddingBoxRect(), and never use the contentBoxRect. > - We really do need to preserve this behavior. In this case, we need to come > up with some other way to both clip the contents of the <option>s but also > allow for the scrollbar to be painted. > Will check. > I'm not sure which of those options we should do. Hopefully Sam can shed > more light on the issue. Looking at the bug, mitz seems the author and Sam helped to merge it in.
(In reply to comment #15) > (In reply to comment #14) > > Comment on attachment 275687 [details] > > Patch v1. > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=275687&action=review > > > > > Source/WebCore/ChangeLog:28 > > > + No new tests as there is no machinery in place to test overlay scrollbars. > > > > Does window.internals.setUsesOverlayScrollbars() not work? > > Spoke to Simon briefly about it on IRC. > > We agreed on getting on introducing a capability to Internals thats allows > overlay scrollbars to be forcibly hidden, narrow, wide. > > This way we could get a ref mismatch test to verify overlay scrollbar is > painted correctly. > > > > Source/WebCore/rendering/RenderListBox.cpp:736 > > > + LayoutRect clipRect = hasOverlayScrollbars() ? paddingBoxRect() : contentBoxRect(); > > > > The second patch you link to explicitly states "items should not spill into > > the padding box." I don't know the original reason why (because I'm not the > > author) but I would imagine it is there to preserve consistency between > > WebKit list boxes and existing platform controls. > > Key point here seems to be: overlay scrollbars are allow to get paint on the > padding area. I disagree. There are two issues here: 1. Which type of scrollbars are enabled 2. Whether or not list-box contents are able to intrude into the padding These two issues are not related. Specifically, we should not be using item #1 as a signal to change the behavior of #2. > > It should not happen in case of always-on scrollbar. > > This is whats patch tries to preserve. > > > This "spilling" is testable by making an .html file with the following > > contents: > > > > <select multiple="multiple" style="font: 50px Zapfino; padding-right: 50px; > > padding-left: 50px;"> > > <option>faQ'</option> > > <option>a</option> > > <option>a</option> > > <option>a</option> > > <option>a</option> > > </select> > > <div style="font: 50px Zapfino; position: absolute; left: 400px; top: 100px; > > background: red;">f</div> > > > > Notice how the "f" in the list box is clipped so that it does not intrude > > into the padding, and therefore does not look like the "f" outside the list > > box. > > > > Now, either one of two things is true: > > - This behavior is unnecessary. In this situation, we should simply always > > use the paddingBoxRect(), and never use the contentBoxRect. > > - We really do need to preserve this behavior. In this case, we need to come > > up with some other way to both clip the contents of the <option>s but also > > allow for the scrollbar to be painted. > > > > Will check. > > > > I'm not sure which of those options we should do. Hopefully Sam can shed > > more light on the issue. > > Looking at the bug, mitz seems the author and Sam helped to merge it in.
(In reply to comment #16) > (In reply to comment #15) > > (In reply to comment #14) > > > Comment on attachment 275687 [details] > > > Patch v1. > > > > > > View in context: > > > https://bugs.webkit.org/attachment.cgi?id=275687&action=review > > > > > > > Source/WebCore/ChangeLog:28 > > > > + No new tests as there is no machinery in place to test overlay scrollbars. > > > > > > Does window.internals.setUsesOverlayScrollbars() not work? > > > > Spoke to Simon briefly about it on IRC. > > > > We agreed on getting on introducing a capability to Internals thats allows > > overlay scrollbars to be forcibly hidden, narrow, wide. > > > > This way we could get a ref mismatch test to verify overlay scrollbar is > > painted correctly. > > > > > > Source/WebCore/rendering/RenderListBox.cpp:736 > > > > + LayoutRect clipRect = hasOverlayScrollbars() ? paddingBoxRect() : contentBoxRect(); > > > > > > The second patch you link to explicitly states "items should not spill into > > > the padding box." I don't know the original reason why (because I'm not the > > > author) but I would imagine it is there to preserve consistency between > > > WebKit list boxes and existing platform controls. > > > > Key point here seems to be: overlay scrollbars are allow to get paint on the > > padding area. > > I disagree. What I mean is: in Firerox and Chrome, overlay scrollbar on list-box'es get painted on the padding area. > There are two issues here: > 1. Which type of scrollbars are enabled > 2. Whether or not list-box contents are able to intrude into the padding > Right. List-box content intruding into padding is a separate issue.
(In reply to comment #17) > > > (In reply to comment #16) > > (In reply to comment #15) > > > (In reply to comment #14) > > > > Comment on attachment 275687 [details] > > > > Patch v1. > > > > > > > > View in context: > > > > https://bugs.webkit.org/attachment.cgi?id=275687&action=review > > > > > > > > > Source/WebCore/ChangeLog:28 > > > > > + No new tests as there is no machinery in place to test overlay scrollbars. > > > > > > > > Does window.internals.setUsesOverlayScrollbars() not work? > > > > > > Spoke to Simon briefly about it on IRC. > > > > > > We agreed on getting on introducing a capability to Internals thats allows > > > overlay scrollbars to be forcibly hidden, narrow, wide. > > > > > > This way we could get a ref mismatch test to verify overlay scrollbar is > > > painted correctly. > > > > > > > > Source/WebCore/rendering/RenderListBox.cpp:736 > > > > > + LayoutRect clipRect = hasOverlayScrollbars() ? paddingBoxRect() : contentBoxRect(); > > > > > > > > The second patch you link to explicitly states "items should not spill into > > > > the padding box." I don't know the original reason why (because I'm not the > > > > author) but I would imagine it is there to preserve consistency between > > > > WebKit list boxes and existing platform controls. > > > > > > Key point here seems to be: overlay scrollbars are allow to get paint on the > > > padding area. > > > > I disagree. > > What I mean is: in Firerox and Chrome, overlay scrollbar on list-box'es get > painted on the padding area. > > > There are two issues here: > > 1. Which type of scrollbars are enabled > > 2. Whether or not list-box contents are able to intrude into the padding > > > > Right. List-box content intruding into padding is a separate issue. I've created a test at [1] which tests this clipping behavior. If we decide that the clipping behavior is not desirable, we should delete these tests. [1] https://bugs.webkit.org/show_bug.cgi?id=156265
Created attachment 275783 [details] padding box clip in Firefox, Chrome and Safari nightly > > Right. List-box content intruding into padding is a separate issue. > > I've created a test at [1] which tests this clipping behavior. If we decide > that the clipping behavior is not desirable, we should delete these tests. > > [1] https://bugs.webkit.org/show_bug.cgi?id=156265 Hi Myles. Thank you for the test in bug 156265. I have attached a PNG file that compares the result of the following HTML content rendered in Firefox, Chrome and Safari: <p> <select multiple style="height: 105px; padding-bottom: 25px;"> <option>one</option> <option>two</option> <option>three</option> <option>four</option> <option>five</option> <option>six</option> <option>seven</option> <option>eight</option> <option>nine</option> <option>ten</option> <option>eleven</option> <option>twelve</option> <option>thirteen</option> <option>fourteen</option> <option>fifteen</option> <option>sixteen</option> <option>seventeen</option> </select> </p> Note that it has a padding-bottom value set, so it directly involves the "clipping to the padding-box rect" VS "clipping to the content-box rect" behavior of list-box'es that we are discussing. Firefox and Chrome agree in their behavior: listbox'es content in both browsers intrudes the padding area, producing similar output. On the other hand, Safari Nightly clips content out to the content box (excluding padding). I will continuing to check the reason for this webkit-specific behavior through digging into the history of https://trac.webkit.org/changeset/19037.
> > > > Source/WebCore/rendering/RenderListBox.cpp:736 > > > > + LayoutRect clipRect = hasOverlayScrollbars() ? paddingBoxRect() : contentBoxRect(); > > > > > > The second patch you link to explicitly states "items should not spill into > > > the padding box." I don't know the original reason why (because I'm not the > > > author) but I would imagine it is there to preserve consistency between > > > WebKit list boxes and existing platform controls. > > > > Key point here seems to be: overlay scrollbars are allow to get paint on the > > padding area. > > I disagree. > There are two issues here: I agree with you here that we have two issue. Maybe the comment I added in the patch has led to confusion. Let me try to clarify: > 1. Which type of scrollbars are enabled 1.1) when overlay scrollbar is enabled, it takes up no extra space in the container element bounding rect. It means in practice that overlay scrollbars MIGHT physically get painted top of the padding area, if any. With that in mind, the patch intended to allow the padding box rect to be used as the clipping rect, and not clip out overlay scrollbars. Note we are only talking here about scrollbars, not listbox content intruding into the padding area. In other words, ::controlClipRect method is only about controls (e.g. scrollbar) clipping, not the content itself. 1.2) when non-overlay scrollbar is enabled, they take up their own space, and we do not need to use the padding box to clip controls, since scrollbar wont be on top of the padding area. > 2. Whether or not list-box contents are able to intrude into the padding Also agree that this issue exists and is an issue on its own, as you said. > These two issues are not related. Specifically, we should not be using item > #1 as a signal to change the behavior of #2. Patch here is about #1 (1.1 and 1.2) and fixes the problem of overlay scrollbars being clipped out only. The WebKit behavior in attachment 275783 [details] is unchanged. Yet about attachment 275783 [details], issue #2 something that diverges WebKit from Firefox and Chrome, and we should consider changing it to match other vendors. I am happy to work on it as a follow up. That all being said... @Simon/Myles: if you agree I will update my patch with better comments and changelog, as well as provide a regression test for it. Then follow up with fixing (2) afterwards. Does it sound like a good plan?
Created attachment 275808 [details] padding box clip in Firefox, Chrome and Safari nightly
Please enhance your testcase and screen shots to: 1. have long options that spill into the padding box 2. show selected state.
Created attachment 275833 [details] Always-on scrollbar browser comparison Comparison of different browsers with always-on scrollbars
Created attachment 275834 [details] Overlay scrollbar browser comparison
I just attached two videos describing the various browser's behaviors.
(In reply to comment #25) > I just attached two videos describing the various browser's behaviors. Thanks! I spoke briefly to Simon on IRC again today, and he would rather see the issue #2 you listed in comment #16 fixed. I have a WIP patch, and should put it up for review shortly (tomorrow).
Here's another datapoint: I discussed the idea of always clipping list boxes to their padding box (rather than their content box) with Mitz yesterday, and his opinion is that it's a good idea because Thisbe new proposed bahavior would be consistent with the Regular CSS box model. I think I agree with him. So it sounds like the patch I reviewed here a few days ago is very close. It just needs to remove the conditional (and add a test) and then would be good to go.
(In reply to comment #27) > Here's another datapoint: I discussed the idea of always clipping list boxes > to their padding box (rather than their content box) with Mitz yesterday, > and his opinion is that it's a good idea because Thisbe new proposed > bahavior would be consistent with the Regular CSS box model. I think I agree > with him. > > So it sounds like the patch I reviewed here a few days ago is very close. It > just needs to remove the conditional (and add a test) and then would be good > to go. (That is, assuming the boundaries of the selection highlight continue to be correct after the patch. This should be tested.)
(In reply to comment #28) > (In reply to comment #27) > > Here's another datapoint: I discussed the idea of always clipping list boxes > > to their padding box (rather than their content box) with Mitz yesterday, > > and his opinion is that it's a good idea because Thisbe new proposed > > bahavior would be consistent with the Regular CSS box model. I think I agree > > with him. > > > > So it sounds like the patch I reviewed here a few days ago is very close. It > > just needs to remove the conditional (and add a test) and then would be good > > to go. > > (That is, assuming the boundaries of the selection highlight continue to be > correct after the patch. This should be tested.) Thanks (one more) Myles. It is good to get mitz opinion on this. This is aligned to the WIP work for this bug as well.
Created attachment 276024 [details] Patch v2 - based on Simon's and Myles' feedbacl.
Please note: I will be working on a follow up patch in order to allow different test modes for overlay scrollbars, as per my conversation with Simon Fraser on IRC on 2016/April/5. This will allow adding more regression tests for this case. I filed bug 156412 to track this work.
Comment on attachment 276024 [details] Patch v2 - based on Simon's and Myles' feedbacl. Attachment 276024 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1122022 New failing tests: fast/forms/listbox-padding-clip.html fast/forms/listbox-padding-clip-overlay.html
Created attachment 276029 [details] Archive of layout-test-results from ews100 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 276024 [details] Patch v2 - based on Simon's and Myles' feedbacl. Attachment 276024 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1122042 New failing tests: fast/forms/listbox-padding-clip.html fast/forms/listbox-padding-clip-overlay.html
Created attachment 276031 [details] Archive of layout-test-results from ews105 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Created attachment 276033 [details] Patch v2.1 - based on Simon's and Myles' feedback. Fixed failing tests.
Created attachment 276034 [details] Patch v2.2 - based on Simon's and Myles' feedback. Patch v2.2 has improved changelogs with compared against v2.1.
Comment on attachment 276034 [details] Patch v2.2 - based on Simon's and Myles' feedback. Attachment 276034 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1122262 New failing tests: fast/forms/listbox-selection-3.html
Created attachment 276039 [details] Archive of layout-test-results from ews125 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews125 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Created attachment 276041 [details] Patch v2.3 - addressed Dan's review + rebased Skipped fast/forms/listbox-selection-3.html on ios-sim, similarly to fast/forms/listbox-selection-2.html
Created attachment 276076 [details] Patch v2.4 - addressed Simon's and Myles' feedback. (In reply to comment #28) > (In reply to comment #27) > > (..) > > > > So it sounds like the patch I reviewed here a few days ago is very close. It > > just needs to remove the conditional (and add a test) and then would be good > > to go. > > (That is, assuming the boundaries of the selection highlight continue to be > correct after the patch. This should be tested.) Patch v2.4 adds a test for verify clipping behavior of selected list-box items. Result now matches chrome and Firefox too.
Comment on attachment 276076 [details] Patch v2.4 - addressed Simon's and Myles' feedback. Attachment 276076 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1123750 New failing tests: fast/forms/listbox-padding-clip-selected.html
Created attachment 276078 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.10.5
Created attachment 276079 [details] Patch v2.5 - addressed Simon's and Myles' feedback. Same as patch v2.4, but skipped <select> tests for iOS, since iOS uses custom menu to handle it.
Comment on attachment 276079 [details] Patch v2.5 - addressed Simon's and Myles' feedback. View in context: https://bugs.webkit.org/attachment.cgi?id=276079&action=review > LayoutTests/fast/forms/listbox-padding-clip-overlay-expected-mismatch.html:6 > +<p>This test makes sure that the contents of list boxes do not intrude upon their padding. The test passes if you see a tall green box below (and not a single black pixel anywhere inside it)<p> THis explanatory text should be updated (or deleted, because mismatch tests should be as simple as possible to prevent false positives) > Source/WebCore/rendering/RenderListBox.cpp:734 > + // Clip against the padding box, to give <option>s and overlay scrollbar some extra space I don't think this comment is necessary.
Created attachment 276105 [details] Patch v2.6 - addressed Simon's and Myles' feedback. (In reply to comment #45) > Comment on attachment 276079 [details] > Patch v2.5 - addressed Simon's and Myles' feedback. > (..) > > LayoutTests/fast/forms/listbox-padding-clip-overlay-expected-mismatch.html:6 > > +<p>This test makes sure that the contents of list boxes do not intrude upon their padding. The test passes if you see a tall green box below (and not a single black pixel anywhere inside it)<p> > > THis explanatory text should be updated (or deleted, because mismatch tests > should be as simple as possible to prevent false positives) I agree. I deleted them in Patch v2.6. > > Source/WebCore/rendering/RenderListBox.cpp:734 > > + // Clip against the padding box, to give <option>s and overlay scrollbar some extra space > > I don't think this comment is necessary. If you do not feel strong about it, I would like to keep the comment in order to make it clearer for readers the reason this specific clipping rect was chosen. Otherwise, I can remove it, no problem. Also, rebased against ToT (there were a conflict in ios-sim/TestExpectation file.
Created attachment 276294 [details] Before patch v2.6
Created attachment 276295 [details] After patch v2.6
I've added two screenshots that show patch v2.6 causes a regression regarding padding-bottom. We should add a test for this. The markup for the screenshots is: <!DOCTYPE html> <html> <head> </head> <body> <select multiple="multiple" style="padding: 50px;"> <option>January</option> <option>February</option> <option>March</option> <option>April</option> <option>May</option> <option>June</option> <option>July</option> <option>August</option> <option>September</option> <option>October</option> <option>November</option> <option>December</option> </select> </body> </html>
Created attachment 276304 [details] padding-bottom-Chrome Padding-bottom on Chrome (same result as attachment 276295 [details] - "After patch v2.6").
Created attachment 276305 [details] padding-bottom-Firefox Padding-bottom on Firefox (same result as attachment 276295 [details] - "After patch v2.6").
(In reply to comment #49) > I've added two screenshots that show patch v2.6 causes a regression > regarding padding-bottom. We should add a test for this. > > The markup for the screenshots is: > > <!DOCTYPE html> > <html> > <head> > </head> > <body> > <select multiple="multiple" style="padding: 50px;"> > <option>January</option> > <option>February</option> > <option>March</option> > <option>April</option> > <option>May</option> > <option>June</option> > <option>July</option> > <option>August</option> > <option>September</option> > <option>October</option> > <option>November</option> > <option>December</option> > </select> > </body> > </html> Hi Myles. Thanks for the nice review! The behavior change of patch v2.6 pointed out in comment 49, where <select> content also paints on the padding-bottom area, was made on purpose in order to make Safari to match other vendors, including Firefox and Chrome. Please see attachment 276304 [details] (Chrome) and attachment 276305 [details] (Firefox), where the sample HTML in comment 49 is loaded in both. Note that the result of both Firefox and Chrome match of patched Safari (attachment 276295 [details]). This behavior change is also tested on fast/forms/listbox-selection-3.html, added in patch v2.6 - check lines 98 and 99. On the code, the change in RenderListBox::numVisibleItems is responsible for such behavior change.
Created attachment 276316 [details] padding-bottom-Presto (Opera pre-blink)
Comment on attachment 276105 [details] Patch v2.6 - addressed Simon's and Myles' feedback. Wow, padding is definitely supposed to be inside the part that moves when you scroll, not outside it. The problem I mentioned before is definitely (at least partly) a progression.
Created attachment 276414 [details] Right padding hit test
The interaction of list boxes and padding is currently not specced: https://html.spec.whatwg.org/multipage/rendering.html#the-select-element-2 Therefore, I think we should adhere as much as possible to the CSS box model (which would make us agree with Chrome).
Comment on attachment 276105 [details] Patch v2.6 - addressed Simon's and Myles' feedback. After this patch, there remain some major issues with listboxes (see bugs 156587, 156590, 156591, and 156592). However, this patch by itself is definitely a progression, so I'll r+ it. I hope we can fix these terrible follow up bugs soon.
(In reply to comment #57) > Comment on attachment 276105 [details] > Patch v2.6 - addressed Simon's and Myles' feedback. > > After this patch, there remain some major issues with listboxes (see bugs > 156587, 156590, 156591, and 156592). However, this patch by itself is > definitely a progression, so I'll r+ it. I hope we can fix these terrible > follow up bugs soon. Also bug 156594
Comment on attachment 276105 [details] Patch v2.6 - addressed Simon's and Myles' feedback. Thanks Myles. Party has just started..
Comment on attachment 276105 [details] Patch v2.6 - addressed Simon's and Myles' feedback. Rejecting attachment 276105 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 276105, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit /Volumes/Data/EWS/WebKit/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive). Full output: http://webkit-queues.webkit.org/results/1157940
Created attachment 276422 [details] Patch for landing.
Comment on attachment 276422 [details] Patch for landing. Clearing flags on attachment: 276422 Committed r199553: <http://trac.webkit.org/changeset/199553>
All reviewed patches have been landed. Closing bug.
Thank you! I've removed this bug from our Wall. https://github.com/twbs/bootstrap/pull/19737
<rdar://problem/19208483>