amp-bind: Handle attributes on input elements that sometimes only specify initial values#9584
Conversation
extensions/amp-bind/0.1/bind-impl.js
Outdated
| element.removeAttribute(property); | ||
| attributeChanged = true; | ||
| } | ||
| if (requiresJsUpdate) { |
There was a problem hiding this comment.
Can't we move this above the attribute change stuff, and get rid of oldJsPropertyValue?
There was a problem hiding this comment.
good idea getting rid of the var, but I'd rather not set attributeChanged until after the attributes have been changed.
There was a problem hiding this comment.
@jridgewell On second thought, the property value needs to be read before the attributes are changed (in case the user hasn't interacted with the input yet). I've reverted the change. Let me know if you have a cleaner way I'm not seeing.
There was a problem hiding this comment.
set attributeChanged until after the attributes have been changed.
I don't see why not? If you do this, it'll work fine.
There was a problem hiding this comment.
Or you can do:
if (requiresJsUpdate && element[property] !== newValue) {
element[property] = newValue;
attributeChanged = true;
}
7957e37 to
cffc5ec
Compare
extensions/amp-bind/0.1/bind-impl.js
Outdated
| } else { | ||
| attributeChanged = | ||
| (oldValue === null && newValue) || | ||
| (oldValue === '' && !newValue); |
There was a problem hiding this comment.
I think it's more readable if you reverted the attributeChanged = true lines above and removed this else block.
extensions/amp-bind/0.1/bind-impl.js
Outdated
| element.removeAttribute(property); | ||
| attributeChanged = true; | ||
| } | ||
| if (requiresJsUpdate) { |
There was a problem hiding this comment.
Or you can do:
if (requiresJsUpdate && element[property] !== newValue) {
element[property] = newValue;
attributeChanged = true;
}
extensions/amp-bind/0.1/bind-impl.js
Outdated
| // Once the user interacts with these elements, the JS properties | ||
| // underlying these attributes must be updated for the change to be | ||
| // visible to the user. | ||
| const requiresJsUpdate = |
8bd28a5 to
95aa3ab
Compare
extensions/amp-bind/0.1/bind-impl.js
Outdated
| } else { | ||
| attributeChanged = | ||
| (oldValue === null && newValue) || | ||
| (oldValue === '' && !newValue); |
There was a problem hiding this comment.
Still think the old code was easier to read. 😄 Also you'd avoid useless setAttribute/removeAttribute calls on the element.
There was a problem hiding this comment.
I agree, just trying to get the tests to pass before I push 🤣
extensions/amp-bind/0.1/bind-impl.js
Outdated
| attributeChanged = element[property] !== newValue; | ||
| element[property] = newValue; | ||
| attributeChanged = true; | ||
| } else { |
There was a problem hiding this comment.
This can't at the same level, as updateElementProperty && value != newValue
With this PR, if the binding of one of these attributes updates, the actual underlying property will update. For example, the
valueattribute on a text input only specifies the initial text. If the binding to[value]changes, the text in the input will update./to @choumx @jridgewell
/cc @cvializ
Fixes #9556