position:fixed content is not offset by relative positioned ancestors

position:fixed content is not offset by relative positioned ancestors

For positioned objects take into account the relative positioned ancestors' offset
when computing logical top (similar to logical left computing).

BUG=624667
Committed: https://crrev.com/f91316113420b5f208dadb3a47f626a24a455fd7
Cr-Commit-Position: refs/heads/master@{#440125}

[Anton Obzhirov, 2016-12-15 15:34:49]

Hi guys, not sure who is supposed to review layout position issues fixes, plz
add the right person.

[cbiesinger1, 2016-12-15 15:58:28]

cbiesinger@google.com changed reviewers:
+ eae@chromium.org, mstensho@opera.com

[cbiesinger1, 2016-12-15 15:58:28]

Adding some reviewers

[pdr., 2016-12-16 03:37:26]

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

[commit-bot: I haz the power, 2016-12-16 03:37:56]

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

[pdr., 2016-12-16 03:43:11]

For other reviewers: it looks like this WebKit patch is relevant:
https://bugs.webkit.org/show_bug.cgi?id=147284
I'm not sure if we need to do the same.

[commit-bot: I haz the power, 2016-12-16 04:49:02]

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

[commit-bot: I haz the power, 2016-12-16 04:49:03]

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

[mstensho (USE GERRIT), 2016-12-16 11:53:00]

Please move the BUG=624667 line to the bottom of the description.

https://codereview.chromium.org/2577893004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/block/positioning/fixed-position-with-relative-parent-expected.html
(right):

https://codereview.chromium.org/2577893004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/block/positioning/fixed-position-with-relative-parent-expected.html:4:
<div style="background: green; top: auto; width: 40px; height: 40px; position:
absolute;"></div>
Please indent this line. Also no need for the top:auto declaration.

https://codereview.chromium.org/2577893004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/block/positioning/fixed-position-with-relative-parent.html
(right):

https://codereview.chromium.org/2577893004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/block/positioning/fixed-position-with-relative-parent.html:2:
<style> * { margin: 0; } </style>
I think I'd just drop this rule.

https://codereview.chromium.org/2577893004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/block/positioning/fixed-position-with-relative-parent.html:4:
<div style="background: green; top: auto; width: 40px; height: 40px; position:
fixed;"></div>
Please indent this line. Also no need for the top:auto declaration, since that's
the initial value.

Also, how about making a test where the inner block is supposed to cover the
outer block completely? And maybe let the background of the outer one be red?
And even add a pass condition text "There should be no red on this page"?

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

https://codereview.chromium.org/2577893004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4173: staticLogicalTop +=
box.logicalTop();
The table rows change here doesn't look right. I suggest that you revert this
part. Both table cells and table rows are relative to their containing table
section, so you end up adding the offset twice.

[Anton Obzhirov, 2016-12-16 14:41:13]

On 2016/12/16 03:43:11, pdr. wrote:
> For other reviewers: it looks like this WebKit patch is relevant:
> https://bugs.webkit.org/show_bug.cgi?id=147284
> I'm not sure if we need to do the same.

I think blink already has similar to the WebKit patch code in the latest 
computeInlineStaticDistance implementation in blink but without supporting
horizontal writing mode.

And this patch implements another part in computeBlockStaticDistance again
without supporting horizontal writing mode.

Probably there should be another issue created for horizontal writing mode
support for static distances?

[Anton Obzhirov, 2016-12-16 14:46:31]

Thanks for the review, will update the patch and will check failing layout
tests.

https://codereview.chromium.org/2577893004/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/block/positioning/fixed-position-with-relative-parent.html
(right):

https://codereview.chromium.org/2577893004/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/block/positioning/fixed-position-with-relative-parent.html:4:
<div style="background: green; top: auto; width: 40px; height: 40px; position:
fixed;"></div>
On 2016/12/16 11:53:00, mstensho wrote:
> Please indent this line. Also no need for the top:auto declaration, since
that's
> the initial value.
> 
> Also, how about making a test where the inner block is supposed to cover the
> outer block completely? And maybe let the background of the outer one be red?
> And even add a pass condition text "There should be no red on this page"?

Yep, will do.

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

https://codereview.chromium.org/2577893004/diff/1/third_party/WebKit/Source/c...
third_party/WebKit/Source/core/layout/LayoutBox.cpp:4173: staticLogicalTop +=
box.logicalTop();
On 2016/12/16 11:53:00, mstensho wrote:
> The table rows change here doesn't look right. I suggest that you revert this
> part. Both table cells and table rows are relative to their containing table
> section, so you end up adding the offset twice.

Will do, probably I already got an issue with a layout test because of this.

[Anton Obzhirov, 2016-12-21 11:51:50]

I've checked the failing tests and the expected test results were wrong, so I
updated them and th3 patch, plz have a look.

[mstensho (USE GERRIT), 2016-12-21 12:04:02]

On 2016/12/16 11:53:00, mstensho wrote:
> Please move the BUG=624667 line to the bottom of the description.

[Anton Obzhirov, 2016-12-21 13:35:55]

On 2016/12/21 12:04:02, mstensho wrote:
> On 2016/12/16 11:53:00, mstensho wrote:
> > Please move the BUG=624667 line to the bottom of the description.

Done, sorry.

[Anton Obzhirov, 2016-12-21 13:37:56]

On 2016/12/21 13:35:55, Anton Obzhirov wrote:
> On 2016/12/21 12:04:02, mstensho wrote:
> > On 2016/12/16 11:53:00, mstensho wrote:
> > > Please move the BUG=624667 line to the bottom of the description.
> 
> Done, sorry.

That's weird, was updated locally but still not updated here. Let me try again.

[cbiesinger, 2016-12-21 13:43:30]

On 2016/12/21 13:37:56, Anton Obzhirov wrote:
> On 2016/12/21 13:35:55, Anton Obzhirov wrote:
> > On 2016/12/21 12:04:02, mstensho wrote:
> > > On 2016/12/16 11:53:00, mstensho wrote:
> > > > Please move the BUG=624667 line to the bottom of the description.
> > 
> > Done, sorry.
> 
> That's weird, was updated locally but still not updated here. Let me try
again.

Ah that is confusing -- to update the description here you have to either use
"git cl desc" or use the website (the "Edit Issue" link)

[Anton Obzhirov, 2016-12-21 13:46:33]

Description was changed from

==========
position:fixed content is not offset by relative positioned ancestors

BUG=624667

For positioned objects take into account the relative positioned ancestors'
offset
when computing logical top (similar to logical left computing).
==========

to

==========
position:fixed content is not offset by relative positioned ancestors

For positioned objects take into account the relative positioned ancestors'
offset
when computing logical top (similar to logical left computing).

BUG=624667
==========

[Anton Obzhirov, 2016-12-21 13:47:36]

On 2016/12/21 13:43:30, cbiesinger wrote:
> On 2016/12/21 13:37:56, Anton Obzhirov wrote:
> > On 2016/12/21 13:35:55, Anton Obzhirov wrote:
> > > On 2016/12/21 12:04:02, mstensho wrote:
> > > > On 2016/12/16 11:53:00, mstensho wrote:
> > > > > Please move the BUG=624667 line to the bottom of the description.
> > > 
> > > Done, sorry.
> > 
> > That's weird, was updated locally but still not updated here. Let me try
> again.
> 
> Ah that is confusing -- to update the description here you have to either use
> "git cl desc" or use the website (the "Edit Issue" link)

Thanks! Managed to do it at the third attempt :)

[mstensho (USE GERRIT), 2016-12-21 13:55:10]

lgtm

[Anton Obzhirov, 2016-12-21 14:25:25]

The CQ bit was checked by a.obzhirov@samsung.com

[commit-bot: I haz the power, 2016-12-21 14:25:40]

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-12-21 16:37:09]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482330325311460,
"parent_rev": "37b296c3f9a3da85ac28bdd99d510577ccbebc4f", "commit_rev":
"3719f30de87b838c58310b5007dfef7eb239b4ed"}

[commit-bot: I haz the power, 2016-12-21 16:37:43]

Description was changed from

==========
position:fixed content is not offset by relative positioned ancestors

For positioned objects take into account the relative positioned ancestors'
offset
when computing logical top (similar to logical left computing).

BUG=624667
==========

to

==========
position:fixed content is not offset by relative positioned ancestors

For positioned objects take into account the relative positioned ancestors'
offset
when computing logical top (similar to logical left computing).

BUG=624667

Review-Url: https://codereview.chromium.org/2577893004
==========

[commit-bot: I haz the power, 2016-12-21 16:37:44]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-21 16:40:26]

Description was changed from

==========
position:fixed content is not offset by relative positioned ancestors

For positioned objects take into account the relative positioned ancestors'
offset
when computing logical top (similar to logical left computing).

BUG=624667

Review-Url: https://codereview.chromium.org/2577893004
==========

to

==========
position:fixed content is not offset by relative positioned ancestors

For positioned objects take into account the relative positioned ancestors'
offset
when computing logical top (similar to logical left computing).

BUG=624667

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

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

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