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.
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
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 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.
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.
Comment on attachment 80073 [details] Patch to add background-clip to background shorthand and webkitMaskClip to Mask shorthand r=me
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>
All reviewed patches have been landed. Closing bug.
Ademar, can you cherry-pick this to qtwebkit2.1.x? thanks!
(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.
Removing it from Qtwebkit 2.1.1 Nice to have master bug.