Skip to content

🐛 fix purgeShadowRoots sometimes fails to recognize connected nodes as such#15906

Merged
danielrozenberg merged 2 commits intoampproject:masterfrom
danielrozenberg:is-connected-node-shadow-root-hosts
Jun 14, 2018
Merged

🐛 fix purgeShadowRoots sometimes fails to recognize connected nodes as such#15906
danielrozenberg merged 2 commits intoampproject:masterfrom
danielrozenberg:is-connected-node-shadow-root-hosts

Conversation

@danielrozenberg
Copy link
Copy Markdown
Member

The purgeShadowRoots_ method looks to see whether shadow roots are attached by checking whether this.win.document.contains(shadowroot.host) which fails on occasion (for reasons.) This PR replaces this logic with the more predictable isConnected (more specifically with @jridgewell's isConnectedNode polyfill)

Fix is basically 99% from @dvoytenko, thanks!

This issue affects pages that have multiple shadow roots, and so it resolves #12645

Copy link
Copy Markdown
Contributor

@dvoytenko dvoytenko left a comment

Choose a reason for hiding this comment

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

Any tests need to be updated?

@danielrozenberg
Copy link
Copy Markdown
Member Author

Updating some tests now, but they seem to be related to infra changes, not to this PR

@dvoytenko
Copy link
Copy Markdown
Contributor

It looks like test-runtime.js has some tests for this. E.g. see "should stop broadcasting after force-close". Possibly would be good to add this there.

@danielrozenberg danielrozenberg requested a review from rsimha June 8, 2018 15:35
@danielrozenberg
Copy link
Copy Markdown
Member Author

/to @dvoytenko I'm not sure what you mean wrt that test in your previous comment. What should I do with it?

/to @rsimha I modified _init_tests.js to allow duplicate error messages in expectAsyncConsoleError, since some of these tests throw the same error message twice. Is there a reason why you didn't allow duplicate error messages in the first place? I was also thinking that this API should accept RegExp, not just complete strings, and that we can have a 2nd optional parameter to mark that an error should be expected n times (instead of calling this method twice with the same params). wdyt?

@rsimha
Copy link
Copy Markdown
Contributor

rsimha commented Jun 8, 2018

I think @alanorozco has already made it so you can pass in a regex. And yeah, optionally passing in a count sgtm.

@alanorozco
Copy link
Copy Markdown
Member

Yup, it now accepts regex!

@dvoytenko
Copy link
Copy Markdown
Contributor

@danielrozenberg here's an idea of the test, similar to the "should stop broadcasting after force-close". Stub document.contains to always return false and try to purge the doc. It should stay b/c now it will always return true by isConnected.

@danielrozenberg danielrozenberg force-pushed the is-connected-node-shadow-root-hosts branch 2 times, most recently from 1f73789 to 69f530e Compare June 12, 2018 20:39
@danielrozenberg danielrozenberg force-pushed the is-connected-node-shadow-root-hosts branch from 69f530e to c30c813 Compare June 12, 2018 21:31
@danielrozenberg
Copy link
Copy Markdown
Member Author

@dvoytenko PTAL

@danielrozenberg danielrozenberg merged commit 9517ad2 into ampproject:master Jun 14, 2018
@danielrozenberg danielrozenberg deleted the is-connected-node-shadow-root-hosts branch June 14, 2018 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

shadow dom: Incorrect values for CANONICAL_URL variable

5 participants