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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: hiro, Assigned: hiro)

References

Details

Attachments

(6 files)

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+
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

Created:
Updated:
Size: