Ensure that we consistently check contains: paint for fixed position containment.

Ensure that we consistently check contains: paint for fixed position containment.

contains: paint; should contain fixed position descendants, however we had only
updated this check in one place while we have many other places in the code
which simply checked for having transform related properties. This patch
attempts to unify all of these code paths to call through
ComputedStyle::canContainFixedPositionObjects.

TEST=fast/css/containment/paint-containment-with-fixed-position-scrolled.html, LayoutGeometryMapTest.ContainsFixedPositionTest, MapCoordinatesTest.FixedPosInTransform, MapCoordinatesTest.FixedPosInContainPaint
BUG=619999
Committed: https://crrev.com/139b50e91d0d86ad4ee31957a003dad264f38435
Cr-Commit-Position: refs/heads/master@{#408002}

[flackr, 2016-07-22 18:21:47]

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

[commit-bot: I haz the power, 2016-07-22 18:22:08]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[flackr, 2016-07-22 18:28:29]

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

[flackr, 2016-07-22 18:28:30]

Hi Chris,

Can you take a look at this patch? Paint containment is supposed to contain
fixed position descendants (and this was already updated in
ComputedStyle::canContainFixedPositionObjects) but we just checked for
hasTransformRelatedProperty in a lot of other places. This updates those other
places and adds a test that it is properly contained.

https://codereview.chromium.org/2173963002/diff/1/third_party/WebKit/Source/c...
File third_party/WebKit/Source/core/paint/PaintLayer.cpp (left):

https://codereview.chromium.org/2173963002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/paint/PaintLayer.cpp:849: static inline bool
isContainerForFixedPositioned(PaintLayer* layer)
Seems like these methods are just asking to get out of sync with LayoutObject's
methods. How about we try just calling into layoutObject, especially considering
we need to to check both fixed and positioned now anyways?

[commit-bot: I haz the power, 2016-07-22 18:49:29]

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

[commit-bot: I haz the power, 2016-07-22 18:49:30]

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

[chrishtr, 2016-07-22 20:38:22]

Looks great! A nice cleanup in addition to the bug fix.

Could you add unittests for mapLocalToAncestor, mapAncestorToLocal, and
LayoutGeometryMap,
for the contains:paint use case? (and transform while you're at it if that is
not already
unittested).

[flackr, 2016-07-25 18:27:36]

Description was changed from

==========
Ensure that we consistently check contains: paint for fixed position
containment.

contains: paint; should contain fixed position descendants, however we had only
updated this check in one place while we have many other places in the code
which simply checked for having transform related properties. This patch
attempts to unify all of these code paths to call through
ComputedStyle::canContainFixedPositionObjects.

TEST=fast/css/containment/paint-containment-with-fixed-position-scrolled.html
BUG=619999
==========

to

==========
Ensure that we consistently check contains: paint for fixed position
containment.

contains: paint; should contain fixed position descendants, however we had only
updated this check in one place while we have many other places in the code
which simply checked for having transform related properties. This patch
attempts to unify all of these code paths to call through
ComputedStyle::canContainFixedPositionObjects.

TEST=fast/css/containment/paint-containment-with-fixed-position-scrolled.html,
LayoutGeometryMapTest.ContainsFixedPositionTest
BUG=619999
==========

[flackr, 2016-07-25 21:00:04]

Description was changed from

==========
Ensure that we consistently check contains: paint for fixed position
containment.

contains: paint; should contain fixed position descendants, however we had only
updated this check in one place while we have many other places in the code
which simply checked for having transform related properties. This patch
attempts to unify all of these code paths to call through
ComputedStyle::canContainFixedPositionObjects.

TEST=fast/css/containment/paint-containment-with-fixed-position-scrolled.html,
LayoutGeometryMapTest.ContainsFixedPositionTest
BUG=619999
==========

to

==========
Ensure that we consistently check contains: paint for fixed position
containment.

contains: paint; should contain fixed position descendants, however we had only
updated this check in one place while we have many other places in the code
which simply checked for having transform related properties. This patch
attempts to unify all of these code paths to call through
ComputedStyle::canContainFixedPositionObjects.

TEST=fast/css/containment/paint-containment-with-fixed-position-scrolled.html,
LayoutGeometryMapTest.ContainsFixedPositionTest,
MapCoordinatesTest.FixedPosInTransform,
MapCoordinatesTest.FixedPosInContainPaint
BUG=619999
==========

[flackr, 2016-07-25 21:00:31]

On 2016/07/22 at 20:38:22, chrishtr wrote:
> Looks great! A nice cleanup in addition to the bug fix.
> 
> Could you add unittests for mapLocalToAncestor, mapAncestorToLocal, and
LayoutGeometryMap,
> for the contains:paint use case? (and transform while you're at it if that is
not already
> unittested).

Done. Added tests for both cases for all of these.

[flackr, 2016-07-25 21:00:53]

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

[commit-bot: I haz the power, 2016-07-25 21:02:01]

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, 2016-07-25 21:11:43]

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

[commit-bot: I haz the power, 2016-07-25 21:11:44]

Dry run: Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[flackr, 2016-07-25 21:49:44]

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

[commit-bot: I haz the power, 2016-07-25 21:50:09]

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, 2016-07-25 23:05:05]

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

[commit-bot: I haz the power, 2016-07-25 23:05:05]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2016-07-26 18:11:03]

https://codereview.chromium.org/2173963002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/MapCoordinatesTest.cpp (right):

https://codereview.chromium.org/2173963002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/MapCoordinatesTest.cpp:509: mappedPoint =
mapAncestorToLocal(target, view, FloatPoint(0, 100));
Shouldn't this be 150px? (including scroll)

[flackr, 2016-07-26 21:06:51]

https://codereview.chromium.org/2173963002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/MapCoordinatesTest.cpp (right):

https://codereview.chromium.org/2173963002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/MapCoordinatesTest.cpp:509: mappedPoint =
mapAncestorToLocal(target, view, FloatPoint(0, 100));
On 2016/07/26 at 18:11:03, chrishtr wrote:
> Shouldn't this be 150px? (including scroll)

No, the fixed position #target doesn't move when you scroll so the #target is
anchored to the top of the #container (just as with fixed elements contained by
transforms).

[chrishtr, 2016-07-26 21:49:11]

https://codereview.chromium.org/2173963002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/MapCoordinatesTest.cpp (right):

https://codereview.chromium.org/2173963002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/MapCoordinatesTest.cpp:509: mappedPoint =
mapAncestorToLocal(target, view, FloatPoint(0, 100));
On 2016/07/26 at 21:06:51, flackr wrote:
> On 2016/07/26 at 18:11:03, chrishtr wrote:
> > Shouldn't this be 150px? (including scroll)
> 
> No, the fixed position #target doesn't move when you scroll so the #target is
anchored to the top of the #container (just as with fixed elements contained by
transforms).

But #container should move when the view is scrolled, no? And you're mapping to
view space, not container space.

[flackr, 2016-07-26 22:36:11]

https://codereview.chromium.org/2173963002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/MapCoordinatesTest.cpp (right):

https://codereview.chromium.org/2173963002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/MapCoordinatesTest.cpp:509: mappedPoint =
mapAncestorToLocal(target, view, FloatPoint(0, 100));
On 2016/07/26 at 21:49:11, chrishtr wrote:
> On 2016/07/26 at 21:06:51, flackr wrote:
> > On 2016/07/26 at 18:11:03, chrishtr wrote:
> > > Shouldn't this be 150px? (including scroll)
> > 
> > No, the fixed position #target doesn't move when you scroll so the #target
is anchored to the top of the #container (just as with fixed elements contained
by transforms).
> 
> But #container should move when the view is scrolled, no? And you're mapping
to view space, not container space.

view (the LayoutView) doesn't seem to refer to the viewport for the purposes of
mapping, you can tell by the container still being 100px down from the view
(despite being absolute position). I've also tested that other fixed position
items do in fact move down. Perhaps if I map to nullptr like some of the other
tests I'd get the offset from the viewport?

[chrishtr, 2016-07-26 23:24:56]

lgtm

https://codereview.chromium.org/2173963002/diff/60001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/MapCoordinatesTest.cpp (right):

https://codereview.chromium.org/2173963002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/MapCoordinatesTest.cpp:509: mappedPoint =
mapAncestorToLocal(target, view, FloatPoint(0, 100));
On 2016/07/26 at 22:36:11, flackr wrote:
> On 2016/07/26 at 21:49:11, chrishtr wrote:
> > On 2016/07/26 at 21:06:51, flackr wrote:
> > > On 2016/07/26 at 18:11:03, chrishtr wrote:
> > > > Shouldn't this be 150px? (including scroll)
> > > 
> > > No, the fixed position #target doesn't move when you scroll so the #target
is anchored to the top of the #container (just as with fixed elements contained
by transforms).
> > 
> > But #container should move when the view is scrolled, no? And you're mapping
to view space, not container space.
> 
> view (the LayoutView) doesn't seem to refer to the viewport for the purposes
of mapping, you can tell by the container still being 100px down from the view
(despite being absolute position). I've also tested that other fixed position
items do in fact move down. Perhaps if I map to nullptr like some of the other
tests I'd get the offset from the viewport?

Forgot that the view is currently special and doesn't add in scroll offset.

[flackr, 2016-07-26 23:29:08]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2016-07-26 23:29:31]

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, 2016-07-27 01:12:18]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-27 01:16:41]

Description was changed from

==========
Ensure that we consistently check contains: paint for fixed position
containment.

contains: paint; should contain fixed position descendants, however we had only
updated this check in one place while we have many other places in the code
which simply checked for having transform related properties. This patch
attempts to unify all of these code paths to call through
ComputedStyle::canContainFixedPositionObjects.

TEST=fast/css/containment/paint-containment-with-fixed-position-scrolled.html,
LayoutGeometryMapTest.ContainsFixedPositionTest,
MapCoordinatesTest.FixedPosInTransform,
MapCoordinatesTest.FixedPosInContainPaint
BUG=619999
==========

to

==========
Ensure that we consistently check contains: paint for fixed position
containment.

contains: paint; should contain fixed position descendants, however we had only
updated this check in one place while we have many other places in the code
which simply checked for having transform related properties. This patch
attempts to unify all of these code paths to call through
ComputedStyle::canContainFixedPositionObjects.

TEST=fast/css/containment/paint-containment-with-fixed-position-scrolled.html,
LayoutGeometryMapTest.ContainsFixedPositionTest,
MapCoordinatesTest.FixedPosInTransform,
MapCoordinatesTest.FixedPosInContainPaint
BUG=619999

Committed: https://crrev.com/139b50e91d0d86ad4ee31957a003dad264f38435
Cr-Commit-Position: refs/heads/master@{#408002}
==========

[commit-bot: I haz the power, 2016-07-27 01:16:43]

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