CSSStyleDeclaration.getPropertyPriority() fails for CSS shorthand properties with 'important' priority

[Xianzhu Wang]

Created attachment 73040 [details]
Test case

For a style rule containing a declaration of a shorthand property (e.g. 'background'):
  background: yellow !important;
rule.getPropertyPriority('background') returns ''.
Firefox returns 'important'.

[Yi Shen]

(In reply to comment #0)
> Created an attachment (id=73040) [details]
> Test case
> 
> For a style rule containing a declaration of a shorthand property (e.g. 'background'):
>   background: yellow !important;
> rule.getPropertyPriority('background') returns ''.
> Firefox returns 'important'.

Is there any specification for this? If yes, I can try work on it.

[Alexis Menard (darktears)]

Created attachment 124937 [details]
Patch

[Alexis Menard (darktears)]

Created attachment 124940 [details]
Patch

[Antti Koivisto]

Comment on attachment 124940 [details]
Patch

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

> Source/WebCore/css/CSSMutableStyleDeclaration.h:147
> +    bool getCommonPriority(const int* properties, size_t) const;

Please don't use get* naming. Follow the normal WebKit style instead (I know this class has existing cases).

The internal priority functions should either return enum or be named differently (bool hasImportantCommonPriority() or similar)

[Antti Koivisto]

It would be good to understand what other engines do in these cases.

[Alexis Menard (darktears)]

Created attachment 124948 [details]
Patch

[Andreas Kling]

Comment on attachment 124948 [details]
Patch

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

> Source/WebCore/ChangeLog:31
> [phone]  Alexis Menard  <[e-mail]>

Double ChangeLog.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:661
> +bool CSSMutableStyleDeclaration::hasPropertyImportantPriority(int propertyID) const

I like propertyIsImportant(int propertyID) better.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:689
> +// Only returns true if all properties have the same priority false otherwise.
> +bool CSSMutableStyleDeclaration::hasImportantCommonPriority(const int* properties, size_t size) const
> +{
> +    bool res = hasPropertyImportantPriority(properties[0]);
> +    // The first longhand doesn't have priority, we can assume the others have equal priority.
> +    // If not it doesn't matter as we can't anyway resolve the priority of the shorthand.
> +    if (!res)
> +        return false;
> +
> +    for (size_t i = 1; i < size; ++i) {
> +        bool priority = hasPropertyImportantPriority(properties[i]);
> +        if (res != priority)
> +            return false;
> +    }
> +    return res;
>  }

This function doesn't do what its name implies. It only checks that all properties return true for hasPropertyImportantPriority() (since you return early if !res)

> Source/WebCore/css/CSSMutableStyleDeclaration.h:147
> +    bool hasImportantCommonPriority(const int* properties, size_t) const;

Should be static as it doesn't require an object.

[Alexis Menard (darktears)]

Created attachment 124978 [details]
Patch

[Andreas Kling]

Comment on attachment 124978 [details]
Patch

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

> Source/WebKit/qt/ChangeLog:8
> +        Update the code as getPropertyPriority has been renamed to hasPropertyImportantPriority.

hasPropertyImportantPriority -> propertyIsImportant

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:663
> +    for (size_t i = 0; i < longhands.length(); ++i) {

longhands.length() returns unsigned so 'i' should be unsigned as well.

> Source/WebCore/css/CSSMutableStyleDeclaration.cpp:680
> +    return containsOnlyImportantProperties(longhands);

I think we can fold containsOnlyImportantProperties() into the propertyIsImportant() here, no need for it to be a separate function.

> LayoutTests/fast/css/shorthand-priority.html:10
> +description("Tests that query the priority for a shorthand works.");

query -> querying

[Alexis Menard (darktears)]

Created attachment 124980 [details]
Patch for landing

[WebKit Review Bot]

Attachment 124980 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 1 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexis Menard (darktears)]

Created attachment 124982 [details]
Patch for landing

[Alexis Menard (darktears)]

Created attachment 124983 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 124983 [details]
Patch for landing

Clearing flags on attachment: 124983

Committed r106490: <http://trac.webkit.org/changeset/106490>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.