Closed
Bug 1425213
Opened 7 years ago
Closed 7 years ago
Animation rendering problem with (ex-Twitter) Bootstrap 3 Carousel
Categories
(Core :: DOM: Animation, defect, P2)
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
(Blocks 1 open bug)
Details
(Keywords: regression)
Attachments
(2 files, 4 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
wcpan
:
review+
|
Details | Diff | Splinter Review |
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•7 years ago
|
||
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)
Comment 2•7 years ago
|
||
This is actually regressed by bug 1190721, not bug 1166500. I mean this is a transform specific issue.
status-firefox57:
--- → unaffected
status-firefox58:
--- → affected
status-firefox59:
--- → affected
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(hikezoe)
Comment 3•7 years ago
|
||
Comment 4•7 years ago
|
||
There is also an issue open on the Bootstrap v4 issue tracker for this bug: https://github.com/twbs/bootstrap/issues/24657
Comment 5•7 years ago
|
||
Setting this to P2 since this is a recent regression affecting a commonly-used component.
Keywords: regression
Priority: -- → P2
Comment 6•7 years ago
|
||
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
Updated•7 years ago
|
Assignee: nobody → wpan
Mentor: boris.chiou
Comment 7•7 years ago
|
||
\o/
Assignee | ||
Comment 8•7 years ago
|
||
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 9•7 years ago
|
||
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+
Comment 10•7 years ago
|
||
(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();
Assignee | ||
Comment 11•7 years ago
|
||
Attachment #8942122 -
Attachment is obsolete: true
Attachment #8942589 -
Flags: review?(hikezoe)
Comment 12•7 years ago
|
||
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.
Assignee | ||
Comment 13•7 years ago
|
||
(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•7 years ago
|
||
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•7 years ago
|
||
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+
Assignee | ||
Comment 16•7 years ago
|
||
Updated testcases.
Attachment #8942589 -
Attachment is obsolete: true
Attachment #8943157 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 17•7 years ago
|
||
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.
Assignee | ||
Comment 18•7 years ago
|
||
Sorry for missed that :(
Attachment #8943157 -
Attachment is obsolete: true
Attachment #8943169 -
Flags: review+
Comment 19•7 years ago
|
||
:wcpan, I cannot land this patch due to some conflict issue.
Can you please check it?
Thanks.
Flags: needinfo?(wpan)
Updated•7 years ago
|
Keywords: checkin-needed
Updated•7 years ago
|
Assignee | ||
Comment 20•7 years ago
|
||
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+
Assignee | ||
Updated•7 years ago
|
Keywords: checkin-needed
Comment 21•7 years ago
|
||
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
Comment 22•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Updated•7 years ago
|
Flags: qe-verify+
Comment 23•7 years ago
|
||
Great! Thanks Cornel, and thank you Wei-Cheng Pan!
Reporter | ||
Comment 24•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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•7 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•