X Tutup
Skip to content

fix(compiler): detect and strip data- prefix#2719

Closed
pkozlowski-opensource wants to merge 1 commit intoangular:masterfrom
pkozlowski-opensource:data_prefix
Closed

fix(compiler): detect and strip data- prefix#2719
pkozlowski-opensource wants to merge 1 commit intoangular:masterfrom
pkozlowski-opensource:data_prefix

Conversation

@pkozlowski-opensource
Copy link
Copy Markdown
Member

@pkozlowski-opensource pkozlowski-opensource commented Jun 24, 2015

Fixes #2687

Review on Reviewable

vicb
vicb reviewed Jun 24, 2015
View reviewed changes
modules/angular2/test/render/dom/compiler/property_binding_parser_spec.ts
Copy link
Copy Markdown
Contributor

@vicb vicb Jun 24, 2015

Choose a reason for hiding this comment

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

I'm unsure we want this for [], () and [()] ?

@vicb
Copy link
Copy Markdown
Contributor

vicb commented Jun 24, 2015

As far I understand the original issue, the pb is [data-foo]="bar".

I don't think this PR solves the issue, does it ?

Note: I don't think (data-foo)=... or [(data-foo)]=... make any sense, maybe we should just throw ?

@pkozlowski-opensource
Copy link
Copy Markdown
Member Author

pkozlowski-opensource commented Jun 24, 2015

right, the original issue was for [data-foo] but this would imply binding to dataFoo property - which doesn't make sense. IMO prefixing with data- is there to make HTML validators happy and this PR allows just this. We might throw for other use-cases just to warn users, to be seen.

@pkozlowski-opensource pkozlowski-opensource added the action: review The PR is still awaiting reviews from at least one requested reviewer label Jun 24, 2015
@vicb
Copy link
Copy Markdown
Contributor

vicb commented Jun 24, 2015

but this would imply binding to dataFoo property

Shouldn't we special case this to bind to dataset.foo ?

@sebholstein
Copy link
Copy Markdown
Contributor

sebholstein commented Jun 25, 2015

As @pkozlowski-opensource said:

IMO prefixing with data- is there to make HTML validators happy this PR allows just this.

I'm not sure if this fixes the valdiation problem. When validating this example with the W3C Validator:

<!DOCTYPE html>
<html>
  <head>
    <title>demo</title>
  </head>
  <body>
    <div data-[abc]="abc"></div>
  </body>
</html>

I get this Error :
html5

The HTML5 Spec for data-* attributes:

A custom data attribute is an attribute in no namespace whose name starts with the string "data-", has at least one character after the hyphen, is XML-compatible, and contains no uppercase ASCII letters.

Source

@pkozlowski-opensource
Copy link
Copy Markdown
Member Author

pkozlowski-opensource commented Jun 25, 2015

@SebastianM what you are saying is true, but:

  • proposed fix will work for other forms of bindings, ex.: data-bind-foo="exp"
  • as far as I recall there are some inconsistencies in specs, those "special" chars should be allowed in the end

@vicb I'm not sure we should have any special casing for [data-foo]="exp" - IMO this form doesn't bring anything for HTML validators and people shouldn't use it. IMO we should throw in this case and we do so currently.

@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Jun 25, 2015

Agree with @pkozlowski-opensource that we should only strip out data-[foo] case.
@SebastianM if you want to make validators happy you could write data-bind-foo

@mhevery
Copy link
Copy Markdown
Contributor

mhevery commented Jun 25, 2015

Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.

modules/angular2/test/render/dom/compiler/property_binding_parser_spec.ts, line 38 [r1] (raw file):
I think the test is correct.

Comments from the review on Reviewable.io

@mhevery mhevery added pr_state: LGTM action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Jun 25, 2015
@vicb vicb added action: discuss and removed action: merge The PR is ready for merge by the caretaker labels Jun 26, 2015
@vicb
Copy link
Copy Markdown
Contributor

vicb commented Jun 26, 2015

I don't agree and I've removed the merge flag.

I can't understand why data-[foo] should bind to foo in angular.

data-foo do not set the fooproperty in HTML.

Needs more discussion before merging.

@gkalpak
Copy link
Copy Markdown
Member

gkalpak commented Jun 26, 2015

Other than (outdated) validators, is there any other reason for stripping data- ?
Is it such a strong use-case ?
Won't validators be broken anyway (from other things such as custom tags) ?

@pkozlowski-opensource
Copy link
Copy Markdown
Member Author

pkozlowski-opensource commented Jun 26, 2015

@vicb would you suggest removing it only before bind- and on-? What are you proposing?

For me the system is simple - we strip data- prefix and then apply things as usual. Simple to understand and covering all the use-cases.

But I don't mind really - I don't use HTML validators myself. Seems like it is a big deal for some people, tough.

@vicb
Copy link
Copy Markdown
Contributor

vicb commented Jun 26, 2015

I suggested #2719 (comment) & #2719 (comment)

Out of angular context <x data-foo="bar"> would set x.dataset.foo (& x.getAttribute('data-foo') == 'bar'). I think this PR deviates from the standard html behavior.

IMO the only thing we need to support (hence special case) is [data-foo] / bind-data-foo

@pkozlowski-opensource
Copy link
Copy Markdown
Member Author

pkozlowski-opensource commented Jun 26, 2015

IMO the only thing we need to support (hence special case) is [data-foo] / bind-data-foo

I'm sorry but I disagree here... [data-foo] and bind-data-foo imply "bind to dataFoo" property which is an error as such property doesn't exist. We should be throwing. I don't see any reason of special-casing this - this would make the whole system less consistent and would introduce one more special case to remember / explain.

This is just my opinion of course, don't have more arguments so I suggest that you and @mhevery reach an agreement here and proceed accordingly.

@tbosch
Copy link
Copy Markdown
Contributor

tbosch commented Jun 29, 2015

@vicb @mhevery ping, what should be done here?

@vicb
Copy link
Copy Markdown
Contributor

vicb commented Jul 2, 2015

@pkozlowski-opensource let's have a chat to discuss about this

@vicb
Copy link
Copy Markdown
Contributor

vicb commented Jul 2, 2015

We have clarified this with @pkozlowski-opensource, LGTM.

There will be a new syntax [data.foo] to bind data attributes to expression.

@vicb vicb added pr_state: LGTM action: merge The PR is ready for merge by the caretaker and removed action: discuss labels Jul 2, 2015
@vicb vicb assigned pkozlowski-opensource and unassigned mhevery Jul 2, 2015
@vicb
Copy link
Copy Markdown
Contributor

vicb commented Jul 2, 2015

@pkozlowski-opensource could you please create a separate issue for [data.foo]

@PatrickJS
Copy link
Copy Markdown
Contributor

PatrickJS commented Jul 2, 2015

In my opinion the DomRenderer should output data- as an W3C Validator option rather than compiler being aware of something that only matters for browsers

@pkozlowski-opensource pkozlowski-opensource deleted the data_prefix branch July 20, 2015 12:54
@vicb vicb mentioned this pull request Sep 30, 2015
Closed
@angular-automatic-lock-bot
Copy link
Copy Markdown

angular-automatic-lock-bot bot commented Sep 6, 2019

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 Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

@pkozlowski-opensource pkozlowski-opensource

Labels

action: merge The PR is ready for merge by the caretaker cla: yes

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Issue with data-* properties not working on elements.

8 participants

@pkozlowski-opensource @vicb @sebholstein @mhevery @gkalpak @tbosch @PatrickJS @googlebot
X Tutup