Bug 52080 - Add background-clip to background shorthand
Summary: Add background-clip to background shorthand
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P3 Normal
Assignee: krithigassree.sambamurthy
URL: http://www.w3.org/TR/css3-background/...
Keywords:
Depends on:
Blocks:
 
Reported: 2011-01-07 13:37 PST by krithigassree.sambamurthy
Modified: 2011-02-10 13:07 PST (History)
10 users (show)

See Also:


Attachments
Patch to add background-clip to background shorthand (6.77 KB, patch)
2011-01-07 14:26 PST, krithigassree.sambamurthy
eric: review-
Details | Formatted Diff | Diff
Patch to add background-clip to background shorthand and webkitMaskClip to Mask shorthand (12.36 KB, patch)
2011-01-25 09:50 PST, krithigassree.sambamurthy
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description krithigassree.sambamurthy 2011-01-07 13:37:41 PST
Background clip is not set correctly when given as a part of shorthand declaration
Comment 1 krithigassree.sambamurthy 2011-01-07 14:26:37 PST
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.
Comment 2 Eric Seidel (no email) 2011-01-10 13:44:03 PST
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.
Comment 3 krithigassree.sambamurthy 2011-01-11 13:04:31 PST
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
Comment 4 Eric Seidel (no email) 2011-01-20 13:37:08 PST
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.
Comment 5 Chang Shu 2011-01-20 18:32:14 PST
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
Comment 6 krithigassree.sambamurthy 2011-01-21 11:45:44 PST
@ 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.
Comment 7 Dave Hyatt 2011-01-21 12:17:58 PST
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.
Comment 8 krithigassree.sambamurthy 2011-01-25 09:50:56 PST
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.
Comment 9 Dave Hyatt 2011-01-31 11:32:36 PST
Comment on attachment 80073 [details]
Patch to add background-clip to background shorthand and webkitMaskClip to Mask shorthand

r=me
Comment 10 WebKit Commit Bot 2011-01-31 17:46:04 PST
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>
Comment 11 WebKit Commit Bot 2011-01-31 17:46:10 PST
All reviewed patches have been landed.  Closing bug.
Comment 12 Chang Shu 2011-02-01 09:59:21 PST
Ademar, can you cherry-pick this to qtwebkit2.1.x? thanks!
Comment 13 Benjamin Poulain 2011-02-01 10:09:31 PST
(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.
Comment 14 Suresh Voruganti 2011-02-10 13:07:29 PST
Removing it from Qtwebkit 2.1.1 Nice to have master bug.