Closed Bug 1302007 Opened 3 years ago Closed 3 years ago

[Test] Add css-transitions tests of changing to the display property.

Categories

(Core :: DOM: Animation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: mantaroh, Assigned: mantaroh)

References

Details

Attachments

(4 files, 4 obsolete files)

We can cancel to the transition if we set the display property to 'none'.[1] 
However we don't have these tests yet. So we should add the these tests.

[1] https://dxr.mozilla.org/mozilla-central/source/layout/style/nsTransitionManager.cpp?q=UpdateTransitions&redirect_type=direct#424
Comment on attachment 8790138 [details]
Bug 1302007 part 1 - Use the promise_test in transition cancel tests.

https://reviewboard.mozilla.org/r/78092/#review76822
Attachment #8790138 - Flags: review?(bbirtles) → review+
Comment on attachment 8790139 [details]
Bug 1302007 part 2 - Add the test of cancelling the transition when setting display='none'.

https://reviewboard.mozilla.org/r/78098/#review76852

::: dom/animation/test/css-transitions/file_animation-cancel.html:130
(Diff revision 1)
> +  div.style.display = 'none';
> +  flushComputedStyle(div);
> +
> +  assert_equals(getComputedStyle(div).marginLeft, '1000px');
> +  assert_equals(animation.playState, 'idle');

It would be even more interesting to write:

  div.style.display = 'none';
  assert_equals(animation.playState, 'idle');
  assert_equals(getComputedStyle(div).marginLeft, '1000px');

Since querying |playState| should flush style.

::: dom/animation/test/css-transitions/file_animation-cancel.html:135
(Diff revision 1)
> +  div.style.display = 'none';
> +  flushComputedStyle(div);
> +
> +  assert_equals(getComputedStyle(div).marginLeft, '1000px');
> +  assert_equals(animation.playState, 'idle');
> +}, 'Cancel the transition due to set the display=\'none\'');

'Setting display:none on an element cancels its transitions'
Attachment #8790139 - Flags: review?(bbirtles) → review+
See Also: → 1263534
Comment on attachment 8790139 [details]
Bug 1302007 part 2 - Add the test of cancelling the transition when setting display='none'.

https://reviewboard.mozilla.org/r/78098/#review76852

Thanks brian.
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/278a03e949e6
part 1 - Use the promise_test in transition cancel tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/ce71dd457ebe
part 2 - Add the test of cancelling the transition when setting display='none'. r=birtles
Assignee: nobody → mantaroh
I wonder we need a test case that an ancestor element of transitioning element becomes display:none?
https://hg.mozilla.org/mozilla-central/rev/278a03e949e6
https://hg.mozilla.org/mozilla-central/rev/ce71dd457ebe
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
Hi hiro,

(In reply to Hiroyuki Ikezoe (:hiro) from comment #8)
> I wonder we need a test case that an ancestor element of transitioning
> element becomes display:none?
Oh, I think that we need to the these tests.
So I'm going to add this test in this bug.

Thanks.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Hi Brian,

I added the small mochitest for changing ancestor element's display property.

Could you please review this patch?
Attachment #8794724 - Flags: review?(bbirtles)
Comment on attachment 8794724 [details] [diff] [review]
Part 3 - Add test of changing an ancestor's display property.

Review of attachment 8794724 [details] [diff] [review]:
-----------------------------------------------------------------

I am taking this review.

::: dom/animation/test/css-transitions/file_animation-cancel.html
@@ +143,5 @@
> +  childDiv.style.transition = 'margin-left 100s';
> +  childDiv.style.marginLeft = '1000px';
> +  flushComputedStyle(childDiv);
> +
> +  var animation = childDiv.getAnimations()[0];

It's worth checking the animation is running, isn't it?

@@ +148,5 @@
> +  parentDiv.style.display = 'none';
> +
> +  assert_equals(animation.playState, 'idle');
> +  assert_equals(getComputedStyle(childDiv).marginLeft, '1000px');
> +}, 'Setting parent\'s display:none on a child element cancels its transitions');

This description seems to be wrong.   Setting display:none cancels transitions on a child element?
Attachment #8794724 - Flags: review?(bbirtles) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> Comment on attachment 8794724 [details] [diff] [review]
> Part 3 - Add test of changing an ancestor's display property.
> 
> Review of attachment 8794724 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am taking this review.
Thanks hiro!

I addressed test code based on comment 12.
Attachment #8794724 - Attachment is obsolete: true
Attachment #8795092 - Flags: review+
+  var animation = childDiv.getAnimations()[0];
+  return animation.ready.then(function() {
+    parentDiv.style.display = 'none';
+    return waitForFrame().then(function() {
+      assert_equals(animation.playState, 'idle');
+      assert_equals(getComputedStyle(childDiv).marginLeft, '1000px');
+    });
+  });

You might want to check playState == 'running'?

Also, I am not sure we need to wait animation.ready but you can write the promise chain like this;

return animation.ready.then(function() {
   assert_equals(animation.playState, 'running');
   parentDiv.style.display = 'none';
   return waitForFrame();
}).then(function() {
   ...
});
Hi hiro,
Thank you for your confirmation.

(In reply to Hiroyuki Ikezoe (:hiro) from comment #15)
> +  var animation = childDiv.getAnimations()[0];
> +  return animation.ready.then(function() {
> +    parentDiv.style.display = 'none';
> +    return waitForFrame().then(function() {
> +      assert_equals(animation.playState, 'idle');
> +      assert_equals(getComputedStyle(childDiv).marginLeft, '1000px');
> +    });
> +  });
> 
> You might want to check playState == 'running'?
I added this check statement and addressed promise chain.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=76b771a03849b71ca52633df8c972348249e77a1
Attachment #8795092 - Attachment is obsolete: true
Attachment #8795103 - Flags: review+
Can we drop layout/style/test/test_transitions_with_displaynone.html now?  Are all of test cases in the html covered by this?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #17)
> Can we drop layout/style/test/test_transitions_with_displaynone.html now? 
> Are all of test cases in the html covered by this?
I didn't notice this test.
I removed this test from layout/style/test.

Thanks.
Attachment #8795103 - Attachment is obsolete: true
Attachment #8795154 - Flags: review+
It might be better to remove it in a subsequent patch since it does not match to the commit message. No?
Sorry, I didn't update the commit message.
Separated into two patch. Could you please confirm second patch when you have time?
Attachment #8795161 - Flags: review?(hiikezoe)
Attachment #8795161 - Flags: review?(hiikezoe) → review+
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ed32c7a1d975
part 3 - Add test of changing an ancestor's display property. r=hiro
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f642d69c4f3
part 4 - Remove unnecessary transition cancellation tests. r=hiro
Keywords: leave-open
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.