Replace unsafe attribute characters in CSS shim

[natebosch]

dart-archive/angulardart.org#71

[googlebot]

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

[natebosch]

dart-archive/angulardart.org#71

[natebosch]

Original issue was opened on the wrong project...

When using the TranscludingComponentFactory the css is shimmed using a tag pulled from the component selector. Selectors in angular are allowed to include characters which cannot be used in an attribute name. For instance the following is a valid selector for a component:

component-name[attribute-name]

CSS on a component with this selector will not be applied to elements since attempting to add the selector as an attribute causes an InvalidCharacterError (in chrome) in BlinkElement.setAttribute
For an ASCII selector it looks like the valid characters are [a-zA-Z0-9:_-.]

https://github.com/WebKit/webkit/blob/master/Source/WebCore/dom/Document.cpp#L4049
https://github.com/angular/angular.dart/blob/master/lib/core_dom/transcluding_component_factory.dart#L44

[vsavkin]

One of the limitations of the current shim is that the selector of the component has to be an element. This is because we wanted to make our shim compatible with Platform.js.

We have this limitation for the following reason:

We compile

:host {
    color: red;
}

into

componentSelector {
    color: red;
}

So the selector has to be equal to the element for the shim to work.

Read more here: #1300

[natebosch]

Perhaps this limitation should be enforced in some way rather than silently failing. If CSS won't work for components that have a selector other than the element it would be good to have an indication that cssUrl shouldn't be used at all for that component.

Alternatively with a bit more work this could potentially keep the raw selector to replace :host and the modified selector for the attributes.

[natebosch]

I've update the brach to include a change which passes through both the raw selector and the attribute-safe selector. This maintains the shadow-dom behavior with CSS that looks like:

:host { color:red; }
span { color:blue; }

[natebosch]

This bug does exist in the polyfill as well. When using the shadow DOM there is inconsistent behavior between chrome and firefox.

Looks like this is a known issue on the polyfill side
webcomponents/webcomponentsjs#28

[googlebot]

CLAs look good, thanks!

[vsavkin]

This PR would make the polymer and angular behavior differently, which is not ideal.

If CSS won't work for components that have a selector other than the element it would be good to have an indication that cssUrl shouldn't be used at all for that component.

Maybe we should do this instead. TranscludingComponentFactory can throw an exception when trying to instantiate a component with a non-element selectors and css.

[natebosch]

The current state already has different behavior in chrome and non-chrome when not using the TranscludingComponentFactory. If the goal is consistency and we can't get the bug addressed on the polymer side then we'd need to handle this at a higher level than the TranscludingComponentFactory.

[vsavkin]

we'd need to handle this at a higher level than the TranscludingComponentFactory

@natebosch could you expand on that?

[natebosch]

The behavior is inconsistent in two ways
shadow DOM (chrome) <-> non-shadow DOM
shadow DOM (chrome) <-> shadow DOM (non-chrome)

If we add thrown an exception in TranscludingComponentFactory this can alert developers to the first type of inconsistency but does nothing to address the second type of inconsistency since the TrancludingComponentFactory is only involved with non-shadow DOM apps.

[vsavkin]

Can you provide an example of this inconsistency shadow DOM (chrome) <-> shadow DOM (non-chrome) and how we would detect it?

The way I view it is that throwing in TranscludingComponentFactory is OK because we implemented this shim, and we know its limitations. Other libraries do not know them, and as such it is Angular's responsibility to notify the developer about these limitations. Notifying the developer about browsers not handling shadow-dom consistently does not have to be Angular's responsibility.

[natebosch]

The inconsistency I mentioned: shadow DOM (chrome) <-> shadow DOM (non-chrome) is the same as the inconsistency when using Angular's version of the shim. Any component which uses a selector that includes an attribute will not have styles correctly applied outside of chrome. The bug is already filed: webcomponents/webcomponentsjs#28

If the bug on the angular side isn't fixed specifically to maintain consistency with a buggy platform.js then raising a warning in TranscludingComponentFactory will only help out those developers who have explicitly turned off the Shadow DOM. Those developers who are using the shadow DOM will have styles that are applied in Chrome but silently fail elsewhere.