Closed Bug 520239 Opened 10 years ago Closed 10 years ago

SVG SMIL: Add support for animating CSS shorthand properties

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 5 obsolete files)

Bug 474049's main patch will be landing soon, but it doesn't include support for shorthand properties mentioned in the SVG property index ("font", "overflow", and "marker").

This bug is on supporting those shorthand properties.
This is the first patch needed here.

Some background -- currently, nsStyleAnimation::ComputeValue does this:
   A) Parses the given value for the given property.
   B) Creates a temporary nsStyleContext, which we can use to access the result of (A).
   C) Calls ExtractComputedValue to query that context for the computed value of our property.

The above pathway assumes that steps (A) and (C) are interested in the same property.  However, that's not what we want for shorthand properties.

e.g. with "font", we **parse** a value for the "font" property, but we then want to **query** the computed values of multiple different sub-properties.  

Similarly, for the single-valued shorthands "overflow" and "marker", we can **parse** a specified value for the shorthand property, but we still can't directly **query** that shorthand property's computed value -- we have to query (any of) its subproperties instead.

So, this patch splits out steps (A)/(B) from step (C), so that we can e.g. do (A)/(B) _once_ to parse a "font" value, and then re-use the result in multiple iterations of (C). (for each subproperty)
Attachment #412337 - Flags: review?(dbaron)
This patch does two things:
 (1) Turn on animation support for "overflow-x" and "overflow-y" in nsCSSPropList.h, so that the upcoming "patch 3" can query nsStyleAnimation for computed values of these properties.
 (2) Makes the "BuildStyleRule" helper function in nsStyleAnimation work for shorthand properties. (Explanation below)

Explanation:
In current mozilla-central, BuildStyleRule uses this call:
>    declaration->SlotForValue(aProperty)
to check whether parsing succeeded.  But that doesn't work for shorthands, because there's no single slot for a shorthand's value.  Instead, for a shorthand, we can just pass one of its subproperties to SlotForValue.

This patch reworks the logic in this method, to make this change a bit easier.  It breaks up what previously was a giant 11-line "if" statement into a few nested "if" clauses, so that I can declare a local variable in one clause to simplify things.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Attachment #412342 - Flags: review?(dbaron)
This patch handles animation of shorthand properties on the SMIL side.  Most of this is done in the class "nsSMILCSSShorthandType" (which is analogous to nsSMILCSSValueType).

This patch currently works for both "overflow" and "marker" (layering on top of the URI patches in bug 520487).

This currently has some "font" chunks commented out, because we can't yet animate the subproperties "font-stretch", "font-weight" (bug 528234) and "font-family" (bug 528656).  Once we've got those, though, this patch should be fine with those chunks un-commented.

Requesting review on the work here so far, with the understanding that minimal changes will be required here after bug 528234/bug 528656 are fixed.
Attachment #412348 - Flags: review?(roc)
+  // Just using NS_ASSERTION here because this assertion could fail when called
+  // from the cleanup "Destroy()" call in OOM or no-PresContext conditions.

Then it should be a WARNING, right?

+      // Crap! We lack a PresContext or a PresShell
+      success = PR_FALSE;
+      NS_NOTREACHED("Not parsing animation value; unable to get PresContext");

If we can get into this state, then this shouldn't be NS_NOTREACHED.

+struct ShorthandValue {
+  ShorthandValue() : mPropID(eCSSProperty_UNKNOWN), mPresContext(nsnull),
+                     mIsInherit(PR_FALSE), mData(nsnull) {}
+  nsCSSProperty   mPropID;
+  nsPresContext*  mPresContext;
+  PRPackedBool    mIsInherit;
+  GenericWrapper* mData;  // Contents depends on the shorthand property.
+                          // (Or, if mIsInherit is true, mData should be null)
+};

Would it be possible to simply create subclasses of ShorthandValue instead of using this mData field?
(In reply to comment #4)
> Would it be possible to simply create subclasses of ShorthandValue instead of
> using this mData field?

Yeah, and that's probably cleaner.  Reworking the patch to do that...
This patch uses subclasses of ShorthandValue instead of the generic "mData" pointer, as roc suggests in comment 4.

This fixes the inappropriate-warning-level issues roc mentioned in comment 4, as well.

I've split off 'font' support into a different patch, which I'll attach separately.
Attachment #412348 - Attachment is obsolete: true
Attachment #412348 - Flags: review?(roc)
Attachment #412708 - Attachment description: patch 3a: add support on SMIL side for 'marker', 'overflow' → patch 3 v1: add support on SMIL side for 'marker', 'overflow'
Here's a patch with the added code for 'font'.  We still need 'font-family' (bug 528656) before this one will work.
Attachment #412708 - Flags: review?(roc)
Sorry, the last version of "patch 3" still had an XXXdholbert about needing a header-comment for the nsSMILCSSShorthandValueType class.  Added that in this version.
Attachment #412708 - Attachment is obsolete: true
Attachment #412715 - Flags: review?(roc)
Attachment #412708 - Flags: review?(roc)
Comment on attachment 412337 [details] [diff] [review]
patch 1: Split out nsStyleAnimation::BuildCustomSiblingStyleContext

I think you should call the new method StyleWithDeclarationAdded or something like that, since what it does is return a style context for the element's style plus the given declaration overriding it.

You should document that the method also returns null if the given value does not parse correctly (and probably document that clearly for ComputeValue too).

You should change the |return PR_FALSE| to |return nsnull| inside the function.

Finally, you should make ComputeValue return false when this new method returns null.  Right now it looks like it will crash.  (And if we don't have a test that would have caught that, we should add one.)

r=dbaron with those things fixed
Attachment #412337 - Flags: review?(dbaron) → review+
Comment on attachment 412342 [details] [diff] [review]
patch 2: make "BuildStyleRule" work with shorthands

I'd prefer if you keep the code structure so that the error case is the early return and the success case goes to the end of the function (like code with exceptions works).  That's relatively easy in this case: just compute |propertyToCheck| before the if statement.

Also, it looked like part of the nsCSSPropList.h changes from patch 3 crept into this patch; those should be in patch 3 instead.

Also, rather than using:
*nsCSSProps::SubpropertyEntryFor(aProperty)
I'd prefer:
nsCSSProps::SubpropertyEntryFor(aProperty)[0]
which is technically equivalent but more logically correct.
r=dbaron with that
Attachment #412342 - Flags: review?(dbaron) → review+
This addresses review comments from comment #9.  carrying forward r+.

> Finally, you should make ComputeValue return false when this new method returns
> null.  Right now it looks like it will crash.  (And if we don't have a test
> that would have caught that, we should add one.)

FWIW, this was correct -- with the old patch version, we did crash when the CSS parser failed to parse a value.  This version adds a mochitest to catch this sort of thing in the future.
Attachment #412337 - Attachment is obsolete: true
Attachment #413437 - Flags: review+
Blocks: 520487
So -- now that we have bug 529934 (which lets us just serialize to a string-value rather than to a CSS-computed-value), we can actually just handle Shorthand properties like any other CSS property -- serializing them to a single string-value, rather than to a bunch of computed values.

This patch is a replacement for "patch 3" & "patch 4 WIP".  It removes a couple of "no shorthands allowed" assumptions in nsSMILCSSValueType, and it removes the nsISMILCSSValueType interface, since that's no longer needed.

(This also layers on top of the patch in bug 530199.)
Attachment #412712 - Attachment is obsolete: true
Attachment #412715 - Attachment is obsolete: true
Attachment #413737 - Flags: review?(roc)
Depends on: 530199
Depends on: 529934
You need to log in before you can comment on or make changes to this bug.