Implement debounced input action #9277

cvializ · 2017-05-11T22:11:42Z

Implement an input-debounced event and example.

Only merge this after #9252 to use the its performant debounce method.

jridgewell · 2017-05-12T21:00:32Z

src/service/action-impl.js

      });
+    } else if (name == 'input-debounced') {
+      this.root_.addEventListener('input', debounce_(event => {
+        this.trigger(dev().assertElement(event.target), name, event);


Danger! If this is a native event, the target and other properties of the event may be gone by the time this method is executed. Firefox in particular has issues.

Thanks for the heads up. Since the if blocks above have the same caveat, and this is only used to trigger amp actions I think it should be safe for this use-case.

What if block?

The ones just a few lines above this one e.g. if (name == 'keydown') { ... }. Does anything make this case different than those?

Those event listeners are executed sync, this one is async. The event object doesn't behave properly in async execution.

cvializ · 2017-05-15T18:22:43Z

src/utils/rate-limit.js

+ * @param {function(...*)} callback
 * @param {number} minInterval the minimum time interval in millisecond
- * @returns {function()}
+ * @returns {function(...*)}


Is this the right signature for a function that just forwards its arguments? I made this change because check-types showed a type mismatch for the input-debounced debounce callback which is {function(!Event)}, not {function()}

@erwinmombay?

jridgewell · 2017-05-15T21:21:51Z

src/utils/rate-limit.js

+ * @param {function(...*)} callback
 * @param {number} minInterval the minimum time interval in millisecond
- * @returns {function()}
+ * @returns {function(...*)}


@erwinmombay?

jridgewell · 2017-05-15T21:22:35Z

src/service/action-impl.js

+      }, DEFAULT_DEBOUNCE_WAIT);
+
+      this.root_.addEventListener('input', event => {
+        inputDebounced(event.target, event);


We'd need a full clone of the event to avoid the issue.

Do we have a canonical AMP way to clone a native object?

Not yet.

function cloneEvent(event) { const clone = map(); for (const prop in event) { const value = event[prop]; // Avoid functions like stopPropagation, since they can't do anything in an async context. if (typeof value !== 'function') { clone[prop] = event[prop]; } } }

jridgewell · 2017-05-15T22:24:36Z

src/service/action-impl.js

+    } else if (name == 'input-debounced') {
+      const inputDebounced = debounce(this.ampdoc.win, event => {
+        const target = dev().assertElement(event.target);
+        this.trigger(target, name, /** @type {!Event} */ (event));


We're gonna hit bugs later on with this typecast. Why not create a DeferredEvent type?

jridgewell · 2017-05-16T00:00:42Z

src/service/action-impl.js

+  return clone;
+}
+
+/** @private */


Nit: Having this throw will likely let us find bugs quicker.

jridgewell · 2017-05-16T00:00:51Z

src/service/action-impl.js

+  constructor(event) {
+    cloneWithoutFunctions(event, this);
+
+    /** @type {?Object} */


An additional details property was being added to the event object by addChangeDetails_ and the compiler states you cannot add properties to dict types (which ES6 classes are) after construction. So I added the property in the constructor.

The amount of code mutating native objects in AMP is too damn high! It almost always adds technical debt to do it.

aghassemi

let's also wait for #9305 before merging this.

aghassemi · 2017-05-16T00:08:03Z

src/service/action-impl.js

+    } else if (name == 'input-debounced') {
+      const debouncedInput = debounce(this.ampdoc.win, event => {
+        const target = dev().assertElement(event.target);
+        this.trigger(target, name, /** @type {!ActionEventDef} */ (event));


similar tochange event we want to include the value of the input at the time of trigger as details on the event object. Might require some refactoring of addChangeDetails_ to share code.

That's a good point, I'll add that in.

jridgewell · 2017-05-16T16:54:25Z

test/functional/test-action.js

+
+      for (const key in deferredEvent) {
+        if (typeof deferredEvent[key] !== 'function') {
+          expect(deferredEvent[key]).to.deep.equal(event[key]);


Nit deep shouldn't be necessary.

Unfortunately Chrome gives CustomEvent instances a path property and when you assign it to a variable, that variable is somehow not equal to the property on the event. using deep solves this. I'm not sure what else to do other than special-casing this property in the test.

jridgewell · 2017-05-16T16:54:51Z

test/functional/test-action.js

+      const event = createCustomEvent(window, 'MyEvent', {foo: 'bar'});
+      const deferredEvent = new DeferredEvent(event);
+
+      for (const key in deferredEvent) {


Please test for stopPropagation and preventDefault specifically.

aghassemi · 2017-05-17T23:45:02Z

@ericlindley-g @cvializ I like to suggest putting an experiment flag around this event and merging (instead of waiting to figure out the trust-level stuff first). WDYT?

cvializ · 2017-05-18T14:44:19Z

I think that sounds like a safe, low-effort way to minimize risk. I'll add it in

…too.

cvializ requested review from jridgewell and aghassemi May 11, 2017 22:11

googlebot added the cla: yes label May 11, 2017

jridgewell requested changes May 12, 2017

View reviewed changes

cvializ force-pushed the cv-event branch from 32ca77b to 8b7bff6 Compare May 15, 2017 17:41

cvializ commented May 15, 2017

View reviewed changes

jridgewell reviewed May 15, 2017

View reviewed changes

jridgewell approved these changes May 15, 2017

View reviewed changes

jridgewell approved these changes May 16, 2017

View reviewed changes

aghassemi suggested changes May 16, 2017

View reviewed changes

jridgewell reviewed May 16, 2017

View reviewed changes

cvializ force-pushed the cv-event branch from d4f53c7 to 60d3f35 Compare May 16, 2017 18:35

cvializ force-pushed the cv-event branch 2 times, most recently from 8567a0a to eac22d4 Compare May 19, 2017 18:08

Carlos Vializ added 9 commits May 22, 2017 08:38

Implement debounced input action

877da50

Use common util debounce function

2922064

Change signature of rate-limit fns to accept arguments

85146a2

Access event target property synchronously

853453f

Clone the event object to prevent async misbehavior

f0042ed

Create DeferredEvent and ActionEventDef.

93e9bc8

Test DeferredEvent. Throw on function calls.

032b17d

Explicitly test common event methods. Use addChangeDetails for input …

b634868

…too.

Experiment protect and test input-debounced

81c7733

cvializ force-pushed the cv-event branch from eac22d4 to 81c7733 Compare May 22, 2017 15:39

aghassemi approved these changes May 23, 2017

View reviewed changes

aghassemi mentioned this pull request May 23, 2017

autocomplete: list overlay (design & impl) #8825

Closed

Fix accidental find-and-replace

fb910a5

cvializ force-pushed the cv-event branch from 7c37148 to fb910a5 Compare May 23, 2017 17:28

cvializ merged commit dfc7c5a into ampproject:master May 23, 2017

ampprojectbot added the PR use: In Beta / Experimental label May 30, 2017

cvializ deleted the cv-event branch August 31, 2017 20:28

Implement debounced input action #9277

Implement debounced input action #9277

Uh oh!

Conversation

cvializ commented May 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvializ May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aghassemi left a comment

Choose a reason for hiding this comment

Uh oh!

aghassemi May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cvializ May 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aghassemi commented May 17, 2017

Uh oh!

cvializ commented May 18, 2017

Uh oh!

Uh oh!

cvializ commented May 11, 2017 •

edited

Loading

cvializ May 15, 2017 •

edited

Loading

aghassemi May 16, 2017 •

edited

Loading

cvializ May 16, 2017 •

edited

Loading