Exposing input change event and form.submit action #5707

mkhatib · 2016-10-20T00:26:15Z

Implements #4272

Play with the demo at the bottom of the page here. Make sure to toggle both amp-form and form-submit experiments.

mkhatib · 2016-10-20T00:26:47Z

extensions/amp-form/0.1/amp-form.js

@@ -138,6 +135,14 @@ export class AmpForm {
    /** @const @private {!./form-validators.FormValidator} */
    this.validator_ = getFormValidator(this.form_);

+    this.actions_.installActionHandler(this.form_, invocation => {


@dvoytenko is this the right thing to do here? Or using addGlobalMethodHandler a better option?

Something like installActionHandler is definitely better.

mkhatib · 2016-10-20T00:27:30Z

src/service/action-impl.js

+    // TODO: ASK @dima about this? Is this limited to AMP-elements for security
+    // reasons? If yes, shouldn't one check the tagname rather than the id?
+    //user().assert(target.id && target.id.substring(0, 4) == 'amp-',
+    //    'AMP element is expected: %s', debugid);


@dvoytenko why is this limited to AMP elements? Would it be ok to relax this?

Yeah. Probably would be ok to relax. The intention here is really to reduce a probability of error by installing this handler on an accidental element. But I just checked and it looks like installActionHandler is already a restricted API in presubmits. So we don't need to worry too much about misuse.

A slightly bigger problem is on the trigger side - where you also disabled AMP check. The issue is that if a publisher accidentally specifies the wrong target, they'll never know - a very easy mistake to make. So, I think, we still need some form of whitelist.

dvoytenko · 2016-10-21T00:10:05Z

src/service/action-impl.js

+    // TODO: ASK @dima about this? Is this limited to AMP-elements for security
+    // reasons? If yes, shouldn't one check the tagname rather than the id?
+    //user().assert(target.id && target.id.substring(0, 4) == 'amp-',
+    //    'AMP element is expected: %s', debugid);


Yeah. Probably would be ok to relax. The intention here is really to reduce a probability of error by installing this handler on an accidental element. But I just checked and it looks like installActionHandler is already a restricted API in presubmits. So we don't need to worry too much about misuse.

A slightly bigger problem is on the trigger side - where you also disabled AMP check. The issue is that if a publisher accidentally specifies the wrong target, they'll never know - a very easy mistake to make. So, I think, we still need some form of whitelist.

dvoytenko · 2016-10-21T00:18:16Z

extensions/amp-form/0.1/amp-form.js

@@ -138,6 +135,14 @@ export class AmpForm {
    /** @const @private {!./form-validators.FormValidator} */
    this.validator_ = getFormValidator(this.form_);

+    this.actions_.installActionHandler(this.form_, invocation => {


Something like installActionHandler is definitely better.

dvoytenko · 2016-10-21T00:18:59Z

extensions/amp-form/0.1/amp-form.js

@@ -163,20 +173,24 @@ export class AmpForm {
   * invalid. stopImmediatePropagation allows us to make sure we don't trigger it
   *
   *
-   * @param {!Event} e
+   * @param {?Event} optEvent


opt_event

mkhatib · 2016-10-25T21:50:53Z

@dvoytenko added the whitelist, PTAL 👀 - will be adding tests in the next commit

dvoytenko

Looks great. Couple of requests and looking forward to tests.

dvoytenko · 2016-10-26T18:20:36Z

extensions/amp-form/0.1/amp-form.js

@@ -152,6 +157,11 @@ export class AmpForm {
      onInputInteraction_(e);
      this.validator_.onInput(e);
    });
+    this.form_.addEventListener('change', e => {
+      const input = e.target;
+      // TODO(#5702): Consider a debounced-input event for text-type inputs.


TODO(ldap, #issue)

dvoytenko · 2016-10-26T18:23:35Z

src/service/action-impl.js

@@ -271,9 +273,17 @@ export class ActionService {
      return;
    }

+    // Check if this is an element has a known action.
+    const supportedActions = ELEMENTS_ACTIONS_MAP_[lowerTagName];
+    if (target[ACTION_QUEUE_] && supportedActions &&


Why are you not initializing ACTION_QUEUE_ right here? Are risk conditions do not apply here? Is it even worth the cost?

Btw, this if and the previous one (target.id ~= amp-) can be combined.

The problem with initializing ACTION_QUEUE_ here is basically initialize it to what? An empty array? This case specifically will probably be reached if the element uses installActionHandler method to initialize the ACTION_QUEUE_ to be an object with a fake push method to allow executing the handler passed.

So wouldn't initializing it here basically fail silently for these cases? We could throw a better error when not initialized - basically user().error('Action queue not initialized, probably tried to execute before calling installActionHandler on %s', this.element) - or something like that. No?

Not quite. We indeed initialize to an empty array. You can see it just 5 lines above for an amp- case (I mentioned that this should probably be the same code). What this gives you is that it buffers up actions in an array until installActionHandler is called. As you can see here, when installActionHandler is called, it goes over buffered actions and sends them to the handler. This way we remove risk condition between when a user can activate an action and when installActionHandler is called. That makes sense since installActionHandler installation depends on asynchronous script download.

In cases like this it's possible that it's "too late" to execute an action. If this happens, we can add "deferred" flag to an action that's not executed immediately. We do this here, but to my knowledge no actions use it yet. So we can punt on this here.

Make sense! Thanks for explaining. I'll update the code.

dvoytenko · 2016-10-26T18:25:37Z

src/service/action-impl.js

@@ -163,9 +167,6 @@ export class ActionService {
   */
  installActionHandler(target, handler) {
    const debugid = target.tagName + '#' + target.id;
-    user().assert(target.id && target.id.substring(0, 4) == 'amp-',


(1) This should be a dev().assert()
(2) Why not also check here supportedActions? This way you get an early notification as a developer that an element (at least) must be whitelisted.

dvoytenko · 2016-10-26T18:28:15Z

src/service/action-impl.js

@@ -33,6 +33,10 @@ const ACTION_QUEUE_ = '__AMP_ACTION_QUEUE__';
 /** @const {string} */
 const DEFAULT_METHOD_ = 'activate';

+/** @const {!Object<string,!Array<string>>} */
+const ELEMENTS_ACTIONS_MAP_ = {


This is good for now, but just FYI: it'd be even enough to just check for element name - the most likely error is confusing element's for another. If this set goes out of control - we can simply move the assertion for "unknown action" into the form's action handler itself.

mkhatib · 2016-10-26T23:44:19Z

@dvoytenko added tests and addressed comments PTAL 👀

jridgewell · 2016-10-27T18:24:51Z

extensions/amp-form/0.1/amp-form.js

+      this.handleSubmit_();
+    }
+  }
+
  /** @private */
  installSubmitHandler_() {


Can we rename this?

Yes 😄 I thought I did. Will do

dvoytenko · 2016-10-27T21:47:24Z

LGTM

mkhatib assigned dvoytenko Oct 20, 2016

mkhatib commented Oct 20, 2016

View reviewed changes

dvoytenko suggested changes Oct 21, 2016

View reviewed changes

dvoytenko suggested changes Oct 26, 2016

View reviewed changes

mkhatib changed the title ~~WIP: Exposing input change event and form.submit action~~ Exposing input change event and form.submit action Oct 26, 2016

mkhatib added the NEEDS REVIEW label Oct 26, 2016

mkhatib force-pushed the input-change-submit branch from a2b7c88 to 1ff2550 Compare October 26, 2016 23:22

jridgewell reviewed Oct 27, 2016

View reviewed changes

mkhatib added 6 commits October 27, 2016 14:43

Exposing input change event and form.submit action

0928a03

add methods whitelist.

c914ccd

fix opt_

81266d8

add tests and fix comments

fc26a64

fix type

2654981

Move change trigger to global event.

1cc2699

mkhatib force-pushed the input-change-submit branch from 2a9ca40 to 1cc2699 Compare October 27, 2016 21:44

dvoytenko approved these changes Oct 27, 2016

View reviewed changes

dvoytenko added LGTM and removed NEEDS REVIEW labels Oct 27, 2016

mkhatib merged commit 62ca0b6 into ampproject:master Oct 27, 2016

mkhatib deleted the input-change-submit branch October 27, 2016 22:05

mkhatib mentioned this pull request Oct 28, 2016

A FORM element needs an input type=submit; uncaught console log error #5871

Closed

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

Exposing input change event and form.submit action (ampproject#5707)

1cf7144

Lith pushed a commit to Lith/amphtml that referenced this pull request Dec 22, 2016

Exposing input change event and form.submit action (ampproject#5707)

3f93bd3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exposing input change event and form.submit action #5707

Exposing input change event and form.submit action #5707

mkhatib commented Oct 20, 2016 •

edited

mkhatib Oct 20, 2016

dvoytenko Oct 21, 2016

mkhatib Oct 25, 2016

mkhatib Oct 20, 2016

dvoytenko Oct 21, 2016

mkhatib Oct 25, 2016

dvoytenko Oct 21, 2016

dvoytenko Oct 21, 2016

dvoytenko Oct 21, 2016

mkhatib Oct 25, 2016

mkhatib commented Oct 25, 2016

dvoytenko left a comment

dvoytenko Oct 26, 2016

mkhatib Oct 26, 2016

dvoytenko Oct 26, 2016

mkhatib Oct 26, 2016

dvoytenko Oct 27, 2016

mkhatib Oct 27, 2016

mkhatib Oct 27, 2016

dvoytenko Oct 26, 2016

mkhatib Oct 26, 2016

dvoytenko Oct 26, 2016

mkhatib Oct 26, 2016

mkhatib commented Oct 26, 2016

jridgewell Oct 27, 2016

mkhatib Oct 27, 2016

mkhatib Oct 27, 2016

dvoytenko commented Oct 27, 2016

Exposing input change event and form.submit action #5707

Exposing input change event and form.submit action #5707

Conversation

mkhatib commented Oct 20, 2016 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkhatib commented Oct 25, 2016

dvoytenko left a 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

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

mkhatib commented Oct 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvoytenko commented Oct 27, 2016

mkhatib commented Oct 20, 2016 •

edited