Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2794633002:
[SPv2] Fix logic for blending effect nodes due to non-isolated children of SVG. (Closed)
Description[SPv2] Fix logic for blending effect nodes due to non-isolated children of SVG.
An isolating effect node is needed if there is a non-isolated child.
Previously, we didn't do this correctly for LayoutSVGRoot PaintLayers that
are stacking contexts, as it relied on SVGLayoutSupport::isIsolationRequired,
which only returns true if there are non-isolated descendants *and* the current
node has blend mode.
BUG=665259
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Review-Url: https://codereview.chromium.org/2794633002
Cr-Commit-Position: refs/heads/master@{#461315}
Committed: https://chromium.googlesource.com/chromium/src/+/5e8b428fff011c6bb85fa02a5bdc754b2360a370
Patch Set 1 #Patch Set 2 : none #
Total comments: 4
Patch Set 3 : none #
Total comments: 1
Messages
Total messages: 28 (14 generated)
Description was changed from ========== none none BUG= ========== to ========== none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
Description was changed from ========== none none BUG= CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPv2] Fix logic for blending effect nodes due to non-isolated children of SVG. An isolating effect node is needed if there is a non-isolated child. Previously, we didn't do this correctly for LayoutSVGRoot PaintLayers that are stacking contexts, as it relied on SVGLayoutSupport::isIsolationRequired, which only returns true if there are non-isolated descendants *and* the current node has blend mode. BUG=665259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ==========
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
chrishtr@chromium.org changed reviewers: + trchen@chromium.org
This CL works for the referenced tests. But what about this situation?
<!DOCTYPE HTML>
<html>
<style>
div {
width: 100px;
height: 100px;
background-color: blue;
}
</style>
<body>
<p>The test passes if you see a green square.</p>
<div>
<svg xmlns="http://www.w3.org/2000/svg" version="1.1" width="100"
height="100">
<rect width="100" height="100" fill="#00FF00" style="mix-blend-mode:
difference"/>
</svg>
</div>
</body>
</html>
Should the SVG root act as a stacking context for the purpose of blending? If
so, the
output should be a green square, not light blue. Firefox, Safari and Chrome all
show a
light blue square.)
It seems the spec says to draw a light blue square, since the default for
isolation
of an SVG root is auto:
https://developer.mozilla.org/en-US/docs/Web/CSS/isolation
I think the example you made is okay. I'm thinking about this case:
<div style="width:100px; height:100px; background:blue;">
<svg xmlns="http://www.w3.org/2000/svg" version="1.1" width="100" height="100"
style="position:relative; z-index:0;">
<rect width="100" height="100" fill="#00FF00"
style="mix-blend-mode:difference"/>
</svg>
</div>
That is, the SVG root is considered stacking context (thus needs isolation), but
failed to notice blending children.
I think the logic goes like this:
needs_isolation_node = is_isolation_group && has_children_need_isolation
is_isolation_group = is_css_stacking_context || is_svg_isolation
has_children_need_isolation = contains_svg_flow ?
object->hasNonIsolatedBlendingDescendants() :
layer->hasNonIsolatedDescendantWithBlendMode()
is_css_stacking_context = isCSSIsolatedGroup (implies object.isBoxModelObject()
&& object.hasLayer())
is_svg_isolation =
SVGLayoutSupport::willIsolateBlendingDescendantsForObject(object)
The core problem is that SVG root qualifies both CSS object and SVG object, but
wasn't considered CSS object before this CL. However after your CL now SVG root
is considered CSS object, but then mistook its flow as non-SVG flow. Maybe the
following is the right answer?
// Can't omit effect node if we have paint children with exotic blending.
if (object.isSVG()) {
// Yes, including LayoutSVGRoot, because SVG layout objects don't create
// PaintLayer so PaintLayer::hasNonIsolatedDescendantWithBlendMode()
// doesn't catch SVG descendants.
if ((isCSSIsolatedGroup ||
SVGLayoutSupport::willIsolateBlendingDescendantsForObject(object)) &&
object->hasNonIsolatedBlendingDescendants())
effectNodeNeeded = true;
} else if (PaintLayer* layer = toLayoutBoxModelObject(object).layer()) {
if (layer->hasNonIsolatedDescendantWithBlendMode())
effectNodeNeeded = true;
}
On 2017/03/31 19:33:08, chrishtr wrote:
> This CL works for the referenced tests. But what about this situation?
>
> <!DOCTYPE HTML>
> <html>
> <style>
> div {
> width: 100px;
> height: 100px;
> background-color: blue;
> }
> </style>
> <body>
> <p>The test passes if you see a green square.</p>
> <div>
> <svg xmlns="http://www.w3.org/2000/svg" version="1.1" width="100"
> height="100">
> <rect width="100" height="100" fill="#00FF00" style="mix-blend-mode:
> difference"/>
> </svg>
> </div>
> </body>
> </html>
>
> Should the SVG root act as a stacking context for the purpose of blending? If
> so, the
> output should be a green square, not light blue. Firefox, Safari and Chrome
all
> show a
> light blue square.)
>
> It seems the spec says to draw a light blue square, since the default for
> isolation
> of an SVG root is auto:
>
> https://developer.mozilla.org/en-US/docs/Web/CSS/isolation
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by chrishtr@chromium.org to run a CQ dry run
Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2794633002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2794633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:488: if (layer->hasNonIsolatedDescendantWithBlendMode()) if (object->hasNonIsolatedDescendantWithBlendMode()) https://codereview.chromium.org/2794633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:491: if (SVGLayoutSupport::isIsolationRequired(&object)) else if (SVGLayoutSupport::isIsolationRequired(&object))
https://codereview.chromium.org/2794633002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2794633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:488: if (layer->hasNonIsolatedDescendantWithBlendMode()) On 2017/03/31 at 21:08:31, trchen wrote: > if (object->hasNonIsolatedDescendantWithBlendMode()) Done. https://codereview.chromium.org/2794633002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:491: if (SVGLayoutSupport::isIsolationRequired(&object)) On 2017/03/31 at 21:08:31, trchen wrote: > else if (SVGLayoutSupport::isIsolationRequired(&object)) Done.
lgtm
Oh wait, actually not lgtm. I think SVGRoot would be unconditionally isolating if we do this.
On 2017/03/31 at 21:30:42, trchen wrote: > Oh wait, actually not lgtm. I think SVGRoot would be unconditionally isolating if we do this. Why?
Sorry for the back and forth. I think the version I just commented should work. On 2017/03/31 21:31:07, chrishtr wrote: > On 2017/03/31 at 21:30:42, trchen wrote: > > Oh wait, actually not lgtm. I think SVGRoot would be unconditionally isolating > if we do this. > > Why? Because the early exit above doesn't exit when the object is SVG. We still want to check whether it is actually a CSS isolation group. https://codereview.chromium.org/2794633002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp (right): https://codereview.chromium.org/2794633002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp:486: if (object.isSVGRoot() && object.hasNonIsolatedBlendingDescendants()) if (isCSSIsolatedGroup && object.hasNonIsolatedBlendingDescendants())
Actually lgtm. The early exit checked for isSVGChild, not isSVG, so SVG root wouldn't be included. My bad!
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by chrishtr@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 40001, "attempt_start_ts": 1491018795585520,
"parent_rev": "cbd0a259ed199500f622b444cca8e54e60031203", "commit_rev":
"5e8b428fff011c6bb85fa02a5bdc754b2360a370"}
Message was sent while issue was closed.
Description was changed from ========== [SPv2] Fix logic for blending effect nodes due to non-isolated children of SVG. An isolating effect node is needed if there is a non-isolated child. Previously, we didn't do this correctly for LayoutSVGRoot PaintLayers that are stacking contexts, as it relied on SVGLayoutSupport::isIsolationRequired, which only returns true if there are non-isolated descendants *and* the current node has blend mode. BUG=665259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 ========== to ========== [SPv2] Fix logic for blending effect nodes due to non-isolated children of SVG. An isolating effect node is needed if there is a non-isolated child. Previously, we didn't do this correctly for LayoutSVGRoot PaintLayers that are stacking contexts, as it relied on SVGLayoutSupport::isIsolationRequired, which only returns true if there are non-isolated descendants *and* the current node has blend mode. BUG=665259 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2 Review-Url: https://codereview.chromium.org/2794633002 Cr-Commit-Position: refs/heads/master@{#461315} Committed: https://chromium.googlesource.com/chromium/src/+/5e8b428fff011c6bb85fa02a5bdc... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/5e8b428fff011c6bb85fa02a5bdc... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
