More refactoring of nsTransitionManager::ConsiderStartingTransition

RESOLVED FIXED in mozilla24

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: nrc, Assigned: nrc)

Tracking

unspecified
mozilla24
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
Followup to bug 879255 (probably should have noticed these earlier, but hey).
(Assignee)

Comment 1

5 years ago
  if (!shouldAnimate) {
    if (haveCurrentTransition) {
      // We're in the middle of a transition, but just got a
      // non-transition style change changing to exactly the
      // current in-progress value.   (This is quite easy to cause
      // using 'transition-delay'.)

This comment still doesn't make sense to me, the if clauses only ensure that we have a non-transition change and that there currently exists a transition of the same property, not that the change takes us to the current point.
(Assignee)

Comment 2

5 years ago
Created attachment 758902 [details] [diff] [review]
check the current value of the transition
Attachment #758902 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #758902 - Flags: review? → review?(dbaron)
(Assignee)

Comment 3

5 years ago
Created attachment 758904 [details] [diff] [review]
move the def of pts

We only want to take one patch or the other, and if we take the second, we probably should change the comment to reflect what is happening or at least be more detailed.
Attachment #758904 - Flags: review?(dbaron)

Comment 4

5 years ago
Comment on attachment 758902 [details] [diff] [review]
check the current value of the transition

So the "changing to exactly the current in-progress value" bit is the "!shouldAnimate" part of the condition.  Though that's also true if we just got a style change changing to a non-interpolable value.  But I think in either case the current comment is correct and the code is wrong.
Attachment #758902 - Flags: review?(dbaron) → review-

Comment 5

5 years ago
Comment on attachment 758904 [details] [diff] [review]
move the def of pts

I'd suggest changing the comment:
       // We're in the middle of a transition, but just got a
       // non-transition style change changing to exactly the
       // current in-progress value.   (This is quite easy to cause
       // using 'transition-delay'.)
to something more like this (wrapped at 72 chars):

We're in the middle of a transition, and just got a non-transition style change to something that we can't animate.  This might happen because we got a non-transition style change changing to the current in-progress value (which is particularly easy to cause when we're currently in the 'transition-delay').  It also might happen because we just got a style change to a value that can't be interpolated.
Attachment #758904 - Flags: review?(dbaron) → review+

Comment 7

5 years ago
https://hg.mozilla.org/mozilla-central/rev/a3e65bf60566
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
You need to log in before you can comment on or make changes to this bug.