Closed Bug 1275142 Opened 4 years ago Closed 3 years ago

Intermittent test_restyles_in_smil_animation.html | should restyle in every frame - got 4, expected 5

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- fixed
firefox50 --- fixed

People

(Reporter: philor, Unassigned)

Details

(Keywords: intermittent-failure)

Attachments

(3 files)

I could confirm the failure locally.  There are rare cases the first restyle in the first frame is not observed. i.e., not processed.  I've not investigated how those patches for bug 1209405 work in our scripting and restyling process,  but we should create parent element in the first place, and then append animated SVG into the parent in the next frame.
If we create an animate SVG along with its parent, in rare cases,
the animation does not start in the first frame, i.e, it's the frame
that the animated element and its parent element are created. In such
cases, restyle for the animation is not observed in the first frame.
To avoid it, we need to create parent element in the first place,
then, append an animated element into the parent in the next frame.

Review commit: https://reviewboard.mozilla.org/r/59670/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59670/
Attachment #8763475 - Flags: review?(bbirtles)
Comment on attachment 8763475 [details]
Bug 1275142 - Don't create animate SVG along with its parent element at the same time.

https://reviewboard.mozilla.org/r/59670/#review57562

It would be nice to understand exactly why this occurs, but for now this fixes a frequently failing test and it seems to only affect SMIL so it's probably not worth spending too much time on.

::: layout/style/test/test_restyles_in_smil_animation.html:13
(Diff revision 1)
>  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css">
>  </head>
>  <body>
>  
> +<div id="target-div">
> +  <svg xmlns="http://www.w3.org/2000/svg">

Nit: No need to specify the namespace when using SVG in HTML.
Attachment #8763475 - Flags: review?(bbirtles) → review+
(In reply to Brian Birtles (:birtles) from comment #9)
> Comment on attachment 8763475 [details]
> Bug 1275142 - Don't create animate SVG along with its parent element at the
> same time. r?
> 
> https://reviewboard.mozilla.org/r/59670/#review57562
> 
> It would be nice to understand exactly why this occurs, but for now this
> fixes a frequently failing test and it seems to only affect SMIL so it's
> probably not worth spending too much time on.

Yes, exactly.  I can't catch what a trigger of skipping restyle is at all.  I filed a bug for the problem. Bug 1282337.
Comment on attachment 8763475 [details]
Bug 1275142 - Don't create animate SVG along with its parent element at the same time.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59670/diff/1-2/
Attachment #8763475 - Attachment description: Bug 1275142 - Don't create animate SVG along with its parent element at the same time. r? → Bug 1275142 - Don't create animate SVG along with its parent element at the same time.
Pushed by hiikezoe@mozilla-japan.org:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d347db00f665
Don't create animate SVG along with its parent element at the same time. r=birtles
https://hg.mozilla.org/mozilla-central/rev/d347db00f665
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
I don't see any sign that this had any effect at all on the frequency of the failure.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Thanks, philor.  The message has been slightly changed. 

should restyle *again* - got 4, expected 5

We need to wait a frame after removing display:none style.
I don't think this patch fixes this failure, but it's worth seeing changes by this patch.
Attachment #8770348 - Flags: review?(bbirtles)
Attachment #8770348 - Flags: review?(bbirtles) → review+
https://hg.mozilla.org/mozilla-central/rev/2056f45d739f
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
I am sorry, I forgot to mark leave-open.
Status: RESOLVED → REOPENED
Keywords: leave-open
Resolution: FIXED → ---
Unfortunately I can't take time to investigate bug 866411 for a while, so I'd propose here to allow a defect of restyle when the target element of SMIL animation is associated with an nsIFrame.

Actually the check of restyle count after associating with the nsIFrame is
not a problem in this test.  The purpose of this test is to check no restyles
when the target element has no associated nsIFrame.
We will fix the defect in bug 866411.
Attachment #8776528 - Flags: review?(bbirtles)
Comment on attachment 8776528 [details] [diff] [review]
Allow a defect of restyle when the target element of SMIL animation is associated with an nsIFrame

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

::: layout/style/test/test_restyles_in_smil_animation.html
@@ +83,5 @@
>    yield waitForPaintFlushed();
>  
>    var displayMarkers = yield observeStyling(5);
> +  // FIXME: Bug 866411: SMIL animations sometimes skip restyles when the target
> +  // element is associated with an nsIFrame.

Nit: newly associated? (Likewise below.)
Attachment #8776528 - Flags: review?(bbirtles) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/c83fe95ad2784208327687793eb4766d83afaa7b
Bug 1275142 - Allow a defect of restyle when the target element of SMIL animation is associated with an nsIFrame. r=birtles
Closing because there is no more failure since landing c83fe95ad278.
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Ryan, thanks for noticing it me.  And I am sorry for being inconvenient to sheriffs.  I will land it to aurora once the tree is open.
Flags: needinfo?(hiikezoe)
Landed.
https://hg.mozilla.org/releases/mozilla-aurora/rev/7177e736768960cd807440961beee561ca7ce582

I thought a comment to bugzilla was automatically generated, but actually it wasn't.
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.