WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
128489
Scrollbar hidden on select[multiple] with right padding
https://bugs.webkit.org/show_bug.cgi?id=128489
Summary
Scrollbar hidden on select[multiple] with right padding
Mark Otto
Reported
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.
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
Show Obsolete
(15)
View All
Add attachment
proposed patch, testcase, etc.
Mark Otto
Comment 1
2014-02-08 22:41:05 PST
Relevant bug for Chromium:
https://code.google.com/p/chromium/issues/detail?id=342208
.
Chris Rebert
Comment 2
2014-09-19 21:06:22 PDT
Original Bootstrap issue report:
https://github.com/twbs/bootstrap/issues/12536
Chris Rebert
Comment 3
2014-11-04 10:37:47 PST
Still happening with Safari 8.0 (10600.1.25) on OS X Yosemite.
Chris Rebert
Comment 4
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.
Chris Rebert
Comment 5
2014-12-10 12:10:29 PST
Have also filed this as: <
rdar://problem/19208483
>
Simon Fraser (smfr)
Comment 6
2014-12-10 12:12:41 PST
Does this bug impact a common web site or app?
Chris Rebert
Comment 7
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
).
Simon Fraser (smfr)
Comment 8
2015-07-17 21:25:36 PDT
Still reproduces.
Antonio Gomes
Comment 9
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 ..
Antonio Gomes
Comment 10
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.
Antonio Gomes
Comment 11
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).
Antonio Gomes
Comment 12
2016-04-05 13:36:27 PDT
Created
attachment 275687
[details]
Patch v1.
Antonio Gomes
Comment 13
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.
Myles C. Maxfield
Comment 14
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-.
Antonio Gomes
Comment 15
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.
Myles C. Maxfield
Comment 16
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.
Antonio Gomes
Comment 17
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.
Myles C. Maxfield
Comment 18
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
Antonio Gomes
Comment 19
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
.
Antonio Gomes
Comment 20
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?
Antonio Gomes
Comment 21
2016-04-06 12:31:19 PDT
Created
attachment 275808
[details]
padding box clip in Firefox, Chrome and Safari nightly
Simon Fraser (smfr)
Comment 22
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.
Myles C. Maxfield
Comment 23
2016-04-06 17:07:53 PDT
Created
attachment 275833
[details]
Always-on scrollbar browser comparison Comparison of different browsers with always-on scrollbars
Myles C. Maxfield
Comment 24
2016-04-06 17:10:30 PDT
Created
attachment 275834
[details]
Overlay scrollbar browser comparison
Myles C. Maxfield
Comment 25
2016-04-06 17:13:22 PDT
I just attached two videos describing the various browser's behaviors.
Antonio Gomes
Comment 26
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).
Myles C. Maxfield
Comment 27
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.
Myles C. Maxfield
Comment 28
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.)
Antonio Gomes
Comment 29
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.
Antonio Gomes
Comment 30
2016-04-08 12:19:28 PDT
Created
attachment 276024
[details]
Patch v2 - based on Simon's and Myles' feedbacl.
Antonio Gomes
Comment 31
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.
Build Bot
Comment 32
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
Build Bot
Comment 33
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
Build Bot
Comment 34
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
Build Bot
Comment 35
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
Antonio Gomes
Comment 36
2016-04-08 13:10:05 PDT
Created
attachment 276033
[details]
Patch v2.1 - based on Simon's and Myles' feedback. Fixed failing tests.
Antonio Gomes
Comment 37
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.
Build Bot
Comment 38
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
Build Bot
Comment 39
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
Antonio Gomes
Comment 40
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
Antonio Gomes
Comment 41
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.
Build Bot
Comment 42
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
Build Bot
Comment 43
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
Antonio Gomes
Comment 44
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.
Myles C. Maxfield
Comment 45
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.
Antonio Gomes
Comment 46
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.
Myles C. Maxfield
Comment 47
2016-04-12 17:22:18 PDT
Created
attachment 276294
[details]
Before patch v2.6
Myles C. Maxfield
Comment 48
2016-04-12 17:22:36 PDT
Created
attachment 276295
[details]
After patch v2.6
Myles C. Maxfield
Comment 49
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>
Antonio Gomes
Comment 50
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").
Antonio Gomes
Comment 51
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").
Antonio Gomes
Comment 52
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.
Antonio Gomes
Comment 53
2016-04-13 06:35:20 PDT
Created
attachment 276316
[details]
padding-bottom-Presto (Opera pre-blink)
Myles C. Maxfield
Comment 54
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.
Myles C. Maxfield
Comment 55
2016-04-14 13:13:10 PDT
Created
attachment 276414
[details]
Right padding hit test
Myles C. Maxfield
Comment 56
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).
Myles C. Maxfield
Comment 57
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.
Myles C. Maxfield
Comment 58
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
Antonio Gomes
Comment 59
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..
WebKit Commit Bot
Comment 60
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
Antonio Gomes
Comment 61
2016-04-14 14:00:27 PDT
Created
attachment 276422
[details]
Patch for landing.
WebKit Commit Bot
Comment 62
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
>
WebKit Commit Bot
Comment 63
2016-04-14 14:14:52 PDT
All reviewed patches have been landed. Closing bug.
Chris Rebert
Comment 64
2016-04-14 15:58:18 PDT
Thank you! I've removed this bug from our Wall.
https://github.com/twbs/bootstrap/pull/19737
David Kilzer (:ddkilzer)
Comment 65
2016-05-28 14:50:27 PDT
<
rdar://problem/19208483
>
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug