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

paper-styles destroy custom theme settings when used with importHref #93

Open
IntranetFactory opened this issue Jan 16, 2016 · 14 comments
Milestone

Comments

@IntranetFactory
Copy link
Contributor

When an element which imports paper-styles.html is loaded with importHref then custom theme settings get destroyed.

For example:

I have a basic custom theme defining just the value for one variable

<style is="custom-style">
  :root {
    --default-primary-color: #ff0000;
  }
</style>

In static.html this variable changes the color of a paper-checkbox as expected to red:
image

In failing.html I use importHref to import the paper-checkbox

      Polymer.Base.importHref('../paper-checkbox/paper-checkbox.html', function (e) {
        var cb = document.createElement('paper-checkbox');
        cb.checked = true;
        document.body.appendChild(cb);
      });

and now my-theme is overridden by paper-styles and reverts the checkbox color back to indigo
image

I understand why this currently happens.

I think paper-styles should not be able to just revert my custom theme settings.

@notwaldorf
Copy link
Contributor

Just to double check I understand what's happening, the tldr is that in an index.html you import my-theme.html, which sets --primary-color to red. Then you importHref a paper-checkbox, which brings in default-theme, and resets it to indigo. I think that kind of makes sense (even though it's annoying).

The much simpler analogy (I think) is that in your index.html you have a style

:root { --primary-color: red; }

And then later on (maybe in that same file even), you have another style:

:root { --primary-color: blue; }

I would expect the last write to win, so --primary-color to be blue, which is kind of the same as default-theme being imported last and having the last word when it comes to that color.

I'm going to cc our styling expert @sorvell because I don't have a good idea about a workaround here :(

@notwaldorf notwaldorf self-assigned this Jan 22, 2016
@IntranetFactory
Copy link
Contributor Author

the last write to win

I think that this behaviour might be a problem as this means that I need to know all dependencies of all elements which might change a variable and then import them all in my main document before my custom theme.

I think this breaks encapsulation.

What I'm missing is an option to define a variable as constant. This would then allow to include my custom theme first and no random element would be able to mess with the value of --primary-color defined in my theme.

@akc42
Copy link

akc42 commented Jan 29, 2016

@notwaldorf and @sorvell. If you look at the analysis I've been doing today of the problem in Polymer/polymer #3348 you will see that is pot luck which import of a theme happens first.

Can you put these theme variables into share-styles ?

@akc42
Copy link

akc42 commented Jan 30, 2016

@IntranetFactory. I found a compromise workaround for the issue. Do not load your theme directly in your html, instead in you app.js script add the following:

     app.addEventListener('dom-change', function() {
         var theme;
         function Listener(e) {
               theme.removeEventListener('load', Listener);
               Polymer.updateStyles();
               console.log('APP is underway');
         }
         //inject the theme as a temporary work around to timing problems
         theme = document.createElement('link');
         theme.setAttribute('rel', 'import');
         theme.setAttribute('href', 'styles/theme.html');
         theme.addEventListener('load', Listener);
         document.head.appendChild(theme);
   });

Its working great for me (other than 4 days ago the --text-primary-color is now the --primary-text-color variable, so I have just spent 4 hours figuring out why just one element had still not updated from my theme)

@IntranetFactory
Copy link
Contributor Author

@akc42 importHref already creates an link element dynamically. So you still need to ensure that the import of a custom theme is the absolute last element changing theme relevant vars.

@akc42
Copy link

akc42 commented Jan 31, 2016

@IntranetFactory That is why I wait for the dom-change event to create the element and inject it. By that time all the top level items in your index.html page are all attached. At that point I still get a brief flash of old theme before the new one kicks in.

The problem I was having was that with http/2's multiplexing some imports were arriving before others, with absolutely no control over which and when. Even though I put my theme as the very last thing to be loaded, it was so much shorter than the toolbar element that it was arriving first. Polymer processes the CSS as the element arrives and calls Polymer() with its prototype.

If course lazy loading could still screw things up, but then I only see the above solution as a temporary work around.

I don't know what a longer term solution would be. I suspect it would be something along the lines of
a) Not expecting elements to import a "theme", just call default.theme a default style. This is just a name change.
b) Have Polymer define a new type of extension to the <style> tag is="theme" or something for the theme files.

@notwaldorf
Copy link
Contributor

@IntranetFactory Okay. I after talking a bit with @shans, we think an easy workaround for this might be for your custom-theme to use something that's higher specificity than :root (which the default theme uses); something like body should work

So, in your custom-theme.html, can you replace your :root selector with body and see if this fixes the problem?

@IntranetFactory
Copy link
Contributor Author

So when I change

<style is="custom-style">

  :root {
    --primary-color: #ff0000;
  }

</style>

to

<style is="custom-style">

 body {
    --primary-color: #ff0000;
  }

</style>

the color is not applied at all. So I think that this doesn't work.

@arthurevans
Copy link

See my comment over here: Polymer/polymer#3348 (comment)

I think you can only define a custom property in a rule that matches a custom element (which includes the :host selector in an element's local DOM) or in a rule matching the :root scope in a custom-style tag. So body won't work here.

@IntranetFactory
Copy link
Contributor Author

@arthurevans do you have any alternate idea how to protect variables from a custom theme without the need to know the internals of all elements and their dependencies?

@arthurevans
Copy link

My point is that these shouldn't be treated as internals. If you're going to expose a set of theme-able properties and mixins, you need to:

  1. Have a file or files that define the default values for the theme properties.
  2. Document which properties are used by which elements, and which default files they import.

The person who's overriding these theme properties then knows exactly which files to import and which properties to override.

The method @akc42 described above also works, but it has two drawbacks:

  1. It loads the theme after the default theme has already been applied and the page rendered, causing a flash of unstyled (or at least incorrectly-styled) content, followed by potential re-layout and repaint.

  2. It doesn't componentize well. Imagine you want to vend a set of elements that build on top of the paper-elements, say, "my-paper-elements" and you want those elements to have their own theme (extending the base paper theme), it would be very hard for users to use. They'd have to do something like this:

  • wait for dom-change
    
  • use importHref to load your theme
    
  • wait for that import to load
    
  • use importHref to load their _own_ theme
    

On the other hand, if you use <link rel="import"> to express your dependency on paper-styles,
the end user only needs to make sure their theme file imports your theme file, and they're done.

@IntranetFactory
Copy link
Contributor Author

From my understanding that still means that the developer who creates the main page/the theme needs to know all components which might be used and the whole dependency tree of all elements, to ensure, that all their files which define default values, are loaded before the custom theme.

Whenever a component later changes its structure and e.g. moves the default definitions into a different file my applications theming might be broken.

So when an internal change in the file structure of any element can break my application, I think that some reliable solution should be searched to address that or it will be hard to rely on any 3rd party components if you want to build applications which can be themed.

@akc42
Copy link

akc42 commented Feb 11, 2016

@IntranetFactory I tried @arthurevans solution and is it working great!

I think what @arthurevans is saying is that if you use any third party library, then by default you must import their default-style file (if they have one) in your theme module. So using paper-toolbar must mean loading paper-default styles

But I agree its a pain in the backside. The problem is one of when these things get instanciated. because we already have "scopes" defined with the main app getting :root and any custom element gets own scope with (as far as I can work out) :host meaning a prototype scope and outside of host, but in the <style> element inside its getting a per instance scope.

It should be possible for default styles to only go into the (prototype) scopes of the elements they are importing into. It seems to me that is where custom-styles come in, and the ability to include them in elements. If default styles were as provided as a custom style for including in your element if you needed them we wouldn't get this leakage that is causing the problem

@akc42
Copy link

akc42 commented Feb 11, 2016

Actually I think I am wrong on the detail as I am still struggling with the concepts. I am currently in a styling phase of what I am doing and getting the correct margins round paper card elements is an interesting excercise.

I am not sure I have :host correct. It seems to apply to just the outside view of the element where as the rest of the css applies internally. - but I not even sure about that at the moment.

Nevertheless, the main thrust of what I am saying is still correct

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

4 participants