REGRESSION(r176978): Inline-blocks with overflowing contents have ascents that are too large
Created attachment 246865 [details] Patch
<rdar://problem/19709060>
I'd like to know how the new behavior compares with our previous behavior, with the spec, and with Firefox.
Comment on attachment 246865 [details] Patch Attachment 246865 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5538807063511040 Number of test failures exceeded the failure limit.
Created attachment 246871 [details] Archive of layout-test-results from ews101 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 246865 [details] Patch Attachment 246865 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5065341579296768 Number of test failures exceeded the failure limit.
Created attachment 246873 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 246865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246865&action=review > Source/WebCore/ChangeLog:11 > + When we have an inline-block element, and we want to find its baseline (to lay out other > + elements on the same line) we loop through the element's children and ask them what their > + baselines are. The children use the location of the top of their last line to compute this > + value. However, if the child has overflow, this might not be the correct calculation. overflow-y only. overflow-x should not affect baseline calculation. Also, I think it's worth explaining why font metrics are not taken into account when overflow-y happens. (Or simply stating something like, when content overflows, the baseline becomes the (direction dependent)bottom of the inline-block box.) > Source/WebCore/rendering/RenderBlockFlow.cpp:3004 > + // We are not calling RenderBox::baselinePosition here because the caller should add the margin-top/margin-right, not us. > + return lineDirection == HorizontalLine ? height() + m_marginBox.bottom() : width() + m_marginBox.left(); This line definitely needs an explanation, but I don't think the first part of the comment adds too much value. (also RenderBox::baselinePosition gives different result for inline-blocks with IdeographicBaseline). It might be better to explain why the box dimensions are used to calculate baseline (as opposed to font metrics), though that might be redundant, if changelog has it too. > LayoutTests/ChangeLog:13 > + * fast/css/inline-block-tricky-baselines-expected.html: Added. > + * fast/css/inline-block-tricky-baselines.html: Added. Could you add one more test case where the line has multiple inline-blocks but not all of them has the same overflow-y value? Like 2 inline-blocks, one with overflow-y: hidden while the second one with visible. The point is that it's enough to have one inline-block content with overflow-y to have the entire line's baseline changed. > LayoutTests/fast/css/inline-block-tricky-baselines.html:17 > + Single-line How is it a single line content, when it actually gets wrapped?
Created attachment 248080 [details] Test cases for browser comparison
(In reply to comment #3) > I'd like to know how the new behavior compares with our previous behavior, > with the spec, and with Firefox. All other browsers have the behavior contained in this patch.
This behavior is specc'ed: "The baseline of an 'inline-block' is the baseline of its last line box in the normal flow, unless it has either no in-flow line boxes or if its 'overflow' property has a computed value other than 'visible', in which case the baseline is the bottom margin edge." -- http://www.w3.org/TR/CSS21/visudet.html#leading
Created attachment 248088 [details] WIP
Comment on attachment 246865 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246865&action=review >> Source/WebCore/ChangeLog:11 >> + value. However, if the child has overflow, this might not be the correct calculation. > > overflow-y only. overflow-x should not affect baseline calculation. > Also, I think it's worth explaining why font metrics are not taken into account when overflow-y happens. (Or simply stating something like, when content overflows, the baseline becomes the (direction dependent)bottom of the inline-block box.) I think the quote I added from the spec covers this. >> Source/WebCore/rendering/RenderBlockFlow.cpp:3004 >> + return lineDirection == HorizontalLine ? height() + m_marginBox.bottom() : width() + m_marginBox.left(); > > This line definitely needs an explanation, but I don't think the first part of the comment adds too much value. (also RenderBox::baselinePosition gives different result for inline-blocks with IdeographicBaseline). It might be better to explain why the box dimensions are used to calculate baseline (as opposed to font metrics), though that might be redundant, if changelog has it too. Same as above, I believe the spec quote covers this. >> LayoutTests/ChangeLog:13 >> + * fast/css/inline-block-tricky-baselines.html: Added. > > Could you add one more test case where the line has multiple inline-blocks but not all of them has the same overflow-y value? Like 2 inline-blocks, one with overflow-y: hidden while the second one with visible. The point is that it's enough to have one inline-block content with overflow-y to have the entire line's baseline changed. Okay. >> LayoutTests/fast/css/inline-block-tricky-baselines.html:17 >> + Single-line > > How is it a single line content, when it actually gets wrapped? This test is from Blink. I can only guess that the result should only have a single visible line.
Comment on attachment 248088 [details] WIP Attachment 248088 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5233916730408960 Number of test failures exceeded the failure limit.
Created attachment 248090 [details] Archive of layout-test-results from ews105 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews105 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 248088 [details] WIP Attachment 248088 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5097643860557824 Number of test failures exceeded the failure limit.
Created attachment 248091 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 248104 [details] Hyatt version
Comment on attachment 248104 [details] Hyatt version Attachment 248104 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6316034814377984 New failing tests: fast/box-sizing/box-sizing.html fast/overflow/line-clamp.html fast/forms/search-vertical-alignment.html fast/forms/linebox-overflow-in-textarea-padding-simple-lines.html fast/forms/textfield-overflow-by-value-update.html fast/forms/linebox-overflow-in-textarea-padding.html
Created attachment 248115 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 248104 [details] Hyatt version Attachment 248104 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5075785698246656 New failing tests: fast/box-sizing/box-sizing.html fast/overflow/line-clamp.html fast/forms/search-vertical-alignment.html fast/forms/linebox-overflow-in-textarea-padding-simple-lines.html fast/forms/textfield-overflow-by-value-update.html fast/forms/linebox-overflow-in-textarea-padding.html
Created attachment 248127 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 248134 [details] Patch
Comment on attachment 248134 [details] Patch Attachment 248134 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/4879030025191424 New failing tests: fast/forms/search-vertical-alignment.html
Created attachment 248138 [details] Archive of layout-test-results from ews107 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews107 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Looks like mac-wk2 is a false negative
Comment on attachment 248134 [details] Patch Attachment 248134 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4563980215386112 New failing tests: fast/forms/search-vertical-alignment.html
Created attachment 248140 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Created attachment 248279 [details] Patch
Comment on attachment 248279 [details] Patch Attachment 248279 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5156001728692224 New failing tests: fast/forms/search-vertical-alignment.html
Created attachment 248282 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 248279 [details] Patch Attachment 248279 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/6543880145076224 New failing tests: fast/forms/search-vertical-alignment.html
Created attachment 248285 [details] Archive of layout-test-results from ews104 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5
Comment on attachment 248279 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=248279&action=review r=me > Source/WebCore/rendering/RenderBlockFlow.cpp:3034 > + lastBaseline = lastRootBox()->logicalTop() + style.fontMetrics().ascent(lastRootBox()->baselineType()); I know you didn't change this code, but I do see some issues that warrant followup bugs. The code you're touching looks wrong to me for vertical-lr. In vertical-lr, the lines are flipped, so the logicalTop() is actually the bottom of the line. Making a vertical-lr test case should show this. Second issue I see is that -webkit-line-box-contain could cause us to exclude the block's baseline from consideration, so in that case using the block's font's ascent is wrong.
Committed r181292: <http://trac.webkit.org/changeset/181292>
This broke some Windows tests. I'll rebaseline them.