Bug 52080 – Add background-clip to background shorthand

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

Background clip is not set correctly when given as a part of shorthand declaration

Created attachment 78270 [details]
Patch to add background-clip to background shorthand

Patch adds background-clip to background shorthand. If one <box> value is present both background-origin and background-clip take the same values. If two <box> values are present the first is assigned to origin and the next to clip.

Updated the clipValue to store the right value . Added foundClip boolean variable to ensure that the right value is updated.

[Eric Seidel (no email)]

Comment on attachment 78270 [details]
Patch to add background-clip to background shorthand

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

It seems we need more testing than this, no?  Don't we need examples of it being parsed as part of the shorthand?  Or am I misunderstanding?

> WebCore/css/CSSParser.cpp:1966
> +        if (!parsedProperty[i]) {            

whitespace.

Hi Eric,

Thanks for the review. The background-clip-text.js has examples of background-clip added to the shorthand.

Earlier implementation had background-origin as a part of the background shorthand. If there was a box  value present for origin , clip took the same value. Any other box values were ignored. The code now specifically looks for background-clip values apart from the origin.

According to spec, if one value is present it is assigned to both origin and clip, if two values are present then assigned to origin and clip (in that order).

Do let me know if you feel we need more test cases ? If there is nothing else I can remove the extra whitespace and resubmit the patch.

Thanks

[Eric Seidel (no email)]

Comment on attachment 78270 [details]
Patch to add background-clip to background shorthand

Don't we need examples of this parsing separate background clip example CSS?  I don't see how the background-clip-text.js test is sufficient here.

[Chang Shu]

Comment on attachment 78270 [details]
Patch to add background-clip to background shorthand

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

Just found some cosmetic issues. Could you take care of them in your next patch? thanks.

> WebCore/ChangeLog:9
> +        (WebCore::CSSParser::parseValue): Pass background-clip as additonal property

typo here

> WebCore/css/CSSParser.cpp:1892
> +    bool parsedProperty[cMaxFillProperties] = { false };   

remove extra spaces so it won't appear in the next patch.

> WebCore/css/CSSParser.cpp:1953
> +                   }

need one more space here

> LayoutTests/fast/css/script-tests/background-clip-text.js:-16
> -

revert this change

@ Eric
I am not sure whether I understood your comment correctly,

Background-clip is parsed correctly when specified explicitly or in the "longhand" version. Existing layout test background-clip-values.html tests for this.  My changes will only make sure that when specified as part of shorthand the background-clip value is correctly set. 

As part of background-shorthand , there can be no/one/two box values. The spec has clear instructions on how this should be processed and the background-clip-text.js tests this.

@Chang,
thanks for the review. Will include it in my next patch.

[Dave Hyatt]

Comment on attachment 78270 [details]
Patch to add background-clip to background shorthand

Please add mask clip to the mask shorthand as well. We try to keep background and mask stuff in sync.

Created attachment 80073 [details]
Patch to add background-clip to background shorthand and webkitMaskClip to Mask shorthand

Made the changes as advised by the reviewers

1. Added additional test cases for checking background-clip when declared as shorthand.
2. Made the Mask shorthand in sync and added test cases to check the same.
3. Removed the unnecessary white spaces.

[Dave Hyatt]

Comment on attachment 80073 [details]
Patch to add background-clip to background shorthand and webkitMaskClip to Mask shorthand

r=me

[WebKit Commit Bot]

Comment on attachment 80073 [details]
Patch to add background-clip to background shorthand and webkitMaskClip to Mask shorthand

Clearing flags on attachment: 80073

Committed r77186: <http://trac.webkit.org/changeset/77186>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chang Shu]

Ademar, can you cherry-pick this to qtwebkit2.1.x? thanks!

[Benjamin Poulain]

(In reply to comment #12)
> Ademar, can you cherry-pick this to qtwebkit2.1.x? thanks!

I think it is time to clarify what goes in a patch release, and what is a stabilization fix. IMHO, this is not a stabilization patch, it is a new feature. And that the root of all the problems of 2.1.x, it is not a serious stabilization branch, it is a just a branch.

[Suresh Voruganti]

Removing it from Qtwebkit 2.1.1 Nice to have master bug.