Skip to content
This repository has been archived by the owner on Feb 22, 2018. It is now read-only.

WebPlatform bug ? #1300

Open
vicb opened this issue Aug 2, 2014 · 28 comments
Open

WebPlatform bug ? #1300

vicb opened this issue Aug 2, 2014 · 28 comments

Comments

@vicb
Copy link
Contributor

vicb commented Aug 2, 2014

While testing angular with IE, I had an issue with WebPlatform.

IIUC the goal of the linked code is to add the name of the custom component as an attribute to all child nodes.. That's because we use strict styling.

The thing is we are not using custom components in Angular, a component can be any node such a vanilla div. How are we supposed to use platform.js in such a case - can it even work ?

Let's find there is a way to make this work then there is an other issue with the code: we try to assign the @Component selector. However the component can be anything ".class, [att]=value, ...". Should we get the element name, the selector would have to be modified to extract the tag name. This probably explains #1189.

@codelogic @jbdeboer please correct me if I'm wrong and add more information - or a fix :)

@Pajn
Copy link

Pajn commented Aug 2, 2014

This is already true http://andrew.yurisich.com/work/2014/07/16/dont-link-that-line-number/ :)
Fixed link to WebPlatform

@vicb
Copy link
Contributor Author

vicb commented Aug 2, 2014

@Pajn I think your comments are not relevant here, the issues I described apply to the code you have linked.

@adarshaj
Copy link

adarshaj commented Aug 2, 2014

@vicb That's exactly what @Pajn meant, its relevant to his linked code (to the exact commit + line number), not to the one linked in your OP, it points to master branch, which keeps changing and hence what you linked in your post doesn't make sense anymore now.

@vicb
Copy link
Contributor Author

vicb commented Aug 2, 2014

oh right, thanks for the link then :)

@vsavkin
Copy link
Contributor

vsavkin commented Aug 3, 2014

@vicb can we generate a unique id (per component type) and pass it as selector? I don't think we have to use the name of the custom component.

@vicb
Copy link
Contributor Author

vicb commented Aug 3, 2014

@vsavkin that might be possible, that is something that needs more investigation

@vsavkin
Copy link
Contributor

vsavkin commented Aug 3, 2014

@vicb I can investigate this issue. I started working on a css shim for transcluding components, and I want to make the two shims compatible.

@vicb
Copy link
Contributor Author

vicb commented Aug 3, 2014

That would be great.
Some thinking:

  • We should add something that would allow selecting the child nodes. This something should be unique for the component class (but shared across component instances),
  • As we can attach a shadow DOM to any tag (and not only custom elements) we can not rely on the tag name,
  • This could probably be a class or a custom attribute,
  • What happen if we add nodes to the component ? There is probably a listener on DOM mutations that add the selector to the nodes, how could we add our custom attribute ?

The following repos are probably useful:

@vicb
Copy link
Contributor Author

vicb commented Aug 3, 2014

From ShadowCSS.js "Note that elements that are dynamically added to a scope must have the scope
selector added to them manually."

This is something that is not currently handled by WebPlatform

@vicb
Copy link
Contributor Author

vicb commented Aug 4, 2014

@vsavkin I'll skip web_platform_spec on windows. Could you please re-enable them once you've fixed this issue (if #1288 has been merged, otherwise please add a note in this PR so that tests are re-enabled). Thanks.

vicb added a commit to vicb/angular.dart that referenced this issue Aug 4, 2014
vicb added a commit to vicb/angular.dart that referenced this issue Aug 4, 2014
vsavkin added a commit to vsavkin/angular.dart that referenced this issue Aug 6, 2014
…ents

* Extract ComponentCssLoader so it can be shared by ShadowDomComponentFactory and TranscludingComponentFactory
* Implement a CSS transformer similar to the one provided by Platform.JS
* Rename EventHandler into ShadowBoundary
* Provide a built-in implementation of WebPlatformShim

Closes dart-archive#1300
@vsavkin
Copy link
Contributor

vsavkin commented Aug 6, 2014

@vicb #1300 has been fixed in #1315.

vsavkin added a commit to vsavkin/angular.dart that referenced this issue Aug 6, 2014
…ents

* Extract ComponentCssLoader so it can be shared by ShadowDomComponentFactory and TranscludingComponentFactory
* Implement a CSS transformer similar to the one provided by Platform.JS
* Rename EventHandler into ShadowBoundary
* Provide a built-in implementation of WebPlatformShim

Closes dart-archive#1300
vsavkin added a commit to vsavkin/angular.dart that referenced this issue Aug 6, 2014
…ents

* Extract ComponentCssLoader so it can be shared by ShadowDomComponentFactory and TranscludingComponentFactory
* Implement a CSS transformer similar to the one provided by Platform.JS
* Rename EventHandler into ShadowBoundary
* Provide a built-in implementation of WebPlatformShim

Closes dart-archive#1300
vsavkin added a commit to vsavkin/angular.dart that referenced this issue Aug 6, 2014
…ents

* Extract ComponentCssLoader so it can be shared by ShadowDomComponentFactory and TranscludingComponentFactory
* Implement a CSS transformer similar to the one provided by Platform.JS
* Rename EventHandler into ShadowBoundary
* Provide a built-in implementation of WebPlatformShim

Closes dart-archive#1300
vsavkin added a commit to vsavkin/angular.dart that referenced this issue Aug 6, 2014
…ents

* Extract ComponentCssLoader so it can be shared by ShadowDomComponentFactory and TranscludingComponentFactory
* Implement a CSS transformer similar to the one provided by Platform.JS
* Rename EventHandler into ShadowBoundary
* Provide a built-in implementation of WebPlatformShim

Closes dart-archive#1300
@vsavkin
Copy link
Contributor

vsavkin commented Aug 6, 2014

@vicb After looking into it more I found out that the fix I had implemented is not going to work. The reason is that Platform.js emulates the host selector as follows:

:host {
    color: red;
}

Becomes:

my-component {
    color: red;
}

So we have to use the tag name of the component. Otherwise, :host does not work. I'm going to remove my fix of this issue from #1315.

It looks like fixing this issue will require changing how platform.js shims :host.

@vicb
Copy link
Contributor Author

vicb commented Aug 7, 2014

This issue exists because we can not the tag name as we can add shadom to any node (ie a div).

It seems like there are two layers, cssshim and custom component. Is the host selector handled by the custom component layer via the cssshim layer ? If this is the case could we only use the lower layer (cssshim) and use a class selector instead ?

To be honest I only have limited understanding of the way platform.js works. This might be not possible at all and if it is the case we should work with the polymer guys to add support for other selectors.

@vsavkin
Copy link
Contributor

vsavkin commented Aug 7, 2014

Platform.js uses one selector for handling :host and scoping child nodes.

:host {
    color: red;
}

div {
    color: green;
}

will be transformed into:

my-component {
    color: red;
}

div[my-component] {
    color: green;
}

Where the compiled template will look like:

<my-component>
  <div my-component="">
    <span my-component=""></span>
  </div>
</my-component>

As far as I can see the only way to handle it is to have two separate selectors for the host and the rest.

.mySpecialHostClass {
    color: red;
}

div[my-component] {
    color: green;
}

Where:

<my-component class="mySpecialHostClass">
  <div my-component="">
    <span my-component=""></span>
  </div>
</my-component>

But as far as I know it is not possible in platform.js.

@vsavkin
Copy link
Contributor

vsavkin commented Aug 7, 2014

Can we change Angular to restrict what kind of selectors you can use for components? It would be interesting look at the usage patterns we have today to see if it will be a problem.

@vicb
Copy link
Contributor Author

vicb commented Aug 7, 2014

Can we change Angular to restrict what kind of selectors you can use for components?

I first thought about that but that would be a bad solution. Do you have an estimate on how hard it would be to modify platform.js to support more than just the tag name ? Could you try to contact the polymer authors to discuss that ?

@vsavkin
Copy link
Contributor

vsavkin commented Aug 7, 2014

I first thought about that but that would be a bad solution.

Can you expand on that? I think we already support only one component per element, and it will make Angular components more like Polymer components, which I think is a good thing.

Do you have an estimate on how hard it would be to modify platform.js to support more than just the tag name ?

I don't think it will be too difficult. But my knowledge of platform.js is pretty limited, so I can be wrong here.

@vicb
Copy link
Contributor Author

vicb commented Aug 7, 2014

Can you expand on that?

There is no reason to add this constraint on angular. Selecting a class (or an attribute) should work equally well than selecting a class or an attribute. Plus it would add extra constraints (ie it would be no more possible to discriminate my-cmp[red] and my-cmp[green]). @mhevery suggested to open on ticket on polymer for that.

@vicb
Copy link
Contributor Author

vicb commented Aug 8, 2014

@vsavkin actually we can pass a selector to _shadowCss.callMethod('shimCssText', [css, selector]). What if we set it to [my-attr] and add the attribute on the root as well as on all then children ?

Edit: that would probably not work if the root is a div then div[my-attr] would match the root but would it to match only children nodes. The attribute on the root must be different.

@Pajn
Copy link

Pajn commented Aug 8, 2014

YCan't you just add a host-selector attribute to all children with the value set to the selector? For example childrens to div[my-attr] would have host-selector="div[my-attr]".

EDIT : So to polymer you would give [host-selector=div[my-attr]]

@vsavkin
Copy link
Contributor

vsavkin commented Aug 8, 2014

There is no reason to add this constraint on angular. Selecting a class (or an attribute) should work equally well than selecting a class or an attribute. Plus it would add extra constraints (ie it would be no more possible to discriminate my-cmp[red] and my-cmp[green]).

It makes sense.

@vsavkin
Copy link
Contributor

vsavkin commented Aug 8, 2014

YCan't you just add a host-selector attribute to all children with the value set to the selector? For example childrens to div[my-attr] would have host-selector="div[my-attr]".

@Pajn Could you provide an example of a template and a css file before and after shimming?

@Pajn
Copy link

Pajn commented Aug 8, 2014

Your comment in #1315 says:

one, two {color: red;}
Becomes:
one[tag], two[tag] {color: red;}

If you instead of adding the tag name as attribute adds a host-selector attribute to all children it doesn't matter what that selector is and your example would become one[host-selector="tag"], two[host-selector="tag"] {color: red;} and a slightly more advanced host selector would become three[host-selector="div[my-attr]"] (for the selector in @vicb comment above).

I don't know if this would require patching platform.js or equal but as far as I can tell this should support Angulars very free component selectors without compromises (as the value is just a string it should be able to support everything).

@vsavkin
Copy link
Contributor

vsavkin commented Aug 8, 2014

@Pajn I'm probably missing something, but I don't think it will work with :host:

:host(.myclass) {
    color: red;
}

Will be transformed into:

host-selector="div[my-attr]".myclass {
    color: red;
}

which is not valid css.

@Pajn
Copy link

Pajn commented Aug 8, 2014

You need brackets around [attribute="value"] selectors. Example: http://codepen.io/anon/pen/Hzcpq

@vsavkin
Copy link
Contributor

vsavkin commented Aug 11, 2014

@Pajn what I meant is that platform.js uses the same token for :host and other elements.

:host {}
div {}

will become:

TOKEN {}
div[TOKEN] {}

Which means that for host-selector="div[my-attr]":

host-selector="div[my-attr]" {} <- Incorrect
div[host-selector="div[my-attr]"] {}

and for [host-selector="div[my-attr]"]:

[host-selector="div[my-attr]"] {} 
div[[host-selector="div[my-attr]"]] {} <- Incorrect

@Pajn
Copy link

Pajn commented Aug 12, 2014

I see. Personally I would consider that a bug in platform.js. It should be pretty trivial for it to see if the selector string is a attribute selector (start and ends with bracket) and handle that. I'm sorry I couldn't help, but I still think patching platform.js would be easiest if supporting any valid CSS selector is important to Angular.

@vsavkin
Copy link
Contributor

vsavkin commented Aug 12, 2014

@Pajn Totally agree with you. I looked into it a bit more and I think patching it will be pretty straightforward. I might do it this weekend.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

No branches or pull requests

4 participants