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

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

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.

Comment 1

[Daniel Holbert [:dholbert]]

(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.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: 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.

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

Comment 3

[Daniel Holbert [:dholbert]]

(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)

Comment 4

[Daniel Holbert [:dholbert]]

(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.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: fix v2

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: reftest (as patch)

Here's a reftest for this.  I've verified that it fails (is red) pre-patching, and it passes post-patching.

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: reftest (as patch)

Here's the reftest again, now with newline at the end of reftest.list. :)

Comment 8

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

Comment on attachment 427842 [details] [diff] [review]
fix v2

r=dbaron

Comment 9

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

Comment on attachment 427852 [details] [diff] [review]
reftest (as patch)

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

r=dbaron if so

Comment 10

[Daniel Holbert [:dholbert]]

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

Yes, see comment 6.

> r=dbaron if so

Thanks!

Comment 11

[Daniel Holbert [:dholbert]]

Landed patch: http://hg.mozilla.org/mozilla-central/rev/095c11c68ea8
and test:     http://hg.mozilla.org/mozilla-central/rev/c3de74d507fb

Comment 12

[Daniel Holbert [:dholbert]]

Reopening, as this apparently isn't completely fixed. (see bug 549705)

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: followup v1: Use PostAnimationRestyleEvent when SMIL Override Style changes

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.

Comment 14

[Daniel Holbert [:dholbert]]

Attachment: more reftests

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.

Comment 15

[Daniel Holbert [:dholbert]]

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)

Comment 16

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

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

Comment 17

[Daniel Holbert [:dholbert]]

(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...

Comment 18

[Daniel Holbert [:dholbert]]

Attachment: followup v2: Replace SMILOverrideStyleChanged with RestyleForAnimation

Comment 19

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

Comment on attachment 432027 [details] [diff] [review]
followup v2: Replace SMILOverrideStyleChanged with RestyleForAnimation

r=dbaron

Comment 20

[Daniel Holbert [:dholbert]]

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