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

fix: getRootNode() bug #213

Merged
merged 1 commit into from
Jul 19, 2023
Merged

fix: getRootNode() bug #213

merged 1 commit into from
Jul 19, 2023

Conversation

luwes
Copy link
Contributor

@luwes luwes commented Jul 18, 2023 •
edited
Loading

the <html> element is HTMLHtmlElement

getRootNode() returns

An object inheriting from Node. This will differ in exact form depending on where you called getRootNode(); for example:

  • Calling it on an element inside a standard web page will return an HTMLDocument object representing the entire page (or <iframe>).
  • Calling it on an element inside a shadow DOM will return the associated ShadowRoot.

I ran in to this bug when calling getElementById on the root node but the method didn't exist with Linkedom.
Code here https://github.com/muxinc/media-chrome/blob/main/src/js/media-theme-element.js#L177-L178

@WebReflection
Copy link
Owner

WebReflection commented Jul 19, 2023 •
edited
Loading

This breaks the test which was already showing the getRoot() returns the documentElement so it's not clear what this fixes or why it's needed.

@luwes
Copy link
Contributor Author

luwes commented Jul 19, 2023

^^ updated

@WebReflection
Copy link
Owner

OK, thanks! this looks like an unintentional bug indeed ... however, code-coverage isn't super happy now ... mind trying to understand what's going on or why something is not covered?

npx c8 report --reporter=html

this should give you a coverage/index.html with all stats per file so you can see which one is not covered anymore or why.

@WebReflection WebReflection merged commit 7a0c21e into WebReflection:main Jul 19, 2023
1 check passed
@luwes
Copy link
Contributor Author

luwes commented Jul 19, 2023

it has to do with the mutation observer tests it seems

mutation-observer.js | 100 | 95 | 100 | 100 | 79-80

maybe it needs observed.getRootNode().documentElement now?

@WebReflection
Copy link
Owner

it was passing through the || before, but it works as expected so we're good now ... it's up and running.

@luwes luwes deleted the getrootnode branch July 19, 2023 14:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@luwes @WebReflection