Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

HTMLElement.prototype.offsetParent leaks a node inside a shadow tree #497

Open
rniwa opened this issue May 7, 2016 · 11 comments
Open

HTMLElement.prototype.offsetParent leaks a node inside a shadow tree #497

rniwa opened this issue May 7, 2016 · 11 comments
Labels
blocked needs spec shadow-dom v1

Comments

@rniwa
Copy link
Collaborator

rniwa commented May 7, 2016 •
edited
Loading

We need to patch offsetParent so that when a node's container block is inside a shadow tree, we don't leak a node inside a shadow tree.

Chrome seems to return null in such cases. It's somewhat undesirable in that this makes the feature useless (since you can't determine where your containing block is) but I don't have a strong opinion about this as long as we don't leak a node inside a shadow tree.
@rniwa
Copy link
Collaborator Author

rniwa commented May 7, 2016 •
edited
Loading

@annevk @zcorpan @hayatoito @travisleithead

@rniwa
Copy link
Collaborator Author

rniwa commented May 7, 2016

It looks like Chrome returns null whenever the offset parent is inside a shadow tree. I think a better change would be to return null if the offset parent is not an unclosed node of the context object.

@rniwa
Copy link
Collaborator Author

rniwa commented May 7, 2016

An alternative solution is to walk up offsetParent enough to find the first offset parent that is unclosed node to the context object.

This is probably a better fix since authors won't be able to determine the location of an element which is assigned to a slot otherwise.

@kojiishi
Copy link

kojiishi commented May 8, 2016

Should offsetLeft and offsetTop be adjusted accordingly?

@rniwa
Copy link
Collaborator Author

rniwa commented May 8, 2016 •
edited
Loading

Right. Since offsetTop and offsetLeft are defined to use offsetParent already, that's kind of automatic.

@hayatoito hayatoito mentioned this issue Jun 1, 2016
Closed
@hayatoito
Copy link
Contributor

I have filed an issue for csswg: w3c/csswg-drafts#159

@hayatoito
Copy link
Contributor

PR is merged. Let me close this issue.

@rniwa
Copy link
Collaborator Author

rniwa commented Dec 10, 2016

Sorry, it looks like I missed an crucial point in w3c/csswg-drafts#159

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.

@rniwa rniwa reopened this Dec 10, 2016
@rniwa rniwa mentioned this issue Sep 22, 2018
Closed
@rniwa
Copy link
Collaborator Author

rniwa commented Oct 26, 2018

Rough consensus at TPAC F2F: Use the retargeting algorithm to find the offset parent, and the compute left and top from that node.

@caridy caridy mentioned this issue Oct 29, 2018
Open
kisg pushed a commit to paul99/webkit-mips that referenced this issue Dec 18, 2018
offsetLeft and offsetParent should adjust across shadow boundaries
9698ffb 
https://bugs.webkit.org/show_bug.cgi?id=157437
<rdar://problem/26154021>

Reviewed by Simon Fraser.

Source/WebCore:

Update the WebKit's treatment of shadow boundaries in offsetLeft, offsetTop, and offsetParent to match
the latest discussion in CSS WG. See WICG/webcomponents#497
and WICG/webcomponents#763

The latest consensus is to use the retargeting algorithm (https://dom.spec.whatwg.org/#retarget).
In practice, this would mean that we need to keep walking up the offset parent ancestors until we find
the one which is in the same tree as a shadow-inclusive ancestor of the context object.

For example, if a node (the context object of offsetTop, offsetLeft, offsetParent) was assigned to a slot
inside a shadow tree and its offset parent was in the shadow tree, we need to walk up to its offset parent,
then its offset parent, etc... until we find the offset parent in the same tree as the context object.

Note it's possible that the context object is inside a shadow tree which does not have its own offset parent.
(e.g. all elements have position: static) For this reason, we need to consider not just offset parent in
the same tree as the context object but as well as any offset parent which is in its ancestor trees.

Test: fast/shadow-dom/offsetParent-across-shadow-boundaries.html

* dom/Element.cpp:
(WebCore::adjustOffsetForZoomAndSubpixelLayout): Extracted to share code between offsetLeft and offsetTop.
(WebCore::collectAncestorTreeScopeAsHashSet): Added.
(WebCore::Element::offsetLeftForBindings): Added. Sums up offsetLeft's until it finds the first offset parent
which is a shadow-including ancestor (https://dom.spec.whatwg.org/#concept-shadow-including-ancestor).
(WebCore::Element::offsetLeft): Now uses adjustOffsetForZoomAndSubpixelLayout.
(WebCore::Element::offsetTopForBindings): Added. Like offsetLeftForBindings, this function sums up offsetTop's
until it finds the first offset parent which is a shadow-including ancestor.
(WebCore::Element::offsetTop): Now uses adjustOffsetForZoomAndSubpixelLayout.
(WebCore::Element::offsetParentForBindings): Renamed from bindingsOffsetParent to be consistent with other
functions meant to be used for bindings code.
* dom/Element.h:
* html/HTMLElement.idl:

Source/WebKit:

Use *forBindings variants of offsetLeft, offsetTop, and offsetParent.

* WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElementGtk.cpp:
(webkit_dom_element_get_offset_left):
(webkit_dom_element_get_offset_top):
(webkit_dom_element_get_offset_parent):

Source/WebKitLegacy/mac:

Use *forBindings variants of offsetLeft, offsetTop, and offsetParent.

* DOM/DOMElement.mm:
(-[DOMElement offsetLeft]):
(-[DOMElement offsetTop]):
(-[DOMElement offsetParent]):

LayoutTests:

Added a W3C style testharness.js test.

* fast/shadow-dom/offsetParent-across-shadow-boundaries-expected.txt: Added.
* fast/shadow-dom/offsetParent-across-shadow-boundaries.html: Added.


git-svn-id: http://svn.webkit.org/repository/webkit/trunk@239313 268f45cc-cd09-0410-ab3c-d52691b4dbfc
@rniwa
Copy link
Collaborator Author

rniwa commented Apr 25, 2019

We need to resolve CSS OM View spec change in w3c/csswg-drafts#159

ryanhaddad pushed a commit to WebKit/WebKit that referenced this issue Dec 22, 2020
@rniwa
offsetLeft and offsetParent should adjust across shadow boundaries
b643ac3 
https://bugs.webkit.org/show_bug.cgi?id=157437
<rdar://problem/26154021>

Reviewed by Simon Fraser.

Source/WebCore:

Update the WebKit's treatment of shadow boundaries in offsetLeft, offsetTop, and offsetParent to match
the latest discussion in CSS WG. See WICG/webcomponents#497
and WICG/webcomponents#763

The latest consensus is to use the retargeting algorithm (https://dom.spec.whatwg.org/#retarget).
In practice, this would mean that we need to keep walking up the offset parent ancestors until we find
the one which is in the same tree as a shadow-inclusive ancestor of the context object.

For example, if a node (the context object of offsetTop, offsetLeft, offsetParent) was assigned to a slot
inside a shadow tree and its offset parent was in the shadow tree, we need to walk up to its offset parent,
then its offset parent, etc... until we find the offset parent in the same tree as the context object.

Note it's possible that the context object is inside a shadow tree which does not have its own offset parent.
(e.g. all elements have position: static) For this reason, we need to consider not just offset parent in
the same tree as the context object but as well as any offset parent which is in its ancestor trees.

Test: fast/shadow-dom/offsetParent-across-shadow-boundaries.html

* dom/Element.cpp:
(WebCore::adjustOffsetForZoomAndSubpixelLayout): Extracted to share code between offsetLeft and offsetTop.
(WebCore::collectAncestorTreeScopeAsHashSet): Added.
(WebCore::Element::offsetLeftForBindings): Added. Sums up offsetLeft's until it finds the first offset parent
which is a shadow-including ancestor (https://dom.spec.whatwg.org/#concept-shadow-including-ancestor).
(WebCore::Element::offsetLeft): Now uses adjustOffsetForZoomAndSubpixelLayout.
(WebCore::Element::offsetTopForBindings): Added. Like offsetLeftForBindings, this function sums up offsetTop's
until it finds the first offset parent which is a shadow-including ancestor.
(WebCore::Element::offsetTop): Now uses adjustOffsetForZoomAndSubpixelLayout.
(WebCore::Element::offsetParentForBindings): Renamed from bindingsOffsetParent to be consistent with other
functions meant to be used for bindings code.
* dom/Element.h:
* html/HTMLElement.idl:

Source/WebKit:

Use *forBindings variants of offsetLeft, offsetTop, and offsetParent.

* WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMElementGtk.cpp:
(webkit_dom_element_get_offset_left):
(webkit_dom_element_get_offset_top):
(webkit_dom_element_get_offset_parent):

Source/WebKitLegacy/mac:

Use *forBindings variants of offsetLeft, offsetTop, and offsetParent.

* DOM/DOMElement.mm:
(-[DOMElement offsetLeft]):
(-[DOMElement offsetTop]):
(-[DOMElement offsetParent]):

LayoutTests:

Added a W3C style testharness.js test.

* fast/shadow-dom/offsetParent-across-shadow-boundaries-expected.txt: Added.
* fast/shadow-dom/offsetParent-across-shadow-boundaries.html: Added.


Canonical link: https://commits.webkit.org/207360@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@239313 268f45cc-cd09-0410-ab3c-d52691b4dbfc
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 24, 2021
@josepharhar @chromium-wpt-export-bot
Update offsetParent behavior in shadow trees
e4737da 
New behavior for offsetParent in shadow trees was discussed in [1] and
[2]. This lead to a chromium patch [3] which changed the behavior. After
[3] landed, the desired behavior in the discussions in [1] and [2]
seemed to have changed. Unfortunately, the author of [3] was no longer
working on chromium. This new behavior was then added to the spec and
landed in webkit [4] and firefox [5], and a WPT was added for it [6].

This patch implements the new behavior to follow suit with webkit and
firefox based on the WPT in [6]. Unfortunately, there are several tests
which are either internal to chromium or are only passing in chromium
which appear to oppose this new behavior and will have to be updated or
removed:
- external/wpt/css/css-contain/content-visibility/content-visibility-035.html
- external/wpt/css/css-contain/content-visibility/content-visibility-044.html
- fast/dom/shadow/offset-parent-does-not-leak-ua-shadow.html
- shadow-dom/offsetParent.html
For shadow-dom/offsetParent.html, I verified that firefox and safari
both currently fail the same tests which this patch does.

[1] WICG/webcomponents#497
[2] w3c/csswg-drafts#159
[3] https://chromium.googlesource.com/chromium/src/+/18d455ee833f6a30dcbe2755380861eb75cd9f6f
[4] https://trac.webkit.org/changeset/239313/webkit
[5] https://bugzilla.mozilla.org/show_bug.cgi?id=1514074
[6] https://wpt.fyi/results/shadow-dom/offsetParent-across-shadow-boundaries.html

Fixed: 920069
Change-Id: I168edc5ad0e4fcb92d0c4a440623f2424b14a988
Merged
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Mar 31, 2021
@josepharhar @chromium-wpt-export-bot
Update offsetParent behavior in shadow trees
f5387e1 
New behavior for offsetParent in shadow trees was discussed in [1] and
[2]. This lead to a chromium patch [3] which changed the behavior. After
[3] landed, the desired behavior in the discussions in [1] and [2]
seemed to have changed. Unfortunately, the author of [3] was no longer
working on chromium. This new behavior was then added to the spec and
landed in webkit [4] and firefox [5], and a WPT was added for it [6].

This patch implements the new behavior to follow suit with webkit and
firefox based on the WPT in [6]. Unfortunately, there are several tests
which are either internal to chromium or are only passing in chromium
which appear to oppose this new behavior and will have to be updated or
removed:
- external/wpt/css/css-contain/content-visibility/content-visibility-035.html
- external/wpt/css/css-contain/content-visibility/content-visibility-044.html
- fast/dom/shadow/offset-parent-does-not-leak-ua-shadow.html
- shadow-dom/offsetParent.html
For shadow-dom/offsetParent.html, I verified that firefox and safari
both currently fail the same tests which this patch does.

[1] WICG/webcomponents#497
[2] w3c/csswg-drafts#159
[3] https://chromium.googlesource.com/chromium/src/+/18d455ee833f6a30dcbe2755380861eb75cd9f6f
[4] https://trac.webkit.org/changeset/239313/webkit
[5] https://bugzilla.mozilla.org/show_bug.cgi?id=1514074
[6] https://wpt.fyi/results/shadow-dom/offsetParent-across-shadow-boundaries.html

Fixed: 920069
Change-Id: I168edc5ad0e4fcb92d0c4a440623f2424b14a988
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 14, 2021
@josepharhar @chromium-wpt-export-bot
Update offsetParent behavior in shadow trees
44e3aef 
New behavior for offsetParent in shadow trees was discussed in [1] and
[2]. This lead to a chromium patch [3] which changed the behavior. After
[3] landed, the desired behavior in the discussions in [1] and [2]
seemed to have changed. Unfortunately, the author of [3] was no longer
working on chromium. This new behavior was then added to the spec and
landed in webkit [4] and firefox [5], and a WPT was added for it [6].

This patch implements the new behavior to follow suit with webkit and
firefox based on the WPT in [6]. Unfortunately, there are several tests
which are either internal to chromium or are only passing in chromium
which appear to oppose this new behavior and will have to be updated or
removed:
- external/wpt/css/css-contain/content-visibility/content-visibility-035.html
- external/wpt/css/css-contain/content-visibility/content-visibility-044.html
- fast/dom/shadow/offset-parent-does-not-leak-ua-shadow.html
- shadow-dom/offsetParent.html
For shadow-dom/offsetParent.html, I verified that firefox and safari
both currently fail the same tests which this patch does.

[1] WICG/webcomponents#497
[2] w3c/csswg-drafts#159
[3] https://chromium.googlesource.com/chromium/src/+/18d455ee833f6a30dcbe2755380861eb75cd9f6f
[4] https://trac.webkit.org/changeset/239313/webkit
[5] https://bugzilla.mozilla.org/show_bug.cgi?id=1514074
[6] https://wpt.fyi/results/shadow-dom/offsetParent-across-shadow-boundaries.html

Fixed: 920069
Change-Id: I168edc5ad0e4fcb92d0c4a440623f2424b14a988
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 15, 2021
@josepharhar @chromium-wpt-export-bot
Update offsetParent behavior in shadow trees
372ea22 
New behavior for offsetParent in shadow trees was discussed in [1] and
[2]. This lead to a chromium patch [3] which changed the behavior. After
[3] landed, the desired behavior in the discussions in [1] and [2]
seemed to have changed. Unfortunately, the author of [3] was no longer
working on chromium. This new behavior was then added to the spec and
landed in webkit [4] and firefox [5], and a WPT was added for it [6].

This patch implements the new behavior to follow suit with webkit and
firefox based on the WPT in [6]. Unfortunately, there are several tests
which are either internal to chromium or are only passing in chromium
which appear to oppose this new behavior and will have to be updated or
removed:
- external/wpt/css/css-contain/content-visibility/content-visibility-035.html
- external/wpt/css/css-contain/content-visibility/content-visibility-044.html
- fast/dom/shadow/offset-parent-does-not-leak-ua-shadow.html
- shadow-dom/offsetParent.html
For shadow-dom/offsetParent.html, I verified that firefox and safari
both currently fail the same tests which this patch does.

[1] WICG/webcomponents#497
[2] w3c/csswg-drafts#159
[3] https://chromium.googlesource.com/chromium/src/+/18d455ee833f6a30dcbe2755380861eb75cd9f6f
[4] https://trac.webkit.org/changeset/239313/webkit
[5] https://bugzilla.mozilla.org/show_bug.cgi?id=1514074
[6] https://wpt.fyi/results/shadow-dom/offsetParent-across-shadow-boundaries.html

Fixed: 920069
Change-Id: I168edc5ad0e4fcb92d0c4a440623f2424b14a988
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2775208
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#872658}
chromium-wpt-export-bot pushed a commit to web-platform-tests/wpt that referenced this issue Apr 15, 2021
@josepharhar @chromium-wpt-export-bot
Update offsetParent behavior in shadow trees
8d8b8c4 
New behavior for offsetParent in shadow trees was discussed in [1] and
[2]. This lead to a chromium patch [3] which changed the behavior. After
[3] landed, the desired behavior in the discussions in [1] and [2]
seemed to have changed. Unfortunately, the author of [3] was no longer
working on chromium. This new behavior was then added to the spec and
landed in webkit [4] and firefox [5], and a WPT was added for it [6].

This patch implements the new behavior to follow suit with webkit and
firefox based on the WPT in [6]. Unfortunately, there are several tests
which are either internal to chromium or are only passing in chromium
which appear to oppose this new behavior and will have to be updated or
removed:
- external/wpt/css/css-contain/content-visibility/content-visibility-035.html
- external/wpt/css/css-contain/content-visibility/content-visibility-044.html
- fast/dom/shadow/offset-parent-does-not-leak-ua-shadow.html
- shadow-dom/offsetParent.html
For shadow-dom/offsetParent.html, I verified that firefox and safari
both currently fail the same tests which this patch does.

[1] WICG/webcomponents#497
[2] w3c/csswg-drafts#159
[3] https://chromium.googlesource.com/chromium/src/+/18d455ee833f6a30dcbe2755380861eb75cd9f6f
[4] https://trac.webkit.org/changeset/239313/webkit
[5] https://bugzilla.mozilla.org/show_bug.cgi?id=1514074
[6] https://wpt.fyi/results/shadow-dom/offsetParent-across-shadow-boundaries.html

Fixed: 920069
Change-Id: I168edc5ad0e4fcb92d0c4a440623f2424b14a988
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2775208
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#872658}
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Apr 24, 2021
@josepharhar @moz-wptsync-bot
Bug 1700509 [wpt PR 28201] - Update offsetParent behavior in shadow t…
43ac9a3 
…rees, a=testonly

Automatic update from web-platform-tests
Update offsetParent behavior in shadow trees

New behavior for offsetParent in shadow trees was discussed in [1] and
[2]. This lead to a chromium patch [3] which changed the behavior. After
[3] landed, the desired behavior in the discussions in [1] and [2]
seemed to have changed. Unfortunately, the author of [3] was no longer
working on chromium. This new behavior was then added to the spec and
landed in webkit [4] and firefox [5], and a WPT was added for it [6].

This patch implements the new behavior to follow suit with webkit and
firefox based on the WPT in [6]. Unfortunately, there are several tests
which are either internal to chromium or are only passing in chromium
which appear to oppose this new behavior and will have to be updated or
removed:
- external/wpt/css/css-contain/content-visibility/content-visibility-035.html
- external/wpt/css/css-contain/content-visibility/content-visibility-044.html
- fast/dom/shadow/offset-parent-does-not-leak-ua-shadow.html
- shadow-dom/offsetParent.html
For shadow-dom/offsetParent.html, I verified that firefox and safari
both currently fail the same tests which this patch does.

[1] WICG/webcomponents#497
[2] w3c/csswg-drafts#159
[3] https://chromium.googlesource.com/chromium/src/+/18d455ee833f6a30dcbe2755380861eb75cd9f6f
[4] https://trac.webkit.org/changeset/239313/webkit
[5] https://bugzilla.mozilla.org/show_bug.cgi?id=1514074
[6] https://wpt.fyi/results/shadow-dom/offsetParent-across-shadow-boundaries.html

Fixed: 920069
Change-Id: I168edc5ad0e4fcb92d0c4a440623f2424b14a988
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2775208
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: vmpstr <vmpstr@chromium.org>
Reviewed-by: Ian Kilpatrick <ikilpatrick@chromium.org>
Commit-Queue: Joey Arhar <jarhar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#872658}

--

wpt-commits: 8d8b8c4e2e42d07398fd5e98b541ee239e1d35a4
wpt-pr: 28201
@eKoopmans
Copy link

Note: This behaviour appears to have now been updated in Chrome 109 (and earlier versions of Firefox/Webkit) so that it no longer leaks elements within a shadow DOM.

If you need the original behaviour, see this comment from October 2022 and the linked polyfill:
https://github.com/josepharhar/offsetparent-polyfills

@rniwa rniwa added the blocked label Apr 20, 2023
@graynorton graynorton mentioned this issue Oct 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
blocked needs spec shadow-dom v1
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@hayatoito @rniwa @kojiishi @annevk @eKoopmans