Bug 26084

Summary:

Multiple missing images in webkit-mask-image prevent rendering

Product:

WebKit

Reporter:

Mihnea Ovidenie <mihnea>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

Priority:

Version:

528+ (Nightly build)

Hardware:

Mac

OS:

OS X 10.5

Attachments:

Description	Flags
Propose a patch	darin: review-
New version of patch	darin: review-
WebArchive of LayoutTests before the patch	none
WebArchive of LayoutTests after the patch	none
New version of patch	darin: review+

Mihnea Ovidenie

Reported 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

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
View All Add attachment proposed patch, testcase, etc.

Mihnea Ovidenie

Comment 1 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

Darin Adler

Comment 2 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

Mihnea Ovidenie

Comment 3 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 :)

Mihnea Ovidenie

Comment 4 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?

Mihnea Ovidenie

Comment 5 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.

Mihnea Ovidenie

Comment 6 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.

Darin Adler

Comment 7 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.

Mihnea Ovidenie

Comment 8 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).

Darin Adler

Comment 9 2009-06-03 09:18:06 PDT

Comment on attachment 30900 [details] New version of patch r=me

Brent Fulgham

Comment 10 2009-06-04 11:44:50 PDT

Landed in @r44422.

Note You need to log in before you can comment on or make changes to this bug.