137335 – CSS3-selectors: first-line pseudo-element style's background image is not painted

RESOLVED INVALID 137335

CSS3-selectors: first-line pseudo-element style's background image is not painted

https://bugs.webkit.org/show_bug.cgi?id=137335

Summary CSS3-selectors: first-line pseudo-element style's background image is not pai...

Joonghun Park

Reported 2014-10-02 00:49:20 PDT

::first-line pseudo-element is painted ramdomly. Sometimes background-image specified with png image file is painted well, and sometimes it is not painted. It seems that image loading is completed before painting when the png image is displayed well, and when it is not, image loading is completed after painting. And when image loading is completed, it seems there is no code flow to notify observers the image loading completion. The corresponding test case is in below url: https://github.com/w3c/csswg-test/blob/master/css-backgrounds-3/background-size-033.html

Attachments
Test case (1.22 KB, text/html) 2014-10-07 12:24 PDT, Benjamin Poulain	no flags	Details
Patch (4.84 KB, patch) 2014-10-22 18:42 PDT, Jinwoo Song	no flags	Details Formatted Diff Diff
Patch (10.34 KB, patch) 2014-10-23 03:42 PDT, Jinwoo Song	benjamin: review-	Details Formatted Diff Diff
Patch (12.95 KB, patch) 2015-01-28 05:49 PST, Joonghun Park	darin: review- buildbot: commit-queue-	Details Formatted Diff Diff
Archive of layout-test-results from ews100 for mac-mavericks (622.55 KB, application/zip) 2015-01-28 06:06 PST, Build Bot	no flags	Details
Archive of layout-test-results from ews106 for mac-mavericks-wk2 (691.12 KB, application/zip) 2015-01-28 06:44 PST, Build Bot	no flags	Details
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

Joonghun Park

Comment 1 2014-10-03 05:08:15 PDT

Especially in MacOS 10.9, for gif format this phenomenon is happening also and for png file this try always leads to fail.

Benjamin Poulain

Comment 2 2014-10-07 12:24:01 PDT

Created attachment 239425 [details] Test case

Jinwoo Song

Comment 3 2014-10-22 18:42:37 PDT

Created attachment 240318 [details] Patch

Benjamin Poulain

Comment 4 2014-10-22 20:44:02 PDT

Comment on attachment 240318 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240318&action=review r- because this will need some good test coverage to make sure every case is correct. > Source/WebCore/rendering/RenderElement.cpp:135 > + if (m_style->cachedPseudoStyles()) { if (const PseudoStyleCache* pseudoStyleCache = m_style->cachedPseudoStyles()) { ... > Source/WebCore/rendering/RenderElement.cpp:136 > + for (size_t i = 0; i < m_style->cachedPseudoStyles()->size(); ++i) { I think you could use a modern for-each() loop here.

Jinwoo Song

Comment 5 2014-10-23 03:42:19 PDT

Created attachment 240339 [details] Patch Applied Benjamin's comments and added a layout test.

Benjamin Poulain

Comment 6 2014-10-23 20:16:18 PDT

Comment on attachment 240339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240339&action=review > LayoutTests/ChangeLog:15 > + * css3/pseudo-element-background-image.html: Added. > + * css3/support/blue.png: Added. > + * css3/support/green.png: Added. This is not a test. You should have a test that fails before your patch, and succeed after your patch. Another aspect of a good test is that it should be easy(-ish) to understand the results. Could this be tested with a ref-test for example? Maybe: -Set waitUntilDone -Use 4 images for the 4 types of pseudo elements. -On the load event -Force a layout. -Insert the 4 images with <img> tags -Add an event listener for the image load -When the 4 <img> are loaded, finish the test. For the reference, just use regular colored block to fake the images. Wouldn't that work? > LayoutTests/css3/pseudo-element-background-image.html:2 > +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0 Transitional//EN" "http://www.w3.org/TR/REC-html40/loose.dtd"> > +<HTML> Let's use HTML5 to avoid quirks mode. Use the HTML5 doctype and lowercase tagnames.

Jinwoo Song

Comment 7 2014-10-24 02:48:49 PDT

Comment on attachment 240339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240339&action=review >> LayoutTests/ChangeLog:15 >> + * css3/support/green.png: Added. > > This is not a test. You should have a test that fails before your patch, and succeed after your patch. > > Another aspect of a good test is that it should be easy(-ish) to understand the results. > > Could this be tested with a ref-test for example? Maybe: > -Set waitUntilDone > -Use 4 images for the 4 types of pseudo elements. > -On the load event > -Force a layout. > -Insert the 4 images with <img> tags > -Add an event listener for the image load > -When the 4 <img> are loaded, finish the test. > For the reference, just use regular colored block to fake the images. > > Wouldn't that work? This bug is happening only when the ::first-line style set the background-image. Other pseudo elements does not have this issue. So the simplest test case I was thinking was to compare the pixel test result with the following. <style> p::first-line { background-image: url(./green.png); color: white; } </style> <p id="first">PASS</p> I could not get it exactly your suggestion, especially for <img> tags. Could you explain more for this? Thanks in advance! >> LayoutTests/css3/pseudo-element-background-image.html:2 >> +<HTML> > > Let's use HTML5 to avoid quirks mode. Use the HTML5 doctype and lowercase tagnames. Sure, I'll apply it.

Darin Adler

Comment 8 2014-10-24 14:33:24 PDT

Comment on attachment 240339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240339&action=review > Source/WebCore/rendering/RenderElement.cpp:137 > + if (pseudoStyle.get()->styleType() == FIRST_LINE) { Should not need .get() here. > Source/WebCore/rendering/RenderElement.cpp:138 > + for (const FillLayer* backgroundLayer = pseudoStyle.get()->backgroundLayers(); backgroundLayer; backgroundLayer = backgroundLayer->next()) { Should not need .get() here. > Source/WebCore/rendering/RenderElement.cpp:140 > + backgroundLayer->image()->removeClient(const_cast<RenderElement*>(this)); What is the const_cast for here? This is a destructor, so this should not be const. > Source/WebCore/rendering/style/StyleCachedImage.cpp:97 > + if (!m_image->hasClient(renderer)) > + m_image->addClient(renderer); I don’t understand this change. Doing the extra lookup isn’t so great for performance. Is this just to prevent an assertion from firing? If so, what prevents us from removing the client too soon?

Chris Dumez

Comment 9 2014-10-24 14:37:42 PDT

Comment on attachment 240339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=240339&action=review > Source/WebCore/rendering/RenderBox.cpp:1613 > + for (const FillLayer* curLayer = firstLineStyle().backgroundLayers(); curLayer; curLayer = curLayer->next()) { nit: s/curLayer/currentLayer

Joonghun Park

Comment 10 2015-01-28 05:49:21 PST

Created attachment 245534 [details] Patch

Build Bot

Comment 11 2015-01-28 06:06:41 PST

Comment on attachment 245534 [details] Patch Attachment 245534 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/4584983578542080 New failing tests: fast/gradients/gradient-on-pseudoelement-crash.html

Build Bot

Comment 12 2015-01-28 06:06:45 PST

Created attachment 245536 [details] Archive of layout-test-results from ews100 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews100 Port: mac-mavericks Platform: Mac OS X 10.9.5

Build Bot

Comment 13 2015-01-28 06:44:25 PST

Comment on attachment 245534 [details] Patch Attachment 245534 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5741778258886656 New failing tests: fast/gradients/gradient-on-pseudoelement-crash.html

Build Bot

Comment 14 2015-01-28 06:44:29 PST

Created attachment 245538 [details] Archive of layout-test-results from ews106 for mac-mavericks-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-mavericks-wk2 Platform: Mac OS X 10.9.5

Darin Adler

Comment 15 2015-01-28 23:58:55 PST

Comment on attachment 245534 [details] Patch Patch is causing fast/gradients/gradient-on-pseudoelement-crash.html to crash on EWS.

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

Status RESOLVED

Resolution INVALID

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware All

OS All

Product WebKit

Component CSS

Assignee

Jinwoo Song

Reported

2014-10-02 00:49 PDT

Modified

2019-05-03 06:57 PDT History

CC List

10 users Show

URL

Keywords

Depends on

Blocks