Leak of [hidden] CSS rule with !important declaration

[uwolfer]

The iron-flex-layout element leaks [hidden] CSS rule with !important declaration.

Expected outcome

The element should not leak CSS with important declaration.

Actual outcome

As soon as this element is included (when using shady DOM), it leaks following CSS:
https://github.com/PolymerElements/iron-flex-layout/blob/master/iron-flex-layout.html#L51-L53

Steps to reproduce

I have prepared a JS Bin. When you remove the (unused) iron-flex-layout import, the output is different.

The main issue is that the outcome is different when using shady DOM or shadow DOM. When using shady DOM, the CSS rule is leaked and thing look "correct". As soon as you switch to shadow DOM, things break when you use a display CSS property (see my JS Bin).

This issue got introduced with #26 which as required for IE according to the description. If you can provide a description which bug it fixes exactly, I'm happy to look into this issue and create a PR if possible.

Tested with Chromium 50.

[notwaldorf]

I believe this is a limitation of the shady DOM shim. There's no true encapsulation of styles, so it's true that if you include this somewhere, the style for the hidden attribute will become available everywhere. This was also true in the past if you included the classes version of iron-flex-layout, since they were making hidden available via a /deep/ combinator everywhere throughout the app.

I'm not sure I understand exactly why this is a problem -- if you're expecting hidden to not work like this, may I suggest using a different property, perhaps?

[uwolfer]

@notwaldorf: Thanks for your feedback. My main concern is that things break when switching to shadow DOM while they work fine with shady DOM.

Please see this example to get a better understanding what I'm talking about. Because of the leaked !important declaration, display: {flex,inline-flex,...} rules do not apply to hidden elements with shady DOM enabled. With shadow DOM, the display rules apply to hidden elements and they are visible.

If such a low-level element like iron-flex-layout which is included in many other elements includes (and leaks) this CSS, it will break a lot of apps once they switch to shadow DOM. It would simplify development if shady DOM and shadow DOM behave as similar as possible.

Since the CSS declaration is required only for IE, I would like to get rid of it and fix it in another way which would not impact other code that much. If I would know which IE issues are fixed with it, I could look into it.

[notwaldorf]

@uwolfer To make sure the shadow DOM works in the same way, shouldn't you import iron-flex-layout in every element that needs it? That is how the element is intended to be used. Does that fix your problem?

[uwolfer]

@notwaldorf: iron-flex-layout is imported as a dependency by iron-autogrow-textarea in the example I have posted above. It is not used by any element directly AFAIK.

[andybons]

@uwolfer is right. We don’t use iron-flex-layout directly in any of our elements. It’s pulled in by other iron-* elements.

[uwolfer]

There is a related comment by @kevinpschaaf about hidden attribute with !important in this issue.

[notwaldorf]

@uwolfer In your original JSBin, I don't see a difference between using the shady and shadow DOM. I had created a PR to encapsulate the style, but I'm seeing the same result.

I can't run your gerrit example, unfortunately -- would you mind putting together a JSBin with a smaller display: flex problem? In your original example, unfortunately, I'm seeing the expected result: if you use the hidden attribute, this will add a display:none class to the element.

We added the hidden style in iron-flex-layout to maintain feature parity with the deprecated classes version (that used /deep/): those made hidden work magically everywhere, and if you tried to update to the new iron-flex-layout, users noticed that things like <paper-item hidden> stopped working. To fix that, we added the hidden style you mentioned.

[notwaldorf]

@uwolfer In any case, I think #89 should at least make the shady and shadow dom styles consistent. Can you double check this works for you?

[uwolfer]

@notwaldorf: Thanks a lot, I've verified that it fixes things as expected.

I've also re-checked my JSBin. You are right, it behaves the same way in both shady and shadow DOM. I think it is related to the fact that in the JSBin all includes (and elements) are in one 'file' (also the iron-flex-layout one). This way, it is also applied to the Polymer element my-element. But you can still check things with my JSBin: the text content should be visible in any case (at the moment it is hidden as soon as iron-flex-layout is included).

You can check out the Gerrit example without installing anything:

• shady: https://gerrit-review.googlesource.com/q/status:open?polygerrit=1&dom=shady
• shadow: https://gerrit-review.googlesource.com/q/status:open?polygerrit=1&dom=shadow

You can see for example that the star icon in front of list items is visible in shadow mode while it is hidden in shady. It is hidden because of the leaked display: none !important by iron-flex-layout.

Important note: I really think you should mark this as breaking change (and add a big note in release notes) and release a new major version of iron-flex-layout. At least for PolyGerrit, it needs quite some rework when using the version with your fix included.

[notwaldorf]

Oh, I didn't say I would merge #89, if it's a breaking change (which it might be) -- I just wanted to understand whether that was causing your problem. 😊

[sorvell]

I suggest simply removing the [hidden] rule in iron-flex-layout (when possible). It doesn't seem like the responsibility of iron-flex-layout to have anything to say about how [hidden] behaves.

The root issue here is that the hidden attribute natively behaves in a fairly unhelpful way. Any styling applying to the element that sets the display property overrides the effect of hidden. Here's a simple example: http://jsbin.com/resemewehi/edit?html,output.

A work around for this is to "fix" the hidden selector by making it apply display: none !important;. This should be something an element author chooses to do by including this styling in the element scope. If the change is made to a specific element, it will apply the same within Polymer in shadow or shady dom.

[uwolfer]

I fully agree with @sorvell.

Btw, I think the label "pending-response" can be removed. Or do you require any information from me?