1446158 - Create a handleEvent custom element mixin to forward events from handleEvent to on* methods

Status: RESOLVED FIXED

(Firefox :: WebPayments UI, enhancement, P1)

Description

[Matthew N. [:MattN]]

To avoid the boilerplate of defining `handleEvent` in each custom element and forwarding to an on* method, we should have a mixin to do this for us.

I'm on the fence about whether the mixin should help with attaching the listeners, initially I think it shouldn't and we can then improve it more later.

Example with the mixin:
```js
class BasicCardForm extends HandleEventMixin(HTMLElement) {
  constructor() {
    super();
    this.addEventListener("click", this);
  }

  // no need to define handleEvent!

  onClick(event) {
    console.log("do stuff!");
  }
}
```

Comment 1

[Sam Foster [:sfoster] (he/him)]

Maybe something like this.attach() or this.attachEventListener() to make the mixin magic a bit clearer?

Comment 2

[Matthew N. [:MattN]]

Moving to reserve since I don't think we'll be adding that many more event listeners and things are working okay as-is. This won't impact users but would still be nice to have.

Comment 3

[Diego Pino]

Attachment: Bug-1446158-Create-a-handleEvent-custom-element-mixi.patch

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 9029775 [details] [diff] [review]
Bug-1446158-Create-a-handleEvent-custom-element-mixi.patch

Review of attachment 9029775 [details] [diff] [review]:
-----------------------------------------------------------------

Note that this diff would have been easier to review on Phabricator.

I would give r+ but I want to understand the new addEventListener.

::: browser/components/payments/res/containers/basic-card-form.js
@@ +368,5 @@
>    /**
>     * @param {Event} event - "invalid" event
>     * Note: Keep this in-sync with the equivalent version in address-form.js
>     */
> +  onInvalid(event) {

Is that existing comment supposed to go with `onInvalidField`?

::: browser/components/payments/res/containers/shipping-option-picker.js
@@ +20,1 @@
>    }

Hmm… why is this needed? super() should cause https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/browser/components/payments/res/containers/rich-picker.js#18 to run and add the listener on the dropdown.

::: browser/components/payments/res/mixins/HandleEventMixin.js
@@ +8,5 @@
> +export default function HandleEventMixin(superclass) {
> +  return class HandleEvent extends superclass {
> +    constructor() {
> +      super();
> +    }

Since this constructor doesn't do anything you can omit it.

@@ +18,5 @@
> +      // Check whether event name is a defined function in object.
> +      let fn = "on" + capitalize(evt.type);
> +      if (this[fn] && typeof(this[fn]) === "function") {
> +        return this[fn](evt);
> +      }

You're missing a conditional `super.handleEvent(evt);` call in case an ancestor also wants a handleEvent.

Comment 5

[Diego Pino]

Attachment: Bug-1446158-Create-a-handleEvent-custom-element-mixi.patch

Thanks for the review. I agree on all suggested changes. I'll try to use Phabricator next time for long patches (I just finished setting it up).

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 9032094 [details] [diff] [review]
Bug-1446158-Create-a-handleEvent-custom-element-mixi.patch

Review of attachment 9032094 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/payments/res/mixins/HandleEventMixin.js
@@ +10,5 @@
> +    handleEvent(evt) {
> +      function capitalize(str) {
> +        return str.charAt(0).toUpperCase() + str.slice(1);
> +      }
> +      super.handleEvent(evt);

Sorry, I guess I should have been more clear. I believe this needs to be conditional on super.handleEvent existing like we do in some other places:
```js
if (super.handleEvent) {
  super.handleEvent(evt);
}
```

Comment 7

[Diego Pino]

Attachment: Bug-1446158-Create-a-handleEvent-custom-element-mixi.patch

Updated patch.

Comment 8

[Diego Pino]

Attachment: Bug-1446158-Create-a-handleEvent-custom-element-mixi.patch

The last patch didn't include the new changes. Updated.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ca43b63f0e9
Create a handleEvent custom element mixin to forward events from handleEvent to on* methods. r=MattN

Comment 10

[Cristina Coroiu [:ccoroiu]]

Backed out changeset 7ca43b63f0e9 (bug 1446158) for ESlint failure at browser/components/payments/res/containers/address-form.js 

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/e41601a3a57d1f3d17b38d4c894accc26fe2a051

Failure push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=7ca43b63f0e9fe96859e0cb0cedec494953d37b1

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217752821&repo=mozilla-inbound&lineNumber=280

[task 2018-12-18T20:01:03.868Z] 
[task 2018-12-18T20:01:03.868Z] Error processing command. Ignoring because optional. (optional:packages.txt:comm/build/virtualenv_packages.txt)
[task 2018-12-18T20:06:49.205Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/payments/res/containers/address-form.js:24:1 | Line 24 exceeds the maximum line length of 100. (max-len)
[task 2018-12-18T20:06:49.205Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/payments/res/containers/basic-card-form.js:24:1 | Line 24 exceeds the maximum line length of 100. (max-len)
[task 2018-12-18T20:06:49.205Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/payments/res/containers/completion-error-page.js:19:1 | Line 19 exceeds the maximum line length of 100. (max-len)
[task 2018-12-18T20:06:49.205Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/payments/res/containers/payment-dialog.js:30:1 | Line 30 exceeds the maximum line length of 100. (max-len)
[task 2018-12-18T20:06:49.206Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/payments/res/containers/payment-method-picker.js:121:5 | 'onInputOrChange' is not defined. (no-undef)
[task 2018-12-18T20:06:49.206Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/payments/res/containers/payment-method-picker.js:125:5 | 'onInputOrChange' is not defined. (no-undef)
[task 2018-12-18T20:06:49.206Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/payments/res/mixins/HandleEventMixin.js:5:1 | Missing JSDoc @returns for function. (valid-jsdoc)
[task 2018-12-18T20:06:49.206Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/payments/res/mixins/HandleEventMixin.js:5:1 | Missing JSDoc for parameter 'superclass'. (valid-jsdoc)
[task 2018-12-18T20:06:49.206Z] TEST-UNEXPECTED-ERROR | /builds/worker/checkouts/gecko/browser/components/payments/res/mixins/HandleEventMixin.js:10:5 | Expected to return a value at the end of method 'handleEvent'. (consistent-return)
[taskcluster 2018-12-18 20:06:49.636Z] === Task Finished ===
[taskcluster 2018-12-18 20:06:49.636Z] Unsuccessful task run with exit code: 1 completed in 634.445 second

Comment 11

[Cristina Coroiu [:ccoroiu]]

This is failing too: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=217766906&repo=mozilla-inbound&lineNumber=2108
task 2018-12-18T21:05:28.575Z] 21:05:28     INFO - TEST-START | browser/components/payments/test/mochitest/test_payment_dialog_required_top_level_items.html
[task 2018-12-18T21:05:29.220Z] 21:05:29     INFO - TEST-INFO | started process screentopng
[task 2018-12-18T21:05:30.097Z] 21:05:30     INFO - TEST-INFO | screentopng: exit 0
[task 2018-12-18T21:05:30.097Z] 21:05:30     INFO - Buffered messages logged at 21:05:29
[task 2018-12-18T21:05:30.098Z] 21:05:30     INFO - AddTask.js | Entering test setupOnce
[task 2018-12-18T21:05:30.098Z] 21:05:30     INFO - must wait for focus
[task 2018-12-18T21:05:30.098Z] 21:05:30     INFO - TEST-PASS | browser/components/payments/test/mochitest/test_payment_dialog_required_top_level_items.html | Check some templates found 
[task 2018-12-18T21:05:30.098Z] 21:05:30     INFO - AddTask.js | Leaving test setupOnce
[task 2018-12-18T21:05:30.099Z] 21:05:30     INFO - AddTask.js | Entering test runTests
[task 2018-12-18T21:05:30.099Z] 21:05:30     INFO - Starting testcase shippingAndPayerRequired
[task 2018-12-18T21:05:30.099Z] 21:05:30     INFO - Buffered messages finished
[task 2018-12-18T21:05:30.102Z] 21:05:30     INFO - TEST-UNEXPECTED-FAIL | browser/components/payments/test/mochitest/test_payment_dialog_required_top_level_items.html | uncaught exception - ReferenceError: onInputOrChange is not defined at onInput@http://mochi.test:8888/tests/browser/components/payments/res/containers/payment-method-picker.js:121:5
[task 2018-12-18T21:05:30.103Z] 21:05:30     INFO - handleEvent@http://mochi.test:8888/tests/browser/components/payments/res/mixins/HandleEventMixin.js:20:24
[task 2018-12-18T21:05:30.104Z] 21:05:30     INFO - synthesizeKey@http://mochi.test:8888/tests/SimpleTest/EventUtils.js:869:7
[task 2018-12-18T21:05:30.105Z] 21:05:30     INFO - sendChar@http://mochi.test:8888/tests/SimpleTest/EventUtils.js:305:3
[task 2018-12-18T21:05:30.106Z] 21:05:30     INFO - sendString@http://mochi.test:8888/tests/SimpleTest/EventUtils.js:316:5
[task 2018-12-18T21:05:30.107Z] 21:05:30     INFO - setup@http://mochi.test:8888/tests/browser/components/payments/test/mochitest/test_payment_dialog_required_top_level_items.html:122:3
[task 2018-12-18T21:05:30.114Z] 21:05:30     INFO - async*runTests@http://mochi.test:8888/tests/browser/components/payments/test/mochitest/test_payment_dialog_required_top_level_items.html:181:11
[task 2018-12-18T21:05:30.121Z] 21:05:30     INFO - async*nextTick/<@http://mochi.test:8888/tests/SimpleTest/AddTask.js:70:34
[task 2018-12-18T21:05:30.122Z] 21:05:30     INFO - async*nextTick@http://mochi.test:8888/tests/SimpleTest/AddTask.js:44:10
[task 2018-12-18T21:05:30.124Z] 21:05:30     INFO - setTimeout handler*SimpleTest_setTimeoutShim@http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:684:12
[task 2018-12-18T21:05:30.124Z] 21:05:30     INFO - nextTick@http://mochi.test:8888/tests/SimpleTest/AddTask.js:40:11
[task 2018-12-18T21:05:30.125Z] 21:05:30     INFO - setTimeout handler*SimpleTest_setTimeoutShim@http://mochi.test:8888/tests/SimpleTest/SimpleTest.js:684:12
[task 2018-12-18T21:05:30.125Z] 21:05:30     INFO - add_task@http://mochi.test:8888/tests/SimpleTest/AddTask.js:30:7
[task 2018-12-18T21:05:30.125Z] 21:05:30     INFO - @http://mochi.test:8888/tests/browser/components/payments/test/mochitest/test_payment_dialog_required_top_level_items.html:40:1
[task 2018-12-18T21:05:30.125Z] 21:05:30     INFO - 
[task 2018-12-18T21:05:30.125Z] 21:05:30     INFO -     simpletestOnerror@SimpleTest/SimpleTest.js:1644:13
[task 2018-12-18T21:05:30.126Z] 21:05:30     INFO -     synthesizeKey@SimpleTest/EventUtils.js:869:7
[task 2018-12-18T21:05:30.132Z] 21:05:30     INFO -     sendChar@SimpleTest/EventUtils.js:305:3
[task 2018-12-18T21:05:30.133Z] 21:05:30     INFO -     sendString@SimpleTest/EventUtils.js:316:5
[task 2018-12-18T21:05:30.134Z] 21:05:30     INFO -     setup@browser/components/payments/test/mochitest/test_payment_dialog_required_top_level_items.html:122:3
[task 2018-12-18T21:05:30.136Z] 21:05:30     INFO -     async*runTests@browser/components/payments/test/mochitest/test_payment_dialog_required_top_level_items.html:181:11
[task 2018-12-18T21:05:30.137Z] 21:05:30     INFO -     async*nextTick/<@SimpleTest/AddTask.js:70:34
[task 2018-12-18T21:05:30.141Z] 21:05:30     INFO -     async*nextTick@SimpleTest/AddTask.js:44:10
[task 2018-12-18T21:05:30.142Z] 21:05:30     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:684:12
[task 2018-12-18T21:05:30.144Z] 21:05:30     INFO -     nextTick@SimpleTest/AddTask.js:40:11
[task 2018-12-18T21:05:30.144Z] 21:05:30     INFO -     setTimeout handler*SimpleTest_setTimeoutShim@SimpleTest/SimpleTest.js:684:12
[task 2018-12-18T21:05:30.164Z] 21:05:30     INFO -     add_task@SimpleTest/AddTask.js:30:7
[task 2018-12-18T21:05:30.164Z] 21:05:30     INFO -     @browser/components/payments/test/mochitest/test_payment_dialog_required_top_level_items.html:40:1

Comment 12

[Diego Pino]

Attachment: Bug-1446158-Create-a-handleEvent-custom-element-mixi.patch

Fixed several lint errors.

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 9032373 [details] [diff] [review]
Bug-1446158-Create-a-handleEvent-custom-element-mixi.patch

Review of attachment 9032373 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the fix below. Please run the linting and tests locally.

::: browser/components/payments/res/mixins/HandleEventMixin.js
@@ +13,5 @@
> +    handleEvent(evt) {
> +      function capitalize(str) {
> +        return str.charAt(0).toUpperCase() + str.slice(1);
> +      }
> +      if (super.HandleEvent) {

You have a capital "H" here which should be lowercase

Comment 14

[Diego Pino]

Attachment: Bug-1446158-Create-a-handleEvent-custom-element-mixi.patch

Addressed comment from last review.

Comment 15

[Diego Pino]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa5618b5f85a482ed65c7e70fb9802ff7f6497d9

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/40ffbae188ba
Create a handleEvent custom element mixin to forward events from handleEvent to on* methods. r=MattN

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/40ffbae188ba