Repository navigation
NgClass removes existing class when applying array that contains with 2 empty slots #17852
Description
I'm submitting a ...
[ ] Regression (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
Construct a class array with 4 strings. The middle classes are an empty string. Then change the 2nd class, leaving the other classes unchanged.
Current behavior
NgClass does not apply correct classes. It removes the last class from the array. Internally the last class is added to the add and remove list, and since the add class code does nothing since it's already in the list and does not error, the remove list removes the class.
Expected behavior
NgClass apply correct classes. The action should be to do nothing for the other existing classes and only add the 2nd class.
The following code tries to add the existing class in changes.forEachAddedItem, then removes it in changes.forEachRemovedItem. I would suggest not only fixing the bug, but reverse the order to first remove, then add, in case there are more bugs in the future.
https://github.com/angular/angular/blob/master/packages/common/src/directives/ng_class.ts#L105
private _applyIterableChanges(changes: IterableChanges<string>): void {
changes.forEachAddedItem((record) => {
if (typeof record.item === 'string') {
this._toggleClass(record.item, true);
} else {
throw new Error(
`NgClass can only toggle CSS classes expressed as strings, got ${stringify(record.item)}`);
}
});
changes.forEachRemovedItem((record) => this._toggleClass(record.item, false));
}
Minimal reproduction of the problem with instructions
http://plnkr.co/edit/bTsnyx1EciH66tRROu5o?p=preview
What is the motivation / use case for changing the behavior?
Developers expect binding to be correct.
Please tell us about your environment
Angular version: 4.2.4
Browser:
- [x] Chrome (desktop) version 59
- [ ] Chrome (Android) version XX
- [ ] Chrome (iOS) version XX
- [ ] Firefox version XX
- [ ] Safari (desktop) version XX
- [ ] Safari (iOS) version XX
- [ ] IE version XX
- [x] Edge version 15
For Tooling issues:
- Node version: XX
- Platform:
Others:
Updated all NgStyle to NgClass - sorry for the brain fart.
Activity
tytskyi commentedon Jun 29, 2017
Incorrect issue title, you do refer to
[ngClass], notNgStyleTo not have such surprises do not use
[ngClass]see Add
ngAttrlikengClassandngStyle#13940 (comment)and feat(common): introduce ngAttr directive #13958 (comment)
Perfect use-case for
[ngClass]- is to generate dynamic class-name in runtime. When you don't know class in advance, for example[ngClass]="getMyRandomClassName()". Or attaching multiple classes at once. Everything else could be solved within[class.class-name]="true"binding.Related issues that i found quickly:
[-]NgStyle removes existing style when applying array that contains with 2 empty slots[/-][+]NgClass removes existing style when applying array that contains with 2 empty slots[/+]wizarrc commentedon Jun 29, 2017
@tytskyi
I looked at the 4 related issues you listed, and all but the top one is closed. Does that mean they are already fixed or won't fix? The top related issue mentions using both ngClass and class. I am only using ngClass in my repo. I think this is a separate issue.
Anyways, here is a precise case where it fails, and is clearly a bug. If you debug the code, you can see that it adds the class to both the add and remove list during the
ngDoCheckcall in ngClass.[-]NgClass removes existing style when applying array that contains with 2 empty slots[/-][+]NgClass removes existing class when applying array that contains with 2 empty slots[/+]vicb commentedon Jun 29, 2017
That is definitely a bug in the iterable differ
wizarrc commentedon Jun 30, 2017
@vicb I think you are right.
https://github.com/angular/angular/blob/master/packages/core/src/change_detection/differs/default_iterable_differ.ts#L196
On the third item check, I took a screen shot before and after it calls
_verifyReinsertion. It looks like the record it is returning is also being used to progress to the next record, which in this case, is causing the record to show it being added and then deleted.Before:
After:
Could the fix be as simple as the following?
so record is not changed for when it calls
record = record._next;.wizarrc commentedon Jun 30, 2017
I updated the repo to show duplicated data using only the
DefaultIterableDifferalong with output from the added and removed list..http://plnkr.co/edit/WPWy2Nx0B4LErW7dxQKK?p=preview
vicb commentedon Jun 30, 2017
Thanks guys for helping in debugging this issue, that's really appreciated !
@wizarrc the fix is probably along the lines of what you have proposed. I'll double check tomorrow.
wizarrc commentedon Jun 30, 2017
@vicb No problem. Once I figure out the build system, I could test my proposals and submit PRs, until then, that's the best I can do.
vicb commentedon Jun 30, 2017
Your are probably looking for
./test.sh browserNoRouter?wizarrc commentedon Jun 30, 2017
@vicb I'm pretty sure that's bash talk. I'm coming from the command prompt and PowerShell (bat/ps1 scripts) world. I don't suppose there is vanilla Windows support for building angular? Once Bash for Windows is enabled by default, I will be taking a look into that, but right now it's still kinda beta.
6 remaining items
wizarrc commentedon Jun 30, 2017
@vicb LGTM
I think I understand better how the differ works now. It's a complicated data structure. I'd have to really break it down sometime when I need a good puzzle.
fix(core): fix re-insertions in the iterable differ
vicb commentedon Jul 1, 2017
Thanks for the feedback - and contact me if you like puzzles we probably can find some issues for you to work on :)
wizarrc commentedon Jul 3, 2017
@vicb I'd love to, right now I am still having lots of issues getting angular to build on windows. The
build.shscript addedrsynccommands back in February and I think I found workarounds, including manually adding a junction directory that's out of date in thescripts\windows\create-symlinks.shscript, upgrading node to version 6, even though theDEVELOPER.mdsays version < 6, and modifying the build script to workaround using root directories withrsync. I'm still hanging the process with warning messages. Windows support seems to be a real mess at the moment.I posted my progress here: #17894 (comment)
Any help getting that working would greatly help me to get involved.
fix(core): fix re-insertions in the iterable differ (angular#17891)
fix(core): fix re-insertions in the iterable differ (#17891)
fix(core): fix re-insertions in the iterable differ (angular#17891)
fix(core): fix re-insertions in the iterable differ (angular#17891)
fix(core): fix re-insertions in the iterable differ (angular#17891)
angular-automatic-lock-bot commentedon Sep 12, 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.