[cssom-view] HTMLElement.prototype.offsetParent leaks a node inside a shadow tree

[hayatoito]

See the context: WICG/webcomponents#497

@rniwa, do you have any preference to this issue? I do not have a strong opinion yet.

[tabatkins]

I support @rniwa's answer - walking up until you find a valid (non-leaky) offsetParent, then making sure that offsetLeft and offsetTop are both relative to that, should work great.

This will still leave us with cases where an element is in a shadow tree and the only possible offset parents are outside the shadow. That leaks in the opposite way - info leaking into the shadow. Is that ok? I know this happens with CSS inheritance and such, but I dunno how strict we are about element refs leaking in.

[rniwa]

I think it's okay for shadow tree to access to elements outside the shadow tree since that's already possible via ShadowRoot.prototype.host as well as HTMLSlotElement.prototype.assignedNodes().

[tabatkins]

In that case, 👍 to the idea.

[hayatoito]

Okay. We can fix this by using "unclosed node" (https://dom.spec.whatwg.org/#concept-unclosed-node)

https://drafts.csswg.org/cssom-view/#dom-htmlelement-offsetparent

Return the nearest ancestor element of the element for which at least one of the following is true and terminate this algorithm if such an ancestor is found:

A quick fix can be:

Return the nearest ancestor unclosed element of the element for which at least one of the following is true and terminate this algorithm if such an ancestor is found:

Let me send a PR.

[platosha]

Now with the fix merged, what offsetParent would return for a node that has a container block with position: fixed inside a closed shadow root?

Considering the following code, what would be in the log output? Is it still possible for the node C to discover that it is inside a fixed container?

var A = document.createElement('node-a');
document.body.appendChild(A);

var shadowRoot = A.attachShadow({mode: 'closed'});

var B = document.createElement('node-b');
shadowRoot.appendChild(B);
B.style.position = 'fixed';

var slot = document.createElement('slot');
B.appendChild(slot);

var C = document.createElement('node-c');
A.appendChild(C);

console.log(C.offsetParent.localName);

[rniwa]

Node C's offsetParent will be offsetParent of B.

[platosha]

@rniwa then it looks like it's now completely impossible to discover for the node C that it's inside a fixed container.

We have a component that should behave differently inside a fixed container. So we use a fixed container detection. Previously, it was safe to walk through .offsetParents chain and check their computed styles. Now the detection is broken.

Aside from that, offsetParent has another use case: for an absolutely positioned node it points to the containing block. This is also broken here. In my example, if I assign position: absolute to the node C, its containing block will be the node B, but offsetParent will be null.

[rniwa]

No, offsetParent would point to B's containing block instead and offsetLeft and offsetTop would be adjusted accordingly.

[platosha]

@rniwa node B’s containing block is the viewport, since it is fixed. I assume, that in case when the node’s containing block is the viewport, offsetParent is null. So, according to your answer, it will be null. Though to my understanding of the spec, it should be document.body.

Anyways, it doesn't really matter for this case if offsetParent of C is document.body or null. Neither of these variants allow to find out that C's positioning is actually affected by B. Neither of these variants allow to find out that the C is inside a fixed container.

[platosha]

I feel like the proper solution would be to disallow shadow containing blocks, excluding them from containing block chains. So that when a shadow node has non-static position, it does not affect the positioning of the absolute descendants outside shadow.

This should make offsetParent usable for walking through containing block chains again, as it was before the shadow DOM era.

[hayatoito]

@platosha
My intention is : C.offsetParent == document.body.

Return the nearest ancestor unclosed element of the element for which at least one of the following is true and terminate this algorithm if such an ancestor is found: [DOM]

Ancestor in this context should be interpreted as an ancestor in a flat tree. https://drafts.csswg.org/css-scoping/#flattening

Ancestor chain in this case is:
C => (slot) => (B) => (A) => body

(slot), (B) (its position is fixed, but it is not unclosed) or (A) should be skipped because it does not meet the condition.

[hayatoito]

Anyways, it doesn't really matter for this case if offsetParent of C is document.body or null. Neither of these variants allow to find out that C's positioning is actually affected by B. Neither of these variants allow to find out that the C is inside a fixed container.

It sounds that users of a closed shadow tree should be aware of this limitation.
We do not have an answer for that, except for using an open shadow tree, I guess.

[TakayoshiKochi]

If a user agent naively implements the new behavior, it would just recursively apply the original offsetParent like:

target = element.offsetParent
while target is not unclosed node of element:
    target = target.offsetParent 

and C's .offsetParent would be B, and then B's .offsetParent would be null and thus null is returned, as @niwa #159 (comment) -ed.

But if a user agent implements "the nearest ancestor unclosed node", as @hayatoito #159 (comment), document.body would be returned because B is unclosed. I guess it does not make sense either, as probably .offfsetTop or .offsetLeft for the C would be useless.

So in this case, returning null sounds somewhat better than returning 'document.body' to me, as C is somehow not relative to any unclosed parents, but I'm still unsure.

BTW, I don't understand @platosha 's comment:

I feel like the proper solution would be to disallow shadow containing blocks, excluding them from containing block chains. So that when a shadow node has non-static position, it does not affect the positioning of the absolute descendants outside shadow.

What do "blocks" / "block chains" mean here? If this is implemented, what the A/B/C example above will be styled/rendered, and what is the expected C.offsetParent?

[TakayoshiKochi]

Hmm, I may be wrong. In the CSSOM spec, "ancestor" does not mean DOM tree's ancestor, but box-tree based one. So it will return null even if the spec is unchanged?

[zcorpan]

I think the spec tries to operate on the DOM tree here. I'm not entirely sure if that matches what is implemented though.

[hayatoito]

We should clarify that it operates on a flat tree (or layout block tree based on a flat tree?). See https://drafts.csswg.org/css-scoping/#flattening
It matches the implementation, as least in Blink.

I thought that unless otherwise mentioned, it is based on a flat tree. However, explicit might be better than implicit here.

[TakayoshiKochi]

During the code review conversation with @hayatoito,
we concluded that for @platosha 's case, returning null for C.offsetParent would be better.
(See https://codereview.chromium.org/2051703002/ )

I'll send a PR shortly for changing the spec.

[hayatoito]

Yeah.

@platosha I am sorry. I did not understand the problem deeply.
See https://codereview.chromium.org/2051703002/#msg63.
Now I understand the problem, hopefully.

[rniwa]

Hm... looks like Blink still hasn't implemented this behavior?

@rakina, @tkent-google: Can someone from Blink side follow up on this? We're getting bug reports like https://bugs.webkit.org/show_bug.cgi?id=204195

[tkent-google]

@rniwa I contacted the current owner of Shadow DOM in Chromium project about this issue.

Here is a tracking issue: https://bugs.chromium.org/p/chromium/issues/detail?id=920069

[mfreed7]

The Chromium bug has been fixed. Seems like this issue can be closed now?