-
Notifications
You must be signed in to change notification settings - Fork 191
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
[BUGFIX] Fix reactivity of dynamic attributes #1268
Conversation
The updates to use autotracking through the VM resulted in a bug in dynamic attributes. Previously, the updating opcode for dynamic attributes would run its update function based on tags/autotracking - whenever any tag updated (e.g. a tracked value was changed) the attribute would call its `update` function. The `update` function would then check the new value against the current value of the attribute, and update if they were different. The bug was we introduced a value comparison in the updating opcode itself, based on the last value of the property to set the attribute to. This meant that if the property was updated to the same value twice in a row, it would not call `update`, even if the attribute's value had changed (e.g. because the user typed something).
2ba9d95
to
988a21e
Compare
let value = valueForRef(reference); | ||
|
||
if (initialized === true) { | ||
attribute.update(value, env); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, I still don't think this guarantees that the DynamicAttribute
will actually update things. Some of the subclasses of DynamicAttribute
have caches that do not check if the DOM value was changed out from under them.
For example:
glimmer-vm/packages/@glimmer/runtime/lib/vm/attributes/dynamic.ts
Lines 117 to 127 in 4b69798
update(value: unknown, _env: Environment): void { | |
let { element } = this.attribute; | |
if (this.value !== value) { | |
(element as any)[this.normalizedName] = this.value = value; | |
if (value === null || value === undefined) { | |
this.removeAttribute(); | |
} | |
} | |
} |
The reason the test added above passes is because input.value
has a custom DynamicAttribute
subclass (that doesn't do the last value caching that DefaultDynamicProperty
does):
glimmer-vm/packages/@glimmer/runtime/lib/vm/attributes/dynamic.ts
Lines 175 to 182 in 4b69798
update(value: unknown) { | |
let input = castToBrowser(this.attribute.element, ['input', 'textarea']); | |
let currentValue = input.value; | |
let normalizedValue = normalizeStringValue(value); | |
if (currentValue !== normalizedValue) { | |
input.value = normalizedValue; | |
} | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this almost certainly does address the reported issue, it just doesn't 100% fix the full category of issues. I'm fairly sure you could end up with this same issue with other element + attribute combinations that the user could mutate. We have custom overrides for a few common ones (input.value
, textarea.value
, option.selected
), but that list of overrides is not exhaustive (e.g. video element has some local mutable attributes | properties that could have been changed by the user).
The updates to use autotracking through the VM resulted in a bug in
dynamic attributes. Previously, the updating opcode for dynamic
attributes would run its update function based on tags/autotracking -
whenever any tag updated (e.g. a tracked value was changed) the
attribute would call its
update
function. Theupdate
function wouldthen check the new value against the current value of the attribute, and
update if they were different.
The bug was we introduced a value comparison in the updating opcode
itself, based on the last value of the property to set the attribute to.
This meant that if the property was updated to the same value twice in
a row, it would not call
update
, even if the attribute's value hadchanged (e.g. because the user typed something).
Fixes emberjs/ember.js#19341