Make offsetTop/Left handle a relative positioned inline offsetParent correctly.

Make offsetTop/Left handle a relative positioned inline offsetParent correctly.

offsetTop and offsetLeft happily ignored the fact that offsetParent could
be a relative positioned inline.

I used the opportunity to change some variable names in
LayoutBoxModelObject::adjustedPositionRelativeTo() in order to hopefully
make it clearer what's going on.

BUG=638184
Committed: https://crrev.com/f903f01899c880f15af678b6e44404a7d9b1735f
Cr-Commit-Position: refs/heads/master@{#429571}

[Karl Øygard, 2016-10-12 13:25:51]

The CQ bit was checked by karlo@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-12 13:26:03]

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-10-12 14:41:17]

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

[commit-bot: I haz the power, 2016-10-12 14:41:19]

Dry run: This issue passed the CQ dry run.

[Karl Øygard, 2016-10-12 20:47:04]

Description was changed from

==========
Make offsetTop/Left pay attention to an in flow positioned offsetParent.

BUG=638184
==========

to

==========
Make offsetTop/Left pay attention to an in flow positioned offsetParent.

offsetTop and offsetLeft would ignore the fact that the offsetParent could be a
relative positioned inline.

BUG=638184
==========

[Karl Øygard, 2016-10-13 07:48:24]

Description was changed from

==========
Make offsetTop/Left pay attention to an in flow positioned offsetParent.

offsetTop and offsetLeft would ignore the fact that the offsetParent could be a
relative positioned inline.

BUG=638184
==========

to

==========
Make offsetTop/Left pay attention to an in flow positioned offsetParent.

offsetTop and offsetLeft happily ignored the fact that the offsetParent could be
a relative positioned inline.

BUG=638184
==========

[Karl Øygard, 2016-10-13 11:28:27]

Description was changed from

==========
Make offsetTop/Left pay attention to an in flow positioned offsetParent.

offsetTop and offsetLeft happily ignored the fact that the offsetParent could be
a relative positioned inline.

BUG=638184
==========

to

==========
Make offsetTop/Left handle a relative positioned inline offsetParent correctly.

offsetTop and offsetLeft happily ignored the fact that offsetParent could
be a relative positioned inline.

I used the opportunity to change some variable names in
LayoutBoxModelObject::adjustedPositionRelativeTo() in order to hopefully
make it clearer what's going on.

BUG=638184
==========

[Karl Øygard, 2016-10-13 11:28:45]

The CQ bit was checked by karlo@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 11:28:55]

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-10-13 12:33:19]

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

[commit-bot: I haz the power, 2016-10-13 12:33:20]

Dry run: This issue passed the CQ dry run.

[Karl Øygard, 2016-10-13 13:15:10]

The CQ bit was checked by karlo@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 13:15:18]

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

[Karl Øygard, 2016-10-13 14:59:35]

The CQ bit was checked by karlo@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-13 14:59:45]

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-10-13 16:06:09]

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

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

Dry run: 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_...)

[Karl Øygard, 2016-10-19 13:33:35]

The CQ bit was checked by karlo@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 13:33:52]

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-10-19 14:45:25]

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

[commit-bot: I haz the power, 2016-10-19 14:45:25]

Dry run: 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_...)

[Karl Øygard, 2016-10-19 19:09:00]

The CQ bit was checked by karlo@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-19 19:09:57]

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-10-19 23:15:46]

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

[commit-bot: I haz the power, 2016-10-19 23:15:47]

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

[Karl Øygard, 2016-10-20 13:32:18]

The CQ bit was checked by karlo@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-20 13:32:40]

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-10-20 14:50:43]

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

[commit-bot: I haz the power, 2016-10-20 14:50:44]

Dry run: This issue passed the CQ dry run.

[Karl Øygard, 2016-10-21 08:52:20]

The CQ bit was checked by karlo@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-21 08:52: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, 2016-10-21 10:20:44]

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

[commit-bot: I haz the power, 2016-10-21 10:20:45]

Dry run: This issue passed the CQ dry run.

[Karl Øygard, 2016-10-21 10:37:08]

karlo@opera.com changed reviewers:
+ cbiesinger@chromium.org

[Karl Øygard, 2016-10-21 10:37:10]

ptal

[ikilpatrick, 2016-10-21 16:39:24]

ikilpatrick@chromium.org changed reviewers:
+ atotic@chromium.org, ikilpatrick@chromium.org

[cbiesinger, 2016-10-21 20:32:52]

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

[mstensho (USE GERRIT), 2016-10-24 08:01:47]

I've assisted karlo a bit with this patch, so I would be good if someone other
than me could review it.

[atotic, 2016-10-25 19:28:58]

https://codereview.chromium.org/2414683002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2414683002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1028:
referencePoint += toLayoutBox(offsetParentObject)->topLeftLocation();
Why replace moveBy with '+='? 

moveBy is used throughtout this method, lets stay consistent.

https://codereview.chromium.org/2414683002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1032: const
LayoutInline* inlineParent = toLayoutInline(offsetParentObject);
I am not very familiar with this code. 

Can you explain what this fixes, and why does the fix work?

[Karl Øygard, 2016-10-26 11:27:04]

https://codereview.chromium.org/2414683002/diff/140001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2414683002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1028:
referencePoint += toLayoutBox(offsetParentObject)->topLeftLocation();
On 2016/10/25 19:28:58, atotic2 wrote:
> Why replace moveBy with '+='? 
> 
> moveBy is used throughtout this method, lets stay consistent.

It was just done to conserve line space for cosmetic reasons, moveBy() pushes
the line above 80 characters.  I can revert this.

https://codereview.chromium.org/2414683002/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1032: const
LayoutInline* inlineParent = toLayoutInline(offsetParentObject);
On 2016/10/25 19:28:58, atotic2 wrote:
> I am not very familiar with this code. 
> 
> Can you explain what this fixes, and why does the fix work?

This covers the case when offsetParent is an inline (and in flow positioned). 
In that case, we need to subtract the line position of the offsetParent. 
Additionally, absolute positioned children may or may not already be placed on
the line, offsetForInFlowPositionedInline() handles that.

I'll add a comment to clarify this a little.

[Karl Øygard, 2016-10-26 13:22:37]

The CQ bit was checked by karlo@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-10-26 13:22:53]

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-10-26 15:23:43]

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

[commit-bot: I haz the power, 2016-10-26 15:23:44]

Dry run: This issue passed the CQ dry run.

[ikilpatrick, 2016-10-27 16:12:49]

ikilpatrick@chromium.org changed reviewers:
- ikilpatrick@chromium.org

[Karl Øygard, 2016-10-31 11:46:50]

ptal.  I reverted the code to moveBy() as requested, and added a small comment
to the handling of absolute positioned children.

[atotic, 2016-10-31 22:53:02]

On 2016/10/31 at 11:46:50, karlo wrote:
> ptal.  I reverted the code to moveBy() as requested, and added a small comment
> to the handling of absolute positioned children.

lgtm with nits.

[atotic, 2016-10-31 22:54:25]

https://codereview.chromium.org/2414683002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2414683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1033: if
(offsetParentObject->isLayoutInline()) {
Can offsetParentObject be null here? We're checking for null at 1006.

https://codereview.chromium.org/2414683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1038: // Absolute
positioned objects need to be placed on the line
This still leaves me wondering:
- "on the line" with respect to what?

What do you think about:

"Offset for absolute elements with inline parent is a special case in DOM spec."

Because the real work of how this special case works is inside
offsetForInFlowPositionedInline

https://codereview.chromium.org/2414683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1039: 
Remove blank line

[Karl Øygard, 2016-11-03 11:23:31]

https://codereview.chromium.org/2414683002/diff/160001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp (right):

https://codereview.chromium.org/2414683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1033: if
(offsetParentObject->isLayoutInline()) {
On 2016/10/31 22:54:25, atotic2 wrote:
> Can offsetParentObject be null here? We're checking for null at 1006.

offsetParentObject is checked for null in an if statement above, and is not
subsequently modified, so should be fine.

https://codereview.chromium.org/2414683002/diff/160001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:1038: // Absolute
positioned objects need to be placed on the line
On 2016/10/31 22:54:25, atotic2 wrote:
> This still leaves me wondering:
> - "on the line" with respect to what?
> 
> What do you think about:
> 
> "Offset for absolute elements with inline parent is a special case in DOM
spec."
> 
> Because the real work of how this special case works is inside
> offsetForInFlowPositionedInline

Agreed, I have amended the comment accordingly.

[Karl Øygard, 2016-11-03 11:49:50]

The CQ bit was checked by karlo@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 11:50:00]

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-11-03 12:01:02]

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

[commit-bot: I haz the power, 2016-11-03 12:01:05]

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

[Karl Øygard, 2016-11-03 12:27:14]

The CQ bit was checked by karlo@opera.com to run a CQ dry run

[commit-bot: I haz the power, 2016-11-03 12:27:33]

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-11-03 13:52:43]

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

[commit-bot: I haz the power, 2016-11-03 13:52:44]

Dry run: This issue passed the CQ dry run.

[Karl Øygard, 2016-11-03 13:58:54]

The CQ bit was checked by karlo@opera.com

[Karl Øygard, 2016-11-03 13:58:54]

The patchset sent to the CQ was uploaded after l-g-t-m from atotic@chromium.org
Link to the patchset: https://codereview.chromium.org/2414683002/#ps200001
(title: "Fixed broken rebase.")

[commit-bot: I haz the power, 2016-11-03 13:59:08]

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-11-03 13:59:09]

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

[commit-bot: I haz the power, 2016-11-03 13:59:11]

No L-G-T-M from a valid reviewer yet. 
CQ run can only be started by full committers or once the patch has
received an L-G-T-M from a full committer.
Even if an L-G-T-M may have been provided, it was from a non-committer,
_not_ a full super star committer.
See http://www.chromium.org/getting-involved/become-a-committer
Note that this has nothing to do with OWNERS files.

[mstensho (USE GERRIT), 2016-11-03 14:01:43]

The CQ bit was checked by mstensho@opera.com

[mstensho (USE GERRIT), 2016-11-03 14:01:44]

lgtm

[commit-bot: I haz the power, 2016-11-03 14:02:07]

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-11-03 14:07:03]

Description was changed from

==========
Make offsetTop/Left handle a relative positioned inline offsetParent correctly.

offsetTop and offsetLeft happily ignored the fact that offsetParent could
be a relative positioned inline.

I used the opportunity to change some variable names in
LayoutBoxModelObject::adjustedPositionRelativeTo() in order to hopefully
make it clearer what's going on.

BUG=638184
==========

to

==========
Make offsetTop/Left handle a relative positioned inline offsetParent correctly.

offsetTop and offsetLeft happily ignored the fact that offsetParent could
be a relative positioned inline.

I used the opportunity to change some variable names in
LayoutBoxModelObject::adjustedPositionRelativeTo() in order to hopefully
make it clearer what's going on.

BUG=638184
==========

[commit-bot: I haz the power, 2016-11-03 14:07:04]

Committed patchset #11 (id:200001)

[commit-bot: I haz the power, 2016-11-03 14:08:36]

Description was changed from

==========
Make offsetTop/Left handle a relative positioned inline offsetParent correctly.

offsetTop and offsetLeft happily ignored the fact that offsetParent could
be a relative positioned inline.

I used the opportunity to change some variable names in
LayoutBoxModelObject::adjustedPositionRelativeTo() in order to hopefully
make it clearer what's going on.

BUG=638184
==========

to

==========
Make offsetTop/Left handle a relative positioned inline offsetParent correctly.

offsetTop and offsetLeft happily ignored the fact that offsetParent could
be a relative positioned inline.

I used the opportunity to change some variable names in
LayoutBoxModelObject::adjustedPositionRelativeTo() in order to hopefully
make it clearer what's going on.

BUG=638184

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

[commit-bot: I haz the power, 2016-11-03 14:08:37]

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