1212625 - Transitions don't play on a flex container, if we're reframing it due to anonymous flex item changes

Status: RESOLVED WORKSFORME

(Core :: Layout, defect)

Description

[mitsugami]

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20151001175634

Steps to reproduce:

Well, i did a small library for notifications. When i need show a message, one class is added to the element and the transition starts.


Actual results:

When the transition starts for first time, everything is ok, but when the transition starts for second, third, and n time (obviously add/remove the class) the transition didn't work. You can see the bug in action in this fiddle: http://jsfiddle.net/jk7tkzhn/


Expected results:

A normal transition.

Comment 1

[Alice0775 White]

Regressed since Firefox22

Regression window:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=6ae3ca2f2438&tochange=43ca2f335378

Triggered by: 75cab729c971	Daniel Holbert — Bug 783490: Enable "layout.css.flexbox.enabled" pref. r=dbaron

The bug # is typo, should be Bug 783409.

Comment 2

[Alice0775 White]

Regression window with force layout.css.flexbox.enabled=true & prefixed -moz- for css flex property:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=16ac5cbc7793&
tochange=7ad0763694a3

Triggered by:
ad0763694a3	Daniel Holbert — Bug 797022: Turn on MOZ_FLEXBOX build flag to enable CSS3 flexbox in builds. r=bz

Comment 3

[Boris Zbarsky [:bzbarsky]]

The testcase changes display as part of the class change, right?  This blows away the old box tree and creates a new one, so there isn't a change in computed style for a given box, which is what triggers transitions...

Comment 4

[Daniel Holbert [:dholbert]]

(Clearing 'blocks' field -- this isn't a regression from either of those commits, but rather this testcase just relies on "display:flex" which only does something when we've got both of those commits.)

Comment 5

[Daniel Holbert [:dholbert]]

The important thing here doesn't seem to be the "display" change, but the modification of text's innerHTML.  If I remove this line from the jsfiddle...
>  this.text.innerHTML = message;
then the bug goes away.

Here's a sample jsfiddle with that change, which doesn't exhibit the bug:
 http://jsfiddle.net/jk7tkzhn/2/

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that supporting CSS transitions for style changes simultaneous with reframes (bug 625289) is something we do (so comment 3 is expected to work), but it landed *after* bug 783409 (enabling display:flex).

Comment 7

[Daniel Holbert [:dholbert]]

OK, so:
 (1) this.text (the node whose innerHTML is set in comment 5) has "display:flex", so any raw text inside of it will get wrapped in an anonymous flex item. Creating/removing anonymous flex items make us reframe the flex container. So, the box for .text gets removed & recreated.
 (2) .text's own frame has an anonymous sibling (which holds the placeholder for the abspos .close element), and to make sure we handle *that* anonymous flex item correctly [e.g. merging it with earlier ones if appropriate], we reframe .text's parent when .text is removed.

So, we do end up reframing .notification here, as a result of the text being added inside of .text.

Not sure why this is breaking transitions given comment 6, though.

mitsugami: If you're looking for a workaround for this issue, here's an easy one: just give your .text node an *explicit* block-level child-box, and put the notification-text *inside of that child*.  (Then, we won't have to create/remove an anonymous flex item, which is what's triggering box-tree cleanup & breaking stuff here.)  Here's a version of your original jsfiddle, with only that changed (adding a <div> and putting the text inside of that <div>):  http://jsfiddle.net/jk7tkzhn/3/

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: reporter's testcase

Testcase from http://fiddle.jshell.net/jk7tkzhn/show/ with the extra script and style sheets from jsFiddle removed.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (reduced)

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to David Baron [:dbaron] ⌚UTC-7 from comment #6)
> Note that supporting CSS transitions for style changes simultaneous with
> reframes (bug 625289) is something we do

Maybe the problem here is that the reframe is a bit delayed from the style change [it happens in nsCSSFrameConstructor, not in e.g. RestyleManager], so it's not "simultaneous" with the transition-causing style change?  The comment in bug 625289 patch 1 seems relevant. ("We could also have problems with triggering of CSS transitions/ on elements whose frames are reconstructed, since we depend on the reconstruction happening synchronously.")

dbaron, am I interpreting that comment correctly, & do you agree that this seems like the likely cause?  The reframing in this case happens here, FWIW:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsCSSFrameConstructor.cpp?rev=49d87bbe0122&mark=9126-9126,9145-9146#9107

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bug 1110277 was intended to fix that class of problems, but it's possible that the code you point to is going through different codepaths.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

One possibility where we could do the wrong thing is if we have a frame reconstruction that goes through lazy construction twice, the first of which goes through CreateNeededFrames, and the second of which goes through PostRestyleEvent.

It's pretty annoying that we have two separate mechanisms for posting lazy frame reconstruction, since that leaves us without a clean way to flush all of it out.