Implement closed shadow adjustment for Element.offsetParent

Implement closed shadow adjustment for Element.offsetParent

If |offsetParent| is not an unclosed node of the context
object, search the layout tree up until an unclosed node
is found.

|offsetTop| etc. will be adjusted relative to this
|offsetParent| value.

See also
https://github.com/w3c/webcomponents/issues/497
https://github.com/w3c/csswg-drafts/issues/159
for the discussions about spec clarifications.

BUG=531990
Committed: https://crrev.com/18d455ee833f6a30dcbe2755380861eb75cd9f6f
Cr-Commit-Position: refs/heads/master@{#402757}

[kochi, 2016-06-09 06:03:57]

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

[commit-bot: I haz the power, 2016-06-09 06:04:10]

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

[kochi, 2016-06-09 06:24:53]

Description was changed from

==========
Implement Element.offsetParent for closed shadow adjustment

See also
https://github.com/w3c/webcomponents/issues/497
https://github.com/w3c/csswg-drafts/issues/159

BUG=531990
==========

to

==========
Implement closed shadow adjustment for Element.offsetParent

If |offsetParent| is not an unclosed node of the context
object, search the layout tree up until an unclosed node
is found.

|offsetTop| etc. will be adjusted relative to this
|offsetParent| value.

See also
https://github.com/w3c/webcomponents/issues/497
https://github.com/w3c/csswg-drafts/issues/159
for the discussions about spec clarifications.

BUG=531990
==========

[kochi, 2016-06-09 06:24:53]

kochi@chromium.org changed reviewers:
+ hayato@chromium.org

[kochi, 2016-06-09 06:39:21]

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

[commit-bot: I haz the power, 2016-06-09 06:39:39]

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

[kochi, 2016-06-09 06:39:54]

PTAL

[commit-bot: I haz the power, 2016-06-09 08:07:39]

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

[commit-bot: I haz the power, 2016-06-09 08:07:40]

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

[hayato, 2016-06-09 08:22:03]

https://codereview.chromium.org/2051703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2051703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:651: return
other.isUnclosedNodeOf(toShadowRoot(root).host());
Is this opposite of the definition? Could you re-check the definition of
"unclosed node"?

We do not have to implement "unclosed node" literally, which is O(N^2).

I think we can implement this in O(N). An algorithm can be:

bool Node::isUnclosedNodeOf(const Node& other) const
{
    if (!isInShadowTree() || treeScope() == other.treeScope())
        return true;
    TreeScope* thisScope = &treeScope();
    while (!thisScope.*isClosed()* && thisScope.parentTreeScope()) {
       thisScope = thisScope->parentScope();
    }
    for (TreeScope* scope = &other.treeScope(); scope; scope =
scope->parentTreeScope()) {
        if (scope == thisScope)
            return true;
    }
    return false;
}

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

https://codereview.chromium.org/2051703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3282: Element*
LayoutObject::offsetParent() const
A binding layer is the only client for this method?
Unless that, do not change the behavior of LayoutObject::offsetParent().
That would break something if there are other callers than bindings.

Rather, we should have Element::offsetParentForBinding(), and remove *unclosed
check* from Element::offsetParent and LayoutObject::offsetParent().

[kochi, 2016-06-09 10:49:39]

https://codereview.chromium.org/2051703002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/Node.cpp (right):

https://codereview.chromium.org/2051703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/Node.cpp:651: return
other.isUnclosedNodeOf(toShadowRoot(root).host());
On 2016/06/09 08:22:03, hayato wrote:
> Is this opposite of the definition? Could you re-check the definition of
> "unclosed node"?

Oops, you're right.
I read the latter half of the definition in the opposite way.
This means the test case isn't enough, so I'll add another one to catch it.

> We do not have to implement "unclosed node" literally, which is O(N^2).
> 
> I think we can implement this in O(N). An algorithm can be:
> 
> bool Node::isUnclosedNodeOf(const Node& other) const
> {
>     if (!isInShadowTree() || treeScope() == other.treeScope())
>         return true;
>     TreeScope* thisScope = &treeScope();
>     while (!thisScope.*isClosed()* && thisScope.parentTreeScope()) {
>        thisScope = thisScope->parentScope();
>     }
>     for (TreeScope* scope = &other.treeScope(); scope; scope =
> scope->parentTreeScope()) {
>         if (scope == thisScope)
>             return true;
>     }
>     return false;
> }

Thanks, after checking the tests are working, adopt this alogirthm.

[kochi, 2016-06-09 13:09:27]

kochi@chromium.org changed reviewers:
+ eae@chromium.org

[kochi, 2016-06-09 13:09:28]

@eae, could you take a look at layout/ change?

The background is that for getting Element.offsetParent,
we would like to avoid returning element which should not
visible (i.e. should return "unclosed node").  This happens
e.g. when an element is slotted to closed shadow root.

More background:
https://github.com/w3c/webcomponents/issues/497

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

https://codereview.chromium.org/2051703002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3282: Element*
LayoutObject::offsetParent() const
On 2016/06/09 08:22:03, hayato wrote:
> A binding layer is the only client for this method?
> Unless that, do not change the behavior of LayoutObject::offsetParent().
> That would break something if there are other callers than bindings.
> 
> Rather, we should have Element::offsetParentForBinding(), and remove *unclosed
> check* from Element::offsetParent and LayoutObject::offsetParent().

Good catch!

Currently Element::offsetParent() is only used for HTMLElement.offsetParent
binding
so it should be fine, but LayoutObject::offsetParent() is used for other
purposes,
thus this may break something.

So I separated LayoutObject::offsetParent() into offsetParent() and
offsetParentForBinding(), because LayoutBoxModelObject::offsetTop() et al.
depend on the returned element of offsetParentForBinding() - otherwise
Element.offsetTop et al. would return wrong offset.

The logic in the original offsetParent() looks quite tied with the CSSOM spec,
so I'm not sure how other users (e.g. spatial navigation and hit-test region
visualizer) depend on the Shadow DOM unclosed node behavior.
But I guess spatial navigation should work regardless of Shadow DOM, so keeping
the original offsetParent() should make sense here.

I'll ask for another look from layout team reviewer.

[kochi, 2016-06-09 13:09:39]

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

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

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

[eae, 2016-06-09 13:19:42]

Logic looks fine but it needs a better name and a comment.

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

https://codereview.chromium.org/2051703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:773: // of the
element and stop this algorithm.
Please update this comment to explain what an offsetParentForBidning is and how
it differs from the regular offsetParent call.

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

https://codereview.chromium.org/2051703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3282: Element*
LayoutObject::offsetParentForBinding() const
How about a more descriptive name like nearestUnclosedAncestor?

[commit-bot: I haz the power, 2016-06-09 14:06:52]

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

[commit-bot: I haz the power, 2016-06-09 14:06:53]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[kochi, 2016-06-09 15:44:52]

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

[commit-bot: I haz the power, 2016-06-09 15:45:18]

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

[kochi, 2016-06-09 16:03:26]

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

[commit-bot: I haz the power, 2016-06-09 16:03:48]

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

[kochi, 2016-06-09 16:04:27]

@eae thanks for the review!

I'm now off of any Macs, so I'm not sure why offsetParent.html
tests fail only on Mac, I'll investigate it tomorrow.

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

https://codereview.chromium.org/2051703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:773: // of the
element and stop this algorithm.
On 2016/06/09 13:19:42, eae wrote:
> Please update this comment to explain what an offsetParentForBidning is and
how
> it differs from the regular offsetParent call.

I'm not sure "or is the HTML body element" part, as it does not match the code
already.  Removed the part and updated.

Now I renamed this function to ...RelativeTo"Unclosed"OffsetParent(),
so that it matches what it compares to.

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

https://codereview.chromium.org/2051703002/diff/60001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3282: Element*
LayoutObject::offsetParentForBinding() const
On 2016/06/09 13:19:42, eae wrote:
> How about a more descriptive name like nearestUnclosedAncestor?

I chose "unclosedOffsetParent()", as it still returns offsetParent,
but the difference is that it is expected to return unclosed one.

[eae, 2016-06-09 16:07:39]

LGTM, thanks for making the suggested changes!

[commit-bot: I haz the power, 2016-06-09 17:43:48]

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

[commit-bot: I haz the power, 2016-06-09 17:43:49]

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

[hayato, 2016-06-10 01:41:51]

NOT lgtm.

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

https://codereview.chromium.org/2051703002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:776: Element*
element = unclosedOffsetParent();
Why did you change the behavior of
LayoutBoxModelObject::adjustedPositionRelativeToOffsetParent?
I do not see a reason from the comment. It looks wrong to me.

[kochi, 2016-06-10 02:07:49]

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

https://codereview.chromium.org/2051703002/diff/100001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutBoxModelObject.cpp:776: Element*
element = unclosedOffsetParent();
On 2016/06/10 01:41:51, hayato wrote:
> Why did you change the behavior of
> LayoutBoxModelObject::adjustedPositionRelativeToOffsetParent?
> I do not see a reason from the comment. It looks wrong to me.

adjustedPositionRelativeToOffsetParent() is called from other Layout*
classes (LayoutBox, LayoutInline, LayoutBoxModel) for calculating
offsetTop/offsetLeft which are used for Element.offsetTop/offsetLeft.

Unless they use the relative value to unclosedOffsetParent, it can return
wrong value (not relative to unclosed offset parent, relative to offset parent).

[kochi, 2016-06-10 05:56:35]

Patchset #7 (id:120001) has been deleted

[kochi, 2016-06-10 06:27:31]

Patchset #7 (id:140001) has been deleted

[kochi, 2016-06-10 06:29:53]

@hayato

I think I addressed the concern about Layout API
breakage in patchset 7.

I'm still looking into OSX failure and would like to
add a test for offsetTop/offsetLeft.

[kochi, 2016-06-10 06:32:33]

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

[commit-bot: I haz the power, 2016-06-10 06:32:42]

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

[commit-bot: I haz the power, 2016-06-10 08:15:55]

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

[commit-bot: I haz the power, 2016-06-10 08:15:56]

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

[kochi, 2016-06-10 08:35:48]

Patchset #8 (id:180001) has been deleted

[kochi, 2016-06-10 09:20:59]

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

[commit-bot: I haz the power, 2016-06-10 09:21:12]

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

[commit-bot: I haz the power, 2016-06-10 11:04:42]

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

[commit-bot: I haz the power, 2016-06-10 11:04:43]

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

[hayato, 2016-06-10 11:52:28]

https://codereview.chromium.org/2051703002/diff/220001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/dom/Element.cpp (right):

https://codereview.chromium.org/2051703002/diff/220001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/dom/Element.cpp:638: Element*
Element::offsetParent()
Element::offsetParent() should not use any unclosedXXX check.
Please have a separate function for a binding layer.

[kochi, 2016-06-21 07:42:50]

Patchset #11 (id:260001) has been deleted

[kochi, 2016-06-21 08:26:47]

PTAL

offsetParent.html test is still failing for mac_rel_chromium_ng.
This seems to be a bug in the original code, and I'm taking a look at it.
(looks like we failed to set dirty bit somewhere).

[hayato, 2016-06-22 03:58:34]

I only took a look at the test time. I'll take another look later.

https://codereview.chromium.org/2051703002/diff/280001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/shadow-dom/offsetParent.html (right):

https://codereview.chromium.org/2051703002/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/shadow-dom/offsetParent.html:184:
assert_equals(inner_node.offsetParent, container);
It looks innerNode.offsetParent should be host_closed4. Could you check?

[hayato, 2016-06-22 04:45:35]

https://codereview.chromium.org/2051703002/diff/280001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/shadow-dom/offsetParent.html (right):

https://codereview.chromium.org/2051703002/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/shadow-dom/offsetParent.html:184:
assert_equals(inner_node.offsetParent, container);
On 2016/06/22 at 03:58:34, hayato wrote:
> It looks innerNode.offsetParent should be host_closed4. Could you check?

I understand. Its position is static. That should be skipped.
Could you add "position: relative" to all elements for the purpose of tests?

It is not fair that only the expected element have "position: relative".

[kochi, 2016-06-22 06:33:12]

https://codereview.chromium.org/2051703002/diff/280001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/shadow-dom/offsetParent.html (right):

https://codereview.chromium.org/2051703002/diff/280001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/shadow-dom/offsetParent.html:184:
assert_equals(inner_node.offsetParent, container);
On 2016/06/22 04:45:35, hayato wrote:
> On 2016/06/22 at 03:58:34, hayato wrote:
> > It looks innerNode.offsetParent should be host_closed4. Could you check?
> 
> I understand. Its position is static. That should be skipped.
> Could you add "position: relative" to all elements for the purpose of tests?
> 
> It is not fair that only the expected element have "position: relative".

This test was added to make sure Node.isUnclosedNodeOf() is working correctly,
which had a bug.  So here the relationship between #inner_node (inside closed
shadow) and #container (inside open shadow, contains slot where the closed
shadow
is assigned) made sense to test it.

The case that node inside closed shadow's offsetParent is closed shadow host is
covered in '#host_closed2' test case on line 122.

That said, line 173 is obviously redundant, and updated the comment to clarify
the expectation of this test case.

[kochi, 2016-06-23 02:29:16]

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

[commit-bot: I haz the power, 2016-06-23 02:29:43]

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

[commit-bot: I haz the power, 2016-06-23 03:51:46]

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

[commit-bot: I haz the power, 2016-06-23 03:51:47]

Dry run: This issue passed the CQ dry run.

[kochi, 2016-06-24 11:28:00]

Could you take another look?

[hayato, 2016-06-27 06:58:32]

https://codereview.chromium.org/2051703002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/2051703002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLElement.cpp:1096: while (element &&
(!element->isUnclosedNodeOf(*this) || (element->isInShadowTree() &&
element->containingShadowRoot()->type() == ShadowRootType::UserAgent)))
I do not think it is a good idea to loop here by calling offsetParent
repeatedly.
This loop should be merged into a LayoutObject::offsetParent itself.

Unless that, I am afraid that this would not be a spec compliant.

Also, is there any way to avoid calling isUnclosdeNodeOf repeatedly in each
loop?
If there is, it would be nice.

[kochi, 2016-06-28 07:59:59]

https://codereview.chromium.org/2051703002/diff/320001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/2051703002/diff/320001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/html/HTMLElement.cpp:1096: while (element &&
(!element->isUnclosedNodeOf(*this) || (element->isInShadowTree() &&
element->containingShadowRoot()->type() == ShadowRootType::UserAgent)))
On 2016/06/27 06:58:32, hayato wrote:
> I do not think it is a good idea to loop here by calling offsetParent
> repeatedly.
> This loop should be merged into a LayoutObject::offsetParent itself.
>
> Unless that, I am afraid that this would not be a spec compliant.

That will break LayoutObject::offsetParent, as you already commented previously.

To be spec compliant, all the dependency is isolated here to keep existing
Element::offsetParent() and LayoutObject::offsetParent() users working.
All the HTMLElement.offsetParent()-relative offsets are done in this file
(e.g. HTMLElement::offsetTopForBinding() etc.)

We could have LayoutObject::unclosedOffsetParent(), but it doesn't improve any
performance because it would use the same loop here.

> Also, is there any way to avoid calling isUnclosdeNodeOf repeatedly in each
> loop?
> If there is, it would be nice.

That's a valid concern, but for most cases isUnclosedNodeOf() is called only
once,
so the additional cost for offsetParent should not be significant on average.

In cases where repeated calls are necessary (the element is distributed deep
under
shadow trees), this could be optimized so that only the first unclosed node
(probably
the shadow host of the element) and above are searched, but I may miss some
conditions, and need to give more thought on possible cases.

I added a TODO comment here to note that it is possible to a performance hit.

[hayato, 2016-06-28 08:20:41]

On 2016/06/28 at 07:59:59, kochi wrote:
>
https://codereview.chromium.org/2051703002/diff/320001/third_party/WebKit/Sou...
> File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):
> 
>
https://codereview.chromium.org/2051703002/diff/320001/third_party/WebKit/Sou...
> third_party/WebKit/Source/core/html/HTMLElement.cpp:1096: while (element &&
(!element->isUnclosedNodeOf(*this) || (element->isInShadowTree() &&
element->containingShadowRoot()->type() == ShadowRootType::UserAgent)))
> On 2016/06/27 06:58:32, hayato wrote:
> > I do not think it is a good idea to loop here by calling offsetParent
> > repeatedly.
> > This loop should be merged into a LayoutObject::offsetParent itself.
> >
> > Unless that, I am afraid that this would not be a spec compliant.
> 
> That will break LayoutObject::offsetParent, as you already commented
previously.

No. I am suggesting that LayoutObject::offsetParent will change the behavior by
a parameter so that it does not break the existing customers.

e.g. LayoutObject::offsetParent(bool unclosedOnly)

[kochi, 2016-06-29 02:17:38]

Patchset #17 (id:400001) has been deleted

[kochi, 2016-06-29 02:17:49]

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

[commit-bot: I haz the power, 2016-06-29 02:19:09]

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

[kochi, 2016-06-29 02:21:00]

On 2016/06/28 08:20:41, hayato wrote:
> On 2016/06/28 at 07:59:59, kochi wrote:
> >
>
https://codereview.chromium.org/2051703002/diff/320001/third_party/WebKit/Sou...
> > File third_party/WebKit/Source/core/html/HTMLElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/2051703002/diff/320001/third_party/WebKit/Sou...
> > third_party/WebKit/Source/core/html/HTMLElement.cpp:1096: while (element &&
> (!element->isUnclosedNodeOf(*this) || (element->isInShadowTree() &&
> element->containingShadowRoot()->type() == ShadowRootType::UserAgent)))
> > On 2016/06/27 06:58:32, hayato wrote:
> > > I do not think it is a good idea to loop here by calling offsetParent
> > > repeatedly.
> > > This loop should be merged into a LayoutObject::offsetParent itself.
> > >
> > > Unless that, I am afraid that this would not be a spec compliant.
> > 
> > That will break LayoutObject::offsetParent, as you already commented
> previously.
> 
> No. I am suggesting that LayoutObject::offsetParent will change the behavior
by
> a parameter so that it does not break the existing customers.
> 
> e.g. LayoutObject::offsetParent(bool unclosedOnly)

I realized what you mean.  I missed the discussion happened on
https://github.com/w3c/csswg-drafts/issues/159
after the change to the spec was made.

I made the change for the spec compliance and added one more test case
so it can catch the corner case.

Thanks for pointing it out!

[hayato, 2016-06-29 03:53:52]

https://codereview.chromium.org/2051703002/diff/380001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/shadow-dom/offsetParent.html (right):

https://codereview.chromium.org/2051703002/diff/380001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/shadow-dom/offsetParent.html:180: <!--
Could you add one more test where it returns a non-body element while it is
skipping "position:fixed" element?

e.g.

host:
  fixed
    node

host's shadow tree:
 fixed
   relative
     fixed
        slot

Then, assert node.offsetParent == relative

[commit-bot: I haz the power, 2016-06-29 03:55:05]

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

[commit-bot: I haz the power, 2016-06-29 03:55:07]

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

[kochi, 2016-06-29 04:27:58]

On 2016/06/29 03:53:52, hayato wrote:
>
https://codereview.chromium.org/2051703002/diff/380001/third_party/WebKit/Lay...
> File third_party/WebKit/LayoutTests/shadow-dom/offsetParent.html (right):
> 
>
https://codereview.chromium.org/2051703002/diff/380001/third_party/WebKit/Lay...
> third_party/WebKit/LayoutTests/shadow-dom/offsetParent.html:180: <!--
> Could you add one more test where it returns a non-body element while it is
> skipping "position:fixed" element?
> 
> e.g.
> 
> host:
>   fixed
>     node
> 
> host's shadow tree:
>  fixed
>    relative
>      fixed
>         slot
> 
> Then, assert node.offsetParent == relative

In this case, node's parent (fixed) will be returned for node.offsetParent, as
offsetParent
returns element whose CSS position is not static.

[hayato, 2016-06-29 04:53:46]

I missed the meaning of 'fixed' in the spec. Please forget the last example.

I meant:

host:
  node


host's shadow tree (closed):
  fixed
    slot


In this case, we should not return |host| as |node.offsetParent| because it
*skips* a *fixed* element.
We should not return any element as offsetParent if there is a *fixed* element
between |node| and |node.offsetParent|.
In this case, we should return null.

I think this is a bug in the spec.

[kochi, 2016-06-29 05:08:17]

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

[kochi, 2016-06-29 05:09:15]

PTAL

fixed to return null when 'position: fixed' found inside
the closed shadow.

[commit-bot: I haz the power, 2016-06-29 05:10:24]

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

[kochi, 2016-06-29 05:29:20]

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

[commit-bot: I haz the power, 2016-06-29 05:29:30]

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

[kochi, 2016-06-29 05:29:39]

Updated the CL.
PTAL.

[hayato, 2016-06-29 05:42:46]

https://codereview.chromium.org/2051703002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2051703002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3205: if
(ancestor->isOutOfFlowPositioned() && ancestor->style()->position() ==
FixedPosition)
Could you clean up this function?

e.g. Remove L3187-3188 and have a *fixed check* at one place.

[kochi, 2016-06-29 06:24:37]

https://codereview.chromium.org/2051703002/diff/440001/third_party/WebKit/Sou...
File third_party/WebKit/Source/core/layout/LayoutObject.cpp (right):

https://codereview.chromium.org/2051703002/diff/440001/third_party/WebKit/Sou...
third_party/WebKit/Source/core/layout/LayoutObject.cpp:3205: if
(ancestor->isOutOfFlowPositioned() && ancestor->style()->position() ==
FixedPosition)
On 2016/06/29 05:42:46, hayato wrote:
> Could you clean up this function?
> 
> e.g. Remove L3187-3188 and have a *fixed check* at one place.
> 

Done.

[kochi, 2016-06-29 06:25:12]

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

[commit-bot: I haz the power, 2016-06-29 06:25:35]

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

[hayato, 2016-06-29 07:13:12]

lgtm

https://codereview.chromium.org/2051703002/diff/460001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/shadow-dom/offsetParent.html (right):

https://codereview.chromium.org/2051703002/diff/460001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/shadow-dom/offsetParent.html:185: <div
id='host_closed5'>
Could you add a test for *open* shadow tree with the same tree structure?

[kochi, 2016-06-29 07:29:35]

https://codereview.chromium.org/2051703002/diff/460001/third_party/WebKit/Lay...
File third_party/WebKit/LayoutTests/shadow-dom/offsetParent.html (right):

https://codereview.chromium.org/2051703002/diff/460001/third_party/WebKit/Lay...
third_party/WebKit/LayoutTests/shadow-dom/offsetParent.html:185: <div
id='host_closed5'>
On 2016/06/29 07:13:12, hayato wrote:
> Could you add a test for *open* shadow tree with the same tree structure?

Done.

[kochi, 2016-06-29 07:29:55]

The CQ bit was checked by kochi@chromium.org

[kochi, 2016-06-29 07:29:55]

The patchset sent to the CQ was uploaded after l-g-t-m from eae@chromium.org,
hayato@chromium.org
Link to the patchset: https://codereview.chromium.org/2051703002/#ps480001
(title: "Add one more test case.")

[commit-bot: I haz the power, 2016-06-29 07:30:22]

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-06-29 08:52:57]

Description was changed from

==========
Implement closed shadow adjustment for Element.offsetParent

If |offsetParent| is not an unclosed node of the context
object, search the layout tree up until an unclosed node
is found.

|offsetTop| etc. will be adjusted relative to this
|offsetParent| value.

See also
https://github.com/w3c/webcomponents/issues/497
https://github.com/w3c/csswg-drafts/issues/159
for the discussions about spec clarifications.

BUG=531990
==========

to

==========
Implement closed shadow adjustment for Element.offsetParent

If |offsetParent| is not an unclosed node of the context
object, search the layout tree up until an unclosed node
is found.

|offsetTop| etc. will be adjusted relative to this
|offsetParent| value.

See also
https://github.com/w3c/webcomponents/issues/497
https://github.com/w3c/csswg-drafts/issues/159
for the discussions about spec clarifications.

BUG=531990
==========

[commit-bot: I haz the power, 2016-06-29 08:52:59]

Committed patchset #20 (id:480001)

[commit-bot: I haz the power, 2016-06-29 08:55:50]

Description was changed from

==========
Implement closed shadow adjustment for Element.offsetParent

If |offsetParent| is not an unclosed node of the context
object, search the layout tree up until an unclosed node
is found.

|offsetTop| etc. will be adjusted relative to this
|offsetParent| value.

See also
https://github.com/w3c/webcomponents/issues/497
https://github.com/w3c/csswg-drafts/issues/159
for the discussions about spec clarifications.

BUG=531990
==========

to

==========
Implement closed shadow adjustment for Element.offsetParent

If |offsetParent| is not an unclosed node of the context
object, search the layout tree up until an unclosed node
is found.

|offsetTop| etc. will be adjusted relative to this
|offsetParent| value.

See also
https://github.com/w3c/webcomponents/issues/497
https://github.com/w3c/csswg-drafts/issues/159
for the discussions about spec clarifications.

BUG=531990

Committed: https://crrev.com/18d455ee833f6a30dcbe2755380861eb75cd9f6f
Cr-Commit-Position: refs/heads/master@{#402757}
==========

[commit-bot: I haz the power, 2016-06-29 08:55:52]

Patchset 20 (id:??) landed as
https://crrev.com/18d455ee833f6a30dcbe2755380861eb75cd9f6f
Cr-Commit-Position: refs/heads/master@{#402757}