Closed Bug 1415346 Opened 7 years ago Closed 7 years ago

Bind on event handlers before creating EventWatcher in file_event-dispatch.html

Categories

(Core :: DOM: Animation, defect)

Product:
Core
Component:
DOM: Animation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(6 files)

Bug 1415346 - Use arrow function in file_event-dispatch.html.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1415346 - Use single quote in script in file_event-dispatch.html.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1415346 - Remove unused AnimationEventHandler.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1415346 - Use const instead of var in file_event-dispatch.html.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Bug 1415346 - Bind onxx event handler before creating EventWatcher.
59 bytes, text/x-review-board-request
birtles
: review+
Details
Use { record: 'all' } to catch all events
12.06 KB, text/plain
Details 
To fix bug 1413817, I initially planed to update dom/imptests/testharness.js with the latest one (bug 1415007) to use { record: 'all' } for catching all events in the tests.  :jgraham thinks updating our own testharness.js is not a good idea.  I can agree it basically..

We should eventually move test_event-dispatch.html to web platform tests, but I am hesitating it for now since they use web animations APIs.

So I've decided to take the workaround what we did in bug 1415010, i.e. binding onxx event handlers before creating EventWatcher.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=59b059faf1cc3fab3be8c86442213bc4d719a431


For reference what I and :jgraham talked on IRC.

6:22 AM <hiro> Does anyone know how to use testharness.js in testing/web-platform from our tree (e.g. dom/animation/test)?
6:36 AM <jgraham> hiro: You can't, but why do you want the tests in dom/animation/test? Is it something we can share with other vendors?
6:37 AM <hiro> jgraham: The test cases what we need in dom/animation is checking something that for internal APIs for devtools or some such.
7:41 AM <jgraham> hiro: Oh, internal tests are hard. testing/web-platform/mozilla exists
7:41 AM <jgraham> I would rather not rely on things in imptest if we can avoid it. Maybe things already are
7:42 AM <hiro> jgraham: yep I agree, we should write web-platform test as possible. 
7:45 AM <hiro> jgraham: anyway, I've decided to take a workaround for that bug  for now.  Thanks!
Comment on attachment 8926130 [details]
Bug 1415346 - Bind onxx event handler before creating EventWatcher.

https://reviewboard.mozilla.org/r/197366/#review202542

::: dom/animation/test/css-animations/file_event-dispatch.html:53
(Diff revision 1)
>                                             'animationiteration',
>                                             'animationend',
>                                             'animationcancel' ]);
>    const animation = div.getAnimations()[0];
>  
> -  return [animation, watcher, div];
> +  return [animation, watcher, div, handler];

Honestely I am not a big fan of this kind of returning various variables in an array..
FWIW, here is the patch to use { record: 'all' } there.
Comment on attachment 8926126 [details]
Bug 1415346 - Use arrow function in file_event-dispatch.html.

https://reviewboard.mozilla.org/r/197358/#review202566
Attachment #8926126 - Flags: review?(bbirtles) → review+
Comment on attachment 8926127 [details]
Bug 1415346 - Use single quote in script in file_event-dispatch.html.

https://reviewboard.mozilla.org/r/197360/#review202650
Attachment #8926127 - Flags: review?(bbirtles) → review+
Comment on attachment 8926128 [details]
Bug 1415346 - Remove unused AnimationEventHandler.

https://reviewboard.mozilla.org/r/197362/#review202658
Attachment #8926128 - Flags: review?(bbirtles) → review+
Comment on attachment 8926129 [details]
Bug 1415346 - Use const instead of var in file_event-dispatch.html.

https://reviewboard.mozilla.org/r/197364/#review202660
Attachment #8926129 - Flags: review?(bbirtles) → review+
Comment on attachment 8926130 [details]
Bug 1415346 - Bind onxx event handler before creating EventWatcher.

https://reviewboard.mozilla.org/r/197366/#review202662

::: dom/animation/test/css-animations/file_event-dispatch.html:46
(Diff revision 1)
>    this.animationcancel    = undefined;
>  }
>  
>  function setupAnimation(t, animationStyle) {
>    const div = addDiv(t, { style: 'animation: ' + animationStyle });
> +  const handler = new AnimationEventHandler(div);

Should we add a comment here describing the ordering dependency?

::: dom/animation/test/css-animations/file_event-dispatch.html:53
(Diff revision 1)
>                                             'animationiteration',
>                                             'animationend',
>                                             'animationcancel' ]);
>    const animation = div.getAnimations()[0];
>  
> -  return [animation, watcher, div];
> +  return [animation, watcher, div, handler];

Yeah, perhaps we could use object destructuring:

  https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Destructuring_assignment#Object_destructuring

That would be better for this number of arguments and especially when we are not using all of them.
Attachment #8926130 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #12)
> Comment on attachment 8926130 [details]
> Bug 1415346 - Bind onxx event handler before creating EventWatcher.
> 
> https://reviewboard.mozilla.org/r/197366/#review202662
> 
> ::: dom/animation/test/css-animations/file_event-dispatch.html:46
> (Diff revision 1)
> >    this.animationcancel    = undefined;
> >  }
> >  
> >  function setupAnimation(t, animationStyle) {
> >    const div = addDiv(t, { style: 'animation: ' + animationStyle });
> > +  const handler = new AnimationEventHandler(div);
> 
> Should we add a comment here describing the ordering dependency?

Indeed, absolutely we should!

> ::: dom/animation/test/css-animations/file_event-dispatch.html:53
> (Diff revision 1)
> >                                             'animationiteration',
> >                                             'animationend',
> >                                             'animationcancel' ]);
> >    const animation = div.getAnimations()[0];
> >  
> > -  return [animation, watcher, div];
> > +  return [animation, watcher, div, handler];
> 
> Yeah, perhaps we could use object destructuring:
> 
>  
> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/
> Destructuring_assignment#Object_destructuring
> 
> That would be better for this number of arguments and especially when we are
> not using all of them.

Thanks, I will try to use it, if it will be a big change, I will drop it in this bug.
Pushed by hikezoe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d3ff2908ecb0
Use arrow function in file_event-dispatch.html. r=birtles
https://hg.mozilla.org/integration/autoland/rev/8809ffe82076
Use single quote in script in file_event-dispatch.html. r=birtles
https://hg.mozilla.org/integration/autoland/rev/dfb4033f9695
Remove unused AnimationEventHandler. r=birtles
https://hg.mozilla.org/integration/autoland/rev/8e875e2383ee
Use const instead of var in file_event-dispatch.html. r=birtles
https://hg.mozilla.org/integration/autoland/rev/587320f427ab
Bind onxx event handler before creating EventWatcher. r=birtles
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: