parseBackgroundClip() doesn't accept content-box as a value for background-clip, but the spec includes it.
Created attachment 78430 [details] Patch to add content-box as a valid background-clip value This patch adds "content-box" as a valid value for background-clip.
Comment on attachment 78430 [details] Patch to add content-box as a valid background-clip value Please also add a pixel test in fast/css that tests whether the value actually works.
Created attachment 78843 [details] Patch to add content-box as a valid background-clip value Added pixel test to test the various background-clip values.
Comment on attachment 78843 [details] Patch to add content-box as a valid background-clip value View in context: https://bugs.webkit.org/attachment.cgi?id=78843&action=review > LayoutTests/fast/css/background-clip-values.html:36 > +<div id="box1"> > +Background color should be clipped to the content box. Padding space is white. > +</div> > +<br> > +Background color should be clipped to the padding box. Padding space is yellow. > +<br> > +Background color should be clipped to the border box. Yellow should be visible within the dashed border. > +</div> > +</body> If you remove the visible text from the testcase, your pixel results could be cross-platform (just put them in the same directory as the tests). You can keep the text there in an HTML comment.
Created attachment 78853 [details] Patch to add content-box as a valid background-clip value Thanks for the review. I hope I have incorporated your suggestions correctly. Thanks.
Comment on attachment 78853 [details] Patch to add content-box as a valid background-clip value r=me, but it would be good to make the borders thicker in your testcase so that the last two boxes are more obviously different.
Created attachment 78984 [details] Patch to add content-box as a valid background-clip value Made changes to suggested in the review.
Comment on attachment 78984 [details] Patch to add content-box as a valid background-clip value Very close! But the scrollbar in the pixel results will cause problems because scrollbars look different on different platforms.
Created attachment 79199 [details] Patch to add content-box as a valid background-clip value Hope this be will be it!
Hi Simon, I see a r- and do not see any comments/reason as to why it is a r- ? could you please elaborate. Thanks for taking time to review.
Comment on attachment 79199 [details] Patch to add content-box as a valid background-clip value Sorry, I picked the wrong option! r+
Thanks a lot Simon for the review, and thanks Chang for committing the same
Comment on attachment 79199 [details] Patch to add content-box as a valid background-clip value Rejecting attachment 79199 [details] from commit-queue. Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=eseidel-sf-cq', 'bu..." exit_code: 2 Last 500 characters of output: bgl ........................................................................... fast/clip .................... fast/compact ... fast/constructors . fast/cookies . fast/css-generated-content ........................................... fast/css ........................ fast/css/background-clip-values.html -> failed Exiting early after 1 failures. 6396 tests run. 144.62s total testing time 6395 test cases (99%) succeeded 1 test case (<1%) had incorrect layout 3 test cases (<1%) had stderr output Full output: http://queues.webkit.org/results/7542173
Created attachment 79297 [details] Patch to add content-box as a valid background-clip value Removed the <br> from background-clip-values.html pixel test since it seems to be causing cross platform issues while running layout tests.
Comment on attachment 79297 [details] Patch to add content-box as a valid background-clip value try again
Comment on attachment 79297 [details] Patch to add content-box as a valid background-clip value Clearing flags on attachment: 79297 Committed r76047: <http://trac.webkit.org/changeset/76047>
All reviewed patches have been landed. Closing bug.
Please check to see if any pixel results need to be updated after this change. I think at least fast/backgrounds/size/backgroundSize22.html needs new results.
Hi Simon, I am working on another project, but will definitely check this when I find time. Thanks