Closed Bug 1425213 Opened 2 years ago Closed 2 years ago

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

Categories

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

58 Branch
defect

Tracking

()

VERIFIED FIXED
mozilla59
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- wontfix
firefox59 --- verified
firefox60 --- verified

People

(Reporter: nicolas.sandri, Assigned: wcpan, Mentored)

References

(Depends on 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files, 4 obsolete files)

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.
This is also a bug caused by bug 1190721.  And eventually it might be due to bug 1166500.
Blocks: 1190721
Status: UNCONFIRMED → NEW
Component: Layout → DOM: Animation
Ever confirmed: true
Flags: needinfo?(hikezoe)
This is actually regressed by bug 1190721, not bug 1166500.  I mean this is a transform specific issue.
Attached file A test case
There is also an issue open on the Bootstrap v4 issue tracker for this bug: https://github.com/twbs/bootstrap/issues/24657
Setting this to P2 since this is a recent regression affecting a commonly-used component.
Keywords: regression
Priority: -- → P2
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
Blocks: 1421507
Assignee: nobody → wpan
Mentor: boris.chiou
Attached patch wip.patch (obsolete) — Splinter Review
Unthrottling every 200ms still looks strange on the OP's example page.
Maybe we should not throttle this animation at all?
Attachment #8942122 - Flags: feedback?(hikezoe)
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());

?
Attachment #8942122 - Flags: feedback?(hikezoe) → feedback+
(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 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.
(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.
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 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.
Attachment #8942589 - Flags: review?(hikezoe) → review+
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.
:wcpan, I cannot land this patch due to some conflict issue.
Can you please check it?
Thanks.
Flags: needinfo?(wpan)
Pretty much my last patch in Mozilla.
Hope this can be landed and see you again!
Attachment #8943169 - Attachment is obsolete: true
Flags: needinfo?(wpan)
Attachment #8943502 - Flags: review+
Pushed by dluca@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4567f550794d
Unthrottle transform animations regardless in overflowable frames or not. r=hiro
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4567f550794d
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Great! Thanks Cornel,  and thank you Wei-Cheng Pan!
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
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.
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Duplicate of this bug: 1435902
Depends on: 1530185
You need to log in before you can comment on or make changes to this bug.