search

Polymer / polymer-css-build

BSD 3-Clause "New" or "Revised" License
40 stars 9 forks source link

--no-inline-includes erroneously drops <style> tags #13

Closed freshp86 closed 7 years ago

freshp86 commented 7 years ago

When an element has more than one <style> tags, --no-inline-includes causes the first one to be dropped. Example below

bug.html

<dom-module id="my-style">
  <template>
    <style>
      :host {
        background-color: red;
      }
    </style>
  </template>
</dom-module>

<dom-module id="my-element">
  <template>
    <style include="my-style"></style>
    <style>
      :host([animating]) {
        background-color: green;
      }
    </style>
  </template>
</dom-module>

Then run

./node_modules/polymer-css-build/bin/polymer-css-build bug.html -o foo.html --no-inline-includes

foo.html becomes

<html><head></head><body><dom-module id="my-style" css-build="shadow">
  <template>
    <style scope="my-style">:host {
  background-color: red;
}

</style>
  </template>
</dom-module>

<dom-module id="my-element" css-build="shadow">
  <template>

    <style scope="my-element">:host([animating]) {
  background-color: green;
}

</style>
  </template>
</dom-module>
</body></html>

Notice how my-style is no longer referred by my-element. Omitting -no-inline-includes handles it correctly.

./node_modules/polymer-css-build/bin/polymer-css-build bug.html -o foo.html
<html><head></head><body><dom-module id="my-style" css-build="shadow">
  <template>
    <style scope="my-style">:host {
  background-color: red;
}

</style>
  </template>
</dom-module>

<dom-module id="my-element" css-build="shadow">
  <template>

    <style scope="my-element">:host {
  background-color: red;
}

:host([animating]) {
  background-color: green;
}

</style>
  </template>
</dom-module>
</body></html>
freshp86 commented 7 years ago

/cc @danbeam @kevinpschaaf @sorvell

danbeam commented 7 years ago

@azakus

danbeam commented 7 years ago

So it seems like during the combination of <style> tags part we're not keeping all include= attributes while "consuming" the <style> tags. This makes sense as they were previously being processed and left empty after inlining.

We should either:

  1. preserve includes while combining the text content (PR)
  2. leave <style>s more alone (and change the flag to like --preserve-styles) (PR)

I might favor 1.

dfreedm commented 7 years ago

I favor option 1 as well. Thanks for the PR!

dfreedm commented 7 years ago

Fixed with #14, released as v0.1.1

freshp86 commented 7 years ago

It would be nice to have some test coverage before closing this bug completely.

dfreedm commented 7 years ago

@freshp86 yeah that's fair, I'll go make one up

freshp86 commented 7 years ago

Thanks!

danbeam commented 7 years ago

btw, closed the other PR.

I agree that tests would be swell. I was going to write one; just wanted to know which route @azakus wanted to go first. Now that he's also volunteered to make one up, who am I to stop him :wink:

dfreedm commented 7 years ago

I made some tests, and cleaned up the implementation to actually work 😆

freshp86 commented 7 years ago

I made some tests, and cleaned up the implementation to actually work :laughing:

Thanks. Does this mean that we need v0.1.2 to actually get a working implementation?

dfreedm commented 7 years ago

@freshp86 yes