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

Style node management causes blocking error in ShadowDOM application. #4975

Closed
2 of 6 tasks
Westbrook opened this issue Dec 6, 2017 · 8 comments
Closed
2 of 6 tasks
Assignees

Comments

@Westbrook
Copy link
Contributor

Westbrook commented Dec 6, 2017

The changes found here (819652eb#diff-41695d1bc0c64b86f0e5a7878f58df0f) throw the following error:

Uncaught DOMException: Failed to execute 'insertBefore' on 'Node': The node before which the new node is to be inserted is not a child of this node.

When there is an <svg> element with a <style> tag inside of the template of an element that has styles included from a shared source.

Live Demo

http://jsbin.com/lumapiquda/1/edit?html,console,output

Steps to Reproduce

  1. Create my-element
  2. Add a styled SVG element to that component.
  3. Import external styles into that element.
  4. Run element

Expected Results

No errors throw.

Actual Results

Uncaught DOMException: Failed to execute 'insertBefore' on 'Node': The node before which the new node is to be inserted is not a child of this node.

Browsers Affected

  • Chrome
  • Firefox
  • Edge
  • Safari 11
  • Safari 8
  • IE 11

Versions

  • Polymer: v2.3.0
  • webcomponents: master
@LarsDenBakker
Copy link
Contributor

This is affecting our application as well.

@ghost
Copy link

ghost commented Dec 6, 2017

I am having the same error too.

@TimvdLippe
Copy link
Contributor

Confirmed an issue. I think the queryselector should be changed to :host > style, :host *:not(svg) style, per http://jsbin.com/dafiriquko/1/edit?html,output Assigned @azakus to this issue.

@Westbrook
Copy link
Contributor Author

Westbrook commented Dec 6, 2017

The more I review my code in #4976, the more I think the easier answer would be to use something like:

let lastStyle = template.content.firstElementChild;

Obviously, the variable naming should be updated to match, but this covers for more corner cases around <style>s being in unexpected parts of the template without the possibly false expectation that nested <style>s shouldn't have their content run through _processStyleText. AND, it allows for not changing the querySelectorAll at all, which support a clearer path to x-browser support.

@kevinpschaaf
Copy link
Member

The real issue is that the processing should only occur on HTML styles and exclude SVG styles, which I think is what @TimvdLippe was angling for. However, the querySelectorAll is being run in the template (before being stamped to a SR), so :host selectors will not work. As noted :scope is not x-plat, so we can't rely on that either. One option is to filter the qSA'ed list of <style>s on namespaceURI (<style> in <svg> will have an SVG namespace).

However, I see a couple additional issues. This code assumes lastStyle will be top-level in the template (we've never enforced that <style>s be top-level afaik):

template.content.insertBefore(s, lastStyle);

To correctly insert it before the last style, the code would need to be changed to this:

lastStyle.parentNode.insertBefore(s, lastStyle);

@azakus BUT, I'm actually confused how the existing logic around lastStyle doesn't result in out-of-order styles (which didn't happen with the previous css text gathering code). Doesn't the current logic result in all but the last concrete styles first, then all included styles next, then the last concrete style? This seems bad if so. Seems like "insert before lastStyle" is too simple, and the code should insert included styles after the previous style. Wdyt?

@kevinpschaaf
Copy link
Member

kevinpschaaf commented Dec 6, 2017

Ok, was slightly wrong there. Confirmed we don't actually need to separate SVG styles from HTML styles; once the lastStyle ordering and inserting logic is fixed the processing will be fine for all styles.

dfreedm added a commit that referenced this issue Dec 6, 2017
Find the correct spot to add "included" styles in the template by
keeping track of which "concrete" style in the template we have
encountered and inserting the "included" styles before them.

Fixes #4975
@bennypowers
Copy link

I had this same issue affecting an element which contained within it two style tags, one at the top of the template, and one in the middle of the template.

@dfreedm
Copy link
Member

dfreedm commented Dec 7, 2017

Fixed in #4979 and pushed as 2.3.1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants