[LayoutNG] Add min/max-{width,height} to instrinics check for abs-pos

LayoutNG] Add min/max-{width,height} to instrinics check for abs-pos

Fixes failures for:
fast/css-intrinsic-dimensions/intrinsic-sized-absolutes.html
fast/css-intrinsic-dimensions/intrinsic-sized-replaced-absolutes.html

E.g. when min-width etc, are set to an intrinsic.
<iframe style="min-width: min-content;'></iframe>

See:
https://storage.googleapis.com/chromium-layout-test-archives/linux_layout_tests_layout_ng/622/layout-test-results/results.html

BUG=635619
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng

Review-Url: https://codereview.chromium.org/2969433002
Cr-Commit-Position: refs/heads/master@{#483882}
Committed: https://chromium.googlesource.com/chromium/src/+/8264d60e9d2885b2cd657afff45be608c64d7410

[ikilpatrick, 2017-06-29 15:54:36]

Description was changed from

==========
[LayoutNG] Add min/max-{width,height} to instrinics check for abs-pos

BUG=635619
==========

to

==========
[LayoutNG] Add min/max-{width,height} to instrinics check for abs-pos

BUG=635619
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[ikilpatrick, 2017-06-29 15:54:42]

The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 15:54:57]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[ianjkilpatrick, 2017-06-29 15:59:21]

ianjkilpatrick@gmail.com changed reviewers:
+ cbiesinger@chromium.org, eae@chromium.org, ianjkilpatrick@gmail.com

[commit-bot: I haz the power, 2017-06-29 18:05:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-29 18:05:55]

Dry run: This issue passed the CQ dry run.

[ikilpatrick, 2017-06-29 18:12:38]

ikilpatrick@chromium.org changed reviewers:
- ianjkilpatrick@gmail.com

[ikilpatrick, 2017-06-29 18:12:38]

gah wrong account...

[cbiesinger, 2017-06-29 20:40:01]

https://codereview.chromium.org/2969433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right):

https://codereview.chromium.org/2969433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:18: return
style.MinWidth().IsIntrinsic() || style.MaxWidth().IsIntrinsic() ||
Why does that work? Don't we need to compute the min/max content size when min
or max width is intrinsic?

What problem does this fix? Can you expand on the CL description? :)

[ikilpatrick, 2017-06-29 22:44:07]

Description was changed from

==========
[LayoutNG] Add min/max-{width,height} to instrinics check for abs-pos

BUG=635619
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
LayoutNG] Add min/max-{width,height} to instrinics check for abs-pos

Fixes failures for:
fast/css-intrinsic-dimensions/intrinsic-sized-absolutes.html
fast/css-intrinsic-dimensions/intrinsic-sized-replaced-absolutes.html

E.g. when min-width etc, are set to an intrinsic.
<iframe style="min-width: min-content;'></iframe>

See:
https://storage.googleapis.com/chromium-layout-test-archives/linux_layout_tes...

BUG=635619
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

[ikilpatrick, 2017-06-29 22:45:07]

https://codereview.chromium.org/2969433002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc (right):

https://codereview.chromium.org/2969433002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/ng/ng_absolute_utils.cc:18: return
style.MinWidth().IsIntrinsic() || style.MaxWidth().IsIntrinsic() ||
On 2017/06/29 20:40:01, cbiesinger wrote:
> Why does that work? Don't we need to compute the min/max content size when min
> or max width is intrinsic?
> 
> What problem does this fix? Can you expand on the CL description? :)

I've changed this so its a little more obvious... it leaves these functions
alone, but have a look at the ng_out_of_flow_part.cc

https://codereview.chromium.org/2969433002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc (right):

https://codereview.chromium.org/2969433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc:25: return
constraint_space.IsShrinkToFit() || NeedMinMaxContentSize(style);
I want to try a patch which uses the "forced" sizing for inflow elements which
are "inline-size: auto". And "auto" just becomes ShrinkToFit - thoughts?

[ikilpatrick, 2017-06-29 23:00:29]

The CQ bit was checked by ikilpatrick@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-29 23:00:46]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-30 01:10:47]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-30 01:10:48]

Dry run: This issue passed the CQ dry run.

[cbiesinger, 2017-06-30 22:34:38]

lgtm. I think I previously misread your patch :( This looks good though.

https://codereview.chromium.org/2969433002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc (right):

https://codereview.chromium.org/2969433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc:25: return
constraint_space.IsShrinkToFit() || NeedMinMaxContentSize(style);
On 2017/06/29 22:45:07, ikilpatrick wrote:
> I want to try a patch which uses the "forced" sizing for inflow elements which
> are "inline-size: auto". And "auto" just becomes ShrinkToFit - thoughts?

Sorry, what do you mean with forced sizing? I don't think I follow.

[ikilpatrick, 2017-06-30 22:58:50]

https://codereview.chromium.org/2969433002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc (right):

https://codereview.chromium.org/2969433002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc:25: return
constraint_space.IsShrinkToFit() || NeedMinMaxContentSize(style);
On 2017/06/30 22:34:38, cbiesinger wrote:
> On 2017/06/29 22:45:07, ikilpatrick wrote:
> > I want to try a patch which uses the "forced" sizing for inflow elements
which
> > are "inline-size: auto". And "auto" just becomes ShrinkToFit - thoughts?
> 
> Sorry, what do you mean with forced sizing? I don't think I follow.

As in:

// Within block layout algorithm:
if (is_inflow && inline_size.IsAuto()) {
  space_builder.SetAvailableInlineSize(available_size - child_inline_margins);
  space_builder.SetIsFixedSizeInline(true); // will fix child to available
width.
}

[ikilpatrick, 2017-06-30 22:59:30]

The CQ bit was checked by ikilpatrick@chromium.org

[commit-bot: I haz the power, 2017-06-30 22:59:43]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cbiesinger, 2017-06-30 23:06:58]

On Jun 30, 2017 18:58, <ikilpatrick@chromium.org> wrote:


https://codereview.chromium.org/2969433002/diff/20001/
third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc
File third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc
(right):

https://codereview.chromium.org/2969433002/diff/20001/
third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc#newcode25
third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc:25: return
constraint_space.IsShrinkToFit() || NeedMinMaxContentSize(style);
On 2017/06/30 22:34:38, cbiesinger wrote:
> On 2017/06/29 22:45:07, ikilpatrick wrote:
> > I want to try a patch which uses the "forced" sizing for inflow
elements which
> > are "inline-size: auto". And "auto" just becomes ShrinkToFit -
thoughts?
>
> Sorry, what do you mean with forced sizing? I don't think I follow.

As in:

// Within block layout algorithm:
if (is_inflow && inline_size.IsAuto()) {
space_builder.SetAvailableInlineSize(available_size -
child_inline_margins);
space_builder.SetIsFixedSizeInline(true); // will fix child to
available width.
}

https://codereview.chromium.org/2969433002/


Oh interesting... I'm wondering if shrink to fit is the most common desired
behavior for layout algorithms. That may be a good idea!

-- 
You received this message because you are subscribed to the Google Groups "Blink
Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.

[cbiesinger, 2017-06-30 23:06:59]

On Jun 30, 2017 18:58, <ikilpatrick@chromium.org> wrote:


https://codereview.chromium.org/2969433002/diff/20001/
third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc
File third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc
(right):

https://codereview.chromium.org/2969433002/diff/20001/
third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc#newcode25
third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc:25: return
constraint_space.IsShrinkToFit() || NeedMinMaxContentSize(style);
On 2017/06/30 22:34:38, cbiesinger wrote:
> On 2017/06/29 22:45:07, ikilpatrick wrote:
> > I want to try a patch which uses the "forced" sizing for inflow
elements which
> > are "inline-size: auto". And "auto" just becomes ShrinkToFit -
thoughts?
>
> Sorry, what do you mean with forced sizing? I don't think I follow.

As in:

// Within block layout algorithm:
if (is_inflow && inline_size.IsAuto()) {
space_builder.SetAvailableInlineSize(available_size -
child_inline_margins);
space_builder.SetIsFixedSizeInline(true); // will fix child to
available width.
}

https://codereview.chromium.org/2969433002/


Oh interesting... I'm wondering if shrink to fit is the most common desired
behavior for layout algorithms. That may be a good idea!

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[ikilpatrick, 2017-06-30 23:21:38]

On 2017/06/30 23:06:59, cbiesinger wrote:
> On Jun 30, 2017 18:58, <mailto:ikilpatrick@chromium.org> wrote:
> 
> 
> https://codereview.chromium.org/2969433002/diff/20001/
> third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc
> File third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc
> (right):
> 
> https://codereview.chromium.org/2969433002/diff/20001/
> third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc#newcode25
> third_party/WebKit/Source/core/layout/ng/ng_length_utils.cc:25: return
> constraint_space.IsShrinkToFit() || NeedMinMaxContentSize(style);
> On 2017/06/30 22:34:38, cbiesinger wrote:
> > On 2017/06/29 22:45:07, ikilpatrick wrote:
> > > I want to try a patch which uses the "forced" sizing for inflow
> elements which
> > > are "inline-size: auto". And "auto" just becomes ShrinkToFit -
> thoughts?
> >
> > Sorry, what do you mean with forced sizing? I don't think I follow.
> 
> As in:
> 
> // Within block layout algorithm:
> if (is_inflow && inline_size.IsAuto()) {
> space_builder.SetAvailableInlineSize(available_size -
> child_inline_margins);
> space_builder.SetIsFixedSizeInline(true); // will fix child to
> available width.
> }
> 
> https://codereview.chromium.org/2969433002/
> 
> 
> Oh interesting... I'm wondering if shrink to fit is the most common desired
> behavior for layout algorithms. That may be a good idea!
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.

Yeah I realized that auto is really only special for block level blocks, so it
might be simpler?
I'll throw together a patch, and we can have a look. :)

[commit-bot: I haz the power, 2017-07-01 00:58:15]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1498863570561050,
"parent_rev": "4bc26323b196f3b8957e85fc1eb48c08456fc84d", "commit_rev":
"8264d60e9d2885b2cd657afff45be608c64d7410"}

[commit-bot: I haz the power, 2017-07-01 00:58:29]

Description was changed from

==========
LayoutNG] Add min/max-{width,height} to instrinics check for abs-pos

Fixes failures for:
fast/css-intrinsic-dimensions/intrinsic-sized-absolutes.html
fast/css-intrinsic-dimensions/intrinsic-sized-replaced-absolutes.html

E.g. when min-width etc, are set to an intrinsic.
<iframe style="min-width: min-content;'></iframe>

See:
https://storage.googleapis.com/chromium-layout-test-archives/linux_layout_tes...

BUG=635619
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng
==========

to

==========
LayoutNG] Add min/max-{width,height} to instrinics check for abs-pos

Fixes failures for:
fast/css-intrinsic-dimensions/intrinsic-sized-absolutes.html
fast/css-intrinsic-dimensions/intrinsic-sized-replaced-absolutes.html

E.g. when min-width etc, are set to an intrinsic.
<iframe style="min-width: min-content;'></iframe>

See:
https://storage.googleapis.com/chromium-layout-test-archives/linux_layout_tes...

BUG=635619
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_layout_ng

Review-Url: https://codereview.chromium.org/2969433002
Cr-Commit-Position: refs/heads/master@{#483882}
Committed:
https://chromium.googlesource.com/chromium/src/+/8264d60e9d2885b2cd657afff45b...
==========

[commit-bot: I haz the power, 2017-07-01 00:58:30]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/8264d60e9d2885b2cd657afff45b...