Add offset contributed to sticky position box rect by location containers

Add offset contributed to sticky position box rect by location containers

Tables are constructed using locationContainers which do not serve as the
containing block but do offset the element. These need to be included in the
sticky position offsets in the constraints.

BUG=673538
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2646133002
Cr-Commit-Position: refs/heads/master@{#446155}
Committed: https://chromium.googlesource.com/chromium/src/+/686dbbef585fd2064f1a2a3451b90e501936a589

[flackr, 2017-01-20 14:34:40]

Description was changed from

==========
Add offset contributed to sticky position box rect by location containers

Tables are constructed using locationContainers which do not serve as the
containing block but do offset the element. These need to be included in the
sticky position offsets in the constraints.

BUG=673538
==========

to

==========
Add offset contributed to sticky position box rect by location containers

Tables are constructed using locationContainers which do not serve as the
containing block but do offset the element. These need to be included in the
sticky position offsets in the constraints.

BUG=673538
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

[flackr, 2017-01-20 14:37:02]

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

[flackr, 2017-01-20 14:37:02]

We have the incorrect constraint rects on table elements which happened to
cancel itself out on the main thread but matters for the compositing update
because we try to work out the main thread applied offset.

[chrishtr, 2017-01-20 19:20:56]

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

[chrishtr, 2017-01-20 19:20:57]

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

https://codereview.chromium.org/2646133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:776: LayoutBlock*
containingBlock = this->containingBlock();
Sort of unrelated to your CL:

LayoutObject::containingBlock says it returns a non-anonymous block.
Doesn't this make line 787 irrelevant?

https://codereview.chromium.org/2646133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:780: while
(locationContainer && locationContainer != containingBlock) {
I think this and other code in this method may be duplicating logic already
elsewhere in
geometry-computation code. Adding Xianzhu to see if he has insights.

[Xianzhu, 2017-01-20 23:04:01]

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

https://codereview.chromium.org/2646133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:780: while
(locationContainer && locationContainer != containingBlock) {
On 2017/01/20 19:20:57, chrishtr wrote:
> I think this and other code in this method may be duplicating logic already
> elsewhere in
> geometry-computation code. Adding Xianzhu to see if he has insights.

I think localToAncestorPoint should work.

[flackr, 2017-01-23 23:33:43]

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

https://codereview.chromium.org/2646133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:776: LayoutBlock*
containingBlock = this->containingBlock();
On 2017/01/20 19:20:57, chrishtr wrote:
> Sort of unrelated to your CL:
> 
> LayoutObject::containingBlock says it returns a non-anonymous block.
> Doesn't this make line 787 irrelevant?

Hmm, this was definitely necessary at the time (and has a test which should fail
if it's still necessary), I will try removing it.

https://codereview.chromium.org/2646133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:780: while
(locationContainer && locationContainer != containingBlock) {
On 2017/01/20 23:04:01, Xianzhu wrote:
> On 2017/01/20 19:20:57, chrishtr wrote:
> > I think this and other code in this method may be duplicating logic already
> > elsewhere in
> > geometry-computation code. Adding Xianzhu to see if he has insights.
> 
> I think localToAncestorPoint should work.

Sure, although I need a version that excludes transforms like I've done with the
containing block on 818-825.

[Xianzhu, 2017-01-24 00:00:39]

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

https://codereview.chromium.org/2646133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:780: while
(locationContainer && locationContainer != containingBlock) {
On 2017/01/23 23:33:43, flackr wrote:
> On 2017/01/20 23:04:01, Xianzhu wrote:
> > On 2017/01/20 19:20:57, chrishtr wrote:
> > > I think this and other code in this method may be duplicating logic
already
> > > elsewhere in
> > > geometry-computation code. Adding Xianzhu to see if he has insights.
> > 
> > I think localToAncestorPoint should work.
> 
> Sure, although I need a version that excludes transforms like I've done with
the
> containing block on 818-825.

I'm not familiar with the code here. Can you explain why we need to exclude
transforms?

[flackr, 2017-01-24 22:37:12]

I added a new method which allows computing the localToAncestorQuad without
transforms and used that for both the containing box quad and sticky box offset.
PTAL, thanks.

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

https://codereview.chromium.org/2646133002/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:780: while
(locationContainer && locationContainer != containingBlock) {
On 2017/01/24 00:00:39, Xianzhu wrote:
> On 2017/01/23 23:33:43, flackr wrote:
> > On 2017/01/20 23:04:01, Xianzhu wrote:
> > > On 2017/01/20 19:20:57, chrishtr wrote:
> > > > I think this and other code in this method may be duplicating logic
> already
> > > > elsewhere in
> > > > geometry-computation code. Adding Xianzhu to see if he has insights.
> > > 
> > > I think localToAncestorPoint should work.
> > 
> > Sure, although I need a version that excludes transforms like I've done with
> the
> > containing block on 818-825.
> 
> I'm not familiar with the code here. Can you explain why we need to exclude
> transforms?

Sticky position offsets are computed in terms of layout position. If transform
affected it you end up with a lot of interesting and hard to reason about
problems (i.e. how do you stick with scale, rotation, perspective, etc), and it
would need to update in sync with composited animations. This is consistent with
how Firefox and mostly how Safari implements sticky position.

[Xianzhu, 2017-01-24 22:53:11]

Thanks for the explanations.

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

https://codereview.chromium.org/2646133002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2245: FloatQuad
LayoutObject::localToAncestorQuadWithoutTransforms(
I like the direction of the new code, but
  localAncestorQuadWithoutTransforms( ...  UseTransforms)
looks weird.

How about adding 
  mapLocalToAncestorQuadInternal() which contains line 2249-2258, and

... ::localToAncestorQuad(...) {
  return mapLocalToAncestorQuadInternal(  ... | UseTransforms);
}

... ::localToAncestorQuadWithoutTransforms(...) {
  DCHECK(!(mode & UseTransforms));
  return mapLocalToAncestorQuadInternal(...);
}

[flackr, 2017-01-25 13:23:25]

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

https://codereview.chromium.org/2646133002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:2245: FloatQuad
LayoutObject::localToAncestorQuadWithoutTransforms(
On 2017/01/24 22:53:11, Xianzhu wrote:
> I like the direction of the new code, but
>   localAncestorQuadWithoutTransforms( ...  UseTransforms)
> looks weird.
> 
> How about adding 
>   mapLocalToAncestorQuadInternal() which contains line 2249-2258, and
> 
> ... ::localToAncestorQuad(...) {
>   return mapLocalToAncestorQuadInternal(  ... | UseTransforms);
> }
> 
> ... ::localToAncestorQuadWithoutTransforms(...) {
>   DCHECK(!(mode & UseTransforms));
>   return mapLocalToAncestorQuadInternal(...);
> }

Done.

[flackr, 2017-01-25 14:39:15]

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

[commit-bot: I haz the power, 2017-01-25 14:39:31]

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-01-25 17:02:12]

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

[commit-bot: I haz the power, 2017-01-25 17:02:13]

Dry run: This issue passed the CQ dry run.

[Xianzhu, 2017-01-25 18:52:27]

lgtm

https://codereview.chromium.org/2646133002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp (right):

https://codereview.chromium.org/2646133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp:273:
ASSERT_EQ(IntRect(0, 0, 50, 100),
Nit: I prefer EXPECT_EQ to ASSERT_EQ for non-fatal checks.

[flackr, 2017-01-25 18:56:35]

https://codereview.chromium.org/2646133002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp (right):

https://codereview.chromium.org/2646133002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObjectTest.cpp:273:
ASSERT_EQ(IntRect(0, 0, 50, 100),
On 2017/01/25 18:52:26, Xianzhu wrote:
> Nit: I prefer EXPECT_EQ to ASSERT_EQ for non-fatal checks.

Done.

[flackr, 2017-01-25 18:56:57]

The CQ bit was checked by flackr@chromium.org

[flackr, 2017-01-25 18:56:58]

The patchset sent to the CQ was uploaded after l-g-t-m from
wangxianzhu@chromium.org
Link to the patchset: https://codereview.chromium.org/2646133002/#ps60001
(title: "Merge with master and use EXPECT_EQ for non-fatal checks.")

[commit-bot: I haz the power, 2017-01-25 18:57:24]

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-01-25 21:07:31]

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

[commit-bot: I haz the power, 2017-01-25 21:07:33]

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

[flackr, 2017-01-25 21:25:46]

The CQ bit was checked by flackr@chromium.org

[commit-bot: I haz the power, 2017-01-25 21:26:34]

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-01-25 23:07:48]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1485379546830900,
"parent_rev": "1abacdaff61d7a069304698a3ece0039491a44df", "commit_rev":
"686dbbef585fd2064f1a2a3451b90e501936a589"}

[commit-bot: I haz the power, 2017-01-25 23:08:55]

Description was changed from

==========
Add offset contributed to sticky position box rect by location containers

Tables are constructed using locationContainers which do not serve as the
containing block but do offset the element. These need to be included in the
sticky position offsets in the constraints.

BUG=673538
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
==========

to

==========
Add offset contributed to sticky position box rect by location containers

Tables are constructed using locationContainers which do not serve as the
containing block but do offset the element. These need to be included in the
sticky position offsets in the constraints.

BUG=673538
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2

Review-Url: https://codereview.chromium.org/2646133002
Cr-Commit-Position: refs/heads/master@{#446155}
Committed:
https://chromium.googlesource.com/chromium/src/+/686dbbef585fd2064f1a2a3451b9...
==========

[commit-bot: I haz the power, 2017-01-25 23:08:57]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/686dbbef585fd2064f1a2a3451b9...