User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0 Build ID: 20141126041045 Steps to reproduce: I created a responsive (using percentages) and positioned (relative/absolute) box with HTML and CSS. One box should be able to start 4 transitions on hover, by setting the width of the 4 lines to 0(%) (to make them invisible) but on hover they should be enlarged to 100%. To make this effect possible without creating for each line an element I used CSS selectors (::before and ::after). For the reason that I want to display four lines I had to split the CSS selectors (twice) on two HTML elements. There's no real purpose of showing the 4 lines on hover, it's just an effect that uses CSS transitions. Actual results: Since Firefox 34(!) I have the problem: When "hovering" (:hover) only three CSS transitions start. The right transition doesn't start. In other browsers and before Firefox 34 all transitions are shown. http://jsbin.com/niwisijire/1/edit?html,css,output Expected results: The fourth transition should also start. If all transitions are displayed, a square frame should be displayed (inside the box).

[Tracking Requested - why for this release]: [Tracking Requested - why for this release]: [Tracking Requested - why for this release]: I see this problem in 34 and beta 35 and Aurora 36, but not in a current nightly. The fix range on nightlies is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=eb0d3b3c0b22&tochange=0cf461e62ce5 which includes fun things like bug 1087536 and bug 1073336 and 1087541 and a few others. The regression range is http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=daa84204a11a&tochange=dc352a7bf234 which includes bug 1057129. My money is on that causing the problem... David, can you take a look, please?

A much narrower (one day) fix range for nightlies is: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=6ce1b906c690&tochange=5ba06e4f49e8

Seems most likely to have been fixed by something in bug 907396.

Though it's worth noting that the "fix" in nightlies really just switches from one broken behavior to another: the forward transition now works on four sides, but the reverse transition then only works on two.

Actually, on nightly the forward transition doesn't happen on two of the sides either: it just immediately shows the post-transition value for me. Good catch!

The regression was indeed caused by https://hg.mozilla.org/mozilla-central/rev/2bf015380818 , though doing a relatively-obvious way of reverting that changeset on top of the current aurora tree doesn't fix it.

A few notes on the testcase: In nightly, it's the ::before and ::after of the div (inside the a) that are broken, and the ::before and ::after of the a that are fine. (Earlier, it was just the div::after.) If I change the selector ".boxcont" to "a" then things work. (I'm not sure why that extra element is there.) This made me suspect the problem might be nsLayoutUtils::Get{Before,After}Frame returning null, but that wasn't it. Really, though, we're not getting far into nsTransitionManager::ConsiderStartingTransition at all. (We might hit the beginning; that's a good bit harder to test.)

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #7) > If I change the selector ".boxcont" to "a" then things work. (I'm not sure > why that extra element is there.) This made me suspect the problem might be > nsLayoutUtils::Get{Before,After}Frame returning null, but that wasn't it. Also, if I add: a { display: block; width: 100%; height: 100% } then things work. This makes me think the underlying problem is related to block-in-inline splitting.

The problem also goes away if I change the :hover selectors to: .box:hover a::before, .box:hover a div::before .box:hover a::after, .box:hover a div::after

OK, there are two problems here: (1) ElementRestyler::MaybeReframeForAfterPseudo is posting a reframe. I believe this is erroneous, and related to the IB-split. The obvious fix (changing !aFrame->GetNextContinuation() to !nsLayoutUtils::GetNextContinuationOrIBSplitSibling(aFrame), and in MaybeReframeForBeforePseudo changing !aFrame->GetPrevContinuation() to nsLayoutUtils::IsFirstContinuationOrIBSplitSibling(aFrame)) didn't make the problem go away, but putting an early return in both functions did, so perhaps I did something silly. Or perhaps other issues with those functions need to be fixed. (The posting of this reframe should, I believe, suppress restyling of descendants.) (2) The reconstruction of frames resulting from that (erroneous, I believe) reframe happens after the ReframingStyleContexts object for the restyle has gone away (i.e., after the change list has been processed). I haven't looked into this problem yet, although I suspect lazy frame construction may be involved.

I have fixes for both problems, each of which independently fixes the bug, and also two related patches that I think are probably also a good idea, although I'm going to want more feedback (probably from :bz on patches 1 and 2). I haven't written an automated test yet, but here's a try run without one: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=782129b7eff2 https://tbpl.mozilla.org/?tree=Try&rev=782129b7eff2

(Also, at least on aurora, even with the patches, I still see some flashing of final state at the start of the transition that I should debug.)

For what it's worth, my current patch queue here is: https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/4d9801942296/consider-ib-siblings-before-reframe https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/4d9801942296/last-continuation-or-ib-split-method https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/4d9801942296/look-for-prop-on-first-continuation https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/4d9801942296/pass-lazy-construction-to-wipe-containing-block https://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/4d9801942296/pass-lazy-construction-to-reframe-containing-block

Try runs with the patches I'm about to upload for review: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c1870e1c467a https://tbpl.mozilla.org/?tree=Try&rev=c1870e1c467a I'm still hoping to write tests for patches 1, 3, 4, and 5, although: - writing tests for patches 1 and 3 requires adding a way to detect whether frame construction has happened; I think this is probably a good idea in general for regression-testing specific performance fixes - writing tests for patches 4 and 5 requires finding a specific codepath that I can exercise without triggering bug 1111451, or perhaps fixing bug 1111451 first. I can't promise to do this immediately, though; I have some other things on my plate as well. But I'm interested in your feedback on: - the above plan - whether you think any of the above needs to block landing the patches I'm about to post for review - your thoughts on uplifting the patches that are now ready (I'm inclined to want to uplift to both aurora and beta, given the problems that they fix) Note that if you don't have time to review all the patches, I'd prefer that your prioritize patches 1-3. I could transfer 4-7 to tn.

This patch is not needed to fix the bug, but it seems like it's probably desirable. It's not needed for this bug because MaybeReframeForBeforePseudo and MaybeReframeForAfterPseudo are already called (by ElementRestyler::RestyleChildren) on only the first and last continuation or ib-split sibling with the same style. So this patch should only actually change anything for cases like a block-in-inline split whose initial inline part is inside of a ::first-line (where diffeent parts of the block-in-inline split chain have different style). Since the symptom of this bug is only causing extra reframes, I don't see how to test for it once patches 4 through 6 are in the tree, without adding an API that reports reframes. I hope to do this in a followup bug. (In fact, I haven't actually tested this patch in any way at present, but I still hope to do so.)

This is used in patch 3.

The change to GetAfterFrameForContent prevents the reframe that is part of the chain of events leading to this bug, and thus fixes the bug on its own. The change to GetBeforeFrameForContent seems desirable for symmetry. Note that patch 5 also (on its own) fixes the reported bug. This probably needs somewhat careful review. We should examine: (1) what the rules for calling nsLayoutUtils::GetBeforeFrame and nsLayoutUtils::GetAfterFrame are, and whether both (or neither) need to be patched. (2) What the rules for which frame the GenConProperty() lives on, and whether we should adjust nsIFrame::GetGenConPseudos() to either do something more intelligent, or assert about callers. (We should probably clean up some of these things in a followup bug.) Since the symptom of this bug is only causing extra reframes, I don't see how to test for it once patches 4 through 6 are in the tree, without adding an API that reports reframes. I hope to do this in a followup bug.

This patch is not related to fixing the bug, but is similar to patch 5, which is, and also seems like a good idea.

Note that this presumes that lazy reconstruction is forbidden inside ContentRemoved, which matches the caller to RecreateFramesForContent already in ContentRemoved, but which is perhaps suboptimal and should be passed to ContentRemoved explicitly. (Then again, maybe it *should* always be false?) This patch alone fixes the bug by making transitions still start correctly when the reframe occurs. (We explicitly suppress lazy frame construction in RestyleManager::ProcessRestyledFrames; one of the reasons we do so is so that we can start transitions correctly on elements that are being reframed.) Note that patch 2 plus patch 3 also (on their own) fix the reported bug.

This is needed for the automated test that uses flexbox wrapping in patch 7, but is not related to fixing the originally-reported bug.

This tests a case fixed by patch 6 (which in turn depends on patch 5). In particular, I tested that: * with all of patches 1-6 applied, the two added tests pass * with patches 1-5 applied but patch 6 not applied, the two added tests fail, both reporting rgb(255, 255, 0) * with patches 1-4 *not* applied but patches 5-6 applied, the two added tests pass * with patches 1-6 *not* applied, the two added tests fail, both reporting rgb(255, 255, 0) Note that this test is structured in a rather particular way (with two separate restyles triggered by attributes on different elements, queued in a particular order) to avoid triggering bug 1111451.

Comment on attachment 8536328 [details] [diff] [review] patch 1 - Consider the ib-split chain when checking for ::before and ::after in order to reframe when they're missing > diffeent parts of the block-in-inline split chain have different style). "different" r=me

Comment on attachment 8536329 [details] [diff] [review] patch 2 - Add nsLayoutUtils::LastContinuationOrIBSplitSibling() r=me

Comment on attachment 8536330 [details] [diff] [review] patch 3 - Look for the GenConPseudos() property on the first continuation/ib-split so that we can find it when looking for the ::after frame > (2) What the rules for which frame the GenConProperty() lives on, and "What the rules are for which frame" As for the rules themselves, looking at the frame constructor code, this should always live on the content insertion frame of the first continuation of the first ib split part, because we do all that stuff before we chop up the child list for the ib split and before we've done any layout which would create continuations, and we reframe if gen con stuff is added/removed. Please add an assert to this effect (well, at least about being the first continuation of the first ib split part) in AddGenConPseudoToFrame. And perhaps we should add an assert to this effect in GetGenConPseudos as well? Probably by debug-only calling some non-inline nsIFrame function that does the assert. As far as the rules for GetBeforeFrame/GetAfterFrame, here's what I see callers doing: 1) ChildIterator calls these on the primary frame, which should be the first continuation of the first ib split. 2) nsCSSFrameConstructor::FindFrameForContentSibling ... I _think_ is passing in the primary frame for the insertion point. 3) ElementRestyler::MaybeReframeForBefore/AfterPseudo is doing whatever it's doing, and is what you're primarily fixing here. 4) nsIFrame::GetPseudoElement is called only from nsComputedDOMStyle::GetStyleContextForElementNoFlush and only on the return value of nsLayoutUtils::GetStyleFrame. Which is the primary frame except for tables for which it's the table inner of the primary frame. Since the inner is the content insertion parent of the outer this is fine. 5) AnimationPlayerCollection::GetElementToRestyle is working with the primary frame, so should be ok. Pinning this down to a rule is a bit hard because of the table weirdness, but except for the ElementRestyler cases all the callers have a frame that is the first continuation of the first ib split, so we could make that a rule (enforced by an assert) and get the right frame in ElementRestyler, right? That would incidentally make it clear that there is nontrivial running-around-the-frame-tree cost to these calls. In particular, it's not obvious to me that "Checking for an ::after frame is cheaper than getting the ::after style context" as the ElementRestyler comments say if we have to walk the whole continuation chain... In any case, I'm ok with this change as-is for now as the safe thing to do, and a possible followup to figure out exactly what API invariants we want here. But I'd prefer we stashed the old value of aFrame on the stack in GetAfterFrameForContent so the walk to the last continuation in the "If the last child frame is a pseudo-frame" case doesn't have to walk as far if we started off near the last continuation. r=me

Comment on attachment 8536331 [details] [diff] [review] patch 4 - Pass the aAllowLazyConstruction parameter through to WipeContainingBlock so that we don't do lazy reconstruction when it is forbidden Hmm. So initially the thinking was that we theoretically wanted to always allow lazy construction but some parent frame situations could not deal so we had this boolean. But for WipeContainingBlock we know we're not in any of those cases, hence it always allowing lazy construction. I guess at this point we're using this boolean to mean other things as well? But do those cases apply here? I'm not a huge fan of this change if it's not actually necessary.

Comment on attachment 8536332 [details] [diff] [review] patch 5 - Pass the aAllowLazyConstruction parameter through to ReframeContainingBlock so that we don't do lazy reconstruction when it is forbidden > Note that this presumes that lazy reconstruction is forbidden inside > ContentRemoved It's not obvious to me why it should be offhand, unless this is one of those "other things" the boolean is supposed to mean. In which case we really need to get some documentation on it. I have no idea why the existing RecreateFramesForContent calls in ContentRemoved disallow async construction, but if there is an actual reason we should clearly document it somewhere. Further, I'd really like us to move away from requiring this, since this is the sort of thing that can easily make "remove some elements from the DOM" be O(N*M) where N is the number of elements to remove and M is the size of the entire DOM. I'm OK with taking this patch for now, but could you file a followup to figure out what's actually going on here and how we can avoid these sync frame constructions? Past that, I think the RecreateFramesForContent call in the fieldset/legend case in MaybeRecreateContainerForFrameRemoval should probably pass through the boolean, not just false. Followup bug is probably good for this bit just in case, since we're backporting the patches here. We should also pass through the new boolean to the RecreateFramesForContent call in the IsTablePseudo cases in MaybeRecreateContainerForFrameRemoval, right? And the IsAnonymousFlexOrGridItem cases, and the bidi case. Those all pass unconditional "true"right now. Again, a followup is ok here. And maybe we can find tests that fail due to those "true"s? r=me, but this patch really worries me in perf terms. :(

> We should also pass through the new boolean Oh, I see, that's part 6. ;)

Comment on attachment 8536333 [details] [diff] [review] patch 6 - Pass aAllowLazyConstruction parameter from nsCSSFrameConstructor::MaybeRecreateContainerForFrameRemoval to nsCSSFrameConstructor::RecreateFramesForContent r=me

Comment on attachment 8536334 [details] [diff] [review] patch 8 - Add test for passing lazy construction parameters through, using flexbox reframe case r=me

In terms of what we should land and where.... I think we should spin off patch 4 into a separate bug and figure out what the deal is there and whether we want that patch or something different. I'm ok with landing the other patches with the test you already have and backporting them to Aurora and Beta, with the followups I asked for filed.

(In reply to Vacation Dec 15-31 from comment #25) > Comment on attachment 8536331 [details] [diff] [review] > patch 4 - Pass the aAllowLazyConstruction parameter through to > WipeContainingBlock so that we don't do lazy reconstruction when it is > forbidden > > Hmm. > > So initially the thinking was that we theoretically wanted to always allow > lazy construction but some parent frame situations could not deal so we had > this boolean. But for WipeContainingBlock we know we're not in any of those > cases, hence it always allowing lazy construction. > > I guess at this point we're using this boolean to mean other things as well? > But do those cases apply here? > > I'm not a huge fan of this change if it's not actually necessary. bug 625289 depends on frame reconstruction during restyles not happening lazily in order to start CSS transitions correctly. That's the rationale behind patches 4, 5, and 6. It's not needed in the sense that neither of the two testcases I have need it, but I'm reasonably confident that it is needed in some cases. (Maybe I should find a way to restructure bug 625289, although I think there should be a limit to how long something can be without a frame and we'll still be willing to start a CSS transition for it.)

Hmm. Ok, so for the specific case of reconstruction inside restyles I agree we want the eager behavior. Maybe we should make the boolean into a tristate, with a value that means "construct yourself eagerly, but it's OK to do your ancestors lazily"? Or am I just overthinking it?

The original motivation for the booleans wasn't clear to me; if part of the original motivation didn't apply to when the parent needed to be reconstructed, then that certainly makes sense.

tn, do you recall the original motivation for the boolean, and why it didn't need to apply to cases where the parent was reconstructed?

In particular, we're talking about the changes in patch 4 through patch 6, which fix comment 10 item (2).

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #34) > tn, do you recall the original motivation for the boolean, and why it didn't > need to apply to cases where the parent was reconstructed? I think technically it probably should have applied to parents that needed to be reconstructed but when I realized the problem I decided that letting those reconstructions be processed with the other restyles was fine because we always processed restyles after (lazily) constructing frames. I don't think I considered the case where frames would need to be constructed before processing restyles.

Regression, tracking.

But do you remember what the original case that required non-lazy reconstruction was?

Patch 1 and patch 3 will change the todo status of the first and second tests, respectively, since they are what fix the tests.

There were definitely some editor cases that needed it at the time because they expected the frame for the node it was editing to appear sync.

A few other thoughts: (1) The reason I tend to think ContentRemoved should be sync is that I'm not crazy about having frames in the frame tree for content that's no longer in the document tree. But for that purpose, I'm fine with the removal being sync, and having lazy reconstruction, which addresses the performance issue. (2) It occurs to me that patches (4) - (6) could probably be replaced with a call to CreateNeededFrames in the places that create a ReframingStyleContexts object, which would allow the coalescing that deals with many of the performance problems, especially if we were to move the ReframingStyleContexts object out further. (3) In general, it seems like the approach in thought (2) is better: it's better to flush buffers when they need to be flushed than to disable all of the intermediate coalescing. So perhaps I should replace patches 4-6 along those lines?

(In reply to David Baron [:dbaron] (UTC-5) (needinfo? for questions) from comment #41) > (1) The reason I tend to think ContentRemoved should be sync is that I'm > not crazy about having frames in the frame tree for content that's no longer > in the document tree. But for that purpose, I'm fine with the removal being > sync, and having lazy reconstruction, which addresses the performance issue. Er, we already do that just fine; destruction is always synchronous.

Comment 41 thought (2) sounds great to me.

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=261a9f987512 https://tbpl.mozilla.org/?tree=Try&rev=261a9f987512

Actually, what the try run shows is that I'm going to need to do bug 1115812 first.

Try run: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9649637cdf9a https://tbpl.mozilla.org/?tree=Try&rev=9649637cdf9a https://hg.mozilla.org/try/pushloghtml?changeset=9649637cdf9a

With one more null-check, a combined try run for this and bug 1115812: https://hg.mozilla.org/try/pushloghtml?changeset=e2a4013cb469 https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=e2a4013cb469 https://tbpl.mozilla.org/?tree=Try&rev=e2a4013cb469

This is needed for patch 7.

This is just a little cleanup that follows from patch 4.

This makes the ReframingStyleContexts live across the lifetime of the processing of a full queue of posted restyles. This depends on bug 1115812 to behave sensibly when rebuilding the rule tree (RebuildAllStyleData, etc.). This handles the form of lazy frame construction that is done in nsCSSFrameConstructor::RecreateFramesForContent, which posts a restyle. Patch 7 handles any use of the lazy frame construction mechanism. This patch (with patches 4 and 5 under it, but without patches 1-3) fixes the original testcase in bug 1110277, except for some flashing of the final position as the transition starts. Also fixes bug 1111451.

I don't have any tests that exercise this code, and I can't even find a codepath that demonstrates that it's needed, since the lazy reconstruction that happens during style-triggered frame reconstruction all appears to go through PostRestyleEvent rather than MaybeConstructLazily. But I think we should either do this or add an assertion that it's not needed, and given that it's one line, it seems like we may as well just do it. (Note also that we're currently calling CreateNeededFrames at the start of style reresolution, in RestyleManager::ProcessPendingRestyles; this adds a call at the end.)

We're definitely not taking on this amount of change in our RC candidate on Monday, so this is a wontfix for 35.

Comment on attachment 8542442 [details] [diff] [review] patch 6 - Make the lifetime of the ReframingStyleContexts object longer Review of attachment 8542442 [details] [diff] [review]: ----------------------------------------------------------------- David would you be able to give me a -w version of the diff? (I couldn't apply the patch locally to generate one myself.) ::: layout/base/RestyleTracker.cpp @@ +201,5 @@ > + // BeginProcessingRestyles, which might (if mDoRebuildAllStyleData is > + // true) do substantial amounts of restyle processing, but it needs > + // to be *out* of scope during EndProcessingRestyles, since we should > + // release the style contexts it holds prior to any EndReconstruct > + // call that EndProcessingRestyles makes. Can you explain for my benefit (and perhaps in the comment) why it is that the ReframingStyleContexts style contexts need to be released before the EndReconstruct call? @@ +248,5 @@ > + for (uint32_t i = 0; i < laterSiblingArr.Length(); ++i) { > + Element* element = laterSiblingArr[i]; > + NS_ASSERTION(element->HasFlag(RestyleBit()), "How did that happen?"); > + RestyleData* data; > + #ifdef DEBUG Leave the preprocessor directives in column 1 (and later in the patch too).

generated using "interdiff -w -U3 /dev/null rsc-bigger-scope"

(In reply to Cameron McCormack (:heycam) from comment #53) > Can you explain for my benefit (and perhaps in the comment) why it is that > the ReframingStyleContexts style contexts need to be released before the > EndReconstruct call? EndReconstruct is when we try to destroy the old rule tree using the GC mechanism, which means it only gets destroyed if it's unreferenced. (We assert if it's referenced.) So we want the ReframingStyleContexts (which holds old style contexts) to be destroyed before the EndReconstruct so those style contexts go away first. I could add some or all of that to the comment.

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #55) > I could add some or all of that to the comment. OK, please do.

Comment on attachment 8542442 [details] [diff] [review] patch 6 - Make the lifetime of the ReframingStyleContexts object longer r=me with those previous comments addressed.

Comment on attachment 8542443 [details] [diff] [review] patch 7 - Call CreateNeededFrames from ~ReframingStyleContexts r=me

Landed patch 0 through patch 3; patches 4-7 are till waiting on dependencies (bug 1115812). remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/ca02a661d6dc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/86088b6c69e8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fbbafc2a9573 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1bbebe9fec17 This landing fixes the testcase here by removing the reframe, but doesn't fix the general problem.

I had to revert the assertion in GetGenConPseudos() due to orange (but not the other assertion on setting): https://hg.mozilla.org/integration/mozilla-inbound/rev/98e34be1fb44

The patches in comment 59 and comment 60 did end up on mozilla-central a number of hours ago (in time for the merge to Aurora 37, I believe).

> I had to revert the assertion in GetGenConPseudos() Can you please file a followup bug on Mats to figure out what the story there is and whether this caller is just buggy?

Filed bug 1121201.

https://hg.mozilla.org/try/pushloghtml?changeset=83d5de58d42a https://treeherder.mozilla.org/#/jobs?repo=try&revision=83d5de58d42a https://tbpl.mozilla.org/?tree=Try&rev=83d5de58d42a

https://hg.mozilla.org/integration/mozilla-inbound/rev/a616039ccd7b https://hg.mozilla.org/integration/mozilla-inbound/rev/b232139eb2c7 https://hg.mozilla.org/integration/mozilla-inbound/rev/30666c55966e https://hg.mozilla.org/integration/mozilla-inbound/rev/2a1f0e8d1fc9 https://hg.mozilla.org/integration/mozilla-inbound/rev/4182615c7586

https://hg.mozilla.org/mozilla-central/rev/a616039ccd7b https://hg.mozilla.org/mozilla-central/rev/b232139eb2c7 https://hg.mozilla.org/mozilla-central/rev/30666c55966e https://hg.mozilla.org/mozilla-central/rev/2a1f0e8d1fc9 https://hg.mozilla.org/mozilla-central/rev/4182615c7586

Need to figure out what, if anything, to uplift.

David, do you have an idea when you are going to make a call on this? Thanks

Comment on attachment 8536329 [details] [diff] [review] patch 2 - Add nsLayoutUtils::LastContinuationOrIBSplitSibling() Approval Request Comment for patch 2 and patch 3 (I'd also land patch 0, which is test-only): [Feature/regressing bug #]: bug 1057129 [User impact if declined]: incorrect CSS transition behavior in a relatively obscure case involving both block-inside-inline splitting and ::before/::after. This patch doesn't actually fix the underlying problem (that's what patches 4-7 do, but they depend on some substantial other work and probably aren't suitable for uplift), but it fixes the testcase by avoiding frame reconstruction in a case where we shouldn't be reconstructing frames. So this patch fixes the site/test on which the problem was discovered, in so far as that's an important thing to do. [Describe test coverage new/current, TreeHerder]: mochitests that test that this patch fixes what it's supposed to be fixing; also been in the tree for a few weeks [Risks and why]: I think the change is reasonably safe, although I'm actually pretty neutral on whether to uplift this to beta or not. It's not the lowest-risk area of code (though not the highest either), and there's no known live site that's broken. [String/UUID change made/needed]: no Note that patches 0-3 are already on mozilla-aurora; patches 4-8 are only on mozilla-central and not on mozilla-aurora.

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #69) > [Risks and why]: I think the change is reasonably safe, although I'm > actually pretty neutral on whether to uplift this to beta or not. It's not > the lowest-risk area of code (though not the highest either), and there's no > known live site that's broken. Ok. Therefor, if you don't mind, I am going to reject this uplift for beta. The reasons are that we only have a few more beta (go to build of beta 6 Monday), we already shipped two releases with this bug.

(In reply to David Baron [:dbaron] (UTC-8) (needinfo? for questions) from comment #69) > Note that patches 0-3 are already on mozilla-aurora; patches 4-8 are only on > mozilla-central and not on mozilla-aurora. This bug is still marked as 37 affected. Do any of patches 4-8 need to be uplifted to Aurora 37 or are we good for this release?

The underlying bug is present on aurora, but given that we've shipped it for a few releases already, and given the size of the dependency chain (mostly bug 1115812), I'm inclined not to uplift the fix.

Thanks David. I'm fine with shipping the fix in 38 to reduce risk for 37. I have marked 37 as wontfix.

Added a note in the doc: https://developer.mozilla.org/en-US/Firefox/Releases/38#CSS