Closed
Bug 537139
Opened 15 years ago
Closed 14 years ago
CSS transitions shouldn't be triggered by style changes from SMIL animation
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a3
People
(Reporter: dbaron, Assigned: dholbert)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 3 obsolete files)
6.21 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
1.40 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
4.68 KB,
patch
|
Details | Diff | Splinter Review | |
3.23 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•15 years ago
|
Blocks: css-transitions
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•14 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•14 years ago
|
||
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•14 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•14 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•14 years ago
|
||
Attachment #423113 -
Attachment is obsolete: true
Attachment #427842 -
Flags: review?(dbaron)
Assignee | ||
Comment 6•14 years ago
|
||
Here's a reftest for this. I've verified that it fails (is red) pre-patching, and it passes post-patching.
Assignee | ||
Updated•14 years ago
|
Attachment #427848 -
Flags: review?(dbaron)
Assignee | ||
Updated•14 years ago
|
Flags: in-testsuite?
Assignee | ||
Comment 7•14 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•14 years ago
|
Attachment #427852 -
Flags: review?(dbaron)
Reporter | ||
Comment 8•14 years ago
|
||
Comment on attachment 427842 [details] [diff] [review] fix v2 r=dbaron
Attachment #427842 -
Flags: review?(dbaron) → review+
Reporter | ||
Comment 9•14 years ago
|
||
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•14 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•14 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
Closed: 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Target Milestone: --- → mozilla1.9.3a3
Assignee | ||
Comment 12•14 years ago
|
||
Reopening, as this apparently isn't completely fixed. (see bug 549705)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 13•14 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•14 years ago
|
||
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•14 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)
Reporter | ||
Comment 16•14 years ago
|
||
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•14 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 | ||
Comment 18•14 years ago
|
||
Attachment #430140 -
Attachment is obsolete: true
Attachment #432027 -
Flags: review?(dbaron)
Assignee | ||
Updated•14 years ago
|
Attachment #432027 -
Attachment description: followup v1: Replace SMILOverrideStyleChanged with RestyleForAnimation → followup v2: Replace SMILOverrideStyleChanged with RestyleForAnimation
Reporter | ||
Comment 19•14 years ago
|
||
Comment on attachment 432027 [details] [diff] [review] followup v2: Replace SMILOverrideStyleChanged with RestyleForAnimation r=dbaron
Attachment #432027 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 20•14 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
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•