angular.dart icon indicating copy to clipboard operation
angular.dart copied to clipboard

WebPlatform bug ?

Open vicb opened this issue 10 years ago • 28 comments

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 https://github.com/angular/angular.dart/issues/1189.

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

vicb avatar Aug 02 '14 01:08 vicb

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

Pajn avatar Aug 02 '14 17:08 Pajn

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

vicb avatar Aug 02 '14 18:08 vicb

@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.

adarshaj avatar Aug 02 '14 18:08 adarshaj

oh right, thanks for the link then :)

vicb avatar Aug 02 '14 19:08 vicb

@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.

vsavkin avatar Aug 03 '14 01:08 vsavkin

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

vicb avatar Aug 03 '14 08:08 vicb

@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.

vsavkin avatar Aug 03 '14 13:08 vsavkin

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:

  • https://github.com/Polymer/platform-dev
  • https://github.com/Polymer/ShadowDOM
  • https://github.com/Polymer/CustomElements

vicb avatar Aug 03 '14 14:08 vicb

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 avatar Aug 03 '14 21:08 vicb

@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 avatar Aug 04 '14 17:08 vicb

@vicb #1300 has been fixed in #1315.

vsavkin avatar Aug 06 '14 14:08 vsavkin

@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.

vsavkin avatar Aug 06 '14 20:08 vsavkin

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.

vicb avatar Aug 07 '14 07:08 vicb

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 avatar Aug 07 '14 13:08 vsavkin

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.

vsavkin avatar Aug 07 '14 13:08 vsavkin

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 ?

vicb avatar Aug 07 '14 15:08 vicb

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.

vsavkin avatar Aug 07 '14 16:08 vsavkin

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 avatar Aug 07 '14 17:08 vicb

@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.

vicb avatar Aug 08 '14 09:08 vicb

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]]

Pajn avatar Aug 08 '14 10:08 Pajn

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 avatar Aug 08 '14 12:08 vsavkin

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?

vsavkin avatar Aug 08 '14 12:08 vsavkin

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).

Pajn avatar Aug 08 '14 12:08 Pajn

@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.

vsavkin avatar Aug 08 '14 15:08 vsavkin

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

Pajn avatar Aug 08 '14 17:08 Pajn

@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

vsavkin avatar Aug 11 '14 13:08 vsavkin

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.

Pajn avatar Aug 12 '14 10:08 Pajn

@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.

vsavkin avatar Aug 12 '14 12:08 vsavkin