Skip to content

[cssom-view] Fix #159 update offsetParent spec for Shadow DOM - #252

Merged
zcorpan merged 2 commits into
w3c:masterfrom
TakayoshiKochi:offsetparent
Aug 11, 2016
Merged

[cssom-view] Fix #159 update offsetParent spec for Shadow DOM#252
zcorpan merged 2 commits into
w3c:masterfrom
TakayoshiKochi:offsetparent

Conversation

@TakayoshiKochi

@TakayoshiKochi TakayoshiKochi commented Jun 29, 2016

Copy link
Copy Markdown
Member

The previous attempt to update offsetParent to fix #159 was incomplete:
when the real offsetParent in the closed shadow tree has
'position:fixed', offsetParent should return null, instead
of continue searching for unclosed ancestor.

@TakayoshiKochi TakayoshiKochi changed the title Update offsetParent spec for Shadow DOM [cssom-view] Fix #159 update offsetParent spec for Shadow DOM Jun 29, 2016
@TakayoshiKochi

Copy link
Copy Markdown
Member Author

@zcorpan @tabatkins @hayatoito Could you take a look?

Comment thread cssom-view/Overview.bs Outdated
* It is <a>the HTML <code>body</code> element</a>.
* The computed value of the 'position' property of the element is ''static'' and the ancestor is one of the following <a>HTML elements</a>: <code>td</code>, <code>th</code>, or <code>table</code>.
1. Return null.
1. Let <var>ancestor</var> be the element and repeat these substeps:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ancestor should be the parent of the element here.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, fixed.

Comment thread cssom-view/Overview.bs Outdated
1. Return null.
1. Let <var>ancestor</var> be the parent of the element and repeat these substeps:
1. If <var>ancestor</var> is not an <a lt="unclosed node">unclosed</a> element of the element and its computed value of the 'position' property is ''position/fixed'', terminate this algorithm and return null.
1. If <var>ancestor</var> is an <a lt="unclosed node">unclosed</a> element of the element and satisfies at least one of the following is true, terminate this algorithm and return <var>ancestor</var>.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove "is true"

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

@zcorpan

zcorpan commented Jun 29, 2016

Copy link
Copy Markdown
Contributor

LGTM % wording nit

@TakayoshiKochi

Copy link
Copy Markdown
Member Author

@tabatkins could you take a look?

@TakayoshiKochi

Copy link
Copy Markdown
Member Author

@tabatkins Ping?

1 similar comment
@TakayoshiKochi

Copy link
Copy Markdown
Member Author

@tabatkins Ping?

@TakayoshiKochi

Copy link
Copy Markdown
Member Author

@tabatkins Had a nice vacation ;) ?

@tabatkins

Copy link
Copy Markdown
Member

Hmm, this reads a little weird to me. I'm not certain the first condition can ever be hit - we're walking the ancestor chain of the element, and I think an element's ancestors are always unclosed relative to it.

@TakayoshiKochi

Copy link
Copy Markdown
Member Author

No, it happens when an element is slotted into a closed shadow tree.

<div id="shadow" style="position: relative">
  #shadow-root (closed)
     <div id="container" style="position: relative">
        <slot></slot>
     </div>
  <div id="X">Hello</div>
</div>

div#X's real offsetParent is div#container, but is unclosed to div#X, it will return
div#shadow.

Hmm, I should have used "the parent of the element in the flat tree" for all "the parent of ~"
occurrences.

@TakayoshiKochi

Copy link
Copy Markdown
Member Author

@tabatkins Could you take another look?

Comment thread cssom-view/Overview.bs Outdated
* It is <a>the HTML <code>body</code> element</a>.
* The computed value of the 'position' property of the element is ''static'' and the ancestor is one of the following <a>HTML elements</a>: <code>td</code>, <code>th</code>, or <code>table</code>.
1. Return null.
1. Let <var>ancestor</var> be the parent of the element in the flat tree and repeat these substeps:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Link <a>flat tree</a>.

@tabatkins

Copy link
Copy Markdown
Member

Okay, final nit, then +1 from me.

The previous attempt to update offsetParent was incomplete:
when the real offsetParent in the closed shadow tree has
'position:fixed', offsetParent should return null, instead
of continue searching for unclosed ancestor.
@TakayoshiKochi

Copy link
Copy Markdown
Member Author

Done. Thanks for the review!

@TakayoshiKochi

Copy link
Copy Markdown
Member Author

@zcorpan could you merge this?

@zcorpan
zcorpan merged commit 9386def into w3c:master Aug 11, 2016
@zcorpan

zcorpan commented Aug 11, 2016

Copy link
Copy Markdown
Contributor

Thank you!

@TakayoshiKochi
TakayoshiKochi deleted the offsetparent branch August 12, 2016 02:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

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

4 participants