Bug 221253 – The layout of SVGImage should force the layout for its clients

• Component: CSS
• Status: RESOLVED FIXED

[andrei]

Initialy Safari 14.0.3 doesn't render image added by background-image property which use decoded base64 svg. 
If try set and unset background image from devtools than image showing.
If run script below image showing.
<script> 
document.querySelector('selector').style.setProperty('display', 'none');
setTimeout(()=>document.querySelector('selector').style.setProperty('display', 'block'), 0);
</script>

[Alexey Proskuryakov]

Thank you for the report!

Could you please attach a reduced test case?

[andrei]

html:
<div class="cf-single-answer">
<div class="cf-single-answer__text">asd</div>
</div>
css:
.cf-single-answer {
    background-image: url(data:image/svg+xml;charset=utf8,%3Csvg width='24' height='24' xmlns='http://www.w3.org/2000/svg'%3E%3Ccircle cx='12' cy='12' r='10' stroke='rgba(0, 0, 0, 0.54)' stroke-width='2' fill='none'/%3E%3C/svg%3E);
    background-size: 24px;
    background-repeat: no-repeat;
    background-position: left top;
    padding-left: 40px;
    line-height: 24px;
    min-height: 24px;
    cursor: pointer;
}

[Said Abou-Hallawa]

I think it is doable but the you have to be careful with encoding the angle brackets and the equations or the apostrophes in the Data URI string. See the attached working test case.

[Said Abou-Hallawa]

Created attachment 419108 [details]
working test case

[andrei]

(In reply to Said Abou-Hallawa from comment #3)
> I think it is doable but the you have to be careful with encoding the angle
> brackets and the equations or the apostrophes in the Data URI string. See
> the attached working test case.

I understand. But in Chrome, Firefox, Edge amd IE11 workaround with decoding svg is working clean.

[andrei]

(In reply to Said Abou-Hallawa from comment #4)
> Created attachment 419108 [details]
> working test case

Repro on Safari 15.0 on Catalina, and Safari 14.0 on Mojave, so seems to affect most versions of Safari.

[Said Abou-Hallawa]

Please attach the whole non working test case.

[andrei]

(In reply to Said Abou-Hallawa from comment #7)
> Please attach the whole non working test case.

i have attached above

[Said Abou-Hallawa]

Please attach the whole test case as a file which includes the css, the html document and the javascript. Click the "Add an attachment" above and attach it as an HTML file.

Also the background-image above:

background-image: url(data:image/svg+xml;charset=utf8,%3Csvg width='24' height='24' xmlns='http://www.w3.org/2000/svg'%3E%3Ccircle cx='12' cy='12' r='10' stroke='rgba(0, 0, 0, 0.54)' stroke-width='2' fill='none'/%3E%3C/svg%3E);

is not base54 encoded data url as indicated in the title.

[andrei]

(In reply to Said Abou-Hallawa from comment #9)
> Please attach the whole test case as a file which includes the css, the html
> document and the javascript. Click the "Add an attachment" above and attach
> it as an HTML file.
> 
> Also the background-image above:
> 
> background-image: url(data:image/svg+xml;charset=utf8,%3Csvg width='24'
> height='24' xmlns='http://www.w3.org/2000/svg'%3E%3Ccircle cx='12' cy='12'
> r='10' stroke='rgba(0, 0, 0, 0.54)' stroke-width='2'
> fill='none'/%3E%3C/svg%3E);
> 
> is not base54 encoded data url as indicated in the title.

Sorry. Renamed it.

[andrei]

Created attachment 419261 [details]
Clean Test Case

[andrei]

The issue can be connected with background-repeat property.

[Simon Fraser (smfr)]

I can reproduce.

[Simon Fraser (smfr)]

There's some caching thing going on. With the clean test case, the circle is missing on first load, but appears on reloading.

[Radar WebKit Bug Importer]

<rdar://problem/74138641>

[Said Abou-Hallawa]

This a regression of r257840.

[Jon Lee]

Andrei, which website is affected by this bug?

[andrei]

Jon Lee
It survey made by Confirmit. https://survey.testlab.firmglobal.net/wix/p699368492794.aspx
There doesn't displays radio buttons made by encoded svg in css

[Said Abou-Hallawa]

Created attachment 421617 [details]
Patch

[Said Abou-Hallawa]

Created attachment 421622 [details]
Patch

[Said Abou-Hallawa]

Created attachment 421627 [details]
Patch

[Said Abou-Hallawa]

Created attachment 421628 [details]
Patch

[Antti Koivisto]

Created attachment 421646 [details]
alternative approach

FWIW, here is a patch I made for this bug earlier. The idea is to let the first paint triggered by a resource load to always happen, even if the size has been computed to zero. It is bit hacky (but so is rest of this code). Adding here in case it is helpful.

[Simon Fraser (smfr)]

Comment on attachment 421628 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421628&action=review

> Source/WebCore/rendering/RenderBox.cpp:1818
> +    if (image && WTF::holds_alternative<CachedImage*>(*image) && WTF::get<CachedImage*>(*image)->hasSVGImage())
> +        setNeedsLayout();

It seems wrong to trigger layout for what might just be a CSS image change.

[Said Abou-Hallawa]

(In reply to Antti Koivisto from comment #23)
> Created attachment 421646 [details]
> alternative approach
> 
> FWIW, here is a patch I made for this bug earlier. The idea is to let the
> first paint triggered by a resource load to always happen, even if the size
> has been computed to zero. It is bit hacky (but so is rest of this code).
> Adding here in case it is helpful.

Your approach seems to be better. But this function 

bool canRender(...) { return ... && (m_inFinishLoading || !imageSizeForRenderer(...).isEmpty()); }

Became confusing because I would expect the opposite: canRender() should be true only after loading is finished and there is something to display. Something like:

bool canRender(...) { return ... && !m_inFinishLoading && !imageSizeForRenderer(...).isEmpty(); }

I will refine this approach a little to force calculateBackgroundImageGeometry() when RenderBox::imageChanged() is called.

[Said Abou-Hallawa]

Created attachment 421692 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 421692 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421692&action=review

> Source/WebCore/loader/cache/CachedImage.cpp:587
> +    setLoading(false);

CachedResource::finishLoading() calls setLoading(false) but we need it here to let the image clients know that the image is completely loaded.

[Simon Fraser (smfr)]

Comment on attachment 421692 [details]
Patch

Can we write a layout test?

[Said Abou-Hallawa]

(In reply to Simon Fraser (smfr) from comment #28)
> Comment on attachment 421692 [details]
> Patch
> 
> Can we write a layout test?

I am not able to get the bug to repro with run-webkit-tests. In fact I can't repro this bug with mini browser at all. I can only repro the bug on Safari with external http server. I can't repro it on Safari with local page or with localhost.

I am referring to the attached test case "Clean Test Case".

[alan]

(In reply to Said Abou-Hallawa from comment #29)
> (In reply to Simon Fraser (smfr) from comment #28)
> > Comment on attachment 421692 [details]
> > Patch
> > 
> > Can we write a layout test?
> 
> I am not able to get the bug to repro with run-webkit-tests. In fact I can't
> repro this bug with mini browser at all. I can only repro the bug on Safari
> with external http server. I can't repro it on Safari with local page or
> with localhost.
> 
> I am referring to the attached test case "Clean Test Case".
Not even with the test cases I attached to the radar? I used MiniBrowser to debug them.

[Said Abou-Hallawa]

Created attachment 421884 [details]
Patch

[Said Abou-Hallawa]

(In reply to zalan from comment #30)
> (In reply to Said Abou-Hallawa from comment #29)
> > (In reply to Simon Fraser (smfr) from comment #28)
> > > Comment on attachment 421692 [details]
> > > Patch
> > > 
> > > Can we write a layout test?
> > 
> > I am not able to get the bug to repro with run-webkit-tests. In fact I can't
> > repro this bug with mini browser at all. I can only repro the bug on Safari
> > with external http server. I can't repro it on Safari with local page or
> > with localhost.
> > 
> > I am referring to the attached test case "Clean Test Case".
> Not even with the test cases I attached to the radar? I used MiniBrowser to
> debug them.

You are right. Your test case shows the bug in mini-browser. And using run-webkit-tests, causes the following assertion to fire:

ASSERTION FAILED: !image->size().isEmpty()
svg/graphics/SVGImageCache.cpp(81) : WebCore::Image *WebCore::SVGImageCache::imageForRenderer(const WebCore::RenderObject *) const
1   0x2a9426bc9 WTFCrash
2   0x2886b00bb WTFCrashWithInfo(int, char const*, char const*, int)
3   0x28d47559d WebCore::SVGImageCache::imageForRenderer(WebCore::RenderObject const*) const
4   0x28c2b3ea3 WebCore::CachedImage::imageForRenderer(WebCore::RenderObject const*)
5   0x28c2b6650 WebCore::CachedImage::currentFrameKnownToBeOpaque(WebCore::RenderElement const*)
6   0x28cf85277 WebCore::StyleCachedImage::knownToBeOpaque(WebCore::RenderElement const&) const
7   0x28cf463ed WebCore::FillLayer::hasOpaqueImage(WebCore::RenderElement const&) const
8   0x28cc80de4 WebCore::RenderBox::paintFillLayers(WebCore::PaintInfo const&, WebCore::Color const&, WebCore::FillLayer const&, WebCore::LayoutRect const&, WebCore::BackgroundBleedAvoidance, WebCore::CompositeOperator, WebCore::RenderElement*)
9   0x28cc8447a WebCore::RenderBox::paintBackground(WebCore::PaintInfo const&, WebCore::LayoutRect const&, WebCore::BackgroundBleedAvoidance)
10  0x28cc81b3f WebCore::RenderBox::paintBoxDecorations(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
11  0x28cc1b56d WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
12  0x28cc1a8a3 WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
13  0x28cc1b06c WebCore::RenderBlock::paintChild(WebCore::RenderBox&, WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::PaintInfo&, bool, WebCore::RenderBlock::PaintBlockType)
14  0x28cc1ac81 WebCore::RenderBlock::paintChildren(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::PaintInfo&, bool)
15  0x28cc1abcf WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
16  0x28cc1be25 WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
17  0x28cc1a8a3 WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
18  0x28cc1b06c WebCore::RenderBlock::paintChild(WebCore::RenderBox&, WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::PaintInfo&, bool, WebCore::RenderBlock::PaintBlockType)
19  0x28cc1ac81 WebCore::RenderBlock::paintChildren(WebCore::PaintInfo&, WebCore::LayoutPoint const&, WebCore::PaintInfo&, bool)
20  0x28cc1abcf WebCore::RenderBlock::paintContents(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
21  0x28cc1be25 WebCore::RenderBlock::paintObject(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
22  0x28cc1a8a3 WebCore::RenderBlock::paint(WebCore::PaintInfo&, WebCore::LayoutPoint const&)
23  0x28cd6d8cc WebCore::RenderLayer::paintForegroundForFragmentsWithPhase(WebCore::PaintPhase, WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::PaintBehavior>, WebCore::RenderObject*)
24  0x28cd6b1cc WebCore::RenderLayer::paintForegroundForFragments(WTF::Vector<WebCore::LayerFragment, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&, WebCore::GraphicsContext&, WebCore::GraphicsContext&, WebCore::LayoutRect const&, bool, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::PaintBehavior>, WebCore::RenderObject*)
25  0x28cd66db8 WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
26  0x28cd6620d WebCore::RenderLayer::paintLayerContentsAndReflection(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
27  0x28cd650ba WebCore::RenderLayer::paintLayerWithEffects(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
28  0x28cd64360 WebCore::RenderLayer::paintLayer(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
29  0x28cd6ac68 WebCore::RenderLayer::paintList(WebCore::RenderLayer::LayerList, WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
30  0x28cd66ee0 WebCore::RenderLayer::paintLayerContents(WebCore::GraphicsContext&, WebCore::RenderLayer::LayerPaintingInfo const&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>)
31  0x28cd8be6d WebCore::RenderLayerBacking::paintIntoLayer(WebCore::GraphicsLayer const*, WebCore::GraphicsContext&, WebCore::IntRect const&, WTF::OptionSet<WebCore::PaintBehavior>, WebCore::EventRegionContext*)::$_24::operator()(WebCore::RenderLayer&, WTF::OptionSet<WebCore::RenderLayer::PaintLayerFlag>) const

[EWS]

Committed r273764: <https://commits.webkit.org/r273764>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 421884 [details].

[Simon Fraser (smfr)]

Comment on attachment 421884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421884&action=review

> LayoutTests/fast/css/background-svg-image-loading.html:19
> +        <div class="line">should have a visible background image</div>
> +        <div class="line">should have a visible background image</div>
> +        <div class="line">should have a visible background image</div>
> +        <div class="line">should have a visible background image</div>

Should these have had different sizes?

[Said Abou-Hallawa]

Comment on attachment 421884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=421884&action=review

>> LayoutTests/fast/css/background-svg-image-loading.html:19
>> +        <div class="line">should have a visible background image</div>
> 
> Should these have had different sizes?

Do you want to have different SVGImageForContainers in SVGImageCache one for each client (<div> element)? yes I think this make sense. I will make this change.