[css-flexbox] Implement new abspos handling

[css-flexbox] Implement new abspos handling

The spec no longer requires us to position abspos items as if they
were 0x0 flex items. Instead, they should be positioned as if they were
the sole flex item in the flexbox, i.e. they should respect the align-items
and justify-content properties:
https://drafts.csswg.org/css-flexbox/#abspos-items

Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/MMdPt_5omT4/discussion

BUG=517265
Committed: https://crrev.com/b80c579439ab737f82cddb5c73c91bc1daacd354
Cr-Commit-Position: refs/heads/master@{#390154}

[cbiesinger, 2016-04-23 00:04:13]

Please take a look!

[cbiesinger, 2016-04-23 00:04:30]

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

[commit-bot: I haz the power, 2016-04-23 00:04:57]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1920453003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1920453003/20001

[commit-bot: I haz the power, 2016-04-23 03:34:33]

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

[commit-bot: I haz the power, 2016-04-23 03:34:34]

Dry run: This issue passed the CQ dry run.

[eae, 2016-04-25 02:37:27]

eae@chromium.org changed reviewers:
+ eae@chromium.org

[eae, 2016-04-25 02:37:29]

https://codereview.chromium.org/1920453003/diff/20001/third_party/WebKit/Layo...
File third_party/WebKit/LayoutTests/css3/flexbox/position-absolute-child.html
(right):

https://codereview.chromium.org/1920453003/diff/20001/third_party/WebKit/Layo...
third_party/WebKit/LayoutTests/css3/flexbox/position-absolute-child.html:85: //
crossAxisOffset is used for the absolute-positioned child.
Thanks for adding this description, really helps!

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

https://codereview.chromium.org/1920453003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1228: bool relaidOut =
false;
How about needsLayout or layoutChanged instead?

https://codereview.chromium.org/1920453003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/1920453003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1333: switch
(distribution) {
Probably easier to use a regular if statement here.

if (distribution == ContentDistributionSpaceAround)
  return (mainAxisExtent - mainAxisExtentForChild(child)) / 2;

https://codereview.chromium.org/1920453003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1386: if
(isColumnFlow())
(optional) There is a bit of code duplication here, how about having a helper
for getting the resolvedAxisPosition or using an iif-statement?

LayoutUnit LayoutFlexibleBox::resolvedAxisPosition(...)
{
  return isColumnFlow()
    ? staticCrossAxisPositionForPositionedChild(child)
    : staticMainAxisPositionForPositionedChild(child);
}

LayoutFlexibleBox::staticInlinePositionForPositionedChild(...)
{
  ...
  return staticInlineOffset + resolvedAxisPosition(child);
}


or

return staticInlineOffset + isColumnFlow()
  ? staticCrossAxisPositionForPositionedChild(child)
  : staticMainAxisPositionForPositionedChild(child);



Again, this is just a suggestion and only do it if you think it makes it simpler
or reduces code duplication.

https://codereview.chromium.org/1920453003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h (right):

https://codereview.chromium.org/1920453003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h:79: 
Having a setter return a bool is a bit unusual. You might want to add a comment
explaining the return value.

[cbiesinger, 2016-04-26 18:21:56]

Emil, please take another look!

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

https://codereview.chromium.org/1920453003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:1228: bool relaidOut =
false;
On 2016/04/25 02:37:28, eae wrote:
> How about needsLayout or layoutChanged instead?

Done, though I'm not certain I like this one either :-)

https://codereview.chromium.org/1920453003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp (right):

https://codereview.chromium.org/1920453003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1333: switch
(distribution) {
On 2016/04/25 02:37:28, eae wrote:
> Probably easier to use a regular if statement here.
> 
> if (distribution == ContentDistributionSpaceAround)
>   return (mainAxisExtent - mainAxisExtentForChild(child)) / 2;

I realized I can simplify this by reusing code, so this is quite different now.

Can't do it easily for cross axis though :(

https://codereview.chromium.org/1920453003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.cpp:1386: if
(isColumnFlow())
On 2016/04/25 02:37:28, eae wrote:
> (optional) There is a bit of code duplication here, how about having a helper
> for getting the resolvedAxisPosition or using an iif-statement?
> 
> LayoutUnit LayoutFlexibleBox::resolvedAxisPosition(...)
> {
>   return isColumnFlow()
>     ? staticCrossAxisPositionForPositionedChild(child)
>     : staticMainAxisPositionForPositionedChild(child);
> }
> 
> LayoutFlexibleBox::staticInlinePositionForPositionedChild(...)
> {
>   ...
>   return staticInlineOffset + resolvedAxisPosition(child);
> }
> 
> 
> or
> 
> return staticInlineOffset + isColumnFlow()
>   ? staticCrossAxisPositionForPositionedChild(child)
>   : staticMainAxisPositionForPositionedChild(child);
> 
> 
> 
> Again, this is just a suggestion and only do it if you think it makes it
simpler
> or reduces code duplication.

I like the second idea, thanks! Changed, modulo some parentheses required by the
C++ grammar (thanks clang for pointing that out)

https://codereview.chromium.org/1920453003/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h (right):

https://codereview.chromium.org/1920453003/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutFlexibleBox.h:79: 
On 2016/04/25 02:37:28, eae wrote:
> Having a setter return a bool is a bit unusual. You might want to add a
comment
> explaining the return value.

Done.

[cbiesinger, 2016-04-26 21:03:30]

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

[commit-bot: I haz the power, 2016-04-26 21:04:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1920453003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1920453003/60001

[cbiesinger, 2016-04-26 21:05:07]

So I just created https://codereview.chromium.org/1923103002/ and rebased this
one on top of that, and I hoped that rietveld would notice the dependency.
Nope... this CL now contains that one too :/ Please review independently.
Thanks!

[commit-bot: I haz the power, 2016-04-26 22:19:29]

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

[commit-bot: I haz the power, 2016-04-26 22:19:30]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[eae, 2016-04-27 00:01:34]

LGTM, thanks@!

[cbiesinger, 2016-04-27 16:39:56]

The CQ bit was checked by cbiesinger@chromium.org

[cbiesinger, 2016-04-27 16:39:57]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org
Link to the patchset: https://codereview.chromium.org/1920453003/#ps100001
(title: "fix test")

[commit-bot: I haz the power, 2016-04-27 16:40:21]

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

[cbiesinger, 2016-04-27 19:20:16]

Description was changed from

==========
[css-flexbox] Implement new abspos handling

The spec no longer requires us to position abspos items as if they
were 0x0 flex items. Instead, they should be positioned as if they were
the sole flex item in the flexbox, i.e. they should respect the align-items
and justify-content properties:
https://drafts.csswg.org/css-flexbox/#abspos-items

Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/MMdPt_5omT4/discus...

R=leviw@chromium.org
BUG=517265
==========

to

==========
[css-flexbox] Implement new abspos handling

The spec no longer requires us to position abspos items as if they
were 0x0 flex items. Instead, they should be positioned as if they were
the sole flex item in the flexbox, i.e. they should respect the align-items
and justify-content properties:
https://drafts.csswg.org/css-flexbox/#abspos-items

Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/MMdPt_5omT4/discus...

BUG=517265
==========

[commit-bot: I haz the power, 2016-04-27 19:53:45]

Description was changed from

==========
[css-flexbox] Implement new abspos handling

The spec no longer requires us to position abspos items as if they
were 0x0 flex items. Instead, they should be positioned as if they were
the sole flex item in the flexbox, i.e. they should respect the align-items
and justify-content properties:
https://drafts.csswg.org/css-flexbox/#abspos-items

Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/MMdPt_5omT4/discus...

BUG=517265
==========

to

==========
[css-flexbox] Implement new abspos handling

The spec no longer requires us to position abspos items as if they
were 0x0 flex items. Instead, they should be positioned as if they were
the sole flex item in the flexbox, i.e. they should respect the align-items
and justify-content properties:
https://drafts.csswg.org/css-flexbox/#abspos-items

Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/MMdPt_5omT4/discus...

BUG=517265
==========

[commit-bot: I haz the power, 2016-04-27 19:53:51]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-04-30 17:12:05]

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

[commit-bot: I haz the power, 2016-04-30 17:51:21]

Description was changed from

==========
[css-flexbox] Implement new abspos handling

The spec no longer requires us to position abspos items as if they
were 0x0 flex items. Instead, they should be positioned as if they were
the sole flex item in the flexbox, i.e. they should respect the align-items
and justify-content properties:
https://drafts.csswg.org/css-flexbox/#abspos-items

Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/MMdPt_5omT4/discus...

BUG=517265
==========

to

==========
[css-flexbox] Implement new abspos handling

The spec no longer requires us to position abspos items as if they
were 0x0 flex items. Instead, they should be positioned as if they were
the sole flex item in the flexbox, i.e. they should respect the align-items
and justify-content properties:
https://drafts.csswg.org/css-flexbox/#abspos-items

Intent to implement and ship:
https://groups.google.com/a/chromium.org/d/topic/blink-dev/MMdPt_5omT4/discus...

BUG=517265

Committed: https://crrev.com/b80c579439ab737f82cddb5c73c91bc1daacd354
Cr-Commit-Position: refs/heads/master@{#390154}
==========