520239 - SVG SMIL: Add support for animating CSS shorthand properties

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert] (PTO until 2/24)]

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.

Comment 1

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: patch 1: Split out nsStyleAnimation::BuildCustomSiblingStyleContext

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)

Comment 2

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: patch 2: make "BuildStyleRule" work with shorthands

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.

Comment 3

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: patch 3: add support on SMIL side

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.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  // 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?

Comment 5

[Daniel Holbert [:dholbert] (PTO until 2/24)]

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

Comment 6

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: patch 3 v1: add support on SMIL side for 'marker', 'overflow'

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.

Comment 7

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: patch 4 WIP: add support for 'font'

Here's a patch with the added code for 'font'.  We still need 'font-family' (bug 528656) before this one will work.

Comment 8

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: patch 3 v2: add support on SMIL side for 'marker', 'overflow'

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.

Comment 9

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

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

Comment 10

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

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

Comment 11

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: patch 1 v2: addresses review comments

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.

Comment 12

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Attachment: patch 5: [REPLACES 3 & 4] treat shorthands like any other property (with eUnit_UnparsedString)

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

Comment 13

[Daniel Holbert [:dholbert] (PTO until 2/24)]

Landed patches 1,2, & 5:
http://hg.mozilla.org/mozilla-central/rev/87848ea05432
http://hg.mozilla.org/mozilla-central/rev/79cab5f93f56
http://hg.mozilla.org/mozilla-central/rev/adb68abde645