Closed
Bug 1244637
Opened 8 years ago
Closed 8 years ago
implement AnimationEffectTiming fill
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla48
Tracking | Status | |
---|---|---|
firefox48 | --- | fixed |
People
(Reporter: motozawa, Assigned: hATrayflood)
References
()
Details
Attachments
(1 file)
No description provided.
Updated•8 years ago
|
Assignee: motozawa → daisuke
Comment 1•8 years ago
|
||
Regarding the error-handling behavior here, I discussed this with Cameron on IRC:
> 15:10 <birtles> bz, heycam: is there somewhere in WebIDL that says attributes setters with enum types don't throw?
> 15:11 <birtles> I think we implemented that in bug 765464 but I'm not sure where that behavior comes from
> 15:11 <firebot> https://bugzil.la/765464 — FIXED, Ms2ger@gmail.com — Throw TypeError for binding exceptions
> 15:11 <birtles> specifically, here: https://dxr.mozilla.org/mozilla-central/source/dom/bindings/Codegen.py#7302
> 15:11 <heycam> birtles: step 4 of http://heycam.github.io/webidl/#dfn-attribute-setter
> 15:12 <birtles> heycam: thanks! do you know why that is?
> 15:12 <birtles> was there some compat reason these things couldn't throw?
> 15:13 <heycam> birtles: for consistency with enum-typed reflecting things in HTML, I think
> 15:14 <birtles> heycam: ok, thanks. We came across this when we went to implement the 'fill' setter which takes FillMode
> 15:14 <birtles> anim.effect.timing.fill = "yer" doesn't throw, where as elem.animate(..., { fill: "yer" }) does
> 15:15 <birtles> since the latter one is being processed as a dictionary where that behavior doesn't apply
> 15:15 <birtles> I'm not sure which one we should match
> 15:17 <heycam> birtles: yeah, at this point I'm not convinced it was the right decision. interesting question about whether non-required dictionary members should have similar behaviour.
> 15:18 <birtles> heycam: I guess we could just add extra steps to the setter that introduce the more strict behaviour
> 15:19 <heycam> birtles: while it's a bit odd to be inconsistent with the animate() method, I'm not sure it would be good to make the attribute setter inconsistent with other ones
> 15:20 <birtles> heycam: oh, ok. I don't suppose we can easily change the dictionary processing to make it not throw...
> 15:21 <heycam> birtles: well you could make it DOMString and handle it yourself, but again I'm not sure trying to fix the consistency in this one case is better than introducing an inconsistency with other enum-dictionary-member-taking methods
> 15:21 <birtles> heycam: yeah, true
> 15:21 <birtles> oh well, maybe we just leave it as is
> 15:21 <birtles> heycam: thanks!
Assignee | ||
Comment 4•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/45357/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/45357/
Attachment #8739740 -
Flags: review?(hiikezoe)
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Daisuke Akatsuka (:daisuke) from comment #3) > Hello Abe-san! > How is this bug?? very sorry to late. I pushed https://reviewboard.mozilla.org/r/45357/ to ikezoe-san for review.
Flags: needinfo?(h.rayflood)
Comment 6•8 years ago
|
||
Comment on attachment 8739740 [details] MozReview Request: Bug 1244637 - implement AnimationEffectTiming fill. r=hiro https://reviewboard.mozilla.org/r/45357/#review41891 Thank you, Abe san! I think we also need tests for MutationObserver and tests checking styles after changing fill attribute. Please attach (a) new patch(es) here if you take time for it or open a new bug for it. r=me with that and below nit fixes. ::: testing/web-platform/tests/web-animations/animation-effect-timing/fill.html:4 (Diff revision 1) > +<!DOCTYPE html> > +<meta charset=utf-8> > +<title>delay tests</title> > +<link rel="help" href="https://w3c.github.io/web-animations/#dom-animationeffecttiming-delay"> #dom-animationeffecttiming-fill ::: testing/web-platform/tests/web-animations/animation-effect-timing/fill.html:5 (Diff revision 1) > +<!DOCTYPE html> > +<meta charset=utf-8> > +<title>delay tests</title> > +<link rel="help" href="https://w3c.github.io/web-animations/#dom-animationeffecttiming-delay"> > +<link rel="author" title="ABE Hiroki (hATrayflood)" href="mailto:h.rayflood@gmail.com"> Please drop this author line.
Attachment #8739740 -
Flags: review?(hiikezoe) → review+
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8739740 [details] MozReview Request: Bug 1244637 - implement AnimationEffectTiming fill. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45357/diff/1-2/
Attachment #8739740 -
Attachment description: MozReview Request: Bug 1244637 - implement AnimationEffectTiming fill r?hiro → MozReview Request: Bug 1244637 - implement AnimationEffectTiming fill r=hiro
Assignee | ||
Comment 8•8 years ago
|
||
thank you for your review, ikezoe-san. I fixed a patch and pushed https://reviewboard.mozilla.org/r/45357/
Comment 9•8 years ago
|
||
I was really wondering why several mochitest failed on Android. https://treeherder.mozilla.org/#/jobs?repo=try&revision=49e4a5ddf077 No period before "r=" in the commit message seems to cause the failure. A try with the period: https://treeherder.mozilla.org/#/jobs?repo=try&revision=48ef0eed740c Please insert a period there and change the title in fill.html to "fill tests". Thank you,
Assignee | ||
Comment 10•8 years ago
|
||
Comment on attachment 8739740 [details] MozReview Request: Bug 1244637 - implement AnimationEffectTiming fill. r=hiro Review request updated; see interdiff: https://reviewboard.mozilla.org/r/45357/diff/2-3/
Attachment #8739740 -
Attachment description: MozReview Request: Bug 1244637 - implement AnimationEffectTiming fill r=hiro → MozReview Request: Bug 1244637 - implement AnimationEffectTiming fill. r=hiro
Assignee | ||
Comment 11•8 years ago
|
||
thanks. I pushed https://reviewboard.mozilla.org/r/45357/diff/2-3/ for android try-server tests.
Comment 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f50694e2aa7c
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox48:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in
before you can comment on or make changes to this bug.
Description
•