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

PolymerDomSharedStylesHost breaks Polymer components on non-Chrome browsers #33

Closed
firstor opened this issue Jun 13, 2017 · 8 comments
Closed

Comments

@firstor
Copy link

firstor commented Jun 13, 2017

Currently PolymerDomSharedStylesHost simply wraps styles in <custom-style> elements, but it breaks
Polymer and other web components implementing ShadowDOM v1 spec on non-Chrome browsers.

Here is an example: Polymer's app-drawer component

<app-drawer class="drawer" id="mainDrawer" swipe-open>
  <section class="drawer-content">...</section>
</app-drawer>

This works fine on Chrome and the inspector gives us the following tree:

<!-- Chrome Inspection -->
<app-drawer _ngcontent-c0="" class="drawer" id="mainDrawer" swipe-open="" ...>
  #shadow-root
    <style>
      :host { ... } /* SelectorA */
      ...
      #contentContainer {  /* SelectorB */
        position: absolute;
        top: 0;
        bottom: 0;
        left: 0;
        width: var(--app-drawer-width, 256px);
        padding: 120px 0;
        ... 
      }
      ...
      #scrim { ... }  /* SelectorC */
      ...
    </style>
    <div id="scrim" style="transition-duration: 200ms;"></div>
    <div id="contentContainer" ...>
      <slot></slot> <!-- where the section.drawer-content will be attached -->
    </div>
  <section _ngcontent-c0="" class="drawer-content">
    ...
  </section>
<app-drawer>

So #contentContainer can get styled by app-drawer styling on Chrome. But other browsers don't support ShadowDOM spec and thus ShadyCSS polyfills the component in the following.

<!-- FireFox Inspection -->
<custom-style>
  <style scope="app-drawer" is="custom-style">
    app-drawer:not(.style-scope) { ... }  /* SelectorA */
    ...
    #contentContainer.app-drawer:not(.style-scope) { ... }  /* SelectorB */
    ...
    #scrim.app-drawer:not(.style-scope) { ... }  /* SelectorC */
    ...
  </style>
</custom-style>
...
<app-drawer _ngcontent-c0="" class="drawer" id="mainDrawer" ...>
  <div id="scrim" class="style-scope app-drawer" ...></div>
  <div id="contentContainer" class="style-scope app-drawer" ...>
    <section _ngcontent-c0="" class="drawer-content">
      ...
    </section>
  </div>
</app-drawer>

As you can see, SelectorA matches app-drawer (host element), but SelectorB and SelectorC can never match #contentContainer and #scrim respectively, and eventually #mainDrawer gets broken.

Here is how app-drawer is rendered before it's a pre-custom-styled version of style[scope="app-drawer"].

<!-- FireFox Inspection -->
<custom-style>
  <style scope="app-drawer">
    app-drawer { ... } /* SelectorA */
    ...
    #contentContainer.app-drawer { ... } /* SelectorB */
    ...
    #scrim.app-drawer { ... } /* SelectorC */
    ...
  </style>
</custom-style>
...
<app-drawer _ngcontent-c0="" class="drawer" id="mainDrawer" ...>
  <div id="scrim" class="style-scope app-drawer" ...></div>
  <div id="contentContainer" class="style-scope app-drawer" ...>
    <section _ngcontent-c0="" class="drawer-content">
      ...
    </section>
  </div>
</app-drawer>

I am not sure why this happened, but I suppose the component got double polyfilled, the first time polyfilling was done correctly by referring to both of component's template and style, but second time with component polyfilling wrong by referring to only component's style as an individual.

So I suggest: PolymerDomSharedStylesHost should rather apply custom-styling to the only styles defined app-level (document-level style plus Angular components' styles) since origami handles only Angular4 + Polymer2, or at least it can provide the way to point which styles should be custom-styled.

I made changes on origami myself and it worked fine for me. I also committed my changes on a side branch and get ready to contribute my work and time to origami. (I attach a screenshot since I can't push my updates right now due to permission denied to me)

Thank you.
shared-styles-host ts diff

@hotforfeature
Copy link
Owner

Thank you for the detailed post! I'm having trouble recreating this though.

What raises a red flag for me is this that you mention you see this in Firefox:

<custom-style>
  <style scope="app-drawer" is="custom-style">
  ...

PolymerDomSharedStylesHost will only wrap Angular component styles. That means Angular styles declared in styleUrls or styles in component metadata. That wrapped style element looks like a Polymer-added style, since Angular won't add the scope attribute.

This sounds more like something the old CustomStyleService would do: indiscriminately wrap all style elements in the head.

Let's do a quick sanity check. Can you go into your node_modules and bower_components folders and list the versions for the following packages?

  • node_modules/@codebakery/origami/package.json
  • node_module/@angular/core/package.json
  • bower_components/polymer/.bower.json
  • bower_components/shadycss/.bower.json

I'm running on:

  • origami 1.2.1
  • angular 4.2.2
  • polymer 2.0.1
  • shadycss 1.0.1

Also go through your app and see if you're making any calls to CustomStyleService.updateCustomStyles(). You'll be getting a deprecation warning message in the console if you do.

These are my files:

index.html

<!doctype html>
<html>
<head>
  <meta charset="utf-8">
  <title>Origami Demo</title>
  <base href="/">

  <meta name="viewport" content="width=device-width, initial-scale=1">
  <script src="assets/bower_components/webcomponentsjs/webcomponents-loader.js"></script>
  <link href="assets/bower_components/app-layout/app-drawer/app-drawer.html" rel="import">
</head>
<body>
  <app-root>Loading...</app-root>
</body>
</html>

src/main.ts

import { enableProdMode } from '@angular/core';
import { platformBrowserDynamic } from '@angular/platform-browser-dynamic';
import { webcomponentsReady } from '@codebakery/origami';

import { AppModule } from './app/app.module';
import { environment } from './environments/environment';

if (environment.production) {
  enableProdMode();
}

webcomponentsReady().then(() => {
  platformBrowserDynamic().bootstrapModule(AppModule);
}).catch(error => {
  console.error(error);
});

src/app/app.module.ts

import { CUSTOM_ELEMENTS_SCHEMA, NgModule } from '@angular/core';
import { FormsModule } from '@angular/forms';
import { BrowserModule } from '@angular/platform-browser';
import { PolymerModule } from '@codebakery/origami';
import { AppElementsModule } from '@codebakery/origami/lib/collections';

import { AppComponent } from './app.component';
import { FeaturesComponent } from './features/features.component';

@NgModule({
  imports: [
    AppElementsModule,
    BrowserModule,
    FormsModule,
    PolymerModule.forRoot()
  ],
  declarations: [
    AppComponent
  ],
  schemas: [CUSTOM_ELEMENTS_SCHEMA],
  bootstrap: [AppComponent]
})
export class AppModule { }

src/app/app.component.ts

import { Component } from '@angular/core';
import { PolymerChanges } from '@codebakery/origami';

@Component({
  selector: 'app-root',
  template: `
    <app-drawer class="drawer" id="mainDrawer" swipe-open opened>
      <section class="drawer-content">Hello World</section>
    </app-drawer>
  `
})
export class AppComponent { }

I'm testing in Chrome 60.0.3112.24, Firefox 53.0.3, Edge 38.14393.1066.0. All three are working as I expect. The app loads and the drawer is open. Swiping it to open and close work and the styling looks correct for a default <app-drawer>.

Your description of Chrome is accurate, but Firefox is rendering things differently for me.

<head>
...
<!-- Not wrapping in <custom-style> -->
<style scope="app-drawer">
app-drawer { ... } /* Not adding :not(.style-scope) */
...
</style>
...
</head>
<body>
...
<!-- This matches what you described with the style-scope and app-drawer classes -->
<app-drawer id="mainDrawer" class="drawer" ...>
  <div id="scrim" class="style-scope app-drawer visible" ...></div>
  <div id="contentContainer" class="style-scope app-drawer" ...>
    <section class="drawer-content">Hello World</section>
  </div>
</app-drawer>

My suspicion is that you're on an older version of Polymer or ShadyCSS and that you're using CustomStyleService.updateCustomStyles() from an older version of Origami.

If we can get the exact versions you're using for everything, that'll move us one step closer to figuring out what combination of effects are causing this.

Chrome:
chrome

Firefox:
firefox

Edge:
edge

@firstor
Copy link
Author

firstor commented Jun 13, 2017

Thank you for the details.

I have no place in my code to call CustomStyleService.updateCustomStyles that's been already deprecated. I only use PolymerModule.forRoot() in my app module.

And I'm running on:

  • origami 1.2.1
  • angular 4.0.2
  • polymer 2.0.1
  • shadycss 1.0.1

The only difference is that we're running on different version of Angular. And as I can see through through the screenshots you attached, app-drawer seems rendered on your side just like as it's rendered without using PolymerModule.forRoot() on my side.

In my situation, PolymerDomSharedStylesHost.wrapStyleNodes() wrapped all <style>s (including Polymer component styles) in <custom-style> elements through the document head (as I can check on shared-styles-host.js). Of course, I will try with later version of Angular and Polymer, however, my greatest concern is to remove such possibility, in my current running versions, that Polymer and other web components can be double rendered (you may get hints and find the potential problem through my attachment). (Do I still talk abstract?)

I already confirmed that small fix on PolymerDomSharedStylesHost worked as intended so only app-level styles (Angular component styles + document-level style) are custom-styled and my app gets back its previous look. So I hope you could get generic idea of the code difference I attached and do possible fix to improve this potential issue.

Sorry for writing long words with no sample source or screenshot.

@firstor
Copy link
Author

firstor commented Jun 13, 2017

I am also using Firefox v53.0.3 and Edge v14.14393.

@hotforfeature
Copy link
Owner

There's got to be some Angular component styles that you haven't mentioned. There's no other way PolymerDomSharedStylesHost would even be triggered to wrap anything. Do you have a styleUrls or styles property in your component's metadata?

@firstor
Copy link
Author

firstor commented Jun 13, 2017

I am linking styles to the Angular component by styleUrls.
I used style's scope to check if it's Angular component style or else since I found Angular attaches each component style to the <head> by wrapping in <style> element with no attribute given.

@hotforfeature
Copy link
Owner

I was finally able to reproduce it. It requires a second Angular component with styles to trigger PolymerDomSharedStylesHost after the first component adds its styles. Angular first adds the components styles, then builds the template (which adds polymer styles). This is why I couldn't see it: my repo only had one component with styling, not two.

^ This is kind of why I asked for a full sample repository instead of snippets. It would have been a lot faster to see the details that weren't mentioned in your posts. Your info was good and the problem real, but there were overlooked nuances that made it harder to troubleshoot.

I'll work on a patch for this, it shouldn't take long now that I can reproduce it.

@hotforfeature
Copy link
Owner

Should be good to go with 1.2.2 now

@firstor
Copy link
Author

firstor commented Jun 13, 2017

@hotforfeature I have gone with v1.2.2 and the patch works fine. Thanks a lot!

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

No branches or pull requests

2 participants