Closed Bug 625289 Opened 13 years ago Closed 10 years ago

CSS transitions don't start due to frame reconstruction of ancestor or self that's "simultaneous" with the change that would start the transition

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla34
Tracking Status
blocking2.0 --- -
relnote-firefox --- 34+

People

(Reporter: robarnold, Assigned: dbaron)

References

Details

(Keywords: dev-doc-complete, testcase)

Attachments

(13 files)

2.76 KB, text/html
Details
1.50 KB, patch
heycam
: review+
Details | Diff | Splinter Review
5.20 KB, patch
heycam
: review+
Details | Diff | Splinter Review
2.23 KB, patch
heycam
: review+
Details | Diff | Splinter Review
3.26 KB, patch
heycam
: review+
Details | Diff | Splinter Review
1.46 KB, patch
heycam
: review+
Details | Diff | Splinter Review
2.40 KB, patch
heycam
: review+
Details | Diff | Splinter Review
3.66 KB, patch
heycam
: review+
Details | Diff | Splinter Review
2.24 KB, patch
heycam
: review+
Details | Diff | Splinter Review
2.49 KB, patch
heycam
: review+
Details | Diff | Splinter Review
3.24 KB, patch
heycam
: review+
Details | Diff | Splinter Review
4.00 KB, patch
heycam
: review+
Details | Diff | Splinter Review
6.11 KB, patch
heycam
: review+
Details | Diff | Splinter Review
See attached testcase. Transitions don't appear to skip in Chrome 10 but do sometimes in Firefox 4.0b10pre.
blocking2.0: --- → ?
Assignee: nobody → dbaron
Component: Layout → Style System (CSS)
OS: Windows 7 → All
QA Contact: layout → style-system
Hardware: x86 → All
I suspect this is likely a bug in interpolation of transforms (where some cases fail to interpolate), but I haven't debugged it yet.
Er, no, I think it's a bug on simultaneously starting transforms on elements nested inside each other.
blocking2.0: ? → betaN+
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/482b08600e0a/correct-covering fixes it, although I don't completely understand why yet.  (I'm pretty confident in the patch; I'm just not sure why it's this important for a non-inherited property.)
Actually, I'm not confident in the patch; I think it's wrong.
I guess the problem is the "if we're reframing, let the frame keep its old style context and don't reresolve the kids" code in nsFrameManager::ReResolveStyleContext.
blocking2.0: betaN+ → ?
I'm not sure it's realistic to fix this for Firefox 4.  The basic problem is that switching between "no transform" and "have a transform" requires a reframe, since it changes some invariants of the frame tree structure, like where fixed-positioned frames go.  (I wish it didn't, but that's what the spec says.)

So if you're going to be dynamically switching between no-transform and scale(2), you're a lot better off switching between no-transform expressed as the relevant identity, e.g., scale(1) and scale(2), translateX(0px) and translateX(100px), etc., rather than using 'none'.
Summary: Sometimes transitions don't play → Sometimes transitions don't play due to frame reconstruction of ancestor
Blocks: 764012
Blocks: 788677
Blocks: 800927
Blocks: 821976
Is this still an issue?  I am trying the test case on Mac OS 10.7.5, but I do not see any difference in Firefox 19, Safari 6.0.2, or Chrome 25.0.1364.152.
I wasn't able to reproduce it on a recent build though my machine (and probably Firefox) is much faster now than it was when I reported the bug two years ago.
Yep, it's still prsent in v. 21
example: http://jsfiddle.net/chodorowicz/bc2YC/5/
It's still in v. 22 either
http://jsfiddle.net/jCfwc/
I just want to confirm this is an issue ( see the http://jsfiddle.net/jCfwc/ above ) in FF 23.0.1 MAC.
Also, I think this should be a relatively "higher" priority issue because this isn't just about the animation visually not working ( which i admit might not be a big deal ).  But instead, when this happens the animation event "transitionend" doesn't fire when the animation is skipped.

So actually, writing any sort of logic with the events fail, which I think warrants an higher priority. Its NOT just a presentation thing.
Would changing overflow cause a frame reconstruction? I maybe have just encountered this bug using overflow instead of position. In webkit the menu tray slides out from the left as expected: http://jsfiddle.net/xGZcz/
> Would changing overflow cause a frame reconstruction?

Yes.
Blocks: 915168
Summary: Sometimes transitions don't play due to frame reconstruction of ancestor → CSS transitions don't start due to frame reconstruction of ancestor or self that's "simultaneous" with the change that would start the transition
Test-case http://jsbin.com/eZOsEja/3/edit from #923538.
Problem still exists in 25.0.1.

Here is a simple example with the problem:
http://jsfiddle.net/FJJF6/1/

1. outer div changed to position fixed
and the same time
2. yellow box should fade in ... but it did not fade.
Wasted hours tracking this one down, yesterday.

Here's yet another test case: http://jsbin.com/UwaXOBix/3/edit
Confirmed, another test case: http://codepen.io/anon/pen/Dglmi
Stumbling more and more often over this quite annoying bug in recent projects. 
Here is another really small test case
http://jsfiddle.net/dPg4w/1/
Here's another use case (functionality) that doesn't work in Firefox (sad face). http://jsfiddle.net/langbert/mDcQd/ I really hope this bug can move along. It's making transition so very problematic.
By the way, here's my hackish workaround: http://jsfiddle.net/langbert/mDcQd/2/ Note that in the previous version, there was no transitionend event firing, rendering the whole screen unclickable.
Bumping this - affects the ability of developers working with Firefox OS to create compelling user interactions. 

Two designer-developers have asked me personally about this bug this week. That.. is pretty unusual for any bug, so I tend to take that as a sign that there's a sincere need beyond my own observations.

Cheers & thanks in advance with much gratitude on this one.
This is on my list of things that are important to work on.
Blocks: 1011153
I updated Robert's fiddle with a solution:
http://jsfiddle.net/mDcQd/8/

The key is to have position defined on the container with overflow: hidden.

He had it set once the class was added like so:

.siteNavOpen body {
    overflow: hidden;
}

instead, body needs to have it set beforehand

body {
    overflow: hidden;
    position: relative;
}
However, this prevents the window scrollbar, so I use a JavaScript function to apply overflow: hidden just before the transitions are triggered.

$('body').addClass('menu__open');
setTimeout(function(){
    $(menu).addClass('menu--open');
}, 25);
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla34
This isn't actually a problem for this patch series because the root
element can't have an ancestor that's reframed because it has no
ancestors, and reframes of the element itself trigger a restyling
operation that does actually start transitions.
Attachment #8465890 - Flags: review?(cam)
Without the patch, most of the tests fail (although the e1 test and the
two root tests pass).  With the patch, all tests pass.

I presume the three tests that pass prior to the patch pass because the
restyle actually occurs and starts the transition (which is stored on
the element) before the style context is destroyed.  (The same should, I
think, happen for a style context coming out of the undisplayed map.)
Attachment #8465894 - Flags: review?(cam)
Attachment #8465883 - Flags: review?(cam) → review+
Comment on attachment 8465884 [details] [diff] [review]
patch 2 - Create wrapper function around the only calling pattern of ComputeStyleChangeFor

Review of attachment 8465884 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/RestyleManager.h
@@ +100,5 @@
> +  void ComputeAndProcessStyleChange(nsIFrame* aFrame,
> +                                    nsChangeHint aMinChange,
> +                                    RestyleTracker& aRestyleTracker,
> +                                    nsRestyleHint aRestyleHint);
> +

Let's make this and ComputeStyleChangeFor private, since they're not used outside RestyleManager.
Attachment #8465884 - Flags: review?(cam) → review+
Attachment #8465885 - Flags: review?(cam) → review+
Attachment #8465886 - Flags: review?(cam) → review+
Attachment #8465887 - Flags: review?(cam) → review+
Comment on attachment 8465888 [details] [diff] [review]
patch 6 - Store style contexts being reframed in the ReframingStyleContexts struct

Review of attachment 8465888 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these three things.

::: layout/generic/nsFrame.cpp
@@ +651,5 @@
>      }
>    }
>  
>    // This needs to happen before shell->NotifyDestroyingFrame because that
>    // clears our Properties() table.

Should this comment be moved to just above the TransferActivityToContent call?  I think that's what it's referring to.

@@ +653,5 @@
>  
>    // This needs to happen before shell->NotifyDestroyingFrame because that
>    // clears our Properties() table.
>    bool isPrimaryFrame = (mContent && mContent->GetPrimaryFrame() == this);
> +  // FIXME: Check that this works for ::before/::after.

Can you remove this FIXME given the tests in the final patch?

@@ +658,5 @@
>    if (isPrimaryFrame) {
>      ActiveLayerTracker::TransferActivityToContent(this, mContent);
> +
> +    // Unfortunately, we need to do this for all frames being reframed
> +    // and not only those whose currently style involves CSS

s/currently/current/
Attachment #8465888 - Flags: review?(cam) → review+
Comment on attachment 8465889 [details] [diff] [review]
patch 7 - Expose TryStartingTransition

Review of attachment 8465889 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/RestyleManager.h
@@ +166,5 @@
> +   */
> +  static void
> +  TryStartingTransition(nsPresContext *aPresContext, nsIContent *aContent,
> +                        nsStyleContext *aOldStyleContext,
> +                        nsRefPtr<nsStyleContext> *aNewStyleContext /* inout */);

Please put the "*"s next to the types, and in the .cpp file too.
Attachment #8465889 - Flags: review?(cam) → review+
Comment on attachment 8465890 [details] [diff] [review]
patch 8 - Add FIXME comments suggesting additional use of ResolveStyleContext when resolving the root frame

Review of attachment 8465890 [details] [diff] [review]:
-----------------------------------------------------------------

Is the issue then that the new style context might not have the right parent (as that seems to be what nsCSSFrameConstructor::ResolveStyleContext does)?
Attachment #8465891 - Flags: review?(cam) → review+
Comment on attachment 8465892 [details] [diff] [review]
patch 10 - Check for difference in HasPseudoElementData before starting transitions

Review of attachment 8465892 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/nsTransitionManager.cpp
@@ +146,5 @@
> +    // bail.  We can't hit this codepath for normal style changes
> +    // involving moving frames around the boundaries of these
> +    // pseudo-elements since we don't call StyleContextChanged from
> +    // ReparentStyleContext.  However, we can hit this codepath during
> +    // the handling of transitions that start across reframes.

Can you give me a short example demonstrating when we do get in here?  If it's short enough maybe stick it in the comment here.
Comment on attachment 8465893 [details] [diff] [review]
patch 11 - Call RestyleManager::TryStartingTransition during frame construction

Review of attachment 8465893 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSFrameConstructor.cpp
@@ +1821,5 @@
> +    RestyleManager()->GetReframingStyleContexts();
> +  if (rsc) {
> +    nsStyleContext* oldStyleContext =
> +      (isBefore ? rsc->mBeforePseudoContexts
> +                : rsc->mAfterPseudoContexts).GetWeak(aParentContent);

May be slightly more readable with something like:

  nsStyleContext* oldStyleContext = rsc->Get(aParentContent, aPseudoElement);

and an appropriate definition of ReframingStyleContexts::Get that looks up the right table and asserts the pseudo is expected.  Up to you.

@@ +4783,5 @@
>  
> +  RestyleManager::ReframingStyleContexts* rsc =
> +    RestyleManager()->GetReframingStyleContexts();
> +  if (rsc) {
> +    nsStyleContext* oldStyleContext = rsc->mElementContexts.GetWeak(aContent);

And similarly here with a single argument version of Get().
Attachment #8465893 - Flags: review?(cam) → review+
Comment on attachment 8465888 [details] [diff] [review]
patch 6 - Store style contexts being reframed in the ReframingStyleContexts struct

If you add the ReframingStyleContexts::Get method in patch 11, you should use a Put method rather than switching on the pseudo type here.
Comment on attachment 8465894 [details] [diff] [review]
patch 12 - Tests for transitions on elements that are reframing

Review of attachment 8465894 [details] [diff] [review]:
-----------------------------------------------------------------

Worth doing a dynamic change test on ::before/::after too?  And a test for an element that is changing from display:none to something else?
Attachment #8465894 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #61)
> Is the issue then that the new style context might not have the right parent
> (as that seems to be what nsCSSFrameConstructor::ResolveStyleContext does)?

The issue is that I'm hooking in to ResolveStyleContext in later patches, and in particular I need that hook to be called for all regular element style context resolution except for the root.  So I happen to be fine, but it seems like it's odd for the root to be the only regular element style context for a frame to be constructed not resolved through ResolveStyleContext.

(In reply to Cameron McCormack (:heycam) from comment #62)
> Comment on attachment 8465892 [details] [diff] [review]
> patch 10 - Check for difference in HasPseudoElementData before starting
> transitions
> ::: layout/style/nsTransitionManager.cpp
> @@ +146,5 @@
> > +    // bail.  We can't hit this codepath for normal style changes
> > +    // involving moving frames around the boundaries of these
> > +    // pseudo-elements since we don't call StyleContextChanged from
> > +    // ReparentStyleContext.  However, we can hit this codepath during
> > +    // the handling of transitions that start across reframes.
> 
> Can you give me a short example demonstrating when we do get in here?  If
> it's short enough maybe stick it in the comment here.

An example would be restyling something like:

<!DOCTYPE html>
<style>
div::first-line { color: green }
:link, :visited { color: inherit; transition: color ease 300ms }
</style>
<div>This paragraph has <a href="https://mozilla.org/">a link to Mozilla</a> in it.

with a restyle that:
  1. has the link *not* in the first line prior to the restyle (since I *think* that during frame construction we assume everything is in the first line and then fix it up during layout)
  2. reframes the div or one of its ancestors

This wasn't an issue before since restyling that doesn't involve reframes doesn't change whether anything matches ::first-letter or ::first-line.
Flags: needinfo?(cam)
Attachment #8465890 - Flags: review?(cam) → review+
Flags: needinfo?(cam)
Comment on attachment 8465892 [details] [diff] [review]
patch 10 - Check for difference in HasPseudoElementData before starting transitions

(In reply to David Baron [:dbaron] (UTC-7) (needinfo? for questions) from comment #66)
> An example would be restyling something like:

I guess that might be getting a bit long for a comment.  If you can easily turn this into a test, please do so.
Attachment #8465892 - Flags: review?(cam) → review+
(In reply to Cameron McCormack (:heycam) from comment #65)
> Worth doing a dynamic change test on ::before/::after too?  And a test for
> an element that is changing from display:none to something else?

I've added such tests (and factored the test code into a function). Note that the changing from display:none cases fail, but that's a separate bug.
(In reply to Cameron McCormack (:heycam) from comment #67)
> I guess that might be getting a bit long for a comment.  If you can easily
> turn this into a test, please do so.

I added a patch 13 with a test for that case.
comment 71 shows that this has landed in central.  This means that :

 (1) this bug is fixed in today's nightly builds of Firefox.  (2014-08-14 builds)  See https://nightly.mozilla.org/ for getting nightly builds.

 (2) unless something you see news to the contrary in this bug, this should be fixed in Firefox 34.

Testing that the problems that led you here are now fixed in nightly is appreciated; it's much more useful to have that feedback now than after the fix ships.  That said, if there are other related issues that are still unresolved, those should be filed as separate bugs (if they're not filed already), though it might be worth noting them here.

Note that one related issue is bug 537143, on not supporting transitions in display:none subtrees.
It seems to be resolved in current nightly for me. Thanks!
Hi,

don't know if it can help, I've made a test case while searching a solution for this bug: http://www.nicolas-hoffmann.net/bordel/divers/bug-transition-firefox/index.html

Anyway, some people confirm that it is fixed on last versions fo Firefox. (and buggy on Fx32)
Just came across this issue while deving (FF 32 stable) & can confirm it is indeed fixed for me in the current nightlies.
Release Note Request (optional, but appreciated)
[Why is this notable]: Fixed an often requested issue with CSS transitions
[Suggested wording]: I don't know the best way to describe this change
[Links (documentation, blog post, etc)]:

+Yoshino for Site Compatibility
Status: RESOLVED → VERIFIED
relnote-firefox: --- → ?
Added in the release notes with "Fix glitches in CSS transitions" as wording.
(In reply to Sylvestre Ledru [:sylvestre] from comment #82)
> Added in the release notes with "Fix glitches in CSS transitions" as wording.

I think that's overly broad; could you either reword as "Fixed starting of CSS transitions that start together with changes to display, position, overflow, and similar properties" or drop from the release notes?

It does definitely belong in Firefox 34 for developers, though, so adding dev-doc-needed.  I have no idea why it belongs in the release notes, whose developer notes seem like a randomly-selected subset of Firefox 34 for developers.
Flags: needinfo?(sledru)
Keywords: dev-doc-needed
> I think that's overly broad; could you either reword as "Fixed starting of
> CSS transitions that start together with changes to display, position,
> overflow, and similar properties" or drop from the release notes?
Updated! Thanks.

> It does definitely belong in Firefox 34 for developers, though, so adding
> dev-doc-needed.  I have no idea why it belongs in the release notes, whose
> developer notes seem like a randomly-selected subset of Firefox 34 for
> developers.
We try to highlight the items which seems the most important. However, this is subjective and it is why we ask for review on the release notes for each release.
Flags: needinfo?(sledru)
Used the following for the release note "CSS transitions start correctly when started at the same time as changes to display, position, overflow, and similar properties"
This bug is still not fully fixed.

the link in https://bugzilla.mozilla.org/show_bug.cgi?id=625289#c32 demonstrates it well: http://jsfiddle.net/langbert/mDcQd/

if you set an overflow:hidden either on the body or the html element, the transitions doesn't work.
(In reply to laszlo from comment #86)
> This bug is still not fully fixed.
> 
> the link in https://bugzilla.mozilla.org/show_bug.cgi?id=625289#c32
> demonstrates it well: http://jsfiddle.net/langbert/mDcQd/
> 
> if you set an overflow:hidden either on the body or the html element, the
> transitions doesn't work.

Could you please file a new bug about that? And mention the bug number here? Thanks!
Flags: needinfo?(laszlo)
(In reply to Martijn Wargers [:mwargers] (QA) from comment #87)
> (In reply to laszlo from comment #86)
> > This bug is still not fully fixed.
> > 
> > the link in https://bugzilla.mozilla.org/show_bug.cgi?id=625289#c32
> > demonstrates it well: http://jsfiddle.net/langbert/mDcQd/
> > 
> > if you set an overflow:hidden either on the body or the html element, the
> > transitions doesn't work.
> 
> Could you please file a new bug about that? And mention the bug number here?
> Thanks!

Sorry, I wasn't noticing that it'll be in FF34. Downloaded nightly, works there.
Flags: needinfo?(laszlo)
You need to log in before you can comment on or make changes to this bug.