Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Replace: true doesn't keep attributes that have been set on the element in the compile fn #10465

Open
j-meyer opened this issue Dec 15, 2014 · 5 comments

Comments

@j-meyer
Copy link

j-meyer commented Dec 15, 2014

I'm not sure if this is a bug, or if I missed something in the release notes for upgrading to 1.3 .

Using replace:true on a directive with an input element such as a checkbox in Angular 1.2 sets the classes ng-valid and ng-pristine on the parent element.

However, in Angular 1.3.6, only the ng-valid class is being set on the parent element.

Reproducible: always
Browsers: Chrome 39, Firefox 34, and IE 10
Operating system: Windows 7

Plunkers Demonstrating the Difference in Behavior:
Angular 1.3.6 : http://plnkr.co/edit/VES7ZZXfqrpXU4gVwmYs?p=preview
Angular 1.2.9 : http://plnkr.co/edit/DPAfwHjGOfMw3grYiTxf?p=preview

@Narretz
Copy link
Contributor

Narretz commented Dec 15, 2014

Interesting. However, angular always sets the classes on the element that has ng-model. So the "bug" here ist that ng-pristine is not set. However, ng-model on the label doesn't really make sense in the first place, so what's your motivation to do it like this? As far as I can see the model is never updated when you use it like you do.

@j-meyer
Copy link
Author

j-meyer commented Dec 15, 2014

The plunkers are a simplification of some legacy code I was supporting. The developer had more logic in the directive's link function to basically have the ng-model on the label update the nested checkboxes and have the child checkboxes update the label as well. I wouldn't be opposed to this being filed as a won't fix as it isn't a very Angular way of doing things, but I wanted to file the issue just to document it in case anyone else runs into it. I just found it weird that it was still setting the ng-valid class, but not the pristine class, I would've thought it was an all or nothing thing.

@Narretz
Copy link
Contributor

Narretz commented Dec 17, 2014

Okay, thanks.
In 1.3, we made some efforts to prevent duplicate setting of classes, maybe that's the cause.
I'll put this in the backlog - it doesn't seem to affect anything else.

@Narretz Narretz modified the milestones: Backlog, Purgatory Dec 17, 2014
@Narretz
Copy link
Contributor

Narretz commented Feb 6, 2015

Note: since replace is deprecated, this is unlikely to be get fixed.
This one's actually different than all the other merge-attributes bugs with replace. Sometime in 1.2.x, the initial setting of classes for ngModel was moved to the compile function. addClass is directly called on the element. Afterwards, replace replaces the old with the new node, and merges the attributes (including classes). But it only merges the attributes that are set on the template, not on the element! Since we called addClass on the node that is getting replaced, these are lost.
Now ng-valid is on the element because it gets set after validation passes, too.

A fix would be to call $addClass on the attrs object which gets passed to the compile function, but I don't know if this would eliminate the performance gains.
edit: This is not what we want, as that calls $animate.addClass

@Narretz Narretz modified the milestones: Ice Box, Backlog Feb 6, 2015
@j-meyer
Copy link
Author

j-meyer commented Feb 6, 2015

Yeah, it's not a huge deal for me, the simplest work around for us was to just not use replace : true, because it's deprecated anyways.

@Narretz Narretz changed the title Replace True Breaking Pristine Class Replace: true doesn't keep attributes that have been set on the element in the compile fn Feb 22, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants