Add Event constructor polyfill #10401

jongrim · 2017-07-13T14:26:55Z

Fixes Polyfill Event constructor #9597.
Implements a polyfill for the Event constructor for older browsers (IE 9+).
Replaces usages of document.createEvent with new Event.

There is one location where I did not replace the usage of document.createEvent. It's in test/functional/test-event-helper.js, line 236. The test is creating a custom function to assign to document.createEvent so I've left it alone.

googlebot · 2017-07-13T14:27:04Z

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If your company signed a CLA, they designated a Point of Contact who decides which employees are authorized to participate. You may need to contact the Point of Contact for your company and ask to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again.

jongrim · 2017-07-13T14:29:11Z

I signed it!

googlebot · 2017-07-13T14:29:16Z

CLAs look good, thanks!

dreamofabear

Looks good overall! 👍

There's a couple of lint errors on the Travis build. Can you fix those too please?

dreamofabear · 2017-07-13T18:23:44Z

src/polyfills.js

 import {install as installDocContains} from './polyfills/document-contains';
 import {install as installMathSign} from './polyfills/math-sign';
 import {install as installObjectAssign} from './polyfills/object-assign';
 import {install as installPromise} from './polyfills/promise';
 import {install as installArrayIncludes} from './polyfills/array-includes';
 import {getMode} from './mode';
+import {install as installEvent} from './polyfills/event';


Minor: Please alphabetize.

dreamofabear · 2017-07-13T18:24:44Z

src/polyfills/event.js

+  // win.Event is a function on Edge, Chrome, FF, Safari but
+  // is an object on IE 11.
+  if (typeof win.Event === 'function') {
+    return false;


return; since this function doesn't have a return value in other cases.

dreamofabear · 2017-07-13T18:27:17Z

src/polyfills/event.js

+      params.bubbles,
+      params.cancelable,
+      params.detail
+    );


Event.initEvent doesn't take a detail param (CustomEvent does). https://developer.mozilla.org/en-US/docs/Web/API/Event/initEvent

dreamofabear · 2017-07-13T18:29:37Z

src/polyfills/event.js

+    return false;
+  }
+
+  function Event(event, params) {


Minor: Let's use name instead of event here to be clearer.

- Alphabetizes imports in polyfills.js - Corrects spacing and line lengths per linting rules - Corrects return in Event polyfill - Renames event type parameter - Corrects Event initialization and removes details parameter

dreamofabear · 2017-07-17T23:27:01Z

src/polyfills/event.js

+    return;
+  }
+
+  function Event(name, params) {


The current Travis error is a Closure compiler type error that seems to dislike the fact that this isn't a new-only constructor function (i.e. ES6 class). I'd try fixing it by changing this to be an ES6 class. If that doesn't work, type-cast. 😄

You can re-run the type checker locally by running gulp check-types.

dreamofabear

Final stretch! 💪

Last thing I'd ask is to add a unit test to the test/functional folder. Check out the other polyfill tests for an example, e.g. test-polyfill-array-includes.js.

dreamofabear · 2017-07-18T20:44:21Z

src/polyfills/event.js

+
+/**
+ *@constructor
+  */


Nice, didn't know that @constructor would solve it. 👍

Minor issue:

/** * @constructor */

Actually, this isn't what made it pass the type-check. It was not assigning directly to win.Event and instead using Object.defineProperty.

I included this because I read in the docs that @constructor should be used if a function will be a constructor. I don't really understand the compiler well enough to know if this makes a difference though.

I will correct the spacing, but I could also completely remove this if it's not helpful.

dreamofabear · 2017-07-18T20:47:35Z

test/functional/test-polyfill-event.js

+
+  beforeEach(() => {
+    env.win.Event = Object.create(self.Event.prototype);
+    env.win.Event.prototype = self.Event.prototype;


Are these two lines necessary?

Yes, without this the beforeEach hook fails due to a TypeError originating from event.js. It tries to assign window.Event.prototype as the prototype of the newly created function, but throws an error because window.Event is undefined (recall that the prototype assignment happens before we assign the new function to window.Event). I believe these two lines effectively mimic IE 9+ as well, i.e. window.Event is an object, and not function.

Gotcha, can you add a comment describing this please?

dreamofabear · 2017-07-18T20:49:06Z

test/functional/test-polyfill-event.js

+  });
+
+  it('should require a type argument', () => {
+    expect(() => new Event()).to.throw(TypeError);


We should make sure we're instantiating env.win's Event rather than the test server's Event.

new env.win.Event here and elsewhere.

Doh, that makes sense. Done!

jongrim · 2017-07-18T22:36:58Z

@choumx How do I identify which tests are failing with Travis CI? There are those 5 Video Player tests that consistently fail locally, so I'm assuming they are included, but the others are variable locally. Sometimes they pass and sometimes they fail.

dreamofabear · 2017-07-18T22:44:22Z

Good question. 😅 Looks like a recent build change we've made -- looking into it.

rsimha · 2017-07-18T23:34:29Z

@jongrim, that was my fault. A fix is imminent with #10516. Stay tuned.

Edit: Fix is merged. You should now see a summary of failed tests at the end of the run. You might need to sync your branch to upstream master to see the changes.

dreamofabear · 2017-07-19T16:43:21Z

@jongrim I reran the Travis build and you can see the test failures now.

The video tests are failing because they use a fake Window object where the polyfill isn't installed. You can fix it by adding this.Event = window.Event to the FakeWindow constructor in fake-dom.js.

jongrim · 2017-07-19T20:03:47Z

🎉
Thanks for the guidance @choumx!

dreamofabear

Looking great! Last few changes before merge. 🎉

dreamofabear · 2017-07-22T01:04:00Z

extensions/amp-live-list/0.1/amp-live-list.js

@@ -866,8 +866,10 @@ export class AmpLiveList extends AMP.BaseElement {
  }

  sendAmpDomUpdateEvent_() {
-    const event = this.win.document.createEvent('Event');
-    event.initEvent(AmpEvents.DOM_UPDATE, true, true);
+    const event = new this.win.Event(


const event = new Event(

There are situations where we embed an AMP doc inside another AMP doc, wrapped in an iframe. In those cases, the polyfill won't be installed in the iframe's window so we should make sure we construct events using the top window.

dreamofabear · 2017-07-22T01:04:15Z

src/custom-element.js

@@ -1111,9 +1111,8 @@ function createBaseCustomElementClass(win) {
      const data = opt_data || {};
      // Constructors of events need to come from the correct window. Sigh.
      const win = this.ownerDocument.defaultView;
-      const event = win.document.createEvent('Event');
+      const event = new win.Event(name, {bubbles: true, cancelable: true});


dreamofabear · 2017-07-22T01:04:49Z

testing/fake-dom.js

@@ -64,6 +64,8 @@ export class FakeWindow {
    this.DOMTokenList = window.DOMTokenList;
    /** @const */
    this.Math = window.Math;
+    /**@const */


Nitpick: Space padding. 😅

/** @const */

dreamofabear · 2017-07-22T01:05:09Z

test/functional/test-polyfill-event.js

+
+  beforeEach(() => {
+    env.win.Event = Object.create(self.Event.prototype);
+    env.win.Event.prototype = self.Event.prototype;


Gotcha, can you add a comment describing this please?

dreamofabear · 2017-07-22T01:05:59Z

test/functional/test-polyfill-event.js

+    const ev = new env.win.Event('event');
+    const elm = document.createElement('p');
+    expect(() => elm.dispatchEvent(ev)).to.not.throw();
+  });


This test case seems to be covered by the next one so I think it can be removed.

dreamofabear · 2017-07-22T01:07:15Z

test/functional/test-polyfill-event.js

+  it('should be configurable', () => {
+    const ev = new env.win.Event('event', {bubbles: true, cancelable: true});
+    expect(ev.bubbles).to.be.true;
+    expect(ev.cancelable).to.be.true;


Would you mind adding another case to this test? This protects against the case where bubbles/cancelable are always true.

const defaultEvent = new env.win.Event('event'); expect(defaultEvent.bubbles).to.be.false; expect(defaultEvent.cancelable).to.be.false;

dreamofabear · 2017-07-24T23:58:44Z

Thanks for contributing to AMP @jongrim! 🎉

I hope you'll continue and become a regular contributor here. 😃 We'll be working on writing great second (and third) issues soon, so stay tuned!

In the meantime, please reach out to me @choumx or Joey @mrjoro on Slack.

jongrim · 2017-07-25T14:08:05Z

Awesome! I'll be on the lookout for more issues. Thanks for the help and making the process super easy and welcoming 👏

This reverts commit 28f709f.

jongrim added 9 commits July 12, 2017 16:25

Add Event polyfill

b13f4fa

Swap document.Create with Event constructor

dd801be

Remove global references for window and document

61e9b46

Correct comment

c8c7238

Replace usages of document.createEvent

3fa3472

correct linting error

55138c6

Replace document.createEvent w/ Event constructor

8b3cd17

Replace document.createEvent in test-amp-access

daa07d6

Remove document.createEvent fallback in bind-impl.js

1eaee49

googlebot added the cla: no label Jul 13, 2017

googlebot added cla: yes and removed cla: no labels Jul 13, 2017

jongrim mentioned this pull request Jul 13, 2017

Polyfill Event constructor #9597

Closed

12 tasks

dreamofabear self-requested a review July 13, 2017 18:19

dreamofabear reviewed Jul 13, 2017

View reviewed changes

jongrim added 2 commits July 13, 2017 16:28

Implement corrections per code review comments

ae21c92

- Alphabetizes imports in polyfills.js - Corrects spacing and line lengths per linting rules - Corrects return in Event polyfill - Renames event type parameter - Corrects Event initialization and removes details parameter

Add event.js to whitelist for polyfills dep-check

e48a11d

dreamofabear reviewed Jul 17, 2017

View reviewed changes

Add event polyfill test and correct closure compiler error

aa3c2b6

dreamofabear reviewed Jul 18, 2017

View reviewed changes

Correct tests and add type argument check

c84f541

Add Event constructor to fake-dom

c65d474

dreamofabear reviewed Jul 22, 2017

View reviewed changes

jongrim added 3 commits July 22, 2017 18:11

Construct events using proper window

f8c39a9

Add test for default event configuration

05d6a70

Correct spacing

7afa7ba

dreamofabear approved these changes Jul 24, 2017

View reviewed changes

dreamofabear merged commit 28f709f into ampproject:master Jul 24, 2017

zhouyx mentioned this pull request Jul 25, 2017

TypeError: Cannot read property 'document' of undefined #9870

Closed

dvoytenko mentioned this pull request Jul 29, 2017

Illegal constructor errors on new Event #10709

Closed

dreamofabear pushed a commit to dreamofabear/amphtml that referenced this pull request Jul 29, 2017

Revert "Add Event constructor polyfill (ampproject#10401)"

383f871

This reverts commit 28f709f.

dreamofabear pushed a commit that referenced this pull request Jul 30, 2017

Revert "Add Event constructor polyfill (#10401)" (#10711)

1cc2b8b

This reverts commit 28f709f.

dreamofabear pushed a commit that referenced this pull request Jul 30, 2017

Revert "Add Event constructor polyfill (#10401)" (#10711)

47697cf

This reverts commit 28f709f.

ampprojectbot added PR use: In Beta / Experimental PR use: In Stable labels Aug 2, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Event constructor polyfill #10401

Add Event constructor polyfill #10401

jongrim commented Jul 13, 2017

googlebot commented Jul 13, 2017

jongrim commented Jul 13, 2017

googlebot commented Jul 13, 2017

dreamofabear left a comment

dreamofabear Jul 13, 2017

dreamofabear Jul 13, 2017

dreamofabear Jul 13, 2017

dreamofabear Jul 13, 2017

dreamofabear Jul 17, 2017

dreamofabear left a comment

dreamofabear Jul 18, 2017

jongrim Jul 18, 2017

dreamofabear Jul 18, 2017

jongrim Jul 18, 2017

dreamofabear Jul 22, 2017

dreamofabear Jul 18, 2017

jongrim Jul 18, 2017

jongrim commented Jul 18, 2017

dreamofabear commented Jul 18, 2017

rsimha commented Jul 18, 2017 •

edited

dreamofabear commented Jul 19, 2017

jongrim commented Jul 19, 2017

dreamofabear left a comment

dreamofabear Jul 22, 2017

dreamofabear Jul 22, 2017

dreamofabear Jul 22, 2017

dreamofabear Jul 22, 2017

dreamofabear Jul 22, 2017

dreamofabear Jul 22, 2017

dreamofabear commented Jul 24, 2017

jongrim commented Jul 25, 2017

Add Event constructor polyfill #10401

Add Event constructor polyfill #10401

Conversation

jongrim commented Jul 13, 2017

googlebot commented Jul 13, 2017

jongrim commented Jul 13, 2017

googlebot commented Jul 13, 2017

dreamofabear 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

dreamofabear 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

jongrim commented Jul 18, 2017

dreamofabear commented Jul 18, 2017

rsimha commented Jul 18, 2017 • edited

dreamofabear commented Jul 19, 2017

jongrim commented Jul 19, 2017

dreamofabear 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

dreamofabear commented Jul 24, 2017

jongrim commented Jul 25, 2017

rsimha commented Jul 18, 2017 •

edited