New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 600618 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Dec 2016
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Feature



Sign in to add a comment

Need to include SVG viewboxes in transform paint property tree nodes

Project Member Reported by pdr@chromium.org, Apr 5 2016

Issue description

SVG's viewboxes aren't currently included in transform paint property tree nodes. This causes overflowing icons on codepen.io and https://pr.gg/base64/

Example:
  <svg width="100" height="100">
    <svg width="100" height="100" viewBox="0 0 40 40">
      <rect x="10" y="10" width="10" height="10" fill="red"/>
    </svg>
  </svg>
 

Comment 1 by pdr@chromium.org, Apr 5 2016

Cc: trchen@chromium.org
Consider the following example:
<svg width="100" height="100" viewBox="0 0 40 60" style="transform: translate(50px, 50px);">
  <rect x="10" y="10" width="10" height="10" fill="red"/>
</svg>

In order to support composited animations (which existing content depends on), LayoutSVGRoot needs to use the CSS transform as if it was a regular replaced element. At the same time, LayoutSVGRoot can have an additional transform for the viewbox property. Until now we've just re-used ObjectPaintProperties.transform for both which mostly works because non-root SVG elements cannot have CSS transform properties. I think I will need to add another transform to ObjectPaintProperties to support this.
Project Member

Comment 2 by bugdroid1@chromium.org, Apr 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/143503e05b2d40592e84ec4213af92428dfe3964

commit 143503e05b2d40592e84ec4213af92428dfe3964
Author: pdr <pdr@chromium.org>
Date: Fri Apr 08 16:23:26 2016

Add a transform paint property for local SVG transforms

This patch adds a transform node for local SVG transforms which lets us
distinguish between SVG and CSS transforms. With SVG and CSS transforms
separated, the SVG root supports both types of transforms simultaneously.
This will let us support compositor-driven CSS transform animations in
the future.

With this patch we can remove the paint offset quirk for SVG. I've just
added TODOs to make this easier to review, but I plan it as an immediate
followup.

BUG= 600618 

Review URL: https://codereview.chromium.org/1865093004

Cr-Commit-Position: refs/heads/master@{#386095}

[modify] https://crrev.com/143503e05b2d40592e84ec4213af92428dfe3964/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/143503e05b2d40592e84ec4213af92428dfe3964/third_party/WebKit/Source/core/paint/ObjectPaintProperties.h
[modify] https://crrev.com/143503e05b2d40592e84ec4213af92428dfe3964/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/143503e05b2d40592e84ec4213af92428dfe3964/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
[modify] https://crrev.com/143503e05b2d40592e84ec4213af92428dfe3964/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp
[modify] https://crrev.com/143503e05b2d40592e84ec4213af92428dfe3964/third_party/WebKit/Source/core/paint/SVGRootPainter.cpp

Project Member

Comment 3 by bugdroid1@chromium.org, Apr 12 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d3767e5fce58934c6c34505cb21c0be93b212374

commit d3767e5fce58934c6c34505cb21c0be93b212374
Author: Xianzhu Wang <wangxianzhu@chromium.org>
Date: Tue Apr 12 04:48:40 2016

virtual/spv2/svg/as-image/svg-image-with-data-uri.html also crashes

It crashed more frequently on try-bot mac_chromium_rel_ng:
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=virtual%2Fspv2%2Fsvg%2Fas-image%2Fsvg-image-with-data-uri.html

BUG= 600618 
TBR=pdr@chromium.org

Review URL: https://codereview.chromium.org/1881853002 .

Cr-Commit-Position: refs/heads/master@{#386589}

[modify] https://crrev.com/d3767e5fce58934c6c34505cb21c0be93b212374/third_party/WebKit/LayoutTests/TestExpectations

Comment 4 by pdr@chromium.org, May 20 2016

Status: Fixed (was: Started)

Comment 5 by pdr@chromium.org, Jun 8 2016

Status: Started (was: Fixed)
I landed 1 / 3 patches... not sure why I marked this as fixed.
Project Member

Comment 6 by bugdroid1@chromium.org, Jun 14 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18f6c6a247ea3301c38e4d79a5785c7119f7cbca

commit 18f6c6a247ea3301c38e4d79a5785c7119f7cbca
Author: pdr <pdr@chromium.org>
Date: Tue Jun 14 01:35:42 2016

Re-implement SVG transform paint property nodes [spv2]

This patch adds a new property node for LayoutSVGRoot which maps from
the local SVG space to the HTML border box[1]. This patch also removes
the local SVG transform and makes SVG objects create transform nodes
in PaintPropertyTreeBuilder::updateTransform[2]. This approach will let
us support the other paint property types in SVG fairly easily in a
followup patch.

[1] Why do we need two transform nodes for the SVG root? LayoutSVGRoot
paints itself (e.g., borders, outlines) in the local HTML border box
transform space. LayoutSVGRoot's SVG children cannot paint in this space
because they need an additional transform (called the local svg to
border box) which offsets children by the border and includes SVG's
"viewbox".

[2] Why was m_svgLocalTransform removed? svgLocalTransform was designed
to support the local transform of both the SVG root and all SVG
descendants. Unfortunately, LayoutSVGRoot required that svgLocalTransform
get created after filters/clips in PaintPropertyTreeBuilder but that
left filters/clips in the wrong transform space. To solve this without
a parallel implementation of filters/clips/etc for SVG, SVG children
now create regular transform property nodes in PaintPropertyTreeBuilder.

BUG= 600618 

Review-Url: https://codereview.chromium.org/2045253005
Cr-Commit-Position: refs/heads/master@{#399635}

[modify] https://crrev.com/18f6c6a247ea3301c38e4d79a5785c7119f7cbca/third_party/WebKit/Source/core/paint/ObjectPaintProperties.h
[modify] https://crrev.com/18f6c6a247ea3301c38e4d79a5785c7119f7cbca/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/18f6c6a247ea3301c38e4d79a5785c7119f7cbca/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h
[modify] https://crrev.com/18f6c6a247ea3301c38e4d79a5785c7119f7cbca/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
[modify] https://crrev.com/18f6c6a247ea3301c38e4d79a5785c7119f7cbca/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp
[modify] https://crrev.com/18f6c6a247ea3301c38e4d79a5785c7119f7cbca/third_party/WebKit/Source/core/paint/SVGRootPainter.cpp

Project Member

Comment 7 by bugdroid1@chromium.org, Jun 15 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/18f6c6a247ea3301c38e4d79a5785c7119f7cbca

commit 18f6c6a247ea3301c38e4d79a5785c7119f7cbca
Author: pdr <pdr@chromium.org>
Date: Tue Jun 14 01:35:42 2016

Re-implement SVG transform paint property nodes [spv2]

This patch adds a new property node for LayoutSVGRoot which maps from
the local SVG space to the HTML border box[1]. This patch also removes
the local SVG transform and makes SVG objects create transform nodes
in PaintPropertyTreeBuilder::updateTransform[2]. This approach will let
us support the other paint property types in SVG fairly easily in a
followup patch.

[1] Why do we need two transform nodes for the SVG root? LayoutSVGRoot
paints itself (e.g., borders, outlines) in the local HTML border box
transform space. LayoutSVGRoot's SVG children cannot paint in this space
because they need an additional transform (called the local svg to
border box) which offsets children by the border and includes SVG's
"viewbox".

[2] Why was m_svgLocalTransform removed? svgLocalTransform was designed
to support the local transform of both the SVG root and all SVG
descendants. Unfortunately, LayoutSVGRoot required that svgLocalTransform
get created after filters/clips in PaintPropertyTreeBuilder but that
left filters/clips in the wrong transform space. To solve this without
a parallel implementation of filters/clips/etc for SVG, SVG children
now create regular transform property nodes in PaintPropertyTreeBuilder.

BUG= 600618 

Review-Url: https://codereview.chromium.org/2045253005
Cr-Commit-Position: refs/heads/master@{#399635}

[modify] https://crrev.com/18f6c6a247ea3301c38e4d79a5785c7119f7cbca/third_party/WebKit/Source/core/paint/ObjectPaintProperties.h
[modify] https://crrev.com/18f6c6a247ea3301c38e4d79a5785c7119f7cbca/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/18f6c6a247ea3301c38e4d79a5785c7119f7cbca/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.h
[modify] https://crrev.com/18f6c6a247ea3301c38e4d79a5785c7119f7cbca/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp
[modify] https://crrev.com/18f6c6a247ea3301c38e4d79a5785c7119f7cbca/third_party/WebKit/Source/core/paint/PaintPropertyTreePrinter.cpp
[modify] https://crrev.com/18f6c6a247ea3301c38e4d79a5785c7119f7cbca/third_party/WebKit/Source/core/paint/SVGRootPainter.cpp

Project Member

Comment 8 by bugdroid1@chromium.org, Jun 22 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/e06f2cf02f5ec0ce37164bf3e458593e61fb2e36

commit e06f2cf02f5ec0ce37164bf3e458593e61fb2e36
Author: pdr <pdr@chromium.org>
Date: Wed Jun 22 02:26:37 2016

Use transform paint property nodes in SVG [spv2]

This patch starts using transform paint property nodes when painting
SVG. Typically PaintLayer is responsible for noticing new property
nodes during painting but non-root SVG objects do not create PaintLayers
and need to check for properties themselves.

Several difficult transform areas remain such as marker transforms,
but this patch gets the bulk of SVG switched over. With this patch, 93
new layout tests pass with spv2 enabled.

This patch builds on https://codereview.chromium.org/2045253005

BUG= 600618 

Review-Url: https://codereview.chromium.org/2059433002
Cr-Commit-Position: refs/heads/master@{#401172}

[modify] https://crrev.com/e06f2cf02f5ec0ce37164bf3e458593e61fb2e36/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/e06f2cf02f5ec0ce37164bf3e458593e61fb2e36/third_party/WebKit/Source/core/paint/SVGContainerPainter.cpp
[modify] https://crrev.com/e06f2cf02f5ec0ce37164bf3e458593e61fb2e36/third_party/WebKit/Source/core/paint/SVGForeignObjectPainter.cpp
[modify] https://crrev.com/e06f2cf02f5ec0ce37164bf3e458593e61fb2e36/third_party/WebKit/Source/core/paint/SVGImagePainter.cpp
[modify] https://crrev.com/e06f2cf02f5ec0ce37164bf3e458593e61fb2e36/third_party/WebKit/Source/core/paint/SVGPaintContext.h
[modify] https://crrev.com/e06f2cf02f5ec0ce37164bf3e458593e61fb2e36/third_party/WebKit/Source/core/paint/SVGRootPainter.cpp
[modify] https://crrev.com/e06f2cf02f5ec0ce37164bf3e458593e61fb2e36/third_party/WebKit/Source/core/paint/SVGShapePainter.cpp
[modify] https://crrev.com/e06f2cf02f5ec0ce37164bf3e458593e61fb2e36/third_party/WebKit/Source/core/paint/SVGTextPainter.cpp

Project Member

Comment 9 by bugdroid1@chromium.org, Jul 6 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/58a321c5c07599b51b9a80d00c497049ed9bd171

commit 58a321c5c07599b51b9a80d00c497049ed9bd171
Author: pdr <pdr@chromium.org>
Date: Wed Jul 06 23:21:59 2016

Pixel snap SVG's root border box to local transform [spv2]

This patch extracts out SVGRootPainter's pixel snapping logic into a
common function used by both SVGRootPainter and the property tree
builder. With this patch a DCHECK in SVGTransformContext can be removed
and 77 new tests pass with --enable-slimming-paint-v2.

BUG= 600618 

Review-Url: https://codereview.chromium.org/2128463002
Cr-Commit-Position: refs/heads/master@{#403970}

[modify] https://crrev.com/58a321c5c07599b51b9a80d00c497049ed9bd171/third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2
[modify] https://crrev.com/58a321c5c07599b51b9a80d00c497049ed9bd171/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/58a321c5c07599b51b9a80d00c497049ed9bd171/third_party/WebKit/Source/core/paint/SVGPaintContext.h
[modify] https://crrev.com/58a321c5c07599b51b9a80d00c497049ed9bd171/third_party/WebKit/Source/core/paint/SVGRootPainter.cpp
[modify] https://crrev.com/58a321c5c07599b51b9a80d00c497049ed9bd171/third_party/WebKit/Source/core/paint/SVGRootPainter.h

Project Member

Comment 10 by bugdroid1@chromium.org, Jul 8 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/306affbd20fa8e16b8dd7677061fabde355ead8d

commit 306affbd20fa8e16b8dd7677061fabde355ead8d
Author: pdr <pdr@chromium.org>
Date: Fri Jul 08 20:45:57 2016

Ensure SVG PaintOffset is reset at the HTML/SVG boundary [spv2]

This patch fixes a crash on https://codepen.io where the paint offset
in PaintPropertyTreeBuilder::updateSvgLocalToBorderBoxTransform would
not be reset if transformToPixelSnappedBorderBox was identity.

The new testcase, SvgPixelSnappingShouldResetPaintOffset, both ensures
the DCHECK in PaintPropertyTreeBuilder::updateTransform is not hit, and
verifies the transform property tree is correct in this case.

BUG= 600618 

Review-Url: https://codereview.chromium.org/2131803002
Cr-Commit-Position: refs/heads/master@{#404483}

[modify] https://crrev.com/306affbd20fa8e16b8dd7677061fabde355ead8d/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilder.cpp
[modify] https://crrev.com/306affbd20fa8e16b8dd7677061fabde355ead8d/third_party/WebKit/Source/core/paint/PaintPropertyTreeBuilderTest.cpp

Project Member

Comment 11 by bugdroid1@chromium.org, Dec 13 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d798a7a82ead340d6206380d64e310c222416e62

commit d798a7a82ead340d6206380d64e310c222416e62
Author: pdr <pdr@chromium.org>
Date: Tue Dec 13 02:35:45 2016

Add support for caching and invalidating SVG marker transforms

This patch adds a cache of the SVG marker transform, similar to the pattern
used in LayoutSVGViewportContainer. This should be a small perf win because
we no longer re-calculate the transform when painting each marker instance.
With this new cache logic, invalidation logic has been added which includes
a fix for paint property under-invalidation (marker-viewBox-changes.svg).

BUG= 645667 , 600618 

Review-Url: https://codereview.chromium.org/2565943002
Cr-Commit-Position: refs/heads/master@{#437993}

[modify] https://crrev.com/d798a7a82ead340d6206380d64e310c222416e62/third_party/WebKit/LayoutTests/TestExpectations
[modify] https://crrev.com/d798a7a82ead340d6206380d64e310c222416e62/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.cpp
[modify] https://crrev.com/d798a7a82ead340d6206380d64e310c222416e62/third_party/WebKit/Source/core/layout/svg/LayoutSVGResourceMarker.h
[modify] https://crrev.com/d798a7a82ead340d6206380d64e310c222416e62/third_party/WebKit/Source/core/svg/SVGMarkerElement.cpp

Comment 12 by pdr@chromium.org, Dec 13 2016

Status: Fixed (was: Started)

Sign in to add a comment