1425213 - Animation rendering problem with (ex-Twitter) Bootstrap 3 Carousel

Status: VERIFIED FIXED

(Core :: DOM: Animation, defect, P2)

Description

[Nicolas Sandri]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20171213220121

Steps to reproduce:

Browse to
http://getbootstrap.com/docs/3.3/javascript/#carousel
using FF 58.0+.

Tested on:
 - Firefox Developer Edition (58.0), macOS ;
 - Firefox Nightly (59.0), macOS ;
 - Firefox Nightly (59.0), Windows 8.1.


Actual results:

When the transition is running between two slides, the current slide is correctly leaving moving to the left but the next slide appears brutally, with a brief apparition of the background (sort of white flash).
FF Dev Edition (58.0), macOS: http://recordit.co/oovjqD8uPL
FF Nightly (59.0), Win 8.1: http://recordit.co/bZahTRZKnm


Expected results:

The next slide should appears by moving smoothly from the right.
In current public FF (57.0), the animation displays correctly.

Comment 1

[Hiroyuki Ikezoe (:hiro)]

This is also a bug caused by bug 1190721.  And eventually it might be due to bug 1166500.

Comment 2

[Hiroyuki Ikezoe (:hiro)]

This is actually regressed by bug 1190721, not bug 1166500.  I mean this is a transform specific issue.

Comment 3

[Hiroyuki Ikezoe (:hiro)]

Attachment: A test case

Comment 4

[Ian Gregory]

There is also an issue open on the Bootstrap v4 issue tracker for this bug: https://github.com/twbs/bootstrap/issues/24657

Comment 5

[Brian Birtles (:birtles)]

Setting this to P2 since this is a recent regression affecting a commonly-used component.

Comment 6

[Hiroyuki Ikezoe (:hiro)]

Yeah, I think we should fix this before bug 1421507 because bug 1421507 makes this bug more common.

What currently we do for offscreen transform animations is;

- Unthrottle the animations every 200ms if the animated frame is inside overflowable frames

But actually what we should really do is;

- Unthrottle the animations every 200ms even if the animated frame is not inside overflowable frames since transform might update the frame position

Comment 7

[Hiroyuki Ikezoe (:hiro)]

\o/

Comment 8

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: wip.patch

Unthrottling every 200ms still looks strange on the OP's example page.
Maybe we should not throttle this animation at all?

Comment 9

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942122 [details] [diff] [review]
wip.patch

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

Thanks! Looks pretty good.

A couple of random comments below.

Also we need a test case for this, in dom/animation/mozilla/file_restyles.html or a reftest. Either is fine with me.

::: dom/animation/KeyframeEffectReadOnly.cpp
@@ +1484,5 @@
>    return true;
>  }
>  
>  bool
>  KeyframeEffectReadOnly::CanThrottleTransformChanges(nsIFrame& aFrame) const

I think now we can use const nsIFrame& here.

@@ +1501,5 @@
>        (now - lastSyncTime) < OverflowRegionRefreshInterval()) {
>      return true;
>    }
>  
> +  return false;

We can write this just as

return (!lastSyncTime.IsNull() &&
      (now - lastSyncTime) < OverflowRegionRefreshInterval()) {	
(now - lastSyncTime) < OverflowRegionRefreshInterval());

?

Comment 10

[Hiroyuki Ikezoe (:hiro)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> We can write this just as
> 
> return (!lastSyncTime.IsNull() &&
>       (now - lastSyncTime) < OverflowRegionRefreshInterval()) {	
> (now - lastSyncTime) < OverflowRegionRefreshInterval());
> 
> ?

Do'h!  Copy-and-paste failed.

I meant;

 return !lastSyncTime.IsNull() &
        (now - lastSyncTime) < OverflowRegionRefreshInterval();

Comment 11

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: Bug_1425213___Unthrottle_transform_animations_regardless_in_overflowable_frames_or_not__r_hiro.patch

Comment 12

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942589 [details] [diff] [review]
Bug_1425213___Unthrottle_transform_animations_regardless_in_overflowable_frames_or_not__r_hiro.patch

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

The code change looks good to me.

I have a question about the test cases.  What is the difference between the two tests?  Can you please elaborate them?  Bothe of them are for this bug?
At first glance, I don't think we need to set ui.showHideScrollbars pref since the test cases should work without the pref, I mean they should work regardless scroll bars.

Comment 13

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #12)
> I have a question about the test cases.  What is the difference between the
> two tests?  Can you please elaborate them?  Bothe of them are for this bug?

I'm not sure that if conformant promise handling will affect this a lot or not, so I added two cases for both cases.

Comment 14

[Hiroyuki Ikezoe (:hiro)]

Ah, I see. The one is for a test without the conformant promise handling, the other one is for with the conformant handling.  That makes sense.

Comment 15

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8942589 [details] [diff] [review]
Bug_1425213___Unthrottle_transform_animations_regardless_in_overflowable_frames_or_not__r_hiro.patch

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

Please make sure the test case works fine with the conformant promise handling patch (bug 1193394).

Anyway, thanks for working this!

::: dom/animation/test/mozilla/file_restyles.html
@@ +494,5 @@
>      }
>    );
>  
> +  add_task(
> +    async function restyling_transform_animations_in_scrolled_out_element() {

Please rename this function to something without 'scrolled out'.  Actually the transition is not inside a scrollable element (i.e. the element has no scroll bars), right?

@@ +511,5 @@
> +      if (isAndroid) {
> +        return;
> +      }
> +
> +      await SpecialPowers.pushPrefEnv({ set: [["ui.showHideScrollbars", 1]] });

Drop this.  We don't need the pref.  I mean this test should work without the pref.

@@ +516,5 @@
> +
> +      var parentElement = addDiv(null,
> +        { style: 'overflow: hidden;' });
> +      var div = addDiv(null,
> +        { style: 'animation: move-in 1s;' });

We usually use 100s duration in animation tests to avoid finishing the animation unexpectedly (in the case where the main-thead was busy at the time).

@@ +522,5 @@
> +      var animation = div.getAnimations()[0];
> +      var timeAtStart = document.timeline.currentTime;
> +
> +      ok(!animation.isRunningOnCompositor,
> +         'The transform animation is not running on the compositor');

Nit: 'The transform animation on out of view element is not..'

Note that transform animations generally run on the compositor, it would be nice to have a comment why it's not running on the compositor.

@@ +540,5 @@
> +           'should be throttled until 200ms is elapsed');
> +      }
> +
> +      is(markers.length, 1,
> +         'Transform animation running on the element which is scrolled out ' +

Nit: 'The transform animation on out of view element should be unthrottled ...'

@@ +558,5 @@
> +      if (!offscreenThrottlingEnabled) {
> +        return;
> +      }
> +
> +      await SpecialPowers.pushPrefEnv({ set: [["ui.showHideScrollbars", 1]] });

Drop this as well.

@@ +566,5 @@
> +
> +      var parentElement = addDiv(null,
> +        { style: 'overflow: hidden;' });
> +      var div = addDiv(null,
> +        { style: 'animation: move-in 1s;' });

Use 100s.

@@ +572,5 @@
> +      var animation = div.getAnimations()[0];
> +      var timeAtStart = document.timeline.currentTime;
> +
> +      ok(!animation.isRunningOnCompositor,
> +         'The transform animation is not running on the compositor');

Likewise here, fix the comment.

@@ +589,5 @@
> +        }
> +
> +        markers = await observeStyling(1);
> +        is(markers.length, 0,
> +           'Transform animation running on the element which is scrolled out ' +

Nit: 'The transform animation on out of view element should be unthrottled ...'

@@ +594,5 @@
> +           'should be throttled until 200ms is elapsed');
> +      }
> +
> +      is(markers.length, 1,
> +         'Transform animation running on the element which is scrolled out ' +

Likewise.

Comment 16

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: Bug_1425213___Unthrottle_transform_animations_regardless_in_overflowable_frames_or_not__r_hiro.patch

Updated testcases.

Comment 17

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8943157 [details] [diff] [review]
Bug_1425213___Unthrottle_transform_animations_regardless_in_overflowable_frames_or_not__r_hiro.patch

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

::: dom/animation/test/mozilla/file_restyles.html
@@ +499,5 @@
> +      if (hasConformantPromiseHandling) {
> +        return;
> +      }
> +
> +      if (!offscreenThrottlingEnabled) {

We have to drop this check,  I've already landed the change dropping the variable on autoland.

https://hg.mozilla.org/integration/autoland/rev/bdec197f0448c5c9f1ede5cb3a037933fa49f733

@@ +553,5 @@
> +      if (!hasConformantPromiseHandling) {
> +        return;
> +      }
> +
> +      if (!offscreenThrottlingEnabled) {

Likewise here.

Comment 18

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: Bug_1425213___Unthrottle_transform_animations_regardless_in_overflowable_frames_or_not__r_hiro.patch

Sorry for missed that :(

Comment 19

[Narcis Beleuzu [:NarcisB]]

:wcpan, I cannot land this patch due to some conflict issue.
Can you please check it?
Thanks.

Comment 20

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: 0001-Bug-1425213-Unthrottle-transform-animations-regardle.patch

Pretty much my last patch in Mozilla.
Hope this can be landed and see you again!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4567f550794d
Unthrottle transform animations regardless in overflowable frames or not. r=hiro

Comment 22

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/4567f550794d

Comment 23

[Hiroyuki Ikezoe (:hiro)]

Great! Thanks Cornel,  and thank you Wei-Cheng Pan!

Comment 24

[Nicolas Sandri]

Hello

I just tested in FF 59.0a1 20180122100120 / 5faab9e61990 (macOS) and it's much better but it's not fixed.

Check this modified version of the Hiroyuki's test:
http://jsfiddle.net/mediaxtend/t4svvhsz/embedded/result,html,css,js/
The blue square is not animated while over the orange rectangle. Since the orange area is approximately 40 px wide and the animation has a duration of 0.5 s, it seems that approximately 100 ms of the animation beginning are missing.

And a small white flash appears in the Carousel animation on the BS official page:
http://getbootstrap.com/docs/3.3/javascript/#carousel

Regards

Comment 25

[Hiroyuki Ikezoe (:hiro)]

Yes, indeed.  The case is pretty hard to solve (as far as I can tell I have no idea to solve it so far).  If we fixed the case, we start suffering from bug 1190721 again.  Please file a new bug for the case.  I will try to fix it if I come up with a great idea to fix a bunch of these kind of issues.

Comment 26

[Emil Ghitta, QA [:emilghitta]]

I have managed to reproduce the issue mentioned in comment 0 using Firefox 59.0a1 (BuildId:20171214220032).

This issue is no longer reproducible (with a small note that we also observed the behavior mentioned in comment 24) using Firefox 59.0b5 (BuildId:20180128191456) and Firefox 60.0a1 (BuildId:20180129220114) on Windows 10 64bit, Ubuntu 16.04 64bit and macOS 10.13.2.

Comment 28

[nicolas]

I can reproduce the all of the following issues under Firefox 60.0a1 (BuildId:20180210220139) on macOS 10.13.4.
- http://jsfiddle.net/mediaxtend/t4svvhsz/embedded/result,html,css,js/
- https://foundation.zurb.com/sites/docs/orbit.html
- http://getbootstrap.com/docs/3.3/javascript/#carousel

See: http://www.giphy.com/gifs/3o7WIDHwT2eOxa0RG0