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

[hayatoito]

See the context: https://github.com/w3c/webcomponents/issues/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, :+1: 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 https://github.com/w3c/csswg-drafts/issues/159#issuecomment-226077049 -ed.

But if a user agent implements "the nearest ancestor unclosed node", as @hayatoito https://github.com/w3c/csswg-drafts/issues/159#issuecomment-227640034, 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]

Sorry, it looks like I missed an crucial point here.

We’re exposing any node that’s not closed-shadow-hidden here, which means we’d return an element inside shadow tree as an offset parent. I don’t think we want that even in an open shadow tree.

Let’s imagine you’re trying to traverse over ancestor offsetParents. Such a code would now walk over elements inside a shadow tree. I don’t think this is desirable just as much as finding nodes via querySelector or getElementsByTagName inside shadow trees are not desirable.

[zcorpan]

@rniwa hum, that is a good point. So instead of returning ancestor, it should say:

If ancestor's root is a shadow root, then return null. Otherwise, return ancestor.

?

[rniwa]

No, I think if ancestor's root is a shadow root, then we need to look for its parent and adjust offsetLeft and offsetRight. Basically, we want to find the lowest ancestor offsetParent which is a direct child of one of ancestor trees of the node on which author called offsetParent.

[TakayoshiKochi]

@rniwa could you explain with an example? I don't get what you proposed yet.

[platosha]

@rniwa when I’m trying to traverse over offsetParents, which are the elements defining containing blocks for absolutely-positioned descendants, I want to discover all the nodes affecting position. I am deliberately trying to walk up the layout tree.

This is not the same as finding nodes with querySelector / getElementsByTagName, where layout does not affect the results. If I don’t care about the layout, I would traverse the parentNodes, which will not reveal any shadow-hidden nodes.

If offsetParent does not return shadow-hidden elements, I would expect it to be not allowed to define a containing block for distributed absolutely-positioned descendants with a shadow-hidden element. I. e., shadow-hidden relatively-positioned element does not affect positioning of distributed non-shadow-hidden children.

[rniwa]

@TakayoshiKochi : I’m proposing the step 2 of offsetParent from

If ancestor is not closed-shadow-hidden from the element and satisfies at least one of the following, terminate this algorithm and return ancestor.

to

If ancestor’s root is a shadow-including ancestor of the element and satisfies at least one of the following, terminate this algorithm and return ancestor.

and update offsetTop and offsetLeft accordingly.

@rniwa when I’m trying to traverse over offsetParents, which are the elements defining containing blocks for absolutely-positioned descendants, I want to discover all the nodes affecting position. I am deliberately trying to walk up the layout tree.

That breaks the encapsulation shadow DOM provides. If anything, that functionality should be provided with a new property like composedOffsetParent. We can discuss whether we should be adding such a property separately, but we can’t just let an existing DOM / CSS OM property expose a node inside a shadow tree.

[rniwa]

Actually, we can probably just say “if ancestor is a shadow-including ancestor” since if ancestor’s root is a shadow-including ancestor of the element, then ancestor is definitely an ancestor of the element in the flat tree as well. The only way flat tree's can ancestors can differ from that of shadow-including ancestors is by slots but an element inside a slot’s root can never be a shadow-including ancestor of the starting element.

[TakayoshiKochi]

@rniwa I see,

<div id="host" style="position: relative">
  <:shadow-root (open)>
    <div id="container" style="position: relative">
       <div id="inner">Hello, </div>
       <slot></slot>
     </div>
  </:shadow-root>
  <div id="outer">World</div>
</div>

jsbin demo

#inner.offsetParent is #container, and #outer.offsetParent will be #host, rather than #container (which is currently specced), right?

[TakayoshiKochi]

What's so wrong with exposing an element within open shadow root? That element is also discoverable with 'root.querySelector()'.
For encapsulation, closed shadow root already offers hiding all elements against .offsetParent from outside.

[rniwa]

What's so wrong with exposing an element within open shadow root? That element is also discoverable with root.querySelector().

The whole point of shadow DOM is to encapsulate nodes within a shadow tree accidentally or unintentionally. I’d argue that this is precisely one of those cases where a component can accidentally expose a node inside a shadow tree.

Let me put this way. If we didn’t care about this encapsulation, why do we even need to retarget event.target or event.relatedTarget? You could have easily argued that it’s not that bad to expose nodes inside a shadow tree in those cases too because you’re concerned about where the event originates from. But we didn’t because that’d break encapsulation.

[TakayoshiKochi]

That's a fair argument, thanks for the explanation.

What I am concerned is that if a web author doesn't care if his/her component exposes an internal element as offsetParent, or rather would like to get offsetParent for slotted element intentionally, and if we change offsetParent the way you propose, we lose the ability to know the exact offsetParent which is not closed-shadow-hidden to the context object. I cannot say that anyone doesn't need to poke offsetParent inside shadow trees at all with confidence.

I haven't tried yet, but can we write a polyfill to get the exact offsetParent algorithm (as in cssom-view spec as of today) using .assignedSlot etc.?

[rniwa]

@TakayoshiKochi: Something along the line of the code below should do the trick. Basically, you want to walk up the tree up until the “retargeted” offsetParent, and return offsetParent of a node assigned to the deepest slot that’s different from the original offsetParent. Note: I didn’t think through every edge case so there's mostly certainly a bug in the code.

function composedParentNode(node) {
    let offsetParent = node.offsetParent;
    let ancestor = node;
    let foundInsideSlot = false;
    while (ancestor && ancestor != offsetParent) {
        let assignedSlot = ancestor.assignedSlot;
        if (assignedSlot) {
            ancestor = assignedSlot;
            let newOffsetParent = assignedSlot.offsetParent;
            if (offsetParent != newOffsetParent)
                foundInsideSlot = true;
        } else if (ancestor.host && foundInsideSlot)
            break;
        ancestor = ancestor.host || ancestor.parentNode;
    }
    return offsetParent;
}

[TakayoshiKochi]

Thanks for the code. It looks like the original behavior can be emulated with the code or similar code, and also I missed you already mentioned that we could add composedOffsetParent for the original semantics (exposes nodes in open shadow root). Probably not many developers in the wild depends on the Chrome's current .offsetParent behavior, probably changing the spec earlier is better.
@zcorpan @hayatoito any opinions?

[hayatoito]

Hmm... Both arguments make sense to me. :(
The problem is: "Preventing accidental leak vs Honoring developer ergonomics".

However, as far as I know, offsetParent looks the only exceptional API which would return a node in a shadow tree, even though there is no explicit intention there. Given that, it might make sense to be conservative, as @rniwa suggested.

I know that this decision might be painful for developers, but we must pay the cost for consistency, I guess.

[hayatoito]

If we change the behavior of offserParent, we should add composedOffsetParent-ish because that is what most developers want. Even though it might be emulated in JS, we don't want each developers to write boilerplate code in JS. The platform should return the result much faster because it knows the layout tree.

[rniwa]

Per https://github.com/w3c/webcomponents/issues/497, the consensus at TPAC is to use the retargeting algorithm to find the offset parent, and the compute left and top from that node.

[rniwa]

In practice, this would meant that we walk up offset parents until we find an offset parent which is in the tree of a shadow-inclusive ancestor of the context object.

[emilio]

Yeah, that makes sense, you probably need a box... If the offset parent's retargeted node happens to be an inline or something with display: contents, having the offset be computed relative to that makes no sense (relative to what?).

[rniwa]

Right, simply returning the re-targeted node of offsetParent wouldn't work for that reason.

[rniwa]

https://trac.webkit.org/changeset/239313 implements the new behavior in WebKit.

[emilio]

Gecko patch is up in https://bugzilla.mozilla.org/show_bug.cgi?id=1514074.

[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