Closed Bug 1031319 Opened 6 years ago Closed 6 years ago

Checkbox keeps animating (animation events dispatch for animation-name: none)

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla33
Tracking Status
firefox33 - affected

People

(Reporter: Fanolian+BMO, Assigned: birtles)

References

()

Details

(Keywords: regression)

Attachments

(4 files)

User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 (Beta/Release)
Build ID: 20140626030201

Steps to reproduce:

Visit http://www.polymer-project.org/components/paper-elements/demo.html#paper-checkbox


Actual results:

The check marks keep animating. If you uncheck them the boxes keeps animating too. See attached video for reference.


Expected results:

Animation performs only once per click.
It runs fine in Firefox30, IE11 and Chrome35.


From mozregression:
Last good revision: 9e8e3e903484 (2014-06-12)
First bad revision: 48eee276b1ee (2014-06-13)
Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9e8e3e903484&tochange=48eee276b1ee


No more inbounds to bisect
Last good revision: 18c21532848a
First bad revision: 3b9ed7396f9a
Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=18c21532848a&tochange=3b9ed7396f9a
Regression window(m-c)
Good:
https://hg.mozilla.org/mozilla-central/rev/9e8e3e903484
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140611184734
Bad:
https://hg.mozilla.org/mozilla-central/rev/1d9513f22934
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140612065134
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=9e8e3e903484&tochange=1d9513f22934


Regression window(m-i)
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ee90f5b0aac
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140611203732
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3b9ed7396f9a
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:33.0) Gecko/20100101 Firefox/33.0 ID:20140611211929
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=7ee90f5b0aac&tochange=3b9ed7396f9a


In local build
Last Good: 441a1c280cd8
First Bad: 3b9ed7396f9a

Regressed by:
3b9ed7396f9a	Brian Birtles — Bug 1004377 - Dispatch events for CSS Animations with empty keyframes rules; r=dholbert This patch removes the check that skipped queueing events for animations without keyframes since the spec indicates such animations should dispatch events. There is a further correctness fix here for the case where a keyframes rule is modified using the CSSOM so that it becomes empty. Previously when we came to create the new animation rules we would end up setting ElementAnimations::mNeedRefreshes to false since we check if the keyframes rule is empty and if it is we would skip all further processing (including setting mNeedsRefreshes). That means that: (a) We may end up unregistering from the refresh observer so we would never dispatch the end event for such an animation. (b) If the animation was running on the compositor we may never remove it from the compositor or may not do it in a timely fashion. To fix both these problems, this patch removes the check for an empty keyframes rule so that mNeedsRefreshes is set in this case.
Blocks: 1004377
Status: UNCONFIRMED → NEW
Component: General → CSS Parsing and Computation
Ever confirmed: true
Flags: needinfo?(birtles)
Comment on attachment 8447211 [details]
testcase 1 (partially reduced) (Need to allow mixed content, w/ shield in URLbar)

(The testcase loads content from the non-HTTPS site http://www.polymer-project.org/components/paper-checkbox/paper-checkbox.html , so it needs mixed content enabled, via the shield that appears in the URLbar when you load it.)
Attachment #8447211 - Attachment description: testcase 1 (partially reduced) → testcase 1 (partially reduced) (Need to allow mixed content, w/ shield in URLbar)
Thanks for reducing this Daniel!

The issue seems to be that the default style creates an animation with a single element with an empty name and all the properties set to their default values:

  http://hg.mozilla.org/mozilla-central/file/c3387c5ddba9/layout/style/nsStyleStruct.cpp#l2414

Somewhere in the polymer logic it seems that when we end an animation we end up re-applying this default style temporarily. (Stepping through with a debugger I see us set exactly this animation but I haven't worked out exactly what point in the polymer logic triggers this.)

Previously we would just see the animation has no properties so we'd ignore it. Now, however, we dispatch events for animations with empty keyframe rules so we trigger an animation-end event for this empty 0s animation. That triggers the following logic:

  cl.toggle('checkmark', this.checked && !cl.contains('checkmark'));
  cl.toggle('box', !this.checked && !cl.contains('box'));

Which basically has us re-applying the animation we just finished but since we applied the default style in the meantime we end up running the animation from the beginning again.

I can reproduce this with the following code:

  var div = document.createElement("div");
  div.addEventListener("animationend", function() {
    console.log("ended");
    div.style.animation = ""; // "none" here has the same effect
  });
  document.body.appendChild(div);
  div.style.animation = "0.1s anim";

This code will generate *two* "ended" messages.

I think the solution is to look for StyleAnimations with empty names in BuildAnimation and not generate a corresponding ElementAnimation.
Assignee: nobody → birtles
Status: NEW → ASSIGNED
Flags: needinfo?(birtles)
This patch causes animations whose corresponding animation-name is "none" to be
dropped from the list of generated ElementAnimation objects. This means we avoid
generating events for these animations.
Attachment #8447837 - Flags: review?(dbaron)
Adding tests as a separate patch so we can push just the fix to Aurora
Attachment #8447839 - Flags: review?(dbaron)
(In reply to Brian Birtles (:birtles) from comment #5)
> The issue seems to be that the default style creates an animation with a
> single element with an empty name and all the properties set to their
> default values:
> 
>  
> http://hg.mozilla.org/mozilla-central/file/c3387c5ddba9/layout/style/
> nsStyleStruct.cpp#l2414

For my own reference, the reason we don't have problems in the common case where an element has no specified animation style and picks up the initial values is because we have an explicit check for this here:

  http://hg.mozilla.org/mozilla-central/file/b6408c32a170/layout/style/nsAnimationManager.cpp#l237

For the test case in this patch, however, we revert to the default style after having an animation hence the first part of the test, that collection==null, fails and we continue processing the default style.
Comment on attachment 8447837 [details] [diff] [review]
part 1 - Don't generate element animations when animation-name is "none"

r=dbaron
Attachment #8447837 - Flags: review?(dbaron) → review+
Comment on attachment 8447839 [details] [diff] [review]
part 2 - Add tests for animation-name:none handling

r=dbaron
Attachment #8447839 - Flags: review?(dbaron) → review+
Blocks: 1033881
OS: Windows 8.1 → All
Hardware: x86_64 → All
Summary: Checkbox keeps animating → Checkbox keeps animating (animation events dispatch for animation-name: none)
https://hg.mozilla.org/mozilla-central/rev/1b2923e5a2f7
https://hg.mozilla.org/mozilla-central/rev/04848dd6053d
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in before you can comment on or make changes to this bug.