Bug 141783

Summary: REGRESSION(r176978): Inline-blocks with overflowing contents have ascents that are too large
Product: WebKit Reporter: Myles C. Maxfield <mmaxfield>
Component: New BugsAssignee: Myles C. Maxfield <mmaxfield>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, buildbot, commit-queue, dino, esprehn+autocc, glenn, hyatt, jonlee, kondapallykalyan, rniwa, simon.fraser, thorton, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews101 for mac-mavericks
none
Archive of layout-test-results from ews104 for mac-mavericks-wk2
none
Test cases for browser comparison
none
WIP
none
Archive of layout-test-results from ews105 for mac-mavericks-wk2
none
Archive of layout-test-results from ews100 for mac-mavericks
none
Hyatt version
buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-mavericks-wk2
none
Archive of layout-test-results from ews100 for mac-mavericks
none
Patch
none
Archive of layout-test-results from ews107 for mac-mavericks-wk2
none
Archive of layout-test-results from ews100 for mac-mavericks
none
Patch
hyatt: review+, buildbot: commit-queue-
Archive of layout-test-results from ews100 for mac-mavericks
none
Archive of layout-test-results from ews104 for mac-mavericks-wk2 none

Description Myles C. Maxfield 2015-02-18 18:05:20 PST
REGRESSION(r176978): Inline-blocks with overflowing contents have ascents that are too large
Comment 1 Myles C. Maxfield 2015-02-18 18:10:06 PST
Created attachment 246865 [details]
Patch
Comment 2 Myles C. Maxfield 2015-02-18 18:10:47 PST
<rdar://problem/19709060>
Comment 3 Simon Fraser (smfr) 2015-02-18 18:25:05 PST
I'd like to know how the new behavior compares with our previous behavior, with the spec, and with Firefox.
Comment 4 Build Bot 2015-02-18 18:50:59 PST
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.
Comment 5 Build Bot 2015-02-18 18:51:03 PST
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 6 Build Bot 2015-02-18 18:54:37 PST
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.
Comment 7 Build Bot 2015-02-18 18:54:41 PST
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 8 zalan 2015-02-19 10:09:37 PST
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?
Comment 9 Myles C. Maxfield 2015-03-06 11:24:09 PST
Created attachment 248080 [details]
Test cases for browser comparison
Comment 10 Myles C. Maxfield 2015-03-06 11:24:43 PST
(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.
Comment 11 Myles C. Maxfield 2015-03-06 11:34:42 PST
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
Comment 12 Myles C. Maxfield 2015-03-06 12:25:28 PST
Created attachment 248088 [details]
WIP
Comment 13 Myles C. Maxfield 2015-03-06 12:27:48 PST
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 14 Build Bot 2015-03-06 12:57:49 PST
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.
Comment 15 Build Bot 2015-03-06 12:57:54 PST
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 16 Build Bot 2015-03-06 13:05:35 PST
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.
Comment 17 Build Bot 2015-03-06 13:05:41 PST
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
Comment 18 Myles C. Maxfield 2015-03-06 16:01:53 PST
Created attachment 248104 [details]
Hyatt version
Comment 19 Build Bot 2015-03-06 16:58:16 PST
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
Comment 20 Build Bot 2015-03-06 16:58:22 PST
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 21 Build Bot 2015-03-06 18:12:16 PST
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
Comment 22 Build Bot 2015-03-06 18:12:23 PST
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
Comment 23 Myles C. Maxfield 2015-03-06 19:57:42 PST
Created attachment 248134 [details]
Patch
Comment 24 Build Bot 2015-03-06 20:50:29 PST
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
Comment 25 Build Bot 2015-03-06 20:50:35 PST
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
Comment 26 Myles C. Maxfield 2015-03-06 21:14:27 PST
Looks like mac-wk2 is a false negative
Comment 27 Build Bot 2015-03-06 21:20:04 PST
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
Comment 28 Build Bot 2015-03-06 21:20:09 PST
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
Comment 29 Myles C. Maxfield 2015-03-09 15:01:17 PDT
Created attachment 248279 [details]
Patch
Comment 30 Build Bot 2015-03-09 15:39:50 PDT
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
Comment 31 Build Bot 2015-03-09 15:39:56 PDT
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 32 Build Bot 2015-03-09 15:53:49 PDT
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
Comment 33 Build Bot 2015-03-09 15:53:54 PDT
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 34 Dave Hyatt 2015-03-09 16:30:31 PDT
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.
Comment 35 Myles C. Maxfield 2015-03-09 16:44:08 PDT
Committed r181292: <http://trac.webkit.org/changeset/181292>
Comment 36 Brent Fulgham 2015-03-09 18:37:42 PDT
This broke some Windows tests. I'll rebaseline them.