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.

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

Run adopt as part of insert #25460

Closed
wants to merge 1 commit into from
Closed

Conversation

pshaughn
Copy link
Contributor

@pshaughn pshaughn commented Jan 7, 2020

Changes of whatwg/dom#754 are reflected, but we mostly aren't passing the associated tests. In many cases this is because we don't actually have the shadow DOM functionality that makes this behavior relevant, but that's not all that's being tested and there is probably some other missing piece.

It might make sense to merge this (assuming it passes CI) and open another issue investigating the failures?


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fulfill the letter of Run adopt as part of insert #25265 but aren't passing all the associated tests
  • There are tests for these changes

@highfive
Copy link

highfive commented Jan 7, 2020

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/document.rs, components/script/dom/shadowroot.rs, components/script/dom/node.rs, components/script/dom/documentfragment.rs, components/script/dom/htmltemplateelement.rs
  • @KiChjang: components/script/dom/document.rs, components/script/dom/shadowroot.rs, components/script/dom/node.rs, components/script/dom/documentfragment.rs, components/script/dom/htmltemplateelement.rs

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jan 7, 2020
@highfive
Copy link

highfive commented Jan 7, 2020

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!

if fragment.get_host().is_some() {
// Spec text doesn't actually say what to return,
// but IDL doesn't allow null, and
// returning the node itself seems to be what's expected
Copy link
Member

Choose a reason for hiding this comment

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

I went to open a spec issue about this and found whatwg/dom#813.

@jdm
Copy link
Member

jdm commented Jan 7, 2020

Given whatwg/dom#813 (comment), perhaps we should hold off on this work until it settles in spec land.

@pshaughn
Copy link
Contributor Author

pshaughn commented Jan 7, 2020

That makes sense.

@Manishearth Manishearth added S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jan 23, 2020
@pshaughn
Copy link
Contributor Author

whatwg/dom#819 hasn't landed yet but whatwg/dom#813 is still unresolved, so if adopt is going to be moved, it will probably be in a different enough way to require completely different code changes.

@pshaughn pshaughn closed this Feb 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked-on-external Something, somewhere else, needs to happen before this PR can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants