Closed Bug 1417600 Opened 7 years ago Closed 7 years ago

stylo: browser/components/downloads/test/browser/browser_downloads_autohide.js times out with stylo-chrome

Categories

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

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla59
Tracking Flags:
Tracking Status
firefox57 --- disabled
firefox58 --- disabled
firefox59 --- fixed

People

(Reporter: xidorn, Assigned: xidorn)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1417600 - Wait for another frame before setting animation out to ensure start of the animation.
59 bytes, text/x-review-board-request
jaws
: review+
Details 
The test checkStateDuringPrefFlips in browser/components/downloads/test/browser/browser_downloads_autohide.js times out when waiting for the third gCustomizeMode.addToPanel() in that test [1] after enabling stylo-chrome (via setting layout.css.servo.chrome.enabled to true and restarting the browser).

Looking at the code of gCustomizeMode.addToPanel, it seems that the only place it's waiting is a promise returned from _promiseWidgetAnimationOut [2], and the latter waits for animationend event or finishing customization mode [3].

So it seems that it is an animationend which is supposed to be fired isn't fired. But if I focus to another window, this test can continue running. Probably at that time, the animationend is eventually fired in a whole tree restyle.

birtles, hiro, or boris, could you have a look at this bug? I have no idea what's happening behind...

[1] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/browser/components/downloads/test/browser/browser_downloads_autohide.js#51
[2] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/browser/components/customizableui/CustomizeMode.jsm#659-662
[3] https://searchfox.org/mozilla-central/rev/a662f122c37704456457a526af90db4e3c0fd10e/browser/components/customizableui/CustomizeMode.jsm#611-613
I will look.
Assignee: nobody → hikezoe
I guess you would be able to reproduce this via |mach mochitest browser/components/downloads/test/browser/browser_downloads_autohide.js --setpref=layout.css.servo.chrome.enabled=true|.
Thank you, Xidorn. I can reproduce locally.  It seems the animation itself is not created at that time.
Summary: stylo: animationend event is sometimes not dispatched until a restyle → stylo: browser/components/downloads/test/browser/browser_downloads_autohide.js times out with stylo-chrome
I've almost forgotten how I had been debugging servo styling..  Anyway, after adding 'animate-out' class to the element, the element is not traversed for some reason, that's why the animation is not created.
Emilio helped me to understand what's going on there.  But I don't still understand it yet. :/
Anyway, now what I know is that after adding 'animate-out' class, class_added in collect_invalidations is empty. :/
(In reply to Hiroyuki Ikezoe (:hiro) from comment #5)
> Emilio helped me to understand what's going on there.  But I don't still
> understand it yet. :/
> Anyway, now what I know is that after adding 'animate-out' class,
> class_added in collect_invalidations is empty. :/

That can happen if it was both removed and added, that is, if we had not flushed style before removing it. Is there any chance that that's what's happening?
(In reply to Emilio Cobos Álvarez [:emilio] from comment #6)
> That can happen if it was both removed and added, that is, if we had not
> flushed style before removing it. Is there any chance that that's what's
> happening?

That sounds plausible. If I add
> this.window.getComputedStyle(animationNode).animationName;
before
> animationNode.classList.add("animate-out");
then the test passes.

It feels like a more general issue related to CSS animation, then. Changing animation-name is supposed to always trigger a new animation, I believe.
So, the spec seems to be somehow vague on behavior of this case. It says
> An animation specified by dynamically modifying the element's style
> will start when this style is resolved; that ... may be when the
> scripting engine returns control to the browser (in the case of style
> applied by script). 
so it gives browsers leeway for when to start the animation.

And it seems that the testcase indeed can potentially lead to something broken.

Basically, the "animate-out" class is removed in animationend, and re-added synchronously. There doesn't seem to be anywhere between them which hands control back to the browser.

So I guess we should probably fix this by always forcing a restyle before setting "animate-out", or forcing that after removing it? That does sound like something may harm the performance in UI, though.
I'm not sure whether anyone would want to continuously animate the same node outside this test... If not, we can probably just add force restyle into the test so that it passes...
Comment on attachment 8929092 [details]
Bug 1417600 - Wait for a style flush before setting animation out to ensure start of the animation.

https://reviewboard.mozilla.org/r/200386/#review205544

::: browser/components/customizableui/CustomizeMode.jsm:611
(Diff revision 1)
> +      // Force a restyle before setting animate-out class to ensure we
> +      // do start the animation.
> +      this.window.getComputedStyle(animationNode).animationName;

We can use `BrowserUtils.promiseLayoutFlushed(document, "style", callback)` to return control back to the browser until style has been flushed and then move the remaining code in to the callback.

This would fix the bug without introducing a sync style flush.
Attachment #8929092 - Flags: review?(jaws) → review-
Comment on attachment 8929092 [details]
Bug 1417600 - Wait for a style flush before setting animation out to ensure start of the animation.

https://reviewboard.mozilla.org/r/200386/#review205544

> We can use `BrowserUtils.promiseLayoutFlushed(document, "style", callback)` to return control back to the browser until style has been flushed and then move the remaining code in to the callback.
> 
> This would fix the bug without introducing a sync style flush.

Oh, didn't know that we have something like this. It looks useful. Thanks.
Comment on attachment 8929092 [details]
Bug 1417600 - Wait for a style flush before setting animation out to ensure start of the animation.

https://reviewboard.mozilla.org/r/200386/#review205580

Thanks! Yes it is very cool :)
Attachment #8929092 - Flags: review?(jaws) → review+
Blocks: 1418082
yeah, flushing style fixes this timeout, but I think there is an underlying issue that the element is skipped to be traversed.  I will file bug 1418082.
Assignee: hikezoe → xidorn+moz
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/04dac3301ecd
Wait for a style flush before setting animation out to ensure start of the animation. r=jaws
Priority: -- → P3
It seems that somehow promiseLayoutFlushed is not guaranteed to be triggered...
Flags: needinfo?(xidorn+moz)
jaws, any idea why the callback passed to promiseLayoutFlushed is never executed in test browser/components/customizableui/test/browser_972267_customizationchange_events.js?
Flags: needinfo?(jaws)
Comment on attachment 8929092 [details]
Bug 1417600 - Wait for a style flush before setting animation out to ensure start of the animation.

https://reviewboard.mozilla.org/r/200386/#review207124

::: browser/components/customizableui/CustomizeMode.jsm:613
(Diff revision 2)
>          resolve();
>        }
>  
> +      // Wait for a style flush to ensure we do start the animation.
> +      BrowserUtils.promiseLayoutFlushed(this.document, "style", () => {
> -      animationNode.classList.add("animate-out");
> +        animationNode.classList.add("animate-out");

I don't understand why the callback is not executed, but I do see an error here which might be related.

The callback must not trigger any reflows, and right now it is by setting the className.

You will need to move this within a requestAnimationFrame callback within this callback.

For example,
```
BrowserUtils.promiseLayoutFlushed(this.document, "style", () => {
  this.window.requestAnimationFrame(() => {
    animationNode.classList.add("animate-out");
    ...
  });
});
```
Flags: needinfo?(jaws)
Since the callback is not invoked at all... I think wrapping it with requestAnimationFrame wouldn't fix that failure...
Please flag me for needinfo again if that doesn't help.
I wasn't sure how you were testing that the callback isn't invoked.
Flags: needinfo?(jaws)
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #23)
> I wasn't sure how you were testing that the callback isn't invoked.

I put a dump before and inside promiseLayoutFlushed, and the one inside never shows up.
It is more weird that even if I request a synchronous style flush immediately before promiseLayoutFlushed, the callback still isn't invoked synchronously, and it still gets stuck on waiting for reflow.
How about flushing the style after removing 'animated-out' class?  As far as I observed, we fail to snapshot for the removal (and thus animation is not triggered by adding the class).
(In reply to Hiroyuki Ikezoe (:hiro) from comment #26)
> How about flushing the style after removing 'animated-out' class?  As far as
> I observed, we fail to snapshot for the removal (and thus animation is not
> triggered by adding the class).

That doesn't work... because frontend people don't want to see more synchronous flush, otherwise we can just add a synchronous flush before adding 'animated-out' class. Also there are way more places where the class is removed than where it is added, so adding to those places means more duplicate code that may not have enough tests on.
(In reply to Xidorn Quan [:xidorn] UTC-6 (less responsive Nov 5 ~ Dec 16) from comment #25)
> It is more weird that even if I request a synchronous style flush
> immediately before promiseLayoutFlushed, the callback still isn't invoked
> synchronously, and it still gets stuck on waiting for reflow.

Somehow this works now... I don't think I changed anything, but it does work for some reason... But we don't want synchronous style flush so we still need to figure out some way without this.
So, looking at the code, I now suspect that promiseLayoutFlushed is somehow broken for at least this usage.

The callback would only be triggered if there is any actual reflow happens, which means, if there is no style / content change which needs to trigger reflow (e.g. some neutral style change or changes inside display:none tree), the callback would not be invoked until something else causes an actual reflow.

In the test, then, since nothing really triggers another reflow, it gets blocked.

I'm not sure what's the right approach to solve this. I guess we may need some kind of finer-grained notification to happen after style flush, rather than solely relying on reflow notification.
Attachment #8929092 - Attachment is obsolete: true
Comment on attachment 8930970 [details]
Bug 1417600 - Wait for another frame before setting animation out to ensure start of the animation.

https://reviewboard.mozilla.org/r/202064/#review207608
Attachment #8930970 - Flags: review?(jaws) → review+
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d9c0a75a007
Wait for another frame before setting animation out to ensure start of the animation. r=jaws
https://hg.mozilla.org/mozilla-central/rev/1d9c0a75a007
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox59: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Flags: needinfo?(jaws)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: