Issue 1504403002: Fix computation of min|max-content contribution of non-replaced blocks

Issue 1504403002: Fix computation of min|max-content contribution of non-replaced blocks (Closed)

Created:
5 years ago by svillar

Modified:
5 years ago

Reviewers:
cbiesinger, mstensho (USE GERRIT), eae, rune

CC:
chromium-reviews, szager+layoutwatch_chromium.org, zoltan1, blink-reviews-layout_chromium.org, pdr+renderingwatchlist_chromium.org, eae+blinkwatch, leviw+renderwatch, jchaffraix+rendering, blink-reviews, Manuel Rego, jfernandez

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Fix computation of min|max-content contribution of non-replaced blocks Currently Blink always returns the min preferred logical width for the min-content contribution (and the max preferred logical width for the max-content contribution) for non-replaced blocks. That is not correct according to specs https://drafts.csswg.org/css-sizing/#block-intrinsic. The min-content and max-content contributions actually depend on the computed inline size of the block: * for min-content,max-content or definite sizes: min-content and max-content contributions are that size plus border, margin and padding. * otherwise: min-content contribution is the min-content size and max-content contribution is the max-content size (in both cases plus border, padding and margin). Committed: https://crrev.com/f39acf142b5781ea8ab5da01e7685efbf7b43fee Cr-Commit-Position: refs/heads/master@{#366753}

Patch Set 1 #

Total comments: 2

Patch Set 2 : Do not apply the modifications to tables which behave differently. Also there is no need to relayou… #

Total comments: 6

Patch Set 3 : Improved checks #

Patch Set 4 : Clear dirty bit #

Patch Set 5 : Fixed build #

Total comments: 3

Patch Set 6 : Reworked patch & tests #

Created: 5 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+794 lines, -0 lines)			Patch
A	third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/auto-minmax-content-inlinesize-contribution-nonreplaced-blocks.html	View	1 2 3 4 5	1 chunk	+99 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/definite-minmax-content-inlinesize-contribution-nonreplaced-blocks.html	View	1 2 3 4 5	1 chunk	+167 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/fillavailable-minmax-content-inlinesize-contribution-nonreplaced-blocks.html	View	1 2 3 4 5	1 chunk	+99 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/fitcontent-minmax-content-inlinesize-contribution-nonreplaced-blocks.html	View	1 2 3 4 5	1 chunk	+99 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/indefinite-percent-minmax-content-inlinesize-contribution-nonreplaced-blocks.html	View	1 2 3 4 5	1 chunk	+103 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/maxcontent-minmax-content-inlinesize-contribution-nonreplaced-blocks.html	View	1 2 3 4 5	1 chunk	+100 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/mincontent-minmax-content-inlinesize-contribution-nonreplaced-blocks.html	View	1 2 3 4 5	1 chunk	+99 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/resources/intrinsic-size-contribution.css	View		1 chunk	+17 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/layout/LayoutBlock.cpp	View	1 2 3 4 5	1 chunk	+11 lines, -0 lines	0 comments	Download

Messages

Total messages: 25 (6 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

svillar

Description was changed from ========== Fix computation of min|max-content contribution of non-replaced blocks Currently Blink ...

5 years ago (2015-12-08 19:25:58 UTC) #3

mstensho (USE GERRIT)

https://codereview.chromium.org/1504403002/diff/1/third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/auto-maxcontent-inlinesize-contribution-nonreplaced-blocks.html File third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/auto-maxcontent-inlinesize-contribution-nonreplaced-blocks.html (right): https://codereview.chromium.org/1504403002/diff/1/third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/auto-maxcontent-inlinesize-contribution-nonreplaced-blocks.html#newcode1 third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/auto-maxcontent-inlinesize-contribution-nonreplaced-blocks.html:1: <!DOCTYPE html> Great tests! I don't know how you ...

5 years ago (2015-12-09 13:19:52 UTC) #4

https://codereview.chromium.org/1504403002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/auto-maxcontent-inlinesize-contribution-nonreplaced-blocks.html
(right):

https://codereview.chromium.org/1504403002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/css-intrinsic-dimensions/auto-maxcontent-inlinesize-contribution-nonreplaced-blocks.html:1:
<!DOCTYPE html>
Great tests! I don't know how you wrote them, generated with a script or by
hand? If the former is the case, I'd very much like to have them converted to
check-layout.js tests (using data-expected-width attributes), which are much
faster than reftests. But if that would involve a lot of work for you, just
ignore this request. :)

https://codereview.chromium.org/1504403002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1504403002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:2156:
ASSERT(computedInlineSize.isMinContent() || computedInlineSize.isMaxContent() ||
computedInlineSize.isSpecified());
This is heavy. Start with bogus initial values. Lay out. Calculate correct
values. Lay out again. There's no good way around that when we're dealing with
perpendicular writing modes (code further up), but it should not necessary to
lay out in order to calculate the correct min/max preferred logical widths just
because width is min-content or max-content.

Why don't we just set the correct values right away instead?

If width is max-content, use maximum preferred logical width as minimum
preferred logical width as well.

If width is min-content, use minimum preferred logical width as maximum
preferred logical width as well.

A simpler way of achieving what you want could be:

    minPreferredLogicalWidth = child.minPreferredLogicalWidth();
    maxPreferredLogicalWidth = child.maxPreferredLogicalWidth();
    const Length& computedInlineSize = child.styleRef().logicalWidth();
    if (computedInlineSize.isMaxContent())
        minPreferredLogicalWidth = maxPreferredLogicalWidth;
    else if (computedInlineSize.isMinContent())
        maxPreferredLogicalWidth = minPreferredLogicalWidth;

We'd still get it wrong for table cells and possibly other things (cases where
LayoutBlock::computeChildPreferredLogicalWidths() isn't used), but we can fix
that separately, later.

svillar

Do not apply the modifications to tables which behave differently. Also there is no need ...

5 years ago (2015-12-09 15:45:09 UTC) #5

mstensho (USE GERRIT)

https://codereview.chromium.org/1504403002/diff/20001/third_party/WebKit/Source/core/layout/LayoutBlock.cpp File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1504403002/diff/20001/third_party/WebKit/Source/core/layout/LayoutBlock.cpp#newcode2148 third_party/WebKit/Source/core/layout/LayoutBlock.cpp:2148: if (child.isLayoutBlock() && !child.isTable() && !child.isReplaced()) { Why exclude ...

5 years ago (2015-12-09 16:56:05 UTC) #6

svillar

Thanks for the review Morten! (BTW tests were completely hand made so changing them all ...

5 years ago (2015-12-09 17:41:32 UTC) #7

mstensho (USE GERRIT)

Fine with me to keep the tests as they are. But please consider check-layout.js next ...

5 years ago (2015-12-09 19:17:33 UTC) #8

svillar

Removed isReplaced() call, it was misleading (not checking what I wanted to check) and unneeded ...

5 years ago (2015-12-16 11:04:31 UTC) #9

svillar

mtensho: so all the tests are working fine now with the last proposed CL. Are ...

5 years ago (2015-12-16 14:46:51 UTC) #10

svillar

On 2015/12/16 14:46:51, svillar wrote: > mtensho: so all the tests are working fine now ...

5 years ago (2015-12-17 08:14:52 UTC) #11

mstensho (USE GERRIT)

On 2015/12/17 08:14:52, svillar wrote: > On 2015/12/16 14:46:51, svillar wrote: > > mtensho: so ...

5 years ago (2015-12-18 12:03:47 UTC) #12

svillar

On 2015/12/18 12:03:47, mstensho wrote: > On 2015/12/17 08:14:52, svillar wrote: > > On 2015/12/16 ...

5 years ago (2015-12-18 12:40:25 UTC) #13

svillar

https://codereview.chromium.org/1504403002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBlock.cpp File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right): https://codereview.chromium.org/1504403002/diff/80001/third_party/WebKit/Source/core/layout/LayoutBlock.cpp#newcode2128 third_party/WebKit/Source/core/layout/LayoutBlock.cpp:2128: if (child.isLayoutBlock() && !child.isTable()) { On 2015/12/18 12:03:46, mstensho ...

5 years ago (2015-12-18 12:40:37 UTC) #14

mstensho (USE GERRIT)

On 2015/12/18 12:40:25, svillar wrote: > On 2015/12/18 12:03:47, mstensho wrote: > > On 2015/12/17 ...

5 years ago (2015-12-18 14:05:12 UTC) #15

On 2015/12/18 12:40:25, svillar wrote:
> On 2015/12/18 12:03:47, mstensho wrote:
> > On 2015/12/17 08:14:52, svillar wrote:
> > > On 2015/12/16 14:46:51, svillar wrote:
> > > > mtensho: so all the tests are working fine now with the last proposed
CL.
> > Are
> > > > you OK with the patch?
> > > > 
> > > > BTW the code you proposed is not doing the same as mine, note that for
> > > > min-content/max-content/definitesize we need to use the actual intrinsic
> > size
> > >                                                          
> > ^^^^^^^^^^^^^^^^^^^^^
> > > I meant the "actual inline size"
> > 
> > What's "actual inline size" when containing block size is unknown?
> 
> Fair enough, I've used confusing terminology. This is what specs say:
> 
> "If the computed inline-size of a block-level box is min-content, max-content,
> or a definite size, its max-content inline-size contribution is that size plus
> any inline-axis margin, border, and padding."
> 
> So as you can see, we don't need the size of the containing block at all
> (min-content & max-content are intrinsic sizes and definite sizes are
> resolvable).

Let's remove "definite size" from the equation. That one is already working
fine. So we're left with min-content and max-content. If it's min-content, both
preferred minimum and maximum width will be child.minPreferredLogicalWidth(). If
it's max-content, both preferred minimum and maximum width will be
child.maxPreferredLogicalWidth().

> > Can you show me a test where it makes a difference?
> 
> Sure thing. So I've included a bunch of tests, but most of them are already
> working fine (just included them for completeness and obviously to catch
> potential future regressions). The only ones that are not currently passing
are
> those which have a min-content sized block inside a max-content sized block
and
> viceversa. Something like this:
http://jsbin.com/qoyotaqefo/edit?html,css,output

This also works with my proposal. Isn't that what we're discussing - i.e. why my
simpler proposal is inferior to yours?

https://codereview.chromium.org/1504403002/diff/80001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBlock.cpp (right):

https://codereview.chromium.org/1504403002/diff/80001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:2128: if
(child.isLayoutBlock() && !child.isTable()) {
On 2015/12/18 12:40:37, svillar wrote:
> On 2015/12/18 12:03:46, mstensho wrote:
> > The table check is still here. I had a comment on that in my previous batch.
> 
> I forgot to reply to that comment. The problem with the table code is that it
> does so many "tricks" in sizing computations that removing the check
> automatically breaks tons of table tests (including the ones we have imported
> from other vendors). Apart from that there is this special section for tables
> that is still not defined in the specs. Changing the table code it's a big
chunk
> of work per se, so if you agree, I'd prefer to leave it aside for the moment.

Aha, with your changes a fixed width now means that we just call
computedLogicalWidth() on the child, which isn't going to work for tables, since
specifying a fixed width there is just advisory (I think it roughly means to
treat width as min-width).

svillar

On 2015/12/18 14:05:12, mstensho wrote: > > > Can you show me a test where ...

5 years ago (2015-12-21 08:52:50 UTC) #16

On 2015/12/18 14:05:12, mstensho wrote:
> > > Can you show me a test where it makes a difference?
> > 
> > Sure thing. So I've included a bunch of tests, but most of them are already
> > working fine (just included them for completeness and obviously to catch
> > potential future regressions). The only ones that are not currently passing
> are
> > those which have a min-content sized block inside a max-content sized block
> and
> > viceversa. Something like this:
> http://jsbin.com/qoyotaqefo/edit?html,css,output
> 
> This also works with my proposal. Isn't that what we're discussing - i.e. why
my
> simpler proposal is inferior to yours?

Well I was thinking in terms of correctness not superior/inferior :). Anyway
I've realized that I had misunderstood your proposal which is indeed much
simpler. I'll upload a new patch for it.
 
>
https://codereview.chromium.org/1504403002/diff/80001/third_party/WebKit/Sour...
> third_party/WebKit/Source/core/layout/LayoutBlock.cpp:2128: if
> (child.isLayoutBlock() && !child.isTable()) {
> On 2015/12/18 12:40:37, svillar wrote:
> > On 2015/12/18 12:03:46, mstensho wrote:
> > > The table check is still here. I had a comment on that in my previous
batch.
> > 
> > I forgot to reply to that comment. The problem with the table code is that
it
> > does so many "tricks" in sizing computations that removing the check
> > automatically breaks tons of table tests (including the ones we have
imported
> > from other vendors). Apart from that there is this special section for
tables
> > that is still not defined in the specs. Changing the table code it's a big
> chunk
> > of work per se, so if you agree, I'd prefer to leave it aside for the
moment.
> 
> Aha, with your changes a fixed width now means that we just call
> computedLogicalWidth() on the child, which isn't going to work for tables,
since
> specifying a fixed width there is just advisory (I think it roughly means to
> treat width as min-width).

Makes total sense.

mstensho (USE GERRIT)

lgtm, but please update the commit message. This: "The min-content and max-content contributions actually depend ...

5 years ago (2015-12-21 13:11:51 UTC) #17

svillar

On 2015/12/21 13:11:51, mstensho wrote: > lgtm, but please update the commit message. > > ...

5 years ago (2015-12-23 15:01:23 UTC) #18

svillar

Description was changed from ========== Fix computation of min|max-content contribution of non-replaced blocks Currently Blink ...

5 years ago (2015-12-23 15:03:43 UTC) #19

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1504403002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1504403002/100001

5 years ago (2015-12-23 15:05:46 UTC) #21

commit-bot: I haz the power

Description was changed from ========== Fix computation of min|max-content contribution of non-replaced blocks Currently Blink ...

5 years ago (2015-12-23 16:30:01 UTC) #22

commit-bot: I haz the power

Description was changed from ========== Fix computation of min|max-content contribution of non-replaced blocks Currently Blink ...

5 years ago (2015-12-23 16:31:29 UTC) #24

commit-bot: I haz the power

5 years ago (2015-12-23 16:31:30 UTC) #25

Message was sent while issue was closed.

Patchset 6 (id:??) landed as
https://crrev.com/f39acf142b5781ea8ab5da01e7685efbf7b43fee
Cr-Commit-Position: refs/heads/master@{#366753}

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages