Closed Bug 528234 Opened 15 years ago Closed 15 years ago

support animation of font-weight and font-stretch (excluding bolder, lighter, wider, narrower)

Categories

(Core :: CSS Parsing and Computation, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1

People

(Reporter: dbaron, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Keywords: dev-doc-complete)

Attachments

(2 files, 1 obsolete file)

nsStyleAnimation should support animation of font-weight and font-stretch (excluding bolder, lighter, wider, and narrower, at least until jdaggett's redesign of those values gets implemented)
I sort of thought we'd need this eventually.  We'll need it for visibility too.

I'm not exactly confident about the SMIL changes in this patch.  There's a bunch of code for handling UNKNOWN as the property, etc., and I'm not exactly sure what that means.  Does SMIL always have the correct property at hand in that code, and is this the way to get it?
Attachment #411995 - Flags: review?(dholbert)
Note that I'll fix some of the todos in this patch in a patch I'll attach to bug 173331 shortly (which is itself on top of a patch I'll attach to bug 522320).
Attachment #411996 - Flags: review?(dholbert)
Comment on attachment 411995 [details] [diff] [review]
patch 1: pass property to nsStyleAnimation interpolation functions

>@@ -200,17 +200,17 @@ nsSMILCSSValueType::Add(nsSMILValue& aDe
>                     !valueToAddWrapper->mCSSValue.IsNull(),
>                     "Added amount should be a parsed value");
> 
>   // Special case: some properties are explicitly non-additive
>   if (destWrapper->mPropID == eCSSProperty_font_size_adjust ||
>       destWrapper->mPropID == eCSSProperty_stroke_dasharray) {
>     return NS_ERROR_FAILURE;
>   }
>-  return nsStyleAnimation::Add(destWrapper->mCSSValue,
>+  return nsStyleAnimation::Add(destWrapper->mPropID, destWrapper->mCSSValue,
>                                valueToAddWrapper->mCSSValue, aCount) ?
>     NS_OK : NS_ERROR_FAILURE;
> }

This will need bitrot-fixing, after bug 526875's landing yesterday. (Other places in the same file will, too, because bug 526875 tweaked some assertions that are in your contextual code -- but those should hopefully be fine with some patch-fuzz)

(In reply to comment #1)
> I'm not exactly confident about the SMIL changes in this patch.  There's a
> bunch of code for handling UNKNOWN as the property, etc., and I'm not exactly
> sure what that means.  Does SMIL always have the correct property at hand in
> that code, and is this the way to get it?

Yes, it will, and you're getting it correctly.

I'll briefly explain all the UNKNOWN-handling stuff -- basically, Init() on the nsISMILType interface is supposed to create an "identity" (zero) value.  In nsSMILCSSValueType's implementation of this method, we of course don't yet know which property we're dealing with, so we just set the property to UNKNOWN.

The Add/ComputeDistance/Interpolate methods may receive one of these "just-barely initialized" values, and they have to treat them as zero.  Due to the limited number of ways in which our SMIL code calls these methods, it's always the case that *at least one* of their parameters will be a parsed value, and hence will have a meaningful property & PresContext.  Moreover, this is guaranteed to be true for a *particular* parameter in each method (which is why I don't have to check both parameters for UNKNOWN), except in the rare case caught by bug 526875.

Anyway -- you're passing in the property ID from the correct parameter, in each case.  

r=dholbert with the bitrot fixed.
Attachment #411995 - Flags: review?(dholbert) → review+
Comment on attachment 411996 [details] [diff] [review]
patch 2: add support for animation font-weight and font-stretch

>@@ -124,17 +124,28 @@ nsStyleAnimation::ComputeDistance(nsCSSP
> {
>     case eUnit_Null:
>     case eUnit_None:
>     case eUnit_Enumerated:
>-      success = PR_FALSE;
>+      switch (aProperty) {
>+        case eCSSProperty_font_stretch: {
>+          // just like eUnit_Integer.
>+          PRInt32 startInt = aStartValue.GetIntValue();
>+          PRInt32 endInt = aEndValue.GetIntValue();
>+          aDistance = PR_ABS(endInt - startInt);
>+          break;
>+        }
>+        default:
>+          success = PR_FALSE;
>+          break;
>+      }
>       break;

In the code above, you're assuming that aStartValue.GetIntValue() is meaningful -- and it is for eUnit_Enumerated, but it's most definitely not for eUnit_Null / eUnit_None (which fall into that same general block of code).  I think you should separate out Enumerated to be a different case from Null/None -- i.e. just add
   success = PR_FALSE;
   break;
right below "case eUnit_None".

Also -- why are you using "switch (aProperty)" there instead of just "if (aProperty == eCSSProperty_font_stretch)"?  Are there other enumerated-value properties which will support distance-computation?  If don't have any particular properties in mind, I'd suggest changing that to an "if" statement.

Both of the above changes apply to nsStyleAnimation::AddWeighted, too.

>@@ -1265,16 +1290,36 @@ nsStyleAnimation::ExtractComputedValue(n
>+        case eCSSProperty_font_stretch: {
>+          PRInt16 stretch =
>+            static_cast<const nsStyleFont*>(styleStruct)->mFont.stretch;
>+          if (stretch < -5 || stretch > 5) {
>+            return PR_FALSE;
>+          }
>+          aComputedValue.SetIntValue(stretch, eUnit_Enumerated);
>+          return PR_TRUE;
>+        }

I think the "< -5" and "> 5" there are wrong -- from a look at gfxFontConstants.h, it looks like the relevant extreme values are actually -4 and 4. (NS_FONT_STRETCH_ULTRA_CONDENSED and NS_FONT_STRETCH_ULTRA_EXPANDED). You should probably be comparing for >/< those constants instead of the "5" values, right? (preferably using the #defined names for them)

r=dholbert with the above addressed.
Attachment #411996 - Flags: review?(dholbert) → review+
(In reply to comment #4)
> Also -- why are you using "switch (aProperty)" there instead of just "if
> (aProperty == eCSSProperty_font_stretch)"?  Are there other enumerated-value
> properties which will support distance-computation?  If don't have any
> particular properties in mind, I'd suggest changing that to an "if" statement.

One other property I have in mind is 'visibility', which css3-transitions says should be animated.
(In reply to comment #5)
> One other property I have in mind is 'visibility', which css3-transitions says
> should be animated.

(Ah -- looks like that css3-transitions wants 'visibility' transitions to look much like 'opacity' animations in SVG, if I'm reading the spec correctly.)

Ok, feel free to disregard the bit of comment 4 about the switch statements.
Comment on attachment 412405 [details] [diff] [review]
patch 3: Fix UncomputeValue

Note that most valid font-weight values (200, 300, etc) aren't mapped to keywords -- nsStyleAnimation::UncomputeValue fails for all those values right now.

To rectify this, this patch makes UncomputeValue treat our font-weight values as integers, instead of keyword-mapped enumerated values.
Attachment #412405 - Attachment is patch: true
Attachment #412405 - Attachment mime type: application/octet-stream → text/plain
Attachment #412405 - Flags: review?(dbaron)
(In reply to comment #8)
> (From update of attachment 412405 [details] [diff] [review])
> Note that most valid font-weight values (200, 300, etc) aren't mapped to
> keywords -- nsStyleAnimation::UncomputeValue fails for all those values right
> now.
> 
> To rectify this, this patch makes UncomputeValue treat our font-weight values
> as integers, instead of keyword-mapped enumerated values.

How would an nsStyleAnimation::Value ever end up with a value of font-weight that's eUnit_Enumerated (rather than eUnit_Integer)?  I don't think that should happen.
Attachment #412405 - Attachment is obsolete: true
Attachment #412405 - Flags: review?(dbaron)
Yeah, you're right -- sorry about that.  I had it backwards in my head which property was being represented as an Integer vs. an Enumerated, and I'd thought this would be needed. (It was from a WIP font-weight/stretch patch that I've got in my patch queue, which represented both with Enumerated units, and hence needed the fix)

I think UncomputeValue should be fine as-is.
http://hg.mozilla.org/mozilla-central/rev/d49d4151ad1d
http://hg.mozilla.org/mozilla-central/rev/fa53068a3db5
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Priority: -- → P4
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Blocks: 529406
You need to log in before you can comment on or make changes to this bug.