Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: HostBinding get incorrect semantics when property name is same as literal #18698

Closed
trotyl opened this issue Aug 15, 2017 · 4 comments
Closed
Labels
area: core Issues related to the framework runtime core: host and host bindings freq1: low P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent state: confirmed type: bug/fix
Milestone

Comments

@trotyl
Copy link
Contributor

trotyl commented Aug 15, 2017

I'm submitting a...


[ ] Regression (a behavior that used to work and stopped working in a new release)
[x] Bug report  
[ ] Feature request
[ ] Documentation issue or request
[ ] Support request => Please do not submit support request here, instead see https://github.com/angular/angular/blob/master/CONTRIBUTING.md#question

Current behavior

When using @HostBinding() with a property name is one of true, false or null, Angular will incorrectly regard it as a literal value, for example:

export class App {
  @HostBinding('class.foo') true = false
  @HostBinding('class.bar') false = true
}

Would result to <element class="foo">, even the binding value is false.

Expected behavior

Should got <element class="bar"> according to the binding value.

And reserved words are allowed as property name in JavaScript.

But for:

@Component({
  host: {
    '[class.foo]': 'true'
  }
})
export class App { }

It should still be regarded as literal since it's general expression here.

Possibly we'll need a new boolean value for parser to indicate whether it's in expression context or not.

Minimal reproduction of the problem with instructions

http://plnkr.co/edit/wUsocMIjZ82RnSxOc3U5?p=preview

What is the motivation / use case for changing the behavior?

Angular uses a unified parser for all bindings, so it's indistinguishable whether it came from.

But for HostBinding()s directly written on class properties, there is no expression context at all since the HostBinding() decorator is only for specific property (for what property decorator means), so it should be a bug of Angular parser for the binding recognition. Actually it seems to be a bug of metadata collector, since it's already indistinguishable when it came to parser.

Environment


Angular version: 4.3.4 (likely all)


Browser:
- [x] Chrome (desktop) version XX
- [x] Chrome (Android) version XX
- [x] Chrome (iOS) version XX
- [x] Firefox version XX
- [x] Safari (desktop) version XX
- [x] Safari (iOS) version XX
- [x] IE version XX
- [x] Edge version XX
 
For Tooling issues:
- Node version: XX  
- Platform:  

Others:

@trotyl
Copy link
Contributor Author

trotyl commented Aug 15, 2017

I can help to provide a fix if you agree this is unexpected.

@vicb vicb added area: core Issues related to the framework runtime type: bug/fix labels Aug 15, 2017
@vicb
Copy link
Contributor

vicb commented Aug 15, 2017

sounds like a bug.
your help infixing that would be appreciated.

@atscott
Copy link
Contributor

atscott commented Sep 23, 2020

@jelbourn jelbourn added P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent and removed severity3: broken labels Oct 1, 2020
petebacondarwin pushed a commit to trotyl/angular that referenced this issue Dec 31, 2020
crisbeto added a commit to crisbeto/angular that referenced this issue Dec 31, 2020
Currently when analyzing the metadata of a directive, we bundle together the bindings from `host`
and the `HostBinding` and `HostListener` together. This can become a problem later on in the
compilation pipeline, because we try to evaluate the value of the binding, causing something like
`@HostBinding('class.foo') public true = 1;` to be treated the same as
`host: {'[class.foo]': 'true'}`.

While looking into the issue, I noticed another one that is closely related: we weren't treating
quoted property names correctly. E.g. `@HostBinding('class.foo') public "foo-bar" = 1;` was being
interpreted as `classProp('foo', ctx.foo - ctx.bar)` due to the same issue where property names
were being evaluated.

These changes resolve both of the issues by treating all `HostBinding` instance as if they're
reading the property from `this`. E.g. the `@HostBinding('class.foo') public true = 1;` from above
is now being treated as `host: {'[class.foo]': 'this.true'}` which further down the pipeline becomes
`classProp('foo', ctx.true)`. This doesn't have any payload size implications for existing code,
because we've always been prefixing implicit property reads with `ctx.`. If the property doesn't
have an identifier that can be read using dotted access, we convert it to a quoted one (e.g.
`classProp('foo', ctx['is-foo']))`.

Fixes angular#40220.
Fixes angular#40230.
Fixes angular#18698.
crisbeto added a commit to crisbeto/angular that referenced this issue Jan 6, 2021
Currently when analyzing the metadata of a directive, we bundle together the bindings from `host`
and the `HostBinding` and `HostListener` together. This can become a problem later on in the
compilation pipeline, because we try to evaluate the value of the binding, causing something like
`@HostBinding('class.foo') public true = 1;` to be treated the same as
`host: {'[class.foo]': 'true'}`.

While looking into the issue, I noticed another one that is closely related: we weren't treating
quoted property names correctly. E.g. `@HostBinding('class.foo') public "foo-bar" = 1;` was being
interpreted as `classProp('foo', ctx.foo - ctx.bar)` due to the same issue where property names
were being evaluated.

These changes resolve both of the issues by treating all `HostBinding` instance as if they're
reading the property from `this`. E.g. the `@HostBinding('class.foo') public true = 1;` from above
is now being treated as `host: {'[class.foo]': 'this.true'}` which further down the pipeline becomes
`classProp('foo', ctx.true)`. This doesn't have any payload size implications for existing code,
because we've always been prefixing implicit property reads with `ctx.`. If the property doesn't
have an identifier that can be read using dotted access, we convert it to a quoted one (e.g.
`classProp('foo', ctx['is-foo']))`.

Fixes angular#40220.
Fixes angular#40230.
Fixes angular#18698.
@atscott atscott closed this as completed in 1045465 Jan 7, 2021
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Feb 7, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: core Issues related to the framework runtime core: host and host bindings freq1: low P3 An issue that is relevant to core functions, but does not impede progress. Important, but not urgent state: confirmed type: bug/fix
Projects
None yet
6 participants