Change CSS containment should invalidate layout

Change CSS containment should invalidate layout

This CL does two things:
1. Changing CSS contain property requires a full layout. We can potentially
   do more fine-grained invalidation, but containment changes are expected to
   be rare.

2. Amend positioned descendants list update code to mirror the behavior of
   LayoutObject::canContainFixedPositionObjects().

BUG=619864
Committed: https://crrev.com/1201e5870ce0bcbb9e31518fcbc2ac2635d07187
Cr-Commit-Position: refs/heads/master@{#400899}

[trchen, 2016-06-15 21:43:49]

trchen@chromium.org changed reviewers:
+ pdr@chromium.org

[trchen, 2016-06-15 21:43:57]

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

[commit-bot: I haz the power, 2016-06-15 21:44:22]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2072473003/1

[commit-bot: I haz the power, 2016-06-16 01:12:11]

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

[commit-bot: I haz the power, 2016-06-16 01:12:13]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2016-06-16 04:31:06]

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

[chrishtr, 2016-06-16 04:31:06]

Thanks for the quick fix.

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

https://codereview.chromium.org/2072473003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:147: bool
oldStyleContainsFixedPosition = oldStyle->hasTransformRelatedProperty() ||
oldStyle->containsPaint();
Factor hasTransformRelatedProperty() || oldStyle->containsPaint() into a helper
method, with a style as parameter, used here and in
canContainFixedPositionObjects.

https://codereview.chromium.org/2072473003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:150: bool
oldStyleContainsAbsolutePosition = oldStyle->position() != StaticPosition ||
oldStyleContainsFixedPosition;
Same here.

https://codereview.chromium.org/2072473003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:200: // In
LayoutObject::styleWillChange() we already removed ourself from our old containg
Typo: "containg"

[trchen, 2016-06-16 22:09:53]

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

https://codereview.chromium.org/2072473003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:147: bool
oldStyleContainsFixedPosition = oldStyle->hasTransformRelatedProperty() ||
oldStyle->containsPaint();
On 2016/06/16 04:31:06, chrishtr wrote:
> Factor hasTransformRelatedProperty() || oldStyle->containsPaint() into a
helper
> method, with a style as parameter, used here and in
> canContainFixedPositionObjects.

Done.

https://codereview.chromium.org/2072473003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:150: bool
oldStyleContainsAbsolutePosition = oldStyle->position() != StaticPosition ||
oldStyleContainsFixedPosition;
On 2016/06/16 04:31:06, chrishtr wrote:
> Same here.

Done.

https://codereview.chromium.org/2072473003/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBlock.cpp:200: // In
LayoutObject::styleWillChange() we already removed ourself from our old containg
On 2016/06/16 04:31:06, chrishtr wrote:
> Typo: "containg"

Done.

[trchen, 2016-06-16 22:10:00]

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

[commit-bot: I haz the power, 2016-06-16 22:11:28]

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

[commit-bot: I haz the power, 2016-06-16 23:50:14]

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

[commit-bot: I haz the power, 2016-06-16 23:50:16]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[chrishtr, 2016-06-17 06:27:11]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2016-06-17 06:27:12]

lgtm

[commit-bot: I haz the power, 2016-06-17 06:27:31]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2072473003/20001

[commit-bot: I haz the power, 2016-06-17 07:51:11]

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

[commit-bot: I haz the power, 2016-06-17 07:51:12]

Try jobs failed on following builders:
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[trchen, 2016-06-17 23:56:13]

The CQ bit was checked by trchen@chromium.org

[trchen, 2016-06-17 23:56:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
chrishtr@chromium.org
Link to the patchset: https://codereview.chromium.org/2072473003/#ps40001
(title: "fix dcheck (I made a mistake refactoring
LayoutObject::canContainFixedPositionObjects)")

[commit-bot: I haz the power, 2016-06-17 23:56:24]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2072473003/40001

[commit-bot: I haz the power, 2016-06-18 00:49:11]

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

[commit-bot: I haz the power, 2016-06-18 00:49:13]

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_...)

[trchen, 2016-06-20 22:51:58]

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

[commit-bot: I haz the power, 2016-06-20 22:52:31]

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

[trchen, 2016-06-20 22:55:09]

Patch set 4 should be fully equivalent to the original. However the refactor no
longer looks as appealing. :/

The core problem is that only LayoutBlock respects transform. The used value of
transform should be forced to 'none' if it is applied on inline elements. So at
LayoutObject level hasTransformOrRelatedProperty needs to be guarded by
isLayoutBlock, but not for other conditions.

Now it raises a question... Does contain:paint only applicable on blocks as
well?

[chrishtr, 2016-06-20 23:07:22]

On 2016/06/20 at 22:55:09, trchen wrote:
> Patch set 4 should be fully equivalent to the original. However the refactor
no longer looks as appealing. :/
> 
> The core problem is that only LayoutBlock respects transform. The used value
of transform should be forced to 'none' if it is applied on inline elements. So
at LayoutObject level hasTransformOrRelatedProperty needs to be guarded by
isLayoutBlock, but not for other conditions.

If inlines can't be transformed, why do we let them keep transform-related
properties after StyleAdjuster?

> 
> Now it raises a question... Does contain:paint only applicable on blocks as
well?

It does not apply to non-boxes:

https://codereview.chromium.org/1586723003/

Levi brought this to the CSSWG after that code review also, but I can't seem to
see it reflected in the spec.

[trchen, 2016-06-20 23:23:38]

On 2016/06/20 23:07:22, chrishtr wrote:
> On 2016/06/20 at 22:55:09, trchen wrote:
> > Patch set 4 should be fully equivalent to the original. However the refactor
> no longer looks as appealing. :/
> > 
> > The core problem is that only LayoutBlock respects transform. The used value
> of transform should be forced to 'none' if it is applied on inline elements.
So
> at LayoutObject level hasTransformOrRelatedProperty needs to be guarded by
> isLayoutBlock, but not for other conditions.
> 
> If inlines can't be transformed, why do we let them keep transform-related
> properties after StyleAdjuster?

We can adjust them to 'none'. However this way we introduce more discrepancy
between computed style (the one that can be inherited by children and queried by
getComputedStyle API) and ComputedStyle (really is used value).

The issue of computed style and used style has became increasingly problematic,
but so far we don't have a rule of thumb how to deal with them. We currently
have a few patterns:
1. StyleAdjuster adjust style to used value. (Breaking getComputedStyle API and
inheritance in some rare cases.)
2. Keep computed value in ComputedStyle, add query function to derive used value
on the fly. (e.g. https://codereview.chromium.org/2047283002
https://codereview.chromium.org/2065233002/)
3. Do nothing. Subclasses of LayoutObject ignore properties they are not
interested in. Add guards in parent classes if necessarily. (Like isLayoutBlock
&& hasTransform here.)

None of them is ideal, but I think we should unify to a single pattern.

> > Now it raises a question... Does contain:paint only applicable on blocks as
> well?
> 
> It does not apply to non-boxes:
> 
> https://codereview.chromium.org/1586723003/
> 
> Levi brought this to the CSSWG after that code review also, but I can't seem
to
> see it reflected in the spec.

Cool. Then patch set 4 should comply the specs.

[chrishtr, 2016-06-20 23:26:48]

On 2016/06/20 at 23:23:38, trchen wrote:
> On 2016/06/20 23:07:22, chrishtr wrote:
> > On 2016/06/20 at 22:55:09, trchen wrote:
> > > Patch set 4 should be fully equivalent to the original. However the
refactor
> > no longer looks as appealing. :/
> > > 
> > > The core problem is that only LayoutBlock respects transform. The used
value
> > of transform should be forced to 'none' if it is applied on inline elements.
So
> > at LayoutObject level hasTransformOrRelatedProperty needs to be guarded by
> > isLayoutBlock, but not for other conditions.
> > 
> > If inlines can't be transformed, why do we let them keep transform-related
> > properties after StyleAdjuster?
> 
> We can adjust them to 'none'. However this way we introduce more discrepancy
between computed style (the one that can be inherited by children and queried by
getComputedStyle API) and ComputedStyle (really is used value).
> 
> The issue of computed style and used style has became increasingly
problematic, but so far we don't have a rule of thumb how to deal with them. We
currently have a few patterns:
> 1. StyleAdjuster adjust style to used value. (Breaking getComputedStyle API
and inheritance in some rare cases.)
> 2. Keep computed value in ComputedStyle, add query function to derive used
value on the fly. (e.g. https://codereview.chromium.org/2047283002
https://codereview.chromium.org/2065233002/)
> 3. Do nothing. Subclasses of LayoutObject ignore properties they are not
interested in. Add guards in parent classes if necessarily. (Like isLayoutBlock
&& hasTransform here.)
> 
> None of them is ideal, but I think we should unify to a single pattern.

In this case, what does the spec say about whether transform style applies to
inlines? i.e. what should getComputedStyle on an inline with transform in its
CSS return?

> 
> > > Now it raises a question... Does contain:paint only applicable on blocks
as
> > well?
> > 
> > It does not apply to non-boxes:
> > 
> > https://codereview.chromium.org/1586723003/
> > 
> > Levi brought this to the CSSWG after that code review also, but I can't seem
to
> > see it reflected in the spec.
> 
> Cool. Then patch set 4 should comply the specs.

[trchen, 2016-06-20 23:40:01]

On 2016/06/20 23:26:48, chrishtr wrote:
> On 2016/06/20 at 23:23:38, trchen wrote:
> > We can adjust them to 'none'. However this way we introduce more discrepancy
> > between computed style (the one that can be inherited by children and
queried by
> > getComputedStyle API) and ComputedStyle (really is used value).
> 
> In this case, what does the spec say about whether transform style applies to
> inlines? i.e. what should getComputedStyle on an inline with transform in its
> CSS return?

All properties defined in [css-transforms] only apply to "transformable
elements", basically only the things that have a box (i.e. block alike and
inline replaced). Inline elements should ignore transforms specified on them.

According to [CSS2], "The computed value exists even when the property does not
apply", that means getComputedStyle should still return whatever the resolved
value is.

[commit-bot: I haz the power, 2016-06-20 23:57:03]

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

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

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[chrishtr, 2016-06-20 23:58:46]

lgtm

[trchen, 2016-06-21 04:44:42]

The CQ bit was checked by trchen@chromium.org

[commit-bot: I haz the power, 2016-06-21 04:44:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2072473003/60001

[commit-bot: I haz the power, 2016-06-21 05:16:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-06-21 05:18:04]

Description was changed from

==========
Change CSS containment should invalidate layout

This CL does two things:
1. Changing CSS contain property requires a full layout. We can potentially
   do more fine-grained invalidation, but containment changes are expected to
   be rare.

2. Amend positioned descendants list update code to mirror the behavior of
   LayoutObject::canContainFixedPositionObjects().

BUG=619864
==========

to

==========
Change CSS containment should invalidate layout

This CL does two things:
1. Changing CSS contain property requires a full layout. We can potentially
   do more fine-grained invalidation, but containment changes are expected to
   be rare.

2. Amend positioned descendants list update code to mirror the behavior of
   LayoutObject::canContainFixedPositionObjects().

BUG=619864

Committed: https://crrev.com/1201e5870ce0bcbb9e31518fcbc2ac2635d07187
Cr-Commit-Position: refs/heads/master@{#400899}
==========

[commit-bot: I haz the power, 2016-06-21 05:18:06]

Patchset 4 (id:??) landed as
https://crrev.com/1201e5870ce0bcbb9e31518fcbc2ac2635d07187
Cr-Commit-Position: refs/heads/master@{#400899}