Closed Bug 1366934 Opened 7 years ago Closed 7 years ago

stylo: Failures in layout/style/test/test_animations_event_order.html

Categories

(Core :: CSS Parsing and Computation, defect, P2)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla56
Tracking Flags:
Tracking Status
firefox56 --- fixed

People

(Reporter: hiro, Assigned: mantaroh)

References

Details

Attachments

(1 file)

Bug 1366934 - Remove validation of transitioncancel/animationcanel event order tests.
59 bytes, text/x-review-board-request
hiro
: review+
Details 
Two tests in test_animations_event_order.html fail.  I have no idea why they fail yet, it might be related to recent high failure frequency of bug 1358266. Bug 1358266 is caused by timeout during waiting for animationstart event, I guess.

failed | Simultaneous transitionrun/start/cancel on siblings - got "div0:transitionrun;div1:transitionrun;div1:transitionstart;div0:transitionstart;div0:transitioncancel;div1:transitioncancel", expected "div0:transitionrun;div1:transitionrun;div1:transitionstart;div0:transitionstart;div1:transitioncancel;div0:transitioncancel"
failed | Simultaneous animationcancel on siblings - got "div1:animationstart;div0:animationstart;div0:animationcancel;div1:animationcancel", expected "div1:animationstart;div0:animationstart;div1:animationcancel;div0:animationcancel"
Blocks: stylo-style-mochitest
(In reply to Hiroyuki Ikezoe (:hiro) from comment #0)
> Two tests in test_animations_event_order.html fail.  I have no idea why they
> fail yet, it might be related to recent high failure frequency of bug
> 1358266. Bug 1358266 is caused by timeout during waiting for animationstart
> event, I guess.

Maybe, I was wrong.  It's not clear to me the order of cancel events for different elements when several animation on several elements were cancelled at the same time.
Yeah, normally we sort animations based on their position in the tree (see CSSAnimation::HasLowerCompositeOrderThan) but for cancel events the Animation (CSSAnimation/CSSTransition) objects are no longer tied to markup so we just sort by animation index (see Animation::HasLowerCompositeOrderThan) which might depend on when the transitions/animations were created (i.e. be dependent on how we traversed the tree when we first created those animations/transitions).
Thanks for the quick info. So this failure is caused by different traversal order of the target sibling elements on stylo, right? Then, we can somehow modify test code for stylo if the order on stylo is deterministic (I believe it is).
Ah, or we can simply ignore the order for the two test cases.
Yeah, I don't think the absolute order of cancel events matters, provided it is deterministic. But will it be deterministic?
I did a try with the cancel events order change.  If we eagerly cancel animations/transitions, the order may match to gecko's one, I am not sure though.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f99e8b086118bdfb0d87da4f22b52cfcf33a8ca
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> Ah, or we can simply ignore the order for the two test cases.

I think that we had better to remove this tests.
I added this event order check on bug 1264125[1], it based on gecko's behavior. 

[1] http://searchfox.org/mozilla-central/rev/d840ebd5858a61dbc1622487c1fab74ecf235e03/layout/style/test/test_animations_event_order.html#630-632

(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #7)
> Created attachment 8876574 [details]
> Bug 1366934 - Remove validation of transitioncancel/animationcanel event
> order tests.
> 
> The cancel event should be deterministic, so we will not need to validate
> the cancel event order of sibling elements.
> 
> Review commit: https://reviewboard.mozilla.org/r/147878/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/147878/

This patch removed this validation.
Comment on attachment 8876574 [details]
Bug 1366934 - Remove validation of transitioncancel/animationcanel event order tests.

https://reviewboard.mozilla.org/r/147878/#review152230

I think the patch looks good to me but this patch needs to be explained in more detail.

::: commit-message-981da:3
(Diff revision 1)
> +The cancel event should be deterministic, so we will not need to validate
> +the cancel event order of sibling elements.

This commit message confuses me.  Why don't we need to check the cancel event order if the event (not order?) is deterministic?  I think we can check the order if the order is deterministic, no?

::: layout/style/test/test_animations_event_order.html:651
(Diff revision 1)
> +  // The transitioncancel event order is not absolute when firing siblings
> +  // transitioncancel on same time.
> +  // So in this tests, we will not validate the siblings transitioncancel
> +  // event order.
> +  getComputedStyle(div).display;

Should we leave a comment here why calling getComputedStyle() against each element affects the cancel event order?  This behavior highly depends on styling process I think.
Attachment #8876574 - Flags: review?(hikezoe)
Comment on attachment 8876574 [details]
Bug 1366934 - Remove validation of transitioncancel/animationcanel event order tests.

https://reviewboard.mozilla.org/r/147878/#review152230

Thanks.

> This commit message confuses me.  Why don't we need to check the cancel event order if the event (not order?) is deterministic?  I think we can check the order if the order is deterministic, no?

This mean drop the test of sibling cancel events. But as you mentioned, this comment will bring confusion.
So I modified this comments.
Comment on attachment 8876574 [details]
Bug 1366934 - Remove validation of transitioncancel/animationcanel event order tests.

https://reviewboard.mozilla.org/r/147878/#review152724

::: commit-message-981da:4
(Diff revision 2)
> +Bug 1366934 - Remove validation of sibling transitioncancel/animationcanel event order tests. r?hiro
> +
> +The transitioncancel and animationcancel events based on traversal order of
> +element. This way is different from gecko and stylo, so we will not need to

Nit: not need to check the order of cancel events?

::: layout/style/test/test_animations_event_order.html:651
(Diff revision 2)
>  advance_clock(0);
>  advance_clock(5 * 1000);
> -divs.forEach(div =>  div.style.display = 'none' );
> -getComputedStyle(divs[0]).display;
> +divs.forEach(div =>  {
> +  div.style.display = 'none';
> +  // The transitioncancel event order is not absolute when firing siblings
> +  // transitioncancel on same time.

We no longer cancel the two transition at the same time, right?
What I understand is that this test does;

1) cancel the transition on div[0] 
2) flush style (i.e. the cancel event is queued in this phase)
3) cancel the transition on div[1]
4) flush style

So, I think it's not the same timing, it's just the same elapsed time?

::: layout/style/test/test_animations_event_order.html:655
(Diff revision 2)
> +  // The transitioncancel event order is not absolute when firing siblings
> +  // transitioncancel on same time.
> +  // So in this tests, we will not validate the siblings transitioncancel
> +  // event order.
> +  // This restyle cause the transitioncancel event on  each element event
> +  // immediately, then received event order will be creation order of element.

I think we should comment something like this;

// Force to flush style for the element so that the transition on the element is cancelled and corresponding cancel event is queued respectively.

I am not sure that the spec defines that the queued event is fired first-in-first-out order though.  I guess it's not specced, but we just do it by using stable sort?

::: layout/style/test/test_animations_event_order.html:693
(Diff revision 2)
>  advance_clock(0);  // divs[1]'s animation start
>  advance_clock(5 * 1000);  // divs[0]'s animation start
> -divs.forEach(div => div.style.display = 'none' );
> -getComputedStyle(divs[0]).display;
> +divs.forEach(div => {
> +  div.style.display = 'none';
> +  // The animationcancel event order is not absolute when firing siblings
> +  // animationcancel on same time.

Likewise here. We don't cancel the two transitions at the same time.
Attachment #8876574 - Flags: review?(hikezoe) → review+
Assignee: nobody → mantaroh
Priority: -- → P2
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0277a0564bdb
Remove validation of transitioncancel/animationcanel event order tests. r=hiro
https://hg.mozilla.org/mozilla-central/rev/0277a0564bdb
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: