Dispatch VALUE_CHANGE on `amp-bind` value changes #22840

xrav3nz · 2019-06-14T19:13:34Z

This introduces a new AmpEvents.VALUE_CHANGE event. It is dispatched
when amp-bind mutates form input values, specifically:
<input [value]>, <textarea [text]>, <input [checked]>, and
<option [selected]>.

A custom event is chosen over a mutation observer, because binding
[text] on textarea does not change the DOM (and it's expected), thus
a mutation observer won't pick up the change. The same applies to
input elements, when we eventually have two separate bindings for
[value] and [defaultValue].

Part of #22534.

/cc @GoTcWang @choumx thanks!

GoTcWang

General ideas LG.
Commented on future use cases and naming, feel free to plus Will for answers.

extensions/amp-bind/0.1/test/integration/test-bind-impl.js

src/amp-events.js

extensions/amp-bind/0.1/bind-impl.js

GoTcWang · 2019-06-17T15:11:19Z

extensions/amp-bind/0.1/bind-impl.js

+ */
+function doesPropertyAffectFormValue(tagName, property) {
+  switch (property) {
+    case 'disabled':


Should we consider [hidden] attribute as well in this case then? Feel free to left a TODO and resolve in another PR.

Note that hidden attribute can be mutated on the ancestry of a form field.

Ugh right. So there are:

disabled for form fields and fieldsets

hidden for essentially all elements

type for <input>

I am still leaning towards using a mutation observer on the form for all of the above, and leave AmpEvents.FORM_VALUE_CHANGE as close to the native InputEvent as possible. (It's also semantically cleaner as Will mentioned)

Thoughts? @choumx @GoTcWang

Yup that also works.
I'll left it to Will to make the call.

extensions/amp-bind/0.1/test/integration/test-bind-impl.js

This introduces a new `AmpEvents.VALUE_CHANGE` event. It is dispatched when `amp-bind` mutates form input values, specifically: `<input [value]>`, `<textarea [text]>`, `<input [checked]>`, and `<option [selected]>`. A custom event is chosen over a mutation observer, because binding `[text]` on `textarea` does not change the DOM (and it's expected), thus a mutation observer won't pick up the change. The same applies to `input` elements, when we eventually have two separate bindings for `[value]` and `[defaultValue]`.

dreamofabear · 2019-06-17T19:16:39Z

extensions/amp-bind/0.1/bind-impl.js

+
+        if (isPropertyAFormValue(element.tagName, property)) {
+          const dispatchAt =
+            element.tagName === 'OPTION' ? element.closest('SELECT') : element;


Out of curiosity, why is this necessary? Also use closestAncestorElementBySelector() helper in dom.js in case Element.closest is not available.

Thanks! The native InputEvent is also dispatched at the parent <select> when the selected <option> changes. I think it makes sense, since it is the value of the <select> element that is changed.

I just added a comment explaining this!

dreamofabear · 2019-06-17T19:18:33Z

extensions/amp-bind/0.1/bind-impl.js

+          if (dispatchAt) {
+            dispatchAmpFormValueChangeEvent(this.localWin_, dispatchAt);
+          }
+        }


Nit: Can we keep everything in a single class function? It would also save you from checking tagName twice.

this.dispatchFormValueChangeEventIfNecessary_(element, property);

To only check tagName once, the switch gets a bit intense with the nested ifs and early returns: https://gist.github.com/xrav3nz/6fd7b048080aedbaa1e87c418b4193e4 Are you okay with this?

True, probably better as is. Speaking of switch statements, a lookup map might be more readable.

Optional:

/** @const {!Object<string, !Array<string>>} */ const FORM_VALUE_PROPERTIES = { 'INPUT': ['checked', 'value'], 'OPTION': ['selected'], 'TEXTAREA': ['text'], }; dispatchFormValueChangeEventIfNecessary_(element, property) { const props = FORM_VALUE_PROPERTIES[element.tagName]; if (props && props.includes(property)) { ... } }

~~I think I'm gonna to leave it as is~~ Updated! Thanks again for the reviews! 💪

GoTcWang

LGTM

dreamofabear · 2019-06-18T19:10:12Z

Nice work, thanks for contributing.

* Dispatch VALUE_CHANGE on `amp-bind` value changes This introduces a new `AmpEvents.VALUE_CHANGE` event. It is dispatched when `amp-bind` mutates form input values, specifically: `<input [value]>`, `<textarea [text]>`, `<input [checked]>`, and `<option [selected]>`. A custom event is chosen over a mutation observer, because binding `[text]` on `textarea` does not change the DOM (and it's expected), thus a mutation observer won't pick up the change. The same applies to `input` elements, when we eventually have two separate bindings for `[value]` and `[defaultValue]`. * more elaborate test names * rename event to FORM_VALUE_CHANGE * dispatch event at <select> when <option> changes * extract FORM_VALUE_CHANGE_EVENT_ARGUMENTS in test * dont use `closest` & move everything into a helper * refactor to use a lookup map instead * avoid a linear `includes` search

googlebot added the cla: yes label Jun 14, 2019

GoTcWang reviewed Jun 14, 2019

View reviewed changes

extensions/amp-bind/0.1/test/integration/test-bind-impl.js Outdated Show resolved Hide resolved

src/amp-events.js Outdated Show resolved Hide resolved

xrav3nz marked this pull request as ready for review June 14, 2019 20:30

xrav3nz mentioned this pull request Jun 14, 2019

Dispatch a MUTATE event after amp-bind mutation #22801

Closed

dreamofabear reviewed Jun 14, 2019

View reviewed changes

extensions/amp-bind/0.1/bind-impl.js Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

GoTcWang reviewed Jun 17, 2019

View reviewed changes

xrav3nz added 5 commits June 17, 2019 13:42

more elaborate test names

8c7b7b1

rename event to FORM_VALUE_CHANGE

2ffce9b

dispatch event at <select> when <option> changes

1dc4cff

extract FORM_VALUE_CHANGE_EVENT_ARGUMENTS in test

d356e4d

xrav3nz force-pushed the amp-bind/value-change-event branch from d764b59 to d356e4d Compare June 17, 2019 17:45

dreamofabear reviewed Jun 17, 2019

View reviewed changes

dont use closest & move everything into a helper

1850397

dreamofabear approved these changes Jun 17, 2019

View reviewed changes

xrav3nz added 2 commits June 17, 2019 17:33

refactor to use a lookup map instead

58a068a

avoid a linear includes search

fc3ea20

GoTcWang reviewed Jun 18, 2019

View reviewed changes

dreamofabear merged commit 0be1ae1 into ampproject:master Jun 18, 2019

xrav3nz deleted the amp-bind/value-change-event branch June 18, 2019 19:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dispatch VALUE_CHANGE on `amp-bind` value changes #22840

Dispatch VALUE_CHANGE on `amp-bind` value changes #22840

xrav3nz commented Jun 14, 2019 •

edited

GoTcWang left a comment

This comment has been minimized.

GoTcWang Jun 17, 2019

xrav3nz Jun 17, 2019 •

edited

GoTcWang Jun 17, 2019

dreamofabear Jun 17, 2019

dreamofabear Jun 17, 2019

xrav3nz Jun 17, 2019

dreamofabear Jun 17, 2019

xrav3nz Jun 17, 2019

dreamofabear Jun 17, 2019

xrav3nz Jun 17, 2019 •

edited

GoTcWang left a comment

dreamofabear commented Jun 18, 2019

Dispatch VALUE_CHANGE on amp-bind value changes #22840

Dispatch VALUE_CHANGE on amp-bind value changes #22840

Conversation

xrav3nz commented Jun 14, 2019 • edited

GoTcWang left a comment

Choose a reason for hiding this comment

This comment has been minimized.

Choose a reason for hiding this comment

xrav3nz Jun 17, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xrav3nz Jun 17, 2019 • edited

Choose a reason for hiding this comment

GoTcWang left a comment

Choose a reason for hiding this comment

dreamofabear commented Jun 18, 2019

Dispatch VALUE_CHANGE on `amp-bind` value changes #22840

Dispatch VALUE_CHANGE on `amp-bind` value changes #22840

xrav3nz commented Jun 14, 2019 •

edited

xrav3nz Jun 17, 2019 •

edited

xrav3nz Jun 17, 2019 •

edited