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

Emphasize style attribute binding should not be quoted #2461

Merged

Conversation

balinterdi
Copy link
Contributor

I've spent quite some time debugging this (see my blog post), and although the section in the guides is correct, I think others would benefit by having this emphasized.

@chancancode
Copy link
Member

Seems like a bug

cc @wycats

@wycats
Copy link
Member

wycats commented Feb 4, 2016

@balinterdi This definitely seems like a bug. concat should do the escaping, and produce a safe string.

For what it's worth, the exact semantics of property vs. attribute are somewhat undefined at the moment, so I'm pretty nervous about having people depend on a side effect of property vs. attribute that provides even-less-defined safe string semantics.

@balinterdi
Copy link
Contributor Author

Should I create a proper issue for this in the emberjs repo? I can include the example in the blog post (which I have also extracted to a gist).

it in the template, as this would prevent Ember from seeing it as safe:

```handlebars
<div style={{myStyle}}></div>
Copy link
Sponsor Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is exactly the same example as a few paragraphs above. Perhaps you can just add test to one of those paragraphs making this more explicit? Repeating the code doesn't seem helpful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I've moved that paragraph under the definition of the myStyle property and removed the duplicated template code snippet.

@mixonic
Copy link
Sponsor Member

mixonic commented Mar 18, 2016

This is definitely needed right now. We distrust anything that goes through concat, so anything quoted.

@balinterdi balinterdi force-pushed the emphasize-no-quotes-for-style-binding branch from 8656bbe to f87b8e8 Compare March 18, 2016 19:41
@balinterdi
Copy link
Contributor Author

@mixonic I'm not sure I see what you mean, what is needed? Do you consider the quoted version triggering the warning a bug?

@mixonic
Copy link
Sponsor Member

mixonic commented Mar 18, 2016

@balinterdi sorry for the confusing note- I'm not sure I consider it a "bug". Two safe things, when combined, may be unsafe. However I'm sure we can do better in common cases like style="{{foo}}".

Regardless I'd like to merge this docs improvement. @balinterdi can you also change the JS part of the example to:

return Ember.String.htmlSafe("color: " + color);

instead of using the Handlebars namespace? If you do this I'll :shipit:!

@balinterdi balinterdi force-pushed the emphasize-no-quotes-for-style-binding branch from f87b8e8 to 30ec9c5 Compare March 19, 2016 06:02
@balinterdi
Copy link
Contributor Author

Thank you, just changed it to use Ember.String.htmlSafe, which I actually also prefer.

rwjblue added a commit that referenced this pull request Mar 19, 2016
…e-binding

Emphasize style attribute binding should not be quoted
@rwjblue rwjblue merged commit bf6bab8 into emberjs:master Mar 19, 2016
@mixonic
Copy link
Sponsor Member

mixonic commented Mar 19, 2016

Thanks @balinterdi

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

Successfully merging this pull request may close these issues.

5 participants