Skip to content

Commit f5b520f

Browse files
mdubetmdubet
committed
SVGRenderStyle should repaint on resolved color change
https://bugs.webkit.org/show_bug.cgi?id=250718 rdar://102904403 Reviewed by Antti Koivisto. Before this patch, the diff algorithm would naively compare the color properties value without taking into account the dynamic nature of "currentcolor". * LayoutTests/imported/w3c/web-platform-tests/svg/painting/svg-currentcolor-dynamic-inherit-expected.html: Added. * LayoutTests/imported/w3c/web-platform-tests/svg/painting/svg-currentcolor-dynamic-inherit.html: Added. * Source/WebCore/rendering/style/RenderStyle.cpp: (WebCore::RenderStyle::changeRequiresLayout const): (WebCore::RenderStyle::changeRequiresRepaint const): (WebCore::RenderStyle::diff const): * Source/WebCore/rendering/style/SVGRenderStyle.cpp: (WebCore::colorChangeRequiresRepaint): (WebCore::SVGRenderStyle::changeRequiresLayout const): (WebCore::SVGRenderStyle::changeRequiresRepaint const): (WebCore::SVGRenderStyle::diff const): Deleted. * Source/WebCore/rendering/style/SVGRenderStyle.h: Canonical link: https://commits.webkit.org/259082@main
1 parent cf856cc commit f5b520f

File tree

5 files changed

+110
-56
lines changed

5 files changed

+110
-56
lines changed

‎LayoutTests/imported/w3c/web-platform-tests/svg/painting/svg-currentcolor-dynamic-inherit-expected.html‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
<!DOCTYPE html>
2+
<head>
3+
<style>
4+
svg {
5+
width: 10%;
6+
}
7+
</style>
8+
</head>
9+
<body>
10+
<div>
11+
<svg xmlns="http://www.w3.org/2000/svg" id="root" version="1.1" viewBox="0 0 16 16">
12+
<circle fill="none" stroke="green" cx="8" cy="8" r="6"/>
13+
</svg>
14+
</div>
15+
</style>
16+
</body>
17+
</html>

‎LayoutTests/imported/w3c/web-platform-tests/svg/painting/svg-currentcolor-dynamic-inherit.html‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
<html class="reftest-wait">
2+
<head>
3+
<style>
4+
.active {
5+
color: green;
6+
}
7+
svg {
8+
width: 10%;
9+
}
10+
</style>
11+
<script src="/resources/testdriver.js"></script>
12+
<script src="/resources/testdriver-actions.js"></script>
13+
<script src="/resources/testdriver-vendor.js"></script>
14+
<script>
15+
addEventListener("load", async () => {
16+
setTimeout(() => {
17+
var svg = document.getElementById("root")
18+
svg.classList.add("active")
19+
requestAnimationFrame( () => {
20+
document.documentElement.classList.remove("reftest-wait");
21+
})
22+
},10);
23+
});
24+
</script>
25+
</head>
26+
<body>
27+
<div style="color:red">
28+
<svg xmlns="http://www.w3.org/2000/svg" id="root" version="1.1" viewBox="0 0 16 16">
29+
<circle fill="none" stroke="currentColor" cx="8" cy="8" r="6"/>
30+
</svg>
31+
</div>
32+
</body>
33+
</html>

‎Source/WebCore/rendering/style/RenderStyle.cpp‎

Lines changed: 8 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -890,6 +890,9 @@ static bool rareInheritedDataChangeRequiresLayout(const StyleRareInheritedData&
890890

891891
bool RenderStyle::changeRequiresLayout(const RenderStyle& other, OptionSet<StyleDifferenceContextSensitiveProperty>& changedContextSensitiveProperties) const
892892
{
893+
if (m_svgStyle->changeRequiresLayout(other.m_svgStyle))
894+
return true;
895+
893896
if (m_boxData.ptr() != other.m_boxData.ptr()) {
894897
if (m_boxData->width() != other.m_boxData->width()
895898
|| m_boxData->minWidth() != other.m_boxData->minWidth()
@@ -1197,6 +1200,11 @@ inline static bool changedCustomPaintWatchedProperty(const RenderStyle& a, const
11971200

11981201
bool RenderStyle::changeRequiresRepaint(const RenderStyle& other, OptionSet<StyleDifferenceContextSensitiveProperty>& changedContextSensitiveProperties) const
11991202
{
1203+
bool currentColorDiffers = m_inheritedData->color != other.m_inheritedData->color;
1204+
1205+
if (m_svgStyle->changeRequiresRepaint(other.m_svgStyle, currentColorDiffers))
1206+
return true;
1207+
12001208
if (!requiresPainting(*this) && !requiresPainting(other))
12011209
return false;
12021210

@@ -1207,7 +1215,6 @@ bool RenderStyle::changeRequiresRepaint(const RenderStyle& other, OptionSet<Styl
12071215
return true;
12081216

12091217

1210-
bool currentColorDiffers = m_inheritedData->color != other.m_inheritedData->color;
12111218
if (m_backgroundData.ptr() != other.m_backgroundData.ptr()) {
12121219
if (!m_backgroundData->isEquivalentForPainting(*other.m_backgroundData, currentColorDiffers))
12131220
return true;
@@ -1277,23 +1284,9 @@ StyleDifference RenderStyle::diff(const RenderStyle& other, OptionSet<StyleDiffe
12771284
{
12781285
changedContextSensitiveProperties = OptionSet<StyleDifferenceContextSensitiveProperty>();
12791286

1280-
StyleDifference svgChange = StyleDifference::Equal;
1281-
if (m_svgStyle != other.m_svgStyle) {
1282-
svgChange = m_svgStyle->diff(other.m_svgStyle.get());
1283-
if (svgChange == StyleDifference::Layout)
1284-
return svgChange;
1285-
}
1286-
12871287
if (changeRequiresLayout(other, changedContextSensitiveProperties))
12881288
return StyleDifference::Layout;
12891289

1290-
// SVGRenderStyle::diff() might have returned StyleDifference::Repaint, eg. if fill changes.
1291-
// If eg. the font-size changed at the same time, we're not allowed to return StyleDifference::Repaint,
1292-
// but have to return StyleDifference::Layout, that's why this if branch comes after all branches
1293-
// that are relevant for SVG and might return StyleDifference::Layout.
1294-
if (svgChange != StyleDifference::Equal)
1295-
return svgChange;
1296-
12971290
if (changeRequiresPositionedLayoutOnly(other, changedContextSensitiveProperties))
12981291
return StyleDifference::LayoutPositionedMovementOnly;
12991292

‎Source/WebCore/rendering/style/SVGRenderStyle.cpp‎

Lines changed: 50 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -135,17 +135,28 @@ void SVGRenderStyle::copyNonInheritedFrom(const SVGRenderStyle& other)
135135
m_layoutData = other.m_layoutData;
136136
}
137137

138-
StyleDifference SVGRenderStyle::diff(const SVGRenderStyle& other) const
138+
static bool colorChangeRequiresRepaint(const StyleColor& a, const StyleColor& b, bool currentColorDiffers)
139139
{
140-
// NOTE: All comparisions that may return StyleDifference::Layout have to go before those who return StyleDifference::Repaint
140+
if (a != b)
141+
return true;
141142

143+
if (a.isCurrentColor()) {
144+
ASSERT(b.isCurrentColor());
145+
return currentColorDiffers;
146+
}
147+
148+
return false;
149+
}
150+
151+
bool SVGRenderStyle::changeRequiresLayout(const SVGRenderStyle& other) const
152+
{
142153
// If kerning changes, we need a relayout, to force SVGCharacterData to be recalculated in the SVGRootInlineBox.
143154
if (m_textData != other.m_textData)
144-
return StyleDifference::Layout;
155+
return true;
145156

146157
// If markers change, we need a relayout, as marker boundaries are cached in RenderSVGPath.
147158
if (m_inheritedResourceData != other.m_inheritedResourceData)
148-
return StyleDifference::Layout;
159+
return true;
149160

150161
// All text related properties influence layout.
151162
if (m_inheritedFlags.textAnchor != other.m_inheritedFlags.textAnchor
@@ -154,72 +165,71 @@ StyleDifference SVGRenderStyle::diff(const SVGRenderStyle& other) const
154165
|| m_nonInheritedFlags.flagBits.alignmentBaseline != other.m_nonInheritedFlags.flagBits.alignmentBaseline
155166
|| m_nonInheritedFlags.flagBits.dominantBaseline != other.m_nonInheritedFlags.flagBits.dominantBaseline
156167
|| m_nonInheritedFlags.flagBits.baselineShift != other.m_nonInheritedFlags.flagBits.baselineShift)
157-
return StyleDifference::Layout;
168+
return true;
158169

159170
// Text related properties influence layout.
160-
bool miscNotEqual = m_miscData != other.m_miscData;
161-
if (miscNotEqual && m_miscData->baselineShiftValue != other.m_miscData->baselineShiftValue)
162-
return StyleDifference::Layout;
171+
if (m_miscData->baselineShiftValue != other.m_miscData->baselineShiftValue)
172+
return true;
163173

164174
// The x or y properties require relayout.
165175
if (m_layoutData != other.m_layoutData)
166-
return StyleDifference::Layout;
176+
return true;
167177

168178
// Some stroke properties, requires relayouts, as the cached stroke boundaries need to be recalculated.
169-
if (m_strokeData != other.m_strokeData) {
170-
if (m_strokeData->paintType != other.m_strokeData->paintType
171-
|| m_strokeData->paintColor != other.m_strokeData->paintColor
172-
|| m_strokeData->paintUri != other.m_strokeData->paintUri
173-
|| m_strokeData->dashArray != other.m_strokeData->dashArray
174-
|| m_strokeData->dashOffset != other.m_strokeData->dashOffset
175-
|| m_strokeData->visitedLinkPaintColor != other.m_strokeData->visitedLinkPaintColor
176-
|| m_strokeData->visitedLinkPaintUri != other.m_strokeData->visitedLinkPaintUri
177-
|| m_strokeData->visitedLinkPaintType != other.m_strokeData->visitedLinkPaintType)
178-
return StyleDifference::Layout;
179-
180-
// Only the stroke-opacity case remains, where we only need a repaint.
181-
ASSERT(m_strokeData->opacity != other.m_strokeData->opacity);
182-
return StyleDifference::Repaint;
183-
}
179+
if (m_strokeData->paintType != other.m_strokeData->paintType
180+
|| m_strokeData->paintUri != other.m_strokeData->paintUri
181+
|| m_strokeData->dashArray != other.m_strokeData->dashArray
182+
|| m_strokeData->dashOffset != other.m_strokeData->dashOffset
183+
|| m_strokeData->visitedLinkPaintUri != other.m_strokeData->visitedLinkPaintUri
184+
|| m_strokeData->visitedLinkPaintType != other.m_strokeData->visitedLinkPaintType)
185+
return true;
184186

185187
// vector-effect changes require a re-layout.
186188
if (m_nonInheritedFlags.flagBits.vectorEffect != other.m_nonInheritedFlags.flagBits.vectorEffect)
187-
return StyleDifference::Layout;
189+
return true;
188190

189-
// NOTE: All comparisions below may only return StyleDifference::Repaint
191+
return false;
192+
}
193+
194+
bool SVGRenderStyle::changeRequiresRepaint(const SVGRenderStyle& other, bool currentColorDiffers) const
195+
{
196+
if (m_strokeData->opacity != other.m_strokeData->opacity
197+
|| colorChangeRequiresRepaint(m_strokeData->paintColor, other.m_strokeData->paintColor, currentColorDiffers)
198+
|| colorChangeRequiresRepaint(m_strokeData->visitedLinkPaintColor, other.m_strokeData->visitedLinkPaintColor, currentColorDiffers))
199+
return true;
190200

191201
// Painting related properties only need repaints.
192-
if (miscNotEqual) {
193-
if (m_miscData->floodColor != other.m_miscData->floodColor
194-
|| m_miscData->floodOpacity != other.m_miscData->floodOpacity
195-
|| m_miscData->lightingColor != other.m_miscData->lightingColor)
196-
return StyleDifference::Repaint;
197-
}
202+
if (colorChangeRequiresRepaint(m_miscData->floodColor, other.m_miscData->floodColor, currentColorDiffers)
203+
|| m_miscData->floodOpacity != other.m_miscData->floodOpacity
204+
|| colorChangeRequiresRepaint(m_miscData->lightingColor, other.m_miscData->lightingColor, currentColorDiffers))
205+
return true;
198206

199207
// If fill data changes, we just need to repaint. Fill boundaries are not influenced by this, only by the Path, that RenderSVGPath contains.
200-
if (m_fillData->paintType != other.m_fillData->paintType || m_fillData->paintColor != other.m_fillData->paintColor
201-
|| m_fillData->paintUri != other.m_fillData->paintUri || m_fillData->opacity != other.m_fillData->opacity)
202-
return StyleDifference::Repaint;
208+
if (m_fillData->paintType != other.m_fillData->paintType
209+
|| colorChangeRequiresRepaint(m_fillData->paintColor, other.m_fillData->paintColor, currentColorDiffers)
210+
|| m_fillData->paintUri != other.m_fillData->paintUri
211+
|| m_fillData->opacity != other.m_fillData->opacity)
212+
return true;
203213

204214
// If gradient stops change, we just need to repaint. Style updates are already handled through RenderSVGGradientSTop.
205215
if (m_stopData != other.m_stopData)
206-
return StyleDifference::Repaint;
216+
return true;
207217

208218
// Changes of these flags only cause repaints.
209219
if (m_inheritedFlags.shapeRendering != other.m_inheritedFlags.shapeRendering
210220
|| m_inheritedFlags.clipRule != other.m_inheritedFlags.clipRule
211221
|| m_inheritedFlags.fillRule != other.m_inheritedFlags.fillRule
212222
|| m_inheritedFlags.colorInterpolation != other.m_inheritedFlags.colorInterpolation
213223
|| m_inheritedFlags.colorInterpolationFilters != other.m_inheritedFlags.colorInterpolationFilters)
214-
return StyleDifference::Repaint;
224+
return true;
215225

216226
if (m_nonInheritedFlags.flagBits.bufferedRendering != other.m_nonInheritedFlags.flagBits.bufferedRendering)
217-
return StyleDifference::Repaint;
227+
return true;
218228

219229
if (m_nonInheritedFlags.flagBits.maskType != other.m_nonInheritedFlags.flagBits.maskType)
220-
return StyleDifference::Repaint;
230+
return true;
221231

222-
return StyleDifference::Equal;
232+
return false;
223233
}
224234

225235
}

‎Source/WebCore/rendering/style/SVGRenderStyle.h‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,8 @@ class SVGRenderStyle : public RefCounted<SVGRenderStyle> {
4343
void inheritFrom(const SVGRenderStyle&);
4444
void copyNonInheritedFrom(const SVGRenderStyle&);
4545

46-
StyleDifference diff(const SVGRenderStyle&) const;
46+
bool changeRequiresRepaint(const SVGRenderStyle& other, bool currentColorDiffers) const;
47+
bool changeRequiresLayout(const SVGRenderStyle& other) const;
4748

4849
bool operator==(const SVGRenderStyle&) const;
4950
bool operator!=(const SVGRenderStyle& other) const { return !(*this == other); }

0 commit comments

Comments
 (0)