| Summary: | CSS3-selectors: first-line pseudo-element style's background image is not painted | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joonghun Park <jh718.park> | ||||||||||||||
| Component: | CSS | Assignee: | Jinwoo Song <jinwoo7.song> | ||||||||||||||
| Status: | RESOLVED INVALID | ||||||||||||||||
| Severity: | Normal | CC: | benjamin, buildbot, commit-queue, darin, esprehn+autocc, glenn, gyuyoung.kim, jinwoo7.song, kondapallykalyan, rniwa | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||
| Hardware: | All | ||||||||||||||||
| OS: | All | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Joonghun Park
2014-10-02 00:49:20 PDT
Especially in MacOS 10.9, for gif format this phenomenon is happening also and for png file this try always leads to fail. Created attachment 239425 [details]
Test case
Created attachment 240318 [details]
Patch
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. Created attachment 240339 [details]
Patch
Applied Benjamin's comments and added a layout test.
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. 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. 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? 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 Created attachment 245534 [details]
Patch
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 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
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 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
Comment on attachment 245534 [details]
Patch
Patch is causing fast/gradients/gradient-on-pseudoelement-crash.html to crash on EWS.
|