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

Undesirable ng-attr-disabled behavior on custom elements #16602

Closed
morewry opened this Issue Jun 15, 2018 · 5 comments

Comments

Projects
None yet
3 participants
@morewry

morewry commented Jun 15, 2018

I'm submitting a ...

  • bug report
  • feature request
  • other

Current behavior:

ng-attr conditionally sets attributes. For most attributes, including ad hoc custom attributes, this means that if the expression interpolates to a string, that attribute will be set with that string as its value. But certain attributes, such as disabled, are recognized specifically and have different behavior. On some elements (e.g. button, input, etc.) disabled is a boolean HTML attribute and valid HTML requires it be either <button disabled> or <button disabled="disabled">. And that's how ng-attr-disabled behaves. But disabled isn't a global attribute and these semantics and rules really only apply for the standard HTML elements that are specified with the standard disabled attribute.

I have defined a Custom Element that observes a disabled attribute. But due to this behavior, consumers of my Custom Element can't use ng-attr on that custom element for the disabled attribute, because instead of outputting the value they're trying to set, the string disabled always gets output instead.

Actually, it's "worse" than I originally thought. There's no way to set any other value with Angular. See final section.

Expected / new behavior:

React has some similar issues affecting support for Web Components where it attempts to provide value to developers by throwing errors when standard HTML elements and attributes are used in invalid ways in JSX. My loose understanding is that React now checks for a hyphen, assumes any tag with hyphen in its name is a custom element, and goes down a different path without its potentially disruptive HTML validation behavior for those.

I think it might be preferable if ngAttr and ngDisabled (among, probably, some other directives) did something similar. The enforcement of disabled="disabled" should really only apply on standard HTML elements, not on Custom Elements.

Especially since, without the draconian error handling of serving XHTML as XML (perhaps even with it, I don't recall, it's been so long), it really doesn't matter what the value is for any element. Strictly speaking, it's invalid, but practically speaking, you can write the HTML <button disabled="woooooooo!!!"> and it's handled the same in browsers as <button disabled="disabled">.

Minimal reproduction of the problem with instructions:

https://codepen.io/morewry/pen/QxqrmJ

AngularJS version: 1.6.7, 1.7.3

Browser: all

Anything else:

I get that this could be problematic since it's a change in how Angular has behaved for a long time. It's a pain for using Web Components with Angular as-is, however, because it's almost impossible to work around without changing the attribute name, since the behavior shows up in all of the following:

  1. When using ng-attr-disabled
  2. When using ng-disabled
  3. When using disabled with interpolation, e.g. disabled="{{something}}"

But there's no particular reason this attribute name should have to change. The best vocabulary choice is disabled, not disable, not disabled-whatever or whatever-disabled, in part because it's familiar.

@gkalpak

This comment has been minimized.

Member

gkalpak commented Jun 16, 2018

A little background:

  • Special handling of boolean attributes in $compile was introduced long ago in 4f78fd6.
  • Later, in de9464c, boolean attributes where limited to specific elements (as the previous approach was too aggressive).
  • However, while boolean attributes are limited to specific elements as far as $compile's $attrs is concerned (e.g. see here), jqLite's attr —which is used by $attrs under the hood to set the attribute value— only takes the attribute name (not the element) into account.
  • jqLite#attr() matches the behavior of jQuery#attr().

So, I would say it is reasonable to take the element into account in attr(), but we need to keep it compatible with jQuery 😞

Thoughts @mgol, @jbedard?

It is possible to special-case $attrs.$set() to handle boolean properties more consistently, but I would rather avoid special casing if possible.

@gkalpak

This comment has been minimized.

Member

gkalpak commented Jun 21, 2018

Discussed this and agreed this is an issue with jQuery. We decided it doesn't make sense to break compatiblity with jQuery atm. (If this is changed in jQuery in the future, we can reconsider.)

In the meantime, it is fairly straight forward to work around this by creating your own directive that set disabled via setAttribute() and not .attr().

@gkalpak gkalpak closed this Jun 21, 2018

@morewry

This comment has been minimized.

morewry commented Jun 21, 2018

Didn't think of that workaround, thanks! It necessitates more education and a habit change for consuming developers, but it'll certainly mitigate.

Do you think I should file this for jQuery? Do you happen to know if the maintainers/community are concerned about Web Component compatibility?

@gkalpak

This comment has been minimized.

Member

gkalpak commented Jun 21, 2018

No idea about jQuery, but @mgol said he will bring it up in an upcoming meeting.
Probably doesn't hurt to open an issue on their tracker (if one doesn't exist already) 😉

@mgol

This comment has been minimized.

Member

mgol commented Jun 25, 2018

A relevant jQuery ticket has been opened: jquery/jquery#4114.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment