525530 - make dynamic changes to '-moz-transition-property' stop running transitions

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

Ehsan asked me a week or two ago about wanting a better way to stop transitions that are already running.

One possibility (which fits somewhat well with the initial values of the properties) was to special case dynamic changes that cause both -moz-transition-duration and -moz-transition-delay to become 0, and make them stop the transition.  However, this treats changes to 0 differently from other changes, which is ugly.

The alternative, which I think I've concluded that I prefer, is to make dynamic changes to -moz-transition-property stop currently running transitions.  In other words, if -moz-transition-property on an element changes such that it no longer says that a property should transition, any running transitions for that property on that element stop.

This also happens to be what WebKit does based on my test at http://dbaron.org/css/test/2009/transitions/dynamic-transition-change

Comment 1

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

Attachment: patch

Comment 2

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

Attachment: patch

Boy, I botched the changes to nsTransitionManager::WalkTransitionRule in the previous iteration of this patch.  (That was the change I made at the very end to get the tests to pass.)  It caused crashes in both crashtests and my own daily browsing (printing directions on google maps, with the large map option), and probably a measureable perf regression.

In any case, this time WalkTransitionRule deals with a null aData->mContent up front (e.g., a page break frame), and only restyles things that actually *have* transitions.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> In any case, this time WalkTransitionRule deals with a null aData->mContent

Note that this can probably be removed once the patches in bug 525608 land (or rather I'll have to merge those with this....)

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 413278 [details] [diff] [review]
patch

>+++ b/layout/style/nsStyleStruct.cpp
>+++ b/layout/style/nsTransitionManager.cpp
>+  const nsStyleDisplay *oldDisp = aOldStyleContext->GetStyleDisplay();

This should probably technically be a PeekStyleDisplay, except I think style contexts always have a display struct... maybe worth a comment?

>+  PRBool didStop = PR_FALSE;

This seems to be write-only.  Take it out?

r=bzbarsky with that.

Comment 5

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

(In reply to comment #4)
> (From update of attachment 413278 [details] [diff] [review])
> >+++ b/layout/style/nsStyleStruct.cpp
> >+++ b/layout/style/nsTransitionManager.cpp
> >+  const nsStyleDisplay *oldDisp = aOldStyleContext->GetStyleDisplay();
> 
> This should probably technically be a PeekStyleDisplay, except I think style
> contexts always have a display struct... maybe worth a comment?

Hmmm.  Why do you think it should be a Peek?  I think it should be a Get.  If we:
 * start a transition on something
 * reframe one of its ancestors
 * change its transition-property
then if we didn't always have a display, we might not have one here, in a case where we still needed to stop transitions.

> >+  PRBool didStop = PR_FALSE;
> 
> This seems to be write-only.  Take it out?

Oops.  I have no idea what I was planning to use it for.

Comment 6

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

But in StyleContextChanged, I think I'll add:

  // NOTE: Things in this function (and ConsiderStartingTransition)
  // should never call PeekStyleData because we don't preserve gotten
  // structs across reframes.

Comment 7

[Boris Zbarsky [:bzbarsky]]

> then if we didn't always have a display, we might not have one here

Ok, fair enough.  Get is fine.

Comment 8

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

fixed: http://hg.mozilla.org/mozilla-central/rev/3a6b865babdc

(I'm also planning to rework this fix a good bit in bug 522643.)