1300938 - Nested fixed-position element no longer transitions smoothly (after removal of overaggressive reflows for box-shadow changes)

Status: RESOLVED DUPLICATE of bug 1200585

(Core :: Layout, defect)

Description

[kevinjh]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/52.0.2743.116 Safari/537.36

Steps to reproduce:

(copied from my reddit post - https://www.reddit.com/r/firefox/comments/51gn0p/large_regression_in_css_transition_performance/ )

I've been debugging an issue in our app in firefox that seems to occur only on later versions of firefox, and I have determined that the issue occurs exactly from updating from 43.0.4 to 44.0.0+. I tested every version in between, and the performance on versions prior is as expected (and equivalent to chrome and IE), but 44.0.0+ it is broken, with drastically reduced performance. Have tested this on multiple machines (all windows) / internet connections, and the performance is consistent with these versions.  Have also tested the latest nightly and can confirm the bug still exists.

Here's a couple animations of the differences in performance - this is just one example, but it is consistent across machines of different performance qualities as well as internet speeds - the version of firefox is the determining factor.

http://imgur.com/a/cgQ9h

I think the issue here is a CSS quirk with the layout in that there are actually 2 simultaneous transitions happening at the same time on that animation - but they are set to the same width and time, so they should move in sync (as they do on older FF, and chrome and IE).  They are nested html elements, both position fixed, being animated at the same time via css.

As per kbrosnan's suggestion, I ran mozregression against it, and narrow the regression down to these results:

Okay, that is an awesome tool.  Props to the dev work on that, very useful.  Here's the results of mozregression:


    20:36.82 INFO: Using local file: c:\users\kevin\appdata\local\temp\tmph2usqv\d92477504ce4--mozilla-inbound--firefox-44.0a1.en-US.win64.zip (downloaded in background)
    20:36.82 INFO: Running mozilla-inbound build built on 2015-10-02 04:14:11.864000, revision d9247750
    20:38.76 INFO: Launching c:\users\kevin\appdata\local\temp\tmpobrugz\firefox\firefox.exe
    20:38.77 INFO: application_buildid: 20151001194438
    20:38.77 INFO: application_changeset: d92477504ce419e03aefd0f78cd3a4c15ac10183
    20:38.77 INFO: application_name: Firefox
    20:38.78 INFO: application_repository: https://hg.mozilla.org/integration/mozilla-inbound
    20:38.78 INFO: application_version: 44.0a1
    Was this inbound build good, bad, or broken? (type 'good', 'bad', 'skip', 'retry', 'back' or 'exit' and press Enter): good
    21:41.21 INFO: Narrowed inbound regression window from [4657e5ff, 009f9286] (4 revisions) to [d9247750, 009f9286] (2 revisions) (~1 steps left)
    21:41.21 INFO: Oh noes, no (more) inbound revisions :(
    21:41.21 INFO: Last good revision: d92477504ce419e03aefd0f78cd3a4c15ac10183
    21:41.21 INFO: First bad revision: 009f9286101b1331fc9652743d7b702adc80b86d
    21:41.21 INFO: Pushlog:
    https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=d92477504ce419e03aefd0f78cd3a4c15ac10183&tochange=009f9286101b1331fc9652743d7b702adc80b86d

Haven't taken a look at diffs/changesets yet but will do so now, see if anything looks suspect

EDIT: Checked the diff, my suspicion is it's this commit - only 3 to choose from: https://hg.mozilla.org/integration/mozilla-inbound/rev/009f9286101b

The offending animation does have some sort of box shadow on it I believe  (on the element that is sliding) - I'll check on our loading spinner which has a similar performance problem and see if there is some shadow there.  Hope this helps isolate the cause of the issue

EDIT2: On second thought, all 3 of them are suspect, they all make some changes to overflow / reflow, which could potentially affect this behavior.  The css we are encountering issues with is using transition: all (but it is the "right" css positioning element that is being altered).  There are 2 nested elements (both position: fixed) that are being independently and simultaneously being transitioned in with css, using the same time / easing settings.  


Actual results:

See imgur link above for comparison of 44.0.0+ behavior vs pre 43.0.4 behavior


Expected results:

See imgur link above for comparison of pre 44.0.0+ behavior vs pre 43.0.4 behavior

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

The regression window looks like from dholbert's commits. ni? dholbert.

Looking at the commit message, my guess is that the overflow area doesn't take transition into account.

Comment 2

[Daniel Holbert [:dholbert]]

Could you provide a URL to a page that demonstrates the problem?  (Or a saved testcase, e.g. from File | Save as Complete, if that doesn't mess up the interactivity/scripts/etc.)

My initial guess is that we had an older bug here, where something wasn't repainting itself sufficiently -- and the code that I cleaned up in bug 1194480 happened to be working around that in your particular use-case, via excessive painting.  Now that we don't excessively paint, the older bug is revealed.

Hard to tell for sure without a live page to test, though.

Comment 3

[Daniel Holbert [:dholbert]]

(Also - from your imgur screen-capture, I suspect this isn't actually a performance bug -- it looks like we're simply failing to notice that we need to update the position of an element and/or repaint it, so it's left behind at a stale position.)

Comment 4

[kevinjh]

Daniel - this is a single page ember app, so saving it does not work very well.  However, I have set up an account for you on our QA site so you can see it directly.  I'll email you the credentials in a few minutes (to [e-mail]), and set it up so you login on one of the pages that has the bug.  I've also included some basic instructions in the email for other instances of it.

The css for that functionality is rather old and messy, so apologies in advance for that - but hopefully you can get a handle on whats happening.  It's 2 (nested) position:fixed elements that are being animated at the same time via the addition of a class on a parent element.  It's the "right" css property that is being manipulated.

Feel free to contact me on google chat at my gmail address if you need more info.  I'm on pacific time and I'll be at my desk all day today.

Comment 5

[Daniel Holbert [:dholbert]]

Thanks! I can definitely reproduce the bug, using the credentials you provided. I'll go ahead and assign this to myself, and I'll take a look in more detail soon (hopefully tomorrow).

Comment 6

[Liz Henry (:lizzard) (relman/hg->git project)]

I think it is too late for fix here for 49. We could still take a patch for 50 though.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 (partially reduced)

Comment 8

[Daniel Holbert [:dholbert]]

In Firefox versions before this regressed, the "CLICK THE PAGE" header in the attached testcase will slide out (with the rest of the sidebar) when you click the page.

Still needs a bit more reducing & then analysis; restoring needinfo=me.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (reduced)

Comment 10

[Daniel Holbert [:dholbert]]

Note: if I remove the "box-shadow" style from testcase 2 (i.e. if I *only* transition the "left" property), then the bug reproduces in old builds, too.

So this is indeed what I suspected in comment 2 -- bug 1194480 only "caused" this at the reported page because it turns out the page was (accidentally) relying on box-shadow's unnecessary-excessive-overpainting in order to work around another preexisting Firefox bug. :)

Comment 11

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (no box-shadow, so it's broken in older builds as well)

Here's an even simpler version of the testcase, with no "box-shadow" to save us.

EXPECTED RESULTS: Green box should jump to the right.
ACTUAL RESULTS: Red box jumps out from behind the green box.

If I resize my browser-window after I see the red square (forcing a relayout), the rendering fixes itself -- so this is an incremental layout bug of some sort.

I can reproduce the bug with this testcase in builds much earlier than the original regression-range here (with the original URL that was accidentally-being-saved-by-overpainting-box-shadow) -- e.g. a Nightly from 2015-01-01 shows the bug with this testcase.

Comment 12

[Daniel Holbert [:dholbert]]

With testcase 3, I get this regression range:

Last good revision: 6338a8988917 (2012-06-06)
First bad revision: 7e4c2abb9fc9 (2012-06-07)
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6338a8988917&tochange=7e4c2abb9fc9

In that range, this looks likely to be the culprit:
> http://hg.mozilla.org/mozilla-central/rev/df6702c41ddd
> Bug 157681 - Part 2: Optimize positioned frame offset changes by moving the frame as opposed to
>                      reflowing it in case we know that the size of the frame will not change; r=dbaron

...and it looks like this is a duplicate of bug 1200585 (whose description sounds a lot like what's going on here, and is also marked as a regression from that bug).

So: I'll mark this as a duplicate, and we can proceed with potentially fixing this over on bug 1200585.

THOUGHTS ON WORKAROUNDS
=======================
So, Kevin: basically, it looks like your app was unlucky enough to be triggering an old-ish Firefox bug with nested fixed-position elements, but the fact that you were changing "box-shadow" at the same time happened to save you from exposing the bug, because we were overly aggressive in response to box-shadow updates. That accidental-box-shadow-workaround stopped working when we made box-shadow handling more targeted in bug 1194480.

So now, if you'd like to try to work around this, your best bet would be to try to get rid of the nested fixed-position elements.

Poking around at your app in devtools (starting at the "reports" page):  It looks like I can fix it locally if I adjust the ".two-col-panel .slide-out-panel .fixed-header" style rule in your soxhub-client-*** css file to use "position:absolute; top: 0px" instead of "position: fixed; top: 82px". (There's a higher-priority "top: 51px" declaration in another style rule that I have to disable, too, for this change to take effect).  This works both at that "reports" page as well as the front page that I get to after logging in ("new vendor control").

Hopefully that (or something like it) can help you guys to avoid this bug, so your firefox users don't have to wait for bug 1200585 to be fixed.  Thanks again for the bug report & for being helpful with creating a test account for me!