Bug 128489 - Scrollbar hidden on select[multiple] with right padding
Summary: Scrollbar hidden on select[multiple] with right padding
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.9
: P2 Normal
Assignee: Antonio Gomes
URL: http://jsbin.com/vojij/3/edit
Keywords: InRadar
Depends on:
Blocks: 159753
  Show dependency treegraph
 
Reported: 2014-02-08 22:36 PST by Mark Otto
Modified: 2016-08-29 23:50 PDT (History)
14 users (show)

See Also:


Attachments
Screenshot of the bug (273.60 KB, image/png)
2014-12-10 11:51 PST, Chris Rebert
no flags Details
Patch v1. (4.65 KB, patch)
2016-04-05 13:36 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
padding box clip in Firefox, Chrome and Safari nightly (39.86 KB, image/png)
2016-04-06 10:53 PDT, Antonio Gomes
no flags Details
padding box clip in Firefox, Chrome and Safari nightly (62.53 KB, image/png)
2016-04-06 12:31 PDT, Antonio Gomes
no flags Details
Always-on scrollbar browser comparison (2.00 MB, video/quicktime)
2016-04-06 17:07 PDT, Myles C. Maxfield
no flags Details
Overlay scrollbar browser comparison (3.78 MB, video/quicktime)
2016-04-06 17:10 PDT, Myles C. Maxfield
no flags Details
Patch v2 - based on Simon's and Myles' feedbacl. (12.59 KB, patch)
2016-04-08 12:19 PDT, Antonio Gomes
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-yosemite (1.22 MB, application/zip)
2016-04-08 13:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-yosemite-wk2 (919.27 KB, application/zip)
2016-04-08 13:07 PDT, Build Bot
no flags Details
Patch v2.1 - based on Simon's and Myles' feedback. (15.71 KB, patch)
2016-04-08 13:10 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
Patch v2.2 - based on Simon's and Myles' feedback. (16.20 KB, patch)
2016-04-08 13:14 PDT, Antonio Gomes
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (630.62 KB, application/zip)
2016-04-08 14:07 PDT, Build Bot
no flags Details
Patch v2.3 - addressed Dan's review + rebased (16.96 KB, patch)
2016-04-08 14:09 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
Patch v2.4 - addressed Simon's and Myles' feedback. (19.55 KB, patch)
2016-04-08 20:14 PDT, Antonio Gomes
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews124 for ios-simulator-wk2 (800.01 KB, application/zip)
2016-04-08 21:05 PDT, Build Bot
no flags Details
Patch v2.5 - addressed Simon's and Myles' feedback. (19.89 KB, patch)
2016-04-08 21:11 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff
Patch v2.6 - addressed Simon's and Myles' feedback. (21.28 KB, patch)
2016-04-10 10:41 PDT, Antonio Gomes
mmaxfield: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Before patch v2.6 (9.75 KB, image/png)
2016-04-12 17:22 PDT, Myles C. Maxfield
no flags Details
After patch v2.6 (10.59 KB, image/png)
2016-04-12 17:22 PDT, Myles C. Maxfield
no flags Details
padding-bottom-Chrome (32.52 KB, image/png)
2016-04-12 22:22 PDT, Antonio Gomes
no flags Details
padding-bottom-Firefox (39.71 KB, image/png)
2016-04-12 22:28 PDT, Antonio Gomes
no flags Details
padding-bottom-Presto (Opera pre-blink) (29.94 KB, image/png)
2016-04-13 06:35 PDT, Antonio Gomes
no flags Details
Right padding hit test (177.56 KB, video/quicktime)
2016-04-14 13:13 PDT, Myles C. Maxfield
no flags Details
Patch for landing. (21.42 KB, patch)
2016-04-14 14:00 PDT, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Otto 2014-02-08 22:36:49 PST
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.
Comment 1 Mark Otto 2014-02-08 22:41:05 PST
Relevant bug for Chromium: https://code.google.com/p/chromium/issues/detail?id=342208.
Comment 2 Chris Rebert 2014-09-19 21:06:22 PDT
Original Bootstrap issue report: https://github.com/twbs/bootstrap/issues/12536
Comment 3 Chris Rebert 2014-11-04 10:37:47 PST
Still happening with Safari 8.0 (10600.1.25) on OS X Yosemite.
Comment 4 Chris Rebert 2014-12-10 11:51:17 PST
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.
Comment 5 Chris Rebert 2014-12-10 12:10:29 PST
Have also filed this as:
<rdar://problem/19208483>
Comment 6 Simon Fraser (smfr) 2014-12-10 12:12:41 PST
Does this bug impact a common web site or app?
Comment 7 Chris Rebert 2014-12-10 12:41:16 PST
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 ).
Comment 8 Simon Fraser (smfr) 2015-07-17 21:25:36 PDT
Still reproduces.
Comment 9 Antonio Gomes 2016-03-08 11:29:26 PST
(In reply to comment #8)
> Still reproduces.

Reproducible on Safari as of r197784 (Mar/8/2016). Taking a look ..
Comment 10 Antonio Gomes 2016-04-04 08:27:46 PDT
(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.
Comment 11 Antonio Gomes 2016-04-04 14:36:02 PDT
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).
Comment 12 Antonio Gomes 2016-04-05 13:36:27 PDT
Created attachment 275687 [details]
Patch v1.
Comment 13 Antonio Gomes 2016-04-05 15:46:55 PDT
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 14 Myles C. Maxfield 2016-04-05 16:02:06 PDT
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-.
Comment 15 Antonio Gomes 2016-04-05 16:26:03 PDT
(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.
Comment 16 Myles C. Maxfield 2016-04-05 16:44:45 PDT
(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.
Comment 17 Antonio Gomes 2016-04-05 17:03:31 PDT


(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.
Comment 18 Myles C. Maxfield 2016-04-05 17:12:36 PDT
(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
Comment 19 Antonio Gomes 2016-04-06 10:53:59 PDT
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.
Comment 20 Antonio Gomes 2016-04-06 11:59:34 PDT
> > > > 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?
Comment 21 Antonio Gomes 2016-04-06 12:31:19 PDT
Created attachment 275808 [details]
padding box clip in Firefox, Chrome and Safari nightly
Comment 22 Simon Fraser (smfr) 2016-04-06 13:10:23 PDT
Please enhance your testcase and screen shots to:
1. have long options that spill into the padding box
2. show selected state.
Comment 23 Myles C. Maxfield 2016-04-06 17:07:53 PDT
Created attachment 275833 [details]
Always-on scrollbar browser comparison

Comparison of different browsers with always-on scrollbars
Comment 24 Myles C. Maxfield 2016-04-06 17:10:30 PDT
Created attachment 275834 [details]
Overlay scrollbar browser comparison
Comment 25 Myles C. Maxfield 2016-04-06 17:13:22 PDT
I just attached two videos describing the various browser's behaviors.
Comment 26 Antonio Gomes 2016-04-06 20:04:56 PDT
(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).
Comment 27 Myles C. Maxfield 2016-04-07 08:03:36 PDT
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.
Comment 28 Myles C. Maxfield 2016-04-07 08:06:17 PDT
(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.)
Comment 29 Antonio Gomes 2016-04-07 08:13:41 PDT
(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.
Comment 30 Antonio Gomes 2016-04-08 12:19:28 PDT
Created attachment 276024 [details]
Patch v2 - based on Simon's and Myles' feedbacl.
Comment 31 Antonio Gomes 2016-04-08 12:20:39 PDT
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 32 Build Bot 2016-04-08 13:00:31 PDT
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
Comment 33 Build Bot 2016-04-08 13:00:35 PDT
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 34 Build Bot 2016-04-08 13:06:56 PDT
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
Comment 35 Build Bot 2016-04-08 13:07:01 PDT
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
Comment 36 Antonio Gomes 2016-04-08 13:10:05 PDT
Created attachment 276033 [details]
Patch v2.1 - based on Simon's and Myles' feedback.

Fixed failing tests.
Comment 37 Antonio Gomes 2016-04-08 13:14:12 PDT
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 38 Build Bot 2016-04-08 14:07:23 PDT
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
Comment 39 Build Bot 2016-04-08 14:07:27 PDT
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
Comment 40 Antonio Gomes 2016-04-08 14:09:22 PDT
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
Comment 41 Antonio Gomes 2016-04-08 20:14:04 PDT
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 42 Build Bot 2016-04-08 21:05:37 PDT
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
Comment 43 Build Bot 2016-04-08 21:05:42 PDT
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
Comment 44 Antonio Gomes 2016-04-08 21:11:35 PDT
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 45 Myles C. Maxfield 2016-04-09 09:07:32 PDT
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.
Comment 46 Antonio Gomes 2016-04-10 10:41:31 PDT
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.
Comment 47 Myles C. Maxfield 2016-04-12 17:22:18 PDT
Created attachment 276294 [details]
Before patch v2.6
Comment 48 Myles C. Maxfield 2016-04-12 17:22:36 PDT
Created attachment 276295 [details]
After patch v2.6
Comment 49 Myles C. Maxfield 2016-04-12 17:23:38 PDT
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>
Comment 50 Antonio Gomes 2016-04-12 22:22:58 PDT
Created attachment 276304 [details]
padding-bottom-Chrome

Padding-bottom on Chrome (same result as attachment 276295 [details] - "After patch v2.6").
Comment 51 Antonio Gomes 2016-04-12 22:28:31 PDT
Created attachment 276305 [details]
padding-bottom-Firefox

Padding-bottom on Firefox (same result as attachment 276295 [details] - "After patch v2.6").
Comment 52 Antonio Gomes 2016-04-12 22:38:12 PDT
(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.
Comment 53 Antonio Gomes 2016-04-13 06:35:20 PDT
Created attachment 276316 [details]
padding-bottom-Presto (Opera pre-blink)
Comment 54 Myles C. Maxfield 2016-04-14 12:13:08 PDT
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.
Comment 55 Myles C. Maxfield 2016-04-14 13:13:10 PDT
Created attachment 276414 [details]
Right padding hit test
Comment 56 Myles C. Maxfield 2016-04-14 13:27:22 PDT
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 57 Myles C. Maxfield 2016-04-14 13:41:17 PDT
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.
Comment 58 Myles C. Maxfield 2016-04-14 13:44:10 PDT
(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 59 Antonio Gomes 2016-04-14 13:47:28 PDT
Comment on attachment 276105 [details]
Patch v2.6 - addressed Simon's and Myles' feedback.

Thanks Myles. Party has just started..
Comment 60 WebKit Commit Bot 2016-04-14 13:49:54 PDT
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
Comment 61 Antonio Gomes 2016-04-14 14:00:27 PDT
Created attachment 276422 [details]
Patch for landing.
Comment 62 WebKit Commit Bot 2016-04-14 14:14:46 PDT
Comment on attachment 276422 [details]
Patch for landing.

Clearing flags on attachment: 276422

Committed r199553: <http://trac.webkit.org/changeset/199553>
Comment 63 WebKit Commit Bot 2016-04-14 14:14:52 PDT
All reviewed patches have been landed.  Closing bug.
Comment 64 Chris Rebert 2016-04-14 15:58:18 PDT
Thank you!
I've removed this bug from our Wall. https://github.com/twbs/bootstrap/pull/19737
Comment 65 David Kilzer (:ddkilzer) 2016-05-28 14:50:27 PDT
<rdar://problem/19208483>