Bug 141783 – REGRESSION(r176978): Inline-blocks with overflowing contents have ascents that are too large

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Myles C. Maxfield]

REGRESSION(r176978): Inline-blocks with overflowing contents have ascents that are too large

[Myles C. Maxfield]

Created attachment 246865 [details]
Patch

[Myles C. Maxfield]

<rdar://problem/19709060>

[Simon Fraser (smfr)]

I'd like to know how the new behavior compares with our previous behavior, with the spec, and with Firefox.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[zalan]

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?

[Myles C. Maxfield]

Created attachment 248080 [details]
Test cases for browser comparison

[Myles C. Maxfield]

(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.

[Myles C. Maxfield]

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

[Myles C. Maxfield]

Created attachment 248088 [details]
WIP

[Myles C. Maxfield]

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.

[Build Bot]

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.

[Build Bot]

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

[Build Bot]

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.

[Build Bot]

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

[Myles C. Maxfield]

Created attachment 248104 [details]
Hyatt version

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Myles C. Maxfield]

Created attachment 248134 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Myles C. Maxfield]

Looks like mac-wk2 is a false negative

[Build Bot]

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

[Build Bot]

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

[Myles C. Maxfield]

Created attachment 248279 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Dave Hyatt]

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.

[Myles C. Maxfield]

Committed r181292: <http://trac.webkit.org/changeset/181292>

[Brent Fulgham]

This broke some Windows tests. I'll rebaseline them.