537151 - get extra transition when modifying style during transitionend event

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P2)

Description

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

Attachment: testcase (modified version of dolske's)

In bug 521890 comment 13 dolske posted a testcase showing double transitionend events being fired in a testcase reduced from his work on HTML5 videocontrols.

The problem seems to be the following (I didn't verify all the details of the following in the debugger, but I think it makes sense based on the ones I did):

 * at the time we call the transitionend event handler, we've posted a RestyleForAnimation on the element that's transitioning, but we haven't processed it yet
 * in this testcase (a modified version of dolske's), style is modified during the transitionend event handler
 * when that happens, we now have both animation and non-animation restyles for the element; we process the non-animation restyles first, and detect that opacity has changed a minute amount, and initiate *another* transition of 'opacity' on the element

Comment 1

[Boris Zbarsky [:bzbarsky]]

Should the firing of the transitionend event just flush out styles?

Comment 2

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

Maybe, although I want to think about whether there are other problems with the double restyle queue setup.

Comment 3

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

I think it's actually possible to get into this situation in other ways,
especially after bug 528306 lands:  all you need is a posted restyle
that's pending when the refresh happens.

So I think the right solution here is to make the transition manager
hold on to completed transitions for one refresh cycle after they
complete.  This will allow this check:
    if (oldPT.mEndValue == pt.mEndValue) {
      // If we got a style change that changed the value to the endpoint
      // of the currently running transition, we don't want to interrupt
      // its timing function.
      // WalkTransitionRule already called RestyleForAnimation.
      return;
    }
in nsTransitionManager::ConsiderStartingTransition to prevent a new 
transition from starting in these cases.

Comment 4

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

The patch at
http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/431fe376650a/transition-keep-one-cycle
but I need to:
 (a) write tests for it, and
 (b) figure out what to do about its relationship to http://hg.mozilla.org/users/dbaron_mozilla.com/patches/raw-file/431fe376650a/transition-no-compute-distance , which it's currently on top of

Comment 5

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

Er, I forgot the verb in that sentence.  Should have said "seems to fix this bug".

Comment 6

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

Attachment: patch

I added a test.

Regarding the relationship to the other patch in the previous comment:  to swap the order of the patches, I'd just take the introduction of the oldPT variable (and removal of endVal) in that patch and put it in this one.

Comment 7

[Boris Zbarsky [:bzbarsky]]

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

r=bzbarsky

Comment 8

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

http://hg.mozilla.org/mozilla-central/rev/263ab164ef4b