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)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: hiro, Assigned: hiro)
References
Details
Attachments
(6 files)
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
text/x-review-board-request
|
birtles
:
review+
|
Details |
(deleted),
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 hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review |
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..
Assignee | ||
Comment 7•7 years ago
|
||
FWIW, here is the patch to use { record: 'all' } there.
Comment 8•7 years ago
|
||
mozreview-review |
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 9•7 years ago
|
||
mozreview-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 10•7 years ago
|
||
mozreview-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 11•7 years ago
|
||
mozreview-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 12•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 13•7 years ago
|
||
(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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 19•7 years ago
|
||
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
Comment 20•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d3ff2908ecb0
https://hg.mozilla.org/mozilla-central/rev/8809ffe82076
https://hg.mozilla.org/mozilla-central/rev/dfb4033f9695
https://hg.mozilla.org/mozilla-central/rev/8e875e2383ee
https://hg.mozilla.org/mozilla-central/rev/587320f427ab
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in
before you can comment on or make changes to this bug.
Description
•