Bug 15598 – The "font" property is not a shorthand

• Component: CSS
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

font property does not show up as "shorthand" in inspector

This is due to the fact that parseFont does it's own custom parsing which creates a FontValue instead of calling the CSSParser for each sub-value of the font.  It probably does this for speed, but it makes for "incorrect" shorthand information on the CSSComputedStyle element.

[Eric Seidel (no email)]

Created attachment 16771 [details]
test case

[Alexander Pavlov (apavlov)]

Created attachment 92601 [details]
[PATCH] Suggested fix, initial version

[WebKit Review Bot]

Comment on attachment 92601 [details]
[PATCH] Suggested fix, initial version

Attachment 92601 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8598068

New failing tests:
canvas/philip/tests/2d.text.draw.align.left.html
canvas/philip/tests/2d.text.draw.space.basic.html
canvas/philip/tests/2d.text.draw.align.right.html
canvas/philip/tests/2d.text.draw.align.start.rtl.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
canvas/philip/tests/2d.text.draw.align.start.ltr.html
canvas/philip/tests/2d.text.draw.baseline.alphabetic.html
canvas/philip/tests/2d.text.draw.fill.maxWidth.bound.html
canvas/philip/tests/2d.text.draw.align.end.ltr.html
canvas/philip/tests/2d.text.draw.fontface.html
canvas/philip/tests/2d.text.draw.fontface.repeat.html
fast/canvas/2d.fillText.gradient.html
canvas/philip/tests/2d.text.measure.width.basic.html
canvas/philip/tests/2d.text.draw.align.center.html
canvas/philip/tests/2d.text.draw.align.end.rtl.html

[WebKit Review Bot]

Created attachment 92626 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Eric Seidel (no email)]

Hmm... I wonder if these are unrelated failures.

[Adam Barth]

(In reply to comment #5)
> Hmm... I wonder if these are unrelated failures.

Yeah, dunno.  The logs from the bot look legit.  I guess we'll find out if/when the patch lands.

[Luiz Agostini]

Comment on attachment 92601 [details]
[PATCH] Suggested fix, initial version

View in context: https://bugs.webkit.org/attachment.cgi?id=92601&action=review

I am not an official reviewer but here goes some thoughts.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:317
> +    {
> +        const CSSProperty* property = findPropertyWithId(CSSPropertyFontVariant);
> +        if (!property)
> +            return ""; // All longhands must have at least implicit values if "font" is specified.
> +        if (!property->isImplicit()) {
> +            if (!result.isEmpty())
> +                result += " ";
> +            result += property->value()->cssText();
> +        }
> +    }

This code seems to be replicated several times, sometimes with minor differences but many times almost unchanged.
Would not it be possible to create a function and avoid code replication?

> Source/WebCore/css/CSSParser.cpp:595
> +void CSSParser::addPropertyImplicit(int propId, PassRefPtr<CSSValue> value, bool important, bool implicit)

Boolean parameter reduce readability. Would not it be better to create enumerations with descriptive names?

> Source/WebCore/css/CSSParser.cpp:3619
> +                // do nothing, it's the initial value for all three

I think that comments should start with capital letters and finish with a period.

[Alexander Pavlov (apavlov)]

(In reply to comment #7)
> (From update of attachment 92601 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92601&action=review
> 
> I am not an official reviewer but here goes some thoughts.
> 
> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:317
> > +    {
> > +        const CSSProperty* property = findPropertyWithId(CSSPropertyFontVariant);
> > +        if (!property)
> > +            return ""; // All longhands must have at least implicit values if "font" is specified.
> > +        if (!property->isImplicit()) {
> > +            if (!result.isEmpty())
> > +                result += " ";
> > +            result += property->value()->cssText();
> > +        }
> > +    }
> 
> This code seems to be replicated several times, sometimes with minor differences but many times almost unchanged.
> Would not it be possible to create a function and avoid code replication?

I explicitly didn't want to clutter the method namespace but I agree that the current implementation looks quite awkward. Fixed.

> > Source/WebCore/css/CSSParser.cpp:595
> > +void CSSParser::addPropertyImplicit(int propId, PassRefPtr<CSSValue> value, bool important, bool implicit)
> 
> Boolean parameter reduce readability. Would not it be better to create enumerations with descriptive names?

It is an ad-hoc method which is not going to be used in many places around, so I don't think it warrants another enum (which is of course a very good thing for simplifying APIs).

> > Source/WebCore/css/CSSParser.cpp:3619
> > +                // do nothing, it's the initial value for all three
> 
> I think that comments should start with capital letters and finish with a period.

Cleaned up all comments in the method.

[Alexander Pavlov (apavlov)]

Created attachment 92972 [details]
[PATCH] Comments addressed

[WebKit Review Bot]

Comment on attachment 92972 [details]
[PATCH] Comments addressed

Attachment 92972 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8685055

New failing tests:
canvas/philip/tests/2d.text.draw.align.left.html
canvas/philip/tests/2d.text.draw.space.basic.html
canvas/philip/tests/2d.text.draw.align.right.html
canvas/philip/tests/2d.text.draw.align.start.rtl.html
canvas/philip/tests/2d.text.draw.fontface.notinpage.html
canvas/philip/tests/2d.text.draw.align.start.ltr.html
canvas/philip/tests/2d.text.draw.baseline.alphabetic.html
canvas/philip/tests/2d.text.draw.fill.maxWidth.bound.html
canvas/philip/tests/2d.text.draw.align.end.ltr.html
canvas/philip/tests/2d.text.draw.fontface.html
canvas/philip/tests/2d.text.draw.fontface.repeat.html
fast/canvas/2d.fillText.gradient.html
canvas/philip/tests/2d.text.measure.width.basic.html
canvas/philip/tests/2d.text.draw.align.center.html
canvas/philip/tests/2d.text.draw.align.end.rtl.html

[WebKit Review Bot]

Created attachment 92989 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Darin Adler]

Comment on attachment 92972 [details]
[PATCH] Comments addressed

View in context: https://bugs.webkit.org/attachment.cgi?id=92972&action=review

review- because this seems to affect normal property value fetching, not just the inspector functions, and that should be tested and because this needs to use StringBuilder, not String.

> Source/WebCore/ChangeLog:5
> +        font property does not show up as "shorthand" in inspector

I know you discovered this while working on the inspector but the code change here does not seem at all inspector-specific. Can we add some non-inspector CSS test cases to demonstrate this bug and fix?

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:298
> +bool CSSMutableStyleDeclaration::appendMandatoryPropertyValueIfExplicit(const CSSProperty* property, const String& stringBefore, String* result) const

For WebKit coding style, result should be a reference, not a pointer.

For a code path that is appending, result needs to be a StringBuilder, not a String.

Since stringBefore is always a string constant, it should be a const char*, since StringBuilder already has an optimized code path for appending those.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:312
> +    DEFINE_STATIC_LOCAL(String, spaceString, (" "));

Should get rid of this and just use a space after making the change I suggested above.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:313
> +    String result("");

Needs to be a StringBuilder.

> Source/WebCore/css/CSSParser.h:76
>          void addProperty(int propId, PassRefPtr<CSSValue>, bool important);
> +        void addPropertyImplicit(int propId, PassRefPtr<CSSValue>, bool important, bool implicit);

I think this would be clearer as a default argument rather than a separate function. All the callers are already passing a boolean argument, not a constant, and it could simply default to true.

[Alexander Pavlov (apavlov)]

Created attachment 111791 [details]
[PATCH] Darin's comments addressed

[Alexander Pavlov (apavlov)]

(In reply to comment #12)
> (From update of attachment 92972 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92972&action=review
> 
> review- because this seems to affect normal property value fetching, not just the inspector functions, and that should be tested and because this needs to use StringBuilder, not String.

Good point. However, the affected test has nothing to do with web inspector (not sure why it resides in the "inspector-support" directory) and tests exactly the modified functionality: retrieves applicable style rules and examines their properties. In my case, the "font" property has gone in favor of its longhands ("font-family" et al), and that's what the expected result has been updated to reflect.

> > Source/WebCore/ChangeLog:5
> > +        font property does not show up as "shorthand" in inspector
> 
> I know you discovered this while working on the inspector but the code change here does not seem at all inspector-specific. Can we add some non-inspector CSS test cases to demonstrate this bug and fix?

Please see above. If you suggest that there is a better way to test this change than to directly query the CSS style for its properties, I will surely add/modify a proper test.

> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:298
> > +bool CSSMutableStyleDeclaration::appendMandatoryPropertyValueIfExplicit(const CSSProperty* property, const String& stringBefore, String* result) const
> 
> For WebKit coding style, result should be a reference, not a pointer.
> For a code path that is appending, result needs to be a StringBuilder, not a String.

Fixed both.

> Since stringBefore is always a string constant, it should be a const char*, since StringBuilder already has an optimized code path for appending those.
> 
> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:312
> > +    DEFINE_STATIC_LOCAL(String, spaceString, (" "));
> 
> Should get rid of this and just use a space after making the change I suggested above.

Done.

> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:313
> > +    String result("");
> 
> Needs to be a StringBuilder.

Done.

> > Source/WebCore/css/CSSParser.h:76
> >          void addProperty(int propId, PassRefPtr<CSSValue>, bool important);
> > +        void addPropertyImplicit(int propId, PassRefPtr<CSSValue>, bool important, bool implicit);
> 
> I think this would be clearer as a default argument rather than a separate function. All the callers are already passing a boolean argument, not a constant, and it could simply default to true.

Done.

[Alexander Pavlov (apavlov)]

@jchaffraix: I see you made a related fix a while ago to avoid crashes by means of calling updateFont(). I have moved this call just in front of the affected computeLength() call for the CSS_EXS case (since fontMetrics() are not required in other cases). Do you think this particular change is safe enough given the invoking code modification?

[Julien Chaffraix]

Comment on attachment 111791 [details]
[PATCH] Darin's comments addressed

View in context: https://bugs.webkit.org/attachment.cgi?id=111791&action=review

> Source/WebCore/css/CSSStyleSelector.cpp:2592
> +                    updateFont();

I think it is an overkill to move the updateFont here. The case covered by the bug was specific to <canvas> where we cheat by having parentStyle == m_style. The rest of the code would use a different parentStyle which would not be modified in-flight. 

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1888
> +    styleSelector->applyPropertyToCurrentStyle(CSSPropertyFontWeight, tempDecl->getPropertyCSSValue(CSSPropertyFontWeight).get());

Now that you have split the different steps, adding the call to updateFont() just after this line would be the best option.

[Darin Adler]

Comment on attachment 111791 [details]
[PATCH] Darin's comments addressed

View in context: https://bugs.webkit.org/attachment.cgi?id=111791&action=review

review- because of the issue Julien mentions

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:315
> +        return "";

A more efficient empty string is emptyAtom. Is empty string really right for this? I don’t see any test cases covering this.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:319
> +        return "";

Ditto.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:321
> +        return "";

Ditto.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:323
> +        return "";

Ditto.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:328
> +        return "";

Ditto.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:330
> +        return "";

Ditto.

> Source/WebCore/css/CSSParser.cpp:3959
> -                // do nothing, it's the inital value for all three
> +                // Do nothing, it's the initial value for all three.
> +                switch (valueOrdinal) {
> +                case 0:
> +                    styleImplicit = false;
> +                    break;
> +                case 1:
> +                    variantImplicit = false;
> +                    break;
> +                case 2:
> +                    weightImplicit = false;
> +                    break;
> +                }

The comment says “do nothing”, but now the code does something, so “do nothing” no longer makes sense.

> Source/WebCore/css/CSSParser.cpp:4039
> +            // Default value, nothing to do

You didn’t add a period. I’m not sure “nothing to do” really describes code that does something. May need better wording.

[Alexander Pavlov (apavlov)]

(In reply to comment #16)
> (From update of attachment 111791 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111791&action=review
> 
> > Source/WebCore/css/CSSStyleSelector.cpp:2592
> > +                    updateFont();
> 
> I think it is an overkill to move the updateFont here. The case covered by the bug was specific to <canvas> where we cheat by having parentStyle == m_style. The rest of the code would use a different parentStyle which would not be modified in-flight. 

Thanks Julien, I've reverted this change.

> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1888
> > +    styleSelector->applyPropertyToCurrentStyle(CSSPropertyFontWeight, tempDecl->getPropertyCSSValue(CSSPropertyFontWeight).get());
> 
> Now that you have split the different steps, adding the call to updateFont() just after this line would be the best option.

Had to make updateFont() public for this but seems to work fine!

[Alexander Pavlov (apavlov)]

(In reply to comment #17)
> (From update of attachment 111791 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111791&action=review
> 
> review- because of the issue Julien mentions
> 
> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:315
> > +        return "";
> 
> A more efficient empty string is emptyAtom. Is empty string really right for this? I don’t see any test cases covering this.

Sorry for not articulating the intention of these empty strings properly. In fact, ALL mandatory "font" longhands must exist in the style (at least implicitly), so the absence of ANY of those is an error. I have added ASSERT_NOT_REACHED() for this case.


> > Source/WebCore/css/CSSParser.cpp:3959
> > -                // do nothing, it's the inital value for all three
> > +                // Do nothing, it's the initial value for all three.
> > +                switch (valueOrdinal) {
> > +                case 0:
> > +                    styleImplicit = false;
> > +                    break;
> > +                case 1:
> > +                    variantImplicit = false;
> > +                    break;
> > +                case 2:
> > +                    weightImplicit = false;
> > +                    break;
> > +                }
> 
> The comment says “do nothing”, but now the code does something, so “do nothing” no longer makes sense.
> 
> > Source/WebCore/css/CSSParser.cpp:4039
> > +            // Default value, nothing to do
> 
> You didn’t add a period. I’m not sure “nothing to do” really describes code that does something. May need better wording.

Fixed the comments to be a bit more verbose (and correct). Also, fixed the issue Julien was concerned about.

[Alexander Pavlov (apavlov)]

Created attachment 111929 [details]
[PATCH] Comments addressed, take 3

[Nikolas Zimmermann]

Comment on attachment 111929 [details]
[PATCH] Comments addressed, take 3

View in context: https://bugs.webkit.org/attachment.cgi?id=111929&action=review

This looks good to me, especially as all bots are green now. I'd still like to see a test which doesn't use Inspector at all, that examines the font shorthand. Some comments remaining, that still lead to r-:

> Source/WebCore/ChangeLog:7
> +

This needs more explaination. The inline comments are very helpful, but still the whole context is missing.
An important fix deserves a longer description!

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:315
> +        return "";

return emptyString();

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:319
> +    success &= appendMandatoryPropertyValueIfExplicit(findPropertyWithId(CSSPropertyFontStyle), "", result);

Hm, I'd rather pass CSSPropertyFontStyle to appendMandatoryPropertyValueIfExplicit, instead of the CSSProperty*.

There's no need then to pass in stringBefore, as the value can be determined by the property.
static inline void appendStringBefore(CSSPropertyID id, StringBuilder& result) (a better name should be found!)
{
    switch (id) {
    case CSSPropertyFontSize:
        break;
     case CSSPropertyFontVariant:
        result.append(' ');
        return;
...
};

This will also use StringBuilder::append(char) rather than const char* and avoids an unncessary strlen.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:320
> +    success &= appendMandatoryPropertyValueIfExplicit(findPropertyWithId(CSSPropertyFontVariant), " ", result);

This would just read
success &= appendMandatoryPropertyValueIfExplicit(CSSPropertyFontVariant, result)
then.

> Source/WebCore/css/FontValue.cpp:40
> +            result.append(" ");

Use append(' ')

> Source/WebCore/css/FontValue.cpp:45
> +            result.append(" ");

Ditto.

> Source/WebCore/css/FontValue.cpp:50
> +            result.append(" ");

Ditto.

> Source/WebCore/css/FontValue.cpp:55
> +            result.append(" ");

Ditto.

> Source/WebCore/css/FontValue.cpp:56
> +        result.append("/");

'/'

> Source/WebCore/css/FontValue.cpp:61
> +            result.append(" ");

' '

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1888
> +    styleSelector->updateFont();

Why is this needed now? This needs to be documented.

[Alexander Pavlov (apavlov)]

Created attachment 115139 [details]
Patch

WildFox's comments addressed

[Alexander Pavlov (apavlov)]

(In reply to comment #21)
> (From update of attachment 111929 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=111929&action=review
> 
> This looks good to me, especially as all bots are green now. I'd still like to see a test which doesn't use Inspector at all, that examines the font shorthand. Some comments remaining, that still lead to r-:

Test added.

> > Source/WebCore/ChangeLog:7
> > +
> 
> This needs more explaination. The inline comments are very helpful, but still the whole context is missing.
> An important fix deserves a longer description!

Described what the patch actually does.

> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:315
> > +        return "";
> 
> return emptyString();

Done.

> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:319
> > +    success &= appendMandatoryPropertyValueIfExplicit(findPropertyWithId(CSSPropertyFontStyle), "", result);
> 
> Hm, I'd rather pass CSSPropertyFontStyle to appendMandatoryPropertyValueIfExplicit, instead of the CSSProperty*.
> 
> There's no need then to pass in stringBefore, as the value can be determined by the property.
> static inline void appendStringBefore(CSSPropertyID id, StringBuilder& result) (a better name should be found!)
> {
>     switch (id) {
>     case CSSPropertyFontSize:
>         break;
>      case CSSPropertyFontVariant:
>         result.append(' ');
>         return;
> ...
> };
> 
> This will also use StringBuilder::append(char) rather than const char* and avoids an unncessary strlen.

Merged the two suggested methods into one, since there were no other clients of 
appendMandatoryPropertyValueIfExplicit than the "font" property builder.

> > Source/WebCore/css/CSSMutableStyleDeclaration.cpp:320
> > +    success &= appendMandatoryPropertyValueIfExplicit(findPropertyWithId(CSSPropertyFontVariant), " ", result);
> 
> This would just read
> success &= appendMandatoryPropertyValueIfExplicit(CSSPropertyFontVariant, result)
> then.

Done.

> > Source/WebCore/css/FontValue.cpp:40
> > +            result.append(" ");
> 
> Use append(' ')
> 
> > Source/WebCore/css/FontValue.cpp:45
> > +            result.append(" ");
> 
> Ditto.
> 
> > Source/WebCore/css/FontValue.cpp:50
> > +            result.append(" ");
> 
> Ditto.
> 
> > Source/WebCore/css/FontValue.cpp:55
> > +            result.append(" ");
> 
> Ditto.
> 
> > Source/WebCore/css/FontValue.cpp:56
> > +        result.append("/");
> 
> '/'
> 
> > Source/WebCore/css/FontValue.cpp:61
> > +            result.append(" ");
> 
> ' '

Fixed all instances.

> > Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1888
> > +    styleSelector->updateFont();
> 
> Why is this needed now? This needs to be documented.

Added an explanation.

[Nikolas Zimmermann]

Comment on attachment 115139 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=115139&action=review

Looks great, r=me! Assuming everything turns green, it's ready to land.

> Source/WebCore/css/CSSStyleSelector.cpp:2396
> +void CSSStyleSelector::applyPropertyToCurrentStyle(int id, CSSValue *value)

"CSSValue* value". No idea why style bot doesn't complain here :-)

> Source/WebCore/html/canvas/CanvasRenderingContext2D.cpp:1999
> +    // As described in BUG66291, setting font-size on a font may entail a CSSPrimitiveValue::computeLengthDouble call,
> +    // which assumes the fontMetrics are available for the affected font, otherwise a crash occurs (see http://trac.webkit.org/changeset/96122).
> +    // The updateFont() call below updates the fontMetrics and ensures the proper setting of font-size.

Excellent, that's the information you want to read :-)

[Alexander Pavlov (apavlov)]

Committed r100273: <http://trac.webkit.org/changeset/100273>

[Simon Fraser (smfr)]

This broke a test:
http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r100277%20(34712)/platform/mac/plugins/jsobjc-dom-wrappers-pretty-diff.html

[Alexander Pavlov (apavlov)]

(In reply to comment #26)
> This broke a test:
> http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r100277%20(34712)/platform/mac/plugins/jsobjc-dom-wrappers-pretty-diff.html

Test fixed in http://trac.webkit.org/changeset/100451 ("font" is no longer mapped to DOMCSSValue).

[Eric Seidel (no email)]

I believe this change (correctly) caused all of the CSSFontProperty code in StyleResolver::applyProperty to be dead, and now could be removed. :)

[Eric Seidel (no email)]

Well, almost all the CSSPropertyFont code.  Blink removed the dead code in:
https://codereview.chromium.org/19678002/