Bug 48771 - Support background-clip: content-box
Summary: Support background-clip: content-box
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: krithigassree.sambamurthy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-01 11:45 PDT by Simon Fraser (smfr)
Modified: 2011-02-10 13:23 PST (History)
4 users (show)

See Also:


Attachments
Patch to add content-box as a valid background-clip value (3.87 KB, patch)
2011-01-10 12:33 PST, krithigassree.sambamurthy
simon.fraser: review-
Details | Formatted Diff | Diff
Patch to add content-box as a valid background-clip value (89.96 KB, patch)
2011-01-13 12:36 PST, krithigassree.sambamurthy
simon.fraser: review-
Details | Formatted Diff | Diff
Patch to add content-box as a valid background-clip value (11.88 KB, patch)
2011-01-13 14:05 PST, krithigassree.sambamurthy
simon.fraser: review+
Details | Formatted Diff | Diff
Patch to add content-box as a valid background-clip value (12.12 KB, patch)
2011-01-14 13:05 PST, krithigassree.sambamurthy
simon.fraser: review-
Details | Formatted Diff | Diff
Patch to add content-box as a valid background-clip value (11.75 KB, patch)
2011-01-17 11:56 PST, krithigassree.sambamurthy
simon.fraser: review+
commit-queue: commit-queue-
Details | Formatted Diff | Diff
Patch to add content-box as a valid background-clip value (6.20 KB, patch)
2011-01-18 11:03 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 Simon Fraser (smfr) 2010-11-01 11:45:09 PDT
parseBackgroundClip() doesn't accept content-box as a value for background-clip, but the spec includes it.
Comment 1 krithigassree.sambamurthy 2011-01-10 12:33:04 PST
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 2 Simon Fraser (smfr) 2011-01-10 12:54:44 PST
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.
Comment 3 krithigassree.sambamurthy 2011-01-13 12:36:45 PST
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 4 Simon Fraser (smfr) 2011-01-13 12:58:10 PST
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.
Comment 5 krithigassree.sambamurthy 2011-01-13 14:05:34 PST
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 6 Simon Fraser (smfr) 2011-01-13 14:22:43 PST
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.
Comment 7 krithigassree.sambamurthy 2011-01-14 13:05:23 PST
Created attachment 78984 [details]
Patch to add content-box as a valid background-clip value

Made changes to suggested in the review.
Comment 8 Simon Fraser (smfr) 2011-01-14 13:38:56 PST
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.
Comment 9 krithigassree.sambamurthy 2011-01-17 11:56:08 PST
Created attachment 79199 [details]
Patch to add content-box as a valid background-clip value

Hope this be will be it!
Comment 10 krithigassree.sambamurthy 2011-01-17 12:54:05 PST
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 11 Simon Fraser (smfr) 2011-01-17 13:52:23 PST
Comment on attachment 79199 [details]
Patch to add content-box as a valid background-clip value

Sorry, I picked the wrong option! r+
Comment 12 krithigassree.sambamurthy 2011-01-17 16:04:30 PST
Thanks a lot Simon for the review, and thanks Chang for committing the same
Comment 13 WebKit Commit Bot 2011-01-17 18:48:53 PST
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
Comment 14 krithigassree.sambamurthy 2011-01-18 11:03:59 PST
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 15 Chang Shu 2011-01-18 11:16:30 PST
Comment on attachment 79297 [details]
Patch to add content-box as a valid background-clip value

try again
Comment 16 WebKit Commit Bot 2011-01-18 12:38:03 PST
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>
Comment 17 WebKit Commit Bot 2011-01-18 12:38:08 PST
All reviewed patches have been landed.  Closing bug.
Comment 18 Simon Fraser (smfr) 2011-02-04 09:53:44 PST
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.
Comment 19 krithigassree.sambamurthy 2011-02-10 13:23:54 PST
Hi Simon,

I am working on another project, but will definitely check this when I find time.

Thanks