CSS transitions shouldn't be triggered by style changes from SMIL animation

RESOLVED FIXED in mozilla1.9.3a3

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: dbaron, Assigned: dholbert)

Tracking

(Blocks 1 bug)

Trunk
mozilla1.9.3a3
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

We want CSS transitions not to be triggered by style changes that result from SMIL animation.

I think this requires making nsHTMLCSSStyleSheet::RulesMatching check aData->mPresContext->IsProcessingAnimationStyleChange() and making SMIL code nsIPresShell::RestyleForAnimation and probably a few other related changes.  It may be easiest to also hook up the SMIL timing to nsRefreshDriver at the same time.
(Assignee)

Updated

9 years ago
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(Assignee)

Comment 1

9 years ago
(In reply to comment #0)
> It
> may be easiest to also hook up the SMIL timing to nsRefreshDriver at the same
> time.

This suggested change is orthogonal, so I filed  Bug 541588 on it.
(Assignee)

Comment 2

9 years ago
Posted patch fix v1 (obsolete) — Splinter Review
This fix does what dbaron suggested in comment 0.  It actually causes breakage in SMIL animations for "display:none" content, but Bug 541590's patch fixes that.

I'm running this through tryserver to make sure it doesn't break anything.
(Assignee)

Comment 3

9 years ago
(In reply to comment #2)
> It actually causes breakage
> in SMIL animations for "display:none" content, but Bug 541590's patch fixes
> that.

Setting bug 541590 as blocking this one. (since it needs to land before this does)
Depends on: 541590
(Assignee)

Comment 4

9 years ago
(In reply to comment #2)
> Created an attachment (id=423113) [details]
> fix v1
> 
> This fix does what dbaron suggested in comment 0.  It actually causes breakage
> in SMIL animations for "display:none" content, but Bug 541590's patch fixes
> that.

As noted in Bug 541590 comment 6, we actually don't want that bug's patch after all. Instead, we can just fix SMIL animation of "display:none" content by adding an additional check to this bug's patch, to always apply SMIL override style when we're *not* processing restyles.

Put simply, the only time we want to ignore SMIL override style is when we're **currently** doing non-animation restyles.  In order to check for this, we need one more boolean on the presContext.  Patch coming in a sec.
(Assignee)

Comment 5

9 years ago
Posted patch fix v2Splinter Review
Attachment #423113 - Attachment is obsolete: true
Attachment #427842 - Flags: review?(dbaron)
(Assignee)

Updated

9 years ago
No longer depends on: 541590
(Assignee)

Comment 6

9 years ago
Posted patch reftest (as patch) (obsolete) — Splinter Review
Here's a reftest for this.  I've verified that it fails (is red) pre-patching, and it passes post-patching.
(Assignee)

Updated

9 years ago
Attachment #427848 - Flags: review?(dbaron)
(Assignee)

Updated

9 years ago
Flags: in-testsuite?
(Assignee)

Comment 7

9 years ago
Here's the reftest again, now with newline at the end of reftest.list. :)
Attachment #427848 - Attachment is obsolete: true
Attachment #427848 - Flags: review?(dbaron)
(Assignee)

Updated

9 years ago
Attachment #427852 - Flags: review?(dbaron)
Comment on attachment 427842 [details] [diff] [review]
fix v2

r=dbaron
Attachment #427842 - Flags: review?(dbaron) → review+
Comment on attachment 427852 [details] [diff] [review]
reftest (as patch)

You checked that this test failed without the patch, right?

r=dbaron if so
Attachment #427852 - Flags: review?(dbaron) → review+
(Assignee)

Comment 10

9 years ago
(In reply to comment #9)
> You checked that this test failed without the patch, right?

Yes, see comment 6.

> r=dbaron if so

Thanks!
(Assignee)

Comment 11

9 years ago
Landed patch: http://hg.mozilla.org/mozilla-central/rev/095c11c68ea8
and test:     http://hg.mozilla.org/mozilla-central/rev/c3de74d507fb
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a3
(Assignee)

Updated

9 years ago
Blocks: 549705
(Assignee)

Comment 12

9 years ago
Reopening, as this apparently isn't completely fixed. (see bug 549705)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 13

9 years ago
So the remaining problem was that, when SMIL Override style changed, we'd post a normal restyle event instead of an Animation restyle event, which opened us up to creating a CSS Transition for the change.

This patch fixes that.
(Assignee)

Comment 14

9 years ago
Posted patch more reftestsSplinter Review
This patch adds 3 more reftests (smil-transitions-interaction-[234].svg), which test our behavior here a little more thoroughly.

The original reftest (-1.svg) just checked whether we added a transition when SMIL effects are first applied.

The new tests also check that we don't trigger transition *during* an animation's duration (-2.svg and -4.svg), and that we don't trigger a transition when SMIL effects are removed (-3.svg).

Tests -2 and -4 both fail before the followup patch is applied, and they all pass after the followup is applied.
(Assignee)

Comment 15

9 years ago
Comment on attachment 430140 [details] [diff] [review]
followup v1: Use PostAnimationRestyleEvent when SMIL Override Style changes

Requesting review. (one-liner patch, see comment 13 for explanation)
Attachment #430140 - Flags: review?(dbaron)
Comment on attachment 430140 [details] [diff] [review]
followup v1: Use PostAnimationRestyleEvent when SMIL Override Style changes

Better to just write:

mShell->RestyleForAnimation(aContent);

which does the same thing.

r=dbaron with that
Attachment #430140 - Flags: review?(dbaron) → review+
(Assignee)

Comment 17

9 years ago
(In reply to comment #16)
> (From update of attachment 430140 [details] [diff] [review])
> Better to just write:
> mShell->RestyleForAnimation(aContent);

Thanks!  Actually, now that you mention it, we probably should just get rid of SMILOverrideStyleChanged() entirely, and just directly call RestyleForAnimation() instead.  Replacement patch coming up that does that...
(Assignee)

Updated

9 years ago
Attachment #432027 - Attachment description: followup v1: Replace SMILOverrideStyleChanged with RestyleForAnimation → followup v2: Replace SMILOverrideStyleChanged with RestyleForAnimation
Comment on attachment 432027 [details] [diff] [review]
followup v2: Replace SMILOverrideStyleChanged with RestyleForAnimation

r=dbaron
Attachment #432027 - Flags: review?(dbaron) → review+
(Assignee)

Comment 20

9 years ago
pushed: http://hg.mozilla.org/mozilla-central/rev/2522b8b39cf4

and re-enabled test, and added some new tests:
        http://hg.mozilla.org/mozilla-central/rev/cdc9b315087b
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.