dart-archive / angular.dart

Legacy source repository. See github.com/dart-lang/angular
https://webdev.dartlang.org/angular/
1.25k stars 248 forks source link

WebPlatform bug ? #1300

Open vicb opened 10 years ago

vicb commented 10 years ago

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

Pajn commented 10 years ago

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

vicb commented 10 years ago

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

adarshaj commented 10 years ago

@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 commented 10 years ago

oh right, thanks for the link then :)

vsavkin commented 10 years ago

@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 commented 10 years ago

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

vsavkin commented 10 years ago

@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 commented 10 years ago

That would be great. Some thinking:

The following repos are probably useful:

vicb commented 10 years ago

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 commented 10 years ago

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

vsavkin commented 10 years ago

@vicb #1300 has been fixed in #1315.

vsavkin commented 10 years ago

@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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

@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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

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 commented 10 years ago

@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 commented 10 years ago

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

vsavkin commented 10 years ago

@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 commented 10 years ago

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 commented 10 years ago

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