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

Move dynamically added styles into Shadow DOM #26

Closed
ghost opened this issue Nov 27, 2014 · 17 comments
Closed

Move dynamically added styles into Shadow DOM #26

ghost opened this issue Nov 27, 2014 · 17 comments

Comments

@ghost
Copy link

ghost commented Nov 27, 2014

It would be good if google-chart support the Table graph.

@pkuvegeta
Copy link

+1

@sgomes sgomes self-assigned this Dec 2, 2014
@sgomes
Copy link
Contributor

sgomes commented Dec 2, 2014

Thanks for the suggestion!

There's now a few graph types that folks have suggested that are part of extra packages, so I'll need to look into what the best strategy is for loading them; whether we should do so preemptively or only once the chart type is defined.

@SmokyBob
Copy link
Contributor

SmokyBob commented Jan 8, 2015

+1

@sgomes
Copy link
Contributor

sgomes commented Jan 9, 2015

Thanks for the PR, @SmokyBob! I have a similar implementation in the works, but yours is cleaner, so I'll probably just use that, thanks! However, it's running into the same issue as mine: loading extra CSS.

The table chart type, as well as a few others (such as the org chart type) use native HTML elements instead of SVG to draw the charts. This is all well and good, except that they load extra CSS to style them, and they do so by attaching it to the document head. That does not play well with the shadow DOM, and means the elements end up unstyled.

I see several approaches to solve this problem:

  • Leave it up to the user to figure out. Not the best approach, since default styling is something they usually get for free, and it isn't exactly easy for them to get a copy of the dynamically loaded CSS resources to start from.
  • Include copies of the extra CSS with google-chart. We'd need to keep a close eye on any Google Visualization API releases to make sure all CSS is always up to date for all chart types. And this is assuming the API isn't doing any sort of clever on-the-fly CSS rule writing.
  • Set up an object observer on the document head, and for every dynamically added CSS resource rewrite all added CSS rules to pierce the shadow DOM boundaries. Parsing CSS is a bit of a nightmare, so this seems like a bad idea.
  • Set up an object observer on the document head, and copy any dynamically added CSS documents to inside the element's shadow DOM. This would introduce duplication on the order of how many elements there are on the page.

The latter is my currently favoured approach, but I'm not particularly happy with any of these. I'd love to hear what the folks following the issue have to think on this!

@SmokyBob
Copy link
Contributor

Hi @sgomes, glad to help.
I took sometime thinking about what could be a solution that is both functional and the least difficult to manage.
Would be awesome to be able to directly get the styles from the visualization API with something like google.visualization.getStyles() or google.visualization.Table.getStyles() to get the URL and add the import in the shadow DOM from the component without using and observer... but for now observing the head element for changes seems the only viable way.
My2Cents

@SmokyBob
Copy link
Contributor

Tried to set up the observer to get the CSS.
No problem identifying when the CSS element is added to the head, when the browser finish to load the stylesheet.... but I have no Idea how to get the CSS styles themselves to add them in the shadowroot...

Here a gist with the code I've tried
CORS is not supported so the second piece of code doesn't work, the first waits for the link element to be loaded and try to search for one of the classes that are loaded by the API as you can find out in the devtools...

If anyone has any other Ideas, they are more than welcome

@sgomes
Copy link
Contributor

sgomes commented Feb 2, 2015

So to add to this, as far as I can tell there's currently no way to implement support for the added styling.

It's easy enough to detect changes, as @SmokyBob mentions, but we then need a way to actually apply them as needed:

  • adding a link inside the shadow DOM: this approach does not work, since links are ignored inside the shadow DOM (this was a surprise to me). Polymer's apparent support for <link> tags inside the element template is, I suspect, based on some clever static tricks; in any case, any dynamically inserted <link> tags get promptly ignored.
  • adding a style tag with the content: as @SmokyBob mentioned, CORS is not enabled, so we can't make an XHR request to retrieve the styles.

Unless I'm missing something, the only solution for now seems to be to provide the markup, but no styling. Polymer 0.8 may give us a way of solving this, but for now it seems we're stuck :(

@philipwalton
Copy link
Member

I think the <google-chart> element should package up some default styling in it's shadow DOM. That would be the "Web Component-y" way of doing things (rather than using global styles with the /deep/or >>> combinators). The default styling could be the same as the current table chart defaults, but I don't see any reason it has to be (it's kinda ugly).

If users want to override the default styles they have a few options:

  1. Extend the <google-chart> element and set their own style overrides.
  2. Compose the <google-chart> element inside of their own element and use the ::shadow pseudo-element selector to override the default styles.
  3. Set the cssClassNames chart option in combination with either 1) or 2) above to start fresh.

I'm happy to submit a PR with these changes as I want to get this working with the <google-analytics-chart> element anyway.

@sgomes what do you think?

@SmokyBob
Copy link
Contributor

SmokyBob commented Feb 3, 2015

@philipwalton IMO adding the default styling, even if a bit old and didn't age well is a good idea and compromise.
Using the current table styling just to keep the element behave the same as the Google Chart API and avoid causing possible confusion in the users that use the google-chart element.

For different, more modern, Material styles we could have something like a materialTable chart type to load the google.Visualization.table package but apply the new style?

@sgomes
Copy link
Contributor

sgomes commented Feb 4, 2015

@philipwalton Thanks for helping out! I think it's a reasonable compromise, yes. I'm not thrilled about us keeping static CSS against a library with regular updates, but I still think it's a better approach than just throwing markup at users with no context.
As for what exactly the styling should look like, I don't think it's a big concern, as long as we have something and it's easily overridable (your suggested approach is perfect).

@SmokyBob: I'd rather stay away from calling anything "Material" unless it's based on the spec or has gone through the right reviews, so I think we're better off just having the default styling. Again, it should be pretty easy for users to override styling, either outside of the component by using ::shadow, or by extending it and modifying the styling.

@philipwalton
Copy link
Member

Finally got around to working on this. Here's the PR: #34

@SmokyBob
Copy link
Contributor

Finally found a way to import the style sheets added by the Google Chart API.
tl;dr PR: #38

Searching here and there found this article saying to use @import instead of the link tags... and works nicely.

@wesalvaro wesalvaro changed the title Add support for Table charts Move dynamically added styles into Shadow DOM Mar 15, 2016
@admwx7
Copy link

admwx7 commented Jun 15, 2016

Any chance we'll get a resolution to this? I'm in need of the org chart and it has this same issue.

@jlg7
Copy link

jlg7 commented Jun 25, 2016

Hi there,

I reported original related issue #74 and investigated this a bit at the time. A simple approach to resolving seems to be observe type + data changes, assure polymer settings dom is shadow, google vis + service loader api state and internalize the external css for each instance. Sample code:

      // Lazily try to load table css for native shadow dom on type + data changes
      observers: [
          '_loadTableCss(type,data)'
      ],

      // State of table css for shadow dom
      _tableCssLoaded: false,

      // The type or data was changed
      // For shadowDom, we need to internalize css for type table
      _loadTableCss: function (type) {
          var loaded =  this._tableCssLoaded;
          var googReady = window.google != null && window.google.loader != null && window.google.visualization != null;
          var isShadowDom = window.Polymer!=null && window.Polymer.Settings.dom === "shadow";
          if (isShadowDom && !loaded && type == "table" && googReady) {
              this._tableCssLoaded = true;
              this.debounce("_loadTableCss", function(){
                  var url = window.google.loader.ServiceBase +
                          "/api/visualization/" +
                          window.google.visualization.Version +
                          "/" +
                          google.visualization.JSHash +
                          "/table+en.css";
                  var styleNode = document.createElement('style');
                  styleNode.innerHTML = "@import '" + url + "';";
                  this.shadowRoot.appendChild(styleNode);
              });
          }
      }

Some potential improvements would be to:

  • if the css can be shared for all instances, then maintain a static variable for loaded state instead
  • if the locale needs to be parameterized, then sniff out or re-compute as necessary

Thanks

@jongeho1

@wesalvaro
Copy link
Member

So, I believe we can do the following:

  • Add a scopeSubtree container (perhaps we can reuse the chart container). This didn't seem necessary when testing.
    on-ready: this.scopeSubtree(this.$.container, true);
  • Add a callback to duplicate the CSS after packages are loaded.

Moving Callback:

  1. for-each styleSheet matching
    head > link[id^="load-css-"][href^="https://www.gstatic.com/charts/"]
  2. copy them to this.shadowRoot as a link or style node

This would mean that we have a duplicate of each sheet for each google-chart element, but should work generically and works for all dynamic style sheets (i.e. timeline, too).

Something like:

listeners: {
  'loaded': '_checkForAndMoveStyles',
},
_checkForAndMoveStyles() {
  const styleElementId = 'dynamic-chart-styles';
  const chartStyleSelector = 'head > link[id^="load-css-"][href^="https://www.gstatic.com/charts/"]'

  const chartStyles = document.querySelectorAll(chartStyleSelector);
  if (!chartStyles) {
    return;
  }

  const imports = styleSheets.map((css) => {
     return css.getAttribute('href');
  }).join("';\n @import '");

  // In the case of multiple loads, an element already exists.
  let styleElement = this.shadowRoot.querySelector('#${styleElementId}');
  const existing = !!styleElement;
  if (!existing) {
    styleElement = document.createElement('style');
    styleElement.setAttribute('id', styleElementId)
  }
  styleElement.innerHTML = `@import '${imports}';`;
  if (!existing) {
    this.shadowRoot.appendChild(styleElement);
  }
},

@wesalvaro
Copy link
Member

Here is a demo of it in action and a preview of the changes I had in mind.

@e111077
Copy link
Contributor

e111077 commented Nov 28, 2018

fixed in #223

@e111077 e111077 closed this as completed Nov 28, 2018
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

8 participants