Bug 26084 - Multiple missing images in webkit-mask-image prevent rendering
Summary: Multiple missing images in webkit-mask-image prevent rendering
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-05-29 12:03 PDT by Mihnea Ovidenie
Modified: 2009-06-04 11:44 PDT (History)
0 users

See Also:


Attachments
Propose a patch (4.20 KB, patch)
2009-05-29 12:13 PDT, Mihnea Ovidenie
darin: review-
Details | Formatted Diff | Diff
New version of patch (4.50 KB, patch)
2009-06-02 04:51 PDT, Mihnea Ovidenie
darin: review-
Details | Formatted Diff | Diff
WebArchive of LayoutTests before the patch (5.70 KB, text/html)
2009-06-02 04:57 PDT, Mihnea Ovidenie
no flags Details
WebArchive of LayoutTests after the patch (5.31 KB, text/html)
2009-06-02 04:58 PDT, Mihnea Ovidenie
no flags Details
New version of patch (3.99 KB, patch)
2009-06-03 04:17 PDT, Mihnea Ovidenie
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihnea Ovidenie 2009-05-29 12:03:37 PDT
When specifying more than one missing image in webkit-mask-image rule, the element for which the style is applied is not rendered correctly. If only one missing image is specified, the element is rendered.

<html>
<head>
	<script>
	function applyStyle() {
        document.getElementById("div1").setAttribute("style",
        "width:200px;"+
        "height:200px;"+
        "border:10px solid black;"+
        "background-color:blue;"+
        "-webkit-mask-image:url(url-not-found);");
        
        document.getElementById("div2").setAttribute("style",
        "width:200px;"+
        "height:200px;"+
        "border:10px solid black;"+
        "background-color:blue;"+
        "-webkit-mask-image:url(url-not-found), url(url-not-found);");
        
    }  
	</script>
</head>
<body onload="applyStyle()">
<div id="div1">This text is rendered.</div>
<div id="div2">This text is not rendered and it should be.</div>
</body>
</html>

Using the above html, the second div is not rendered while the first element is rendered. The problem appears in Safari 4 beta and WebKit 44021.

Regards,
Mihnea
Comment 1 Mihnea Ovidenie 2009-05-29 12:13:43 PDT
Created attachment 30785 [details]
Propose a patch

The patch modifies the function RenderBox::paintMaskImages. When having more than one mask layer, we have to check that at least one FillLayer contains a valid image before pushing a transparency layer.

Regards,
Mihnea
Comment 2 Darin Adler 2009-06-01 00:31:53 PDT
Comment on attachment 30785 [details]
Propose a patch

Looks good! Thanks for taking this on.

This patch has tabs in it, so we won't be able to land it as-is. Please re-submit it with spaces instead of tabs for indentation.

Formatting of the comment is also incorrect. There should be spaces after the "//" and the comment should use sentence style.

> +    	const FillLayer* fillLayer = style()->maskLayers()->next();
> +    	while (fillLayer) {
> +    		if (fillLayer->hasImage() && fillLayer->image()->canRender(style()->effectiveZoom())) {
> +    			pushTransparencyLayer = true;
> +    			break;
> +    		}
> +    		fillLayer = fillLayer->next();
> +    	}

The above loop would read better as a for loop, I think.

The patch needs to include layout test results as well as the new test; they can be generated with run-webkit-tests. I'm concerned that the results of this layout test won't indicate success or failure. This may need to be a manual test instead, or maybe the pixel result is the only way we can tell the test failed.

review- because of the tabs, formatting, and layout test concerns
Comment 3 Mihnea Ovidenie 2009-06-01 03:30:40 PDT
(In reply to comment #2)
> (From update of attachment 30785 [details] [review])
> Looks good! Thanks for taking this on.
> 
> This patch has tabs in it, so we won't be able to land it as-is. Please
> re-submit it with spaces instead of tabs for indentation.
> 
> Formatting of the comment is also incorrect. There should be spaces after the
> "//" and the comment should use sentence style.
> 
> > +    	const FillLayer* fillLayer = style()->maskLayers()->next();
> > +    	while (fillLayer) {
> > +    		if (fillLayer->hasImage() && fillLayer->image()->canRender(style()->effectiveZoom())) {
> > +    			pushTransparencyLayer = true;
> > +    			break;
> > +    		}
> > +    		fillLayer = fillLayer->next();
> > +    	}
> 
> The above loop would read better as a for loop, I think.
> 
> The patch needs to include layout test results as well as the new test; they
> can be generated with run-webkit-tests. I'm concerned that the results of this
> layout test won't indicate success or failure. This may need to be a manual
> test instead, or maybe the pixel result is the only way we can tell the test
> failed.
> 
> review- because of the tabs, formatting, and layout test concerns
> 

Thank you for your time spent reviewing this. I will take time to rework the patch and address your comments :)
Comment 4 Mihnea Ovidenie 2009-06-02 04:51:54 PDT
Created attachment 30865 [details]
New version of patch

I have reworked the patch to address Darin's comments. For the layout test, i have added a manual test as i was unsure how to test the problem automatically. 
I run the webkit test and checked that the patch does not break anything.
Darin, could you please tell me your #webkit IRC id so that i can ask you further questions about this patch if necessary?
Comment 5 Mihnea Ovidenie 2009-06-02 04:57:19 PDT
Created attachment 30866 [details]
WebArchive of LayoutTests before the patch

I was unsure about whether it is necessary to upload the results of LayoutTests before and after the patch, so here it is the result before the patch as a webarchive.
Comment 6 Mihnea Ovidenie 2009-06-02 04:58:58 PDT
Created attachment 30867 [details]
WebArchive of LayoutTests after the patch

Added the results of running LayoutTests after the patch. The patch does not break any existing tests.
Comment 7 Darin Adler 2009-06-02 12:22:32 PDT
Comment on attachment 30865 [details]
New version of patch

> +		Bug 26084: Multiple missing images in webkit-mask-image prevent rendering
> +		When painting multiple images, make sure that at least one image is valid before
> +		pushing a transparency layer.        

There are tabs here, and in the patch itself too. A patch with tabs can't be landed until the tabs are replaced with spaces; having tabs in the patch makes additional work for the person trying to commit the patch.

It's also good to include the bugs.webkit.org URL for the bug.

> Index: LayoutTests/fast/css/mask-composite-missing-images.html
> ===================================================================
> --- LayoutTests/fast/css/mask-composite-missing-images.html	(revision 0)
> +++ LayoutTests/fast/css/mask-composite-missing-images.html	(revision 0)
> @@ -0,0 +1,17 @@
> +<style>
> +.test {
> +    width: 200px;
> +    height: 200px;
> +    border:10px solid black;
> +    background-color:lime;
> +    -webkit-mask-image: url(url-not-found), url(url-not-found), url(url-not-found), url(url-not-found), url(url-not-found), url(url-not-found), url(url-not-found), url(url-not-found), url(url-not-found);
> +    -webkit-mask-position: top left, top right, bottom left, bottom right, top center, center right, bottom center, center left, center;
> +    -webkit-mask-origin: border;
> +    -webkit-mask-repeat: no-repeat, no-repeat, no-repeat, no-repeat, repeat-x, repeat-y, repeat-x, repeat-y, repeat;
> +    -webkit-mask-composite: copy;
> +}
> +</style>
> +
> +<p>Test for <a href="https://bugs.webkit.org/show_bug.cgi?id=26084">WebKit Bug 26084: Bug 26084: Multiple missing images in webkit-mask-image prevent rendering</a></p>
> +<div class="test">This text should be visible.</div>
> +<p>If the test passes, you should be able to see a lime square with a black border. Inside the border, you should see the text: This text should be visible.</p>
> \ No newline at end of file

If this is a manual test, then it needs to go in WebCore/manual-tests, not in LayoutTests.

Please put a newline at the end of the file.

review- because this adds a test to the LayoutTests directory without including expected results.
Comment 8 Mihnea Ovidenie 2009-06-03 04:17:29 PDT
Created attachment 30900 [details]
New version of patch

New version of patch addressing Darin's comments (thank you for your patience).
Comment 9 Darin Adler 2009-06-03 09:18:06 PDT
Comment on attachment 30900 [details]
New version of patch

r=me
Comment 10 Brent Fulgham 2009-06-04 11:44:50 PDT
Landed in @r44422.