Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1158323005:
Include 'outline' in LayoutSVGBlock::visualOverflowRect (Closed)
|
Created:
5 years, 6 months ago by fs Modified:
5 years, 6 months ago CC:
blink-reviews, blink-reviews-rendering, krit, eae+blinkwatch, f(malita), gyuyoung2, jchaffraix+rendering, kouhei+svg_chromium.org, leviw+renderwatch, pdr+renderingwatchlist_chromium.org, pdr+svgwatchlist_chromium.org, rwlbuis, Stephen Chennney, szager+layoutwatch_chromium.org, zoltan1 Base URL:
svn://svn.chromium.org/blink/trunk Target Ref:
refs/heads/master Project:
blink Visibility:
Public. |
DescriptionInclude 'outline' in LayoutSVGBlock::visualOverflowRect
LayoutSVGBlock doesn't maintain overflow via LayoutBox::m_overflow, but
rather overrides LayoutBox::visualOverflowRect.
Instead of doing that, add visual effect overflow during layout of
LayoutSVGText and stop overriding said method.
This will include outsets for border-image (which doesn't apply
to any of <text> or <foreignObject>) too in the unlikely event it's
specified.
TEST=svg/text/text-outline-2.html (S.P. strict cull rect)
BUG=495368
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=196591
Patch Set 1 #
Total comments: 3
Patch Set 2 : Move fix to layout-side. #
Total comments: 2
Messages
Total messages: 20 (2 generated)
fs@opera.com changed reviewers: + chrishtr@chromium.org, pdr@chromium.org
You should also be able to test this CL in a similar way to https://codereview.chromium.org/1158193011 (position right at a tile boundary, then move the SVG object in the next frame to demonstrate a paint invalidation bug. https://codereview.chromium.org/1158323005/diff/1/Source/core/layout/svg/Layo... File Source/core/layout/svg/LayoutSVGBlock.cpp (right): https://codereview.chromium.org/1158323005/diff/1/Source/core/layout/svg/Layo... Source/core/layout/svg/LayoutSVGBlock.cpp:43: borderRect.expand(computeVisualEffectOverflowOutsets()); LayoutBox computes visual overflow during layout. Why not do the same here? e.g. during layout, call addVisualEffectOverflow() at the appropriate time, and then not override this method here, since LayoutBox already stores the rect.
On 2015/06/02 15:51:05, chrishtr wrote: > You should also be able to test this CL in a similar way to > https://codereview.chromium.org/1158193011 (position right at a tile boundary, > then move the SVG object in the next frame to demonstrate a paint invalidation > bug. This method is not used during paint invalidation - this is related to the below (and the fact that we need to track/compute transforms during repaint for SVG.) > https://codereview.chromium.org/1158323005/diff/1/Source/core/layout/svg/Layo... > File Source/core/layout/svg/LayoutSVGBlock.cpp (right): > > https://codereview.chromium.org/1158323005/diff/1/Source/core/layout/svg/Layo... > Source/core/layout/svg/LayoutSVGBlock.cpp:43: > borderRect.expand(computeVisualEffectOverflowOutsets()); > LayoutBox computes visual overflow during layout. Why not do the same here? e.g. > during layout, call addVisualEffectOverflow() at the appropriate time, and then > not override this method here, since LayoutBox already stores the rect. Yes, preferably that's what we would do. That method is only used for the SVG root right now though - likely because of how paint invalidation differs. I've filed crbug.com/495666 to try and improve this a bit, but felt this was the wrong vehicle for those kinds of fixes.
On 2015/06/02 at 16:23:06, fs wrote: > On 2015/06/02 15:51:05, chrishtr wrote: > > You should also be able to test this CL in a similar way to > > https://codereview.chromium.org/1158193011 (position right at a tile boundary, > > then move the SVG object in the next frame to demonstrate a paint invalidation > > bug. > > This method is not used during paint invalidation - this is related to the below (and the fact that we need to track/compute transforms during repaint for SVG.) How does paint invalidation for this type of object work then? Does it invalidate only at the root? > > > https://codereview.chromium.org/1158323005/diff/1/Source/core/layout/svg/Layo... > > File Source/core/layout/svg/LayoutSVGBlock.cpp (right): > > > > https://codereview.chromium.org/1158323005/diff/1/Source/core/layout/svg/Layo... > > Source/core/layout/svg/LayoutSVGBlock.cpp:43: > > borderRect.expand(computeVisualEffectOverflowOutsets()); > > LayoutBox computes visual overflow during layout. Why not do the same here? e.g. > > during layout, call addVisualEffectOverflow() at the appropriate time, and then > > not override this method here, since LayoutBox already stores the rect. > > Yes, preferably that's what we would do. That method is only used for the SVG root right now though - likely because of how paint invalidation differs. I've filed crbug.com/495666 to try and improve this a bit, but felt this was the wrong vehicle for those kinds of fixes.
On 2015/06/02 16:27:46, chrishtr wrote: > On 2015/06/02 at 16:23:06, fs wrote: > > On 2015/06/02 15:51:05, chrishtr wrote: > > > You should also be able to test this CL in a similar way to > > > https://codereview.chromium.org/1158193011 (position right at a tile > boundary, > > > then move the SVG object in the next frame to demonstrate a paint > invalidation > > > bug. > > > > This method is not used during paint invalidation - this is related to the > below (and the fact that we need to track/compute transforms during repaint for > SVG.) > > How does paint invalidation for this type of object work then? Does it > invalidate only at the root? In a way I guess you could say so, yes. For LayoutSVG* objects there's paintInvalidationRectInLocalCoordinates() method that's used, which is (sort of) intended as the equivalent of visualOverflowRect(). Together with that, the CTM within the SVG root is computed, and then mapped to a final paint invalidation rect via the root's mapRectToPaintInvalidationBacking().
On 2015/06/02 at 16:39:58, fs wrote: > On 2015/06/02 16:27:46, chrishtr wrote: > > On 2015/06/02 at 16:23:06, fs wrote: > > > On 2015/06/02 15:51:05, chrishtr wrote: > > > > You should also be able to test this CL in a similar way to > > > > https://codereview.chromium.org/1158193011 (position right at a tile > > boundary, > > > > then move the SVG object in the next frame to demonstrate a paint > > invalidation > > > > bug. > > > > > > This method is not used during paint invalidation - this is related to the > > below (and the fact that we need to track/compute transforms during repaint for > > SVG.) > > > > How does paint invalidation for this type of object work then? Does it > > invalidate only at the root? > > In a way I guess you could say so, yes. For LayoutSVG* objects there's paintInvalidationRectInLocalCoordinates() method that's used, which is (sort of) intended as the equivalent of visualOverflowRect(). Together with that, the CTM within the SVG root is computed, and then mapped to a final paint invalidation rect via the root's mapRectToPaintInvalidationBacking(). How does mapRectToPaintInvalidationBacking in this case include outline? Is it inherited behavior from LayoutObject or LayoutSVGModelObject?
On 2015/06/02 16:49:45, chrishtr wrote: > On 2015/06/02 at 16:39:58, fs wrote: > > On 2015/06/02 16:27:46, chrishtr wrote: > > > On 2015/06/02 at 16:23:06, fs wrote: > > > > On 2015/06/02 15:51:05, chrishtr wrote: > > > > > You should also be able to test this CL in a similar way to > > > > > https://codereview.chromium.org/1158193011 (position right at a tile > > > boundary, > > > > > then move the SVG object in the next frame to demonstrate a paint > > > invalidation > > > > > bug. > > > > > > > > This method is not used during paint invalidation - this is related to the > > > below (and the fact that we need to track/compute transforms during repaint > for > > > SVG.) > > > > > > How does paint invalidation for this type of object work then? Does it > > > invalidate only at the root? > > > > In a way I guess you could say so, yes. For LayoutSVG* objects there's > paintInvalidationRectInLocalCoordinates() method that's used, which is (sort of) > intended as the equivalent of visualOverflowRect(). Together with that, the CTM > within the SVG root is computed, and then mapped to a final paint invalidation > rect via the root's mapRectToPaintInvalidationBacking(). > > How does mapRectToPaintInvalidationBacking in this case include outline? Is it > inherited behavior from LayoutObject or LayoutSVGModelObject? Currently it's included via the SVGLayoutSupport::clippedOverflowRectForPaintInvalidation helper (which uses the paintInvalidationRectInLocalCoordinates method, and inflates with the outline-width). I'll try to refactor that a bit though to do it in the LayoutObject override instead. (And also fix the paint inv. rects for when there's outline-offset != 0.)
On 2015/06/02 at 18:28:11, fs wrote: > On 2015/06/02 16:49:45, chrishtr wrote: > > On 2015/06/02 at 16:39:58, fs wrote: > > > On 2015/06/02 16:27:46, chrishtr wrote: > > > > On 2015/06/02 at 16:23:06, fs wrote: > > > > > On 2015/06/02 15:51:05, chrishtr wrote: > > > > > > You should also be able to test this CL in a similar way to > > > > > > https://codereview.chromium.org/1158193011 (position right at a tile > > > > boundary, > > > > > > then move the SVG object in the next frame to demonstrate a paint > > > > invalidation > > > > > > bug. > > > > > > > > > > This method is not used during paint invalidation - this is related to the > > > > below (and the fact that we need to track/compute transforms during repaint > > for > > > > SVG.) > > > > > > > > How does paint invalidation for this type of object work then? Does it > > > > invalidate only at the root? > > > > > > In a way I guess you could say so, yes. For LayoutSVG* objects there's > > paintInvalidationRectInLocalCoordinates() method that's used, which is (sort of) > > intended as the equivalent of visualOverflowRect(). Together with that, the CTM > > within the SVG root is computed, and then mapped to a final paint invalidation > > rect via the root's mapRectToPaintInvalidationBacking(). > > > > How does mapRectToPaintInvalidationBacking in this case include outline? Is it > > inherited behavior from LayoutObject or LayoutSVGModelObject? > > Currently it's included via the SVGLayoutSupport::clippedOverflowRectForPaintInvalidation helper (which uses the paintInvalidationRectInLocalCoordinates method, and inflates with the outline-width). I'll try to refactor that a bit though to do it in the LayoutObject override instead. (And also fix the paint inv. rects for when there's outline-offset != 0.) Thanks.
On 2015/06/02 20:48:05, chrishtr wrote: > On 2015/06/02 at 18:28:11, fs wrote: > > On 2015/06/02 16:49:45, chrishtr wrote: > > > On 2015/06/02 at 16:39:58, fs wrote: > > > > On 2015/06/02 16:27:46, chrishtr wrote: > > > > > On 2015/06/02 at 16:23:06, fs wrote: > > > > > > On 2015/06/02 15:51:05, chrishtr wrote: > > > > > > > You should also be able to test this CL in a similar way to > > > > > > > https://codereview.chromium.org/1158193011 (position right at a tile > > > > > boundary, > > > > > > > then move the SVG object in the next frame to demonstrate a paint > > > > > invalidation > > > > > > > bug. > > > > > > > > > > > > This method is not used during paint invalidation - this is related to > the > > > > > below (and the fact that we need to track/compute transforms during > repaint > > > for > > > > > SVG.) > > > > > > > > > > How does paint invalidation for this type of object work then? Does it > > > > > invalidate only at the root? > > > > > > > > In a way I guess you could say so, yes. For LayoutSVG* objects there's > > > paintInvalidationRectInLocalCoordinates() method that's used, which is (sort > of) > > > intended as the equivalent of visualOverflowRect(). Together with that, the > CTM > > > within the SVG root is computed, and then mapped to a final paint > invalidation > > > rect via the root's mapRectToPaintInvalidationBacking(). > > > > > > How does mapRectToPaintInvalidationBacking in this case include outline? Is > it > > > inherited behavior from LayoutObject or LayoutSVGModelObject? > > > > Currently it's included via the > SVGLayoutSupport::clippedOverflowRectForPaintInvalidation helper (which uses the > paintInvalidationRectInLocalCoordinates method, and inflates with the > outline-width). I'll try to refactor that a bit though to do it in the > LayoutObject override instead. (And also fix the paint inv. rects for when > there's outline-offset != 0.) > > Thanks. Looked some more at this today, and realized that the two use (require) different coordinate spaces, so we end up translating back and forth depending on if it's paint inv. or paint (which is where visualOverflowRect is used in this case.)
On 2015/06/03 at 13:58:30, fs wrote: > On 2015/06/02 20:48:05, chrishtr wrote: > > On 2015/06/02 at 18:28:11, fs wrote: > > > On 2015/06/02 16:49:45, chrishtr wrote: > > > > On 2015/06/02 at 16:39:58, fs wrote: > > > > > On 2015/06/02 16:27:46, chrishtr wrote: > > > > > > On 2015/06/02 at 16:23:06, fs wrote: > > > > > > > On 2015/06/02 15:51:05, chrishtr wrote: > > > > > > > > You should also be able to test this CL in a similar way to > > > > > > > > https://codereview.chromium.org/1158193011 (position right at a tile > > > > > > boundary, > > > > > > > > then move the SVG object in the next frame to demonstrate a paint > > > > > > invalidation > > > > > > > > bug. > > > > > > > > > > > > > > This method is not used during paint invalidation - this is related to > > the > > > > > > below (and the fact that we need to track/compute transforms during > > repaint > > > > for > > > > > > SVG.) > > > > > > > > > > > > How does paint invalidation for this type of object work then? Does it > > > > > > invalidate only at the root? > > > > > > > > > > In a way I guess you could say so, yes. For LayoutSVG* objects there's > > > > paintInvalidationRectInLocalCoordinates() method that's used, which is (sort > > of) > > > > intended as the equivalent of visualOverflowRect(). Together with that, the > > CTM > > > > within the SVG root is computed, and then mapped to a final paint > > invalidation > > > > rect via the root's mapRectToPaintInvalidationBacking(). > > > > > > > > How does mapRectToPaintInvalidationBacking in this case include outline? Is > > it > > > > inherited behavior from LayoutObject or LayoutSVGModelObject? > > > > > > Currently it's included via the > > SVGLayoutSupport::clippedOverflowRectForPaintInvalidation helper (which uses the > > paintInvalidationRectInLocalCoordinates method, and inflates with the > > outline-width). I'll try to refactor that a bit though to do it in the > > LayoutObject override instead. (And also fix the paint inv. rects for when > > there's outline-offset != 0.) > > > > Thanks. > > Looked some more at this today, and realized that the two use (require) different coordinate spaces, so we end up translating back and forth depending on if it's paint inv. or paint (which is where visualOverflowRect is used in this case.) What are the two coordinate spaces? Could you give a brief explanation?
On 2015/06/03 16:41:22, chrishtr wrote: > On 2015/06/03 at 13:58:30, fs wrote: > > On 2015/06/02 20:48:05, chrishtr wrote: > > > On 2015/06/02 at 18:28:11, fs wrote: > > > > On 2015/06/02 16:49:45, chrishtr wrote: > > > > > On 2015/06/02 at 16:39:58, fs wrote: > > > > > > On 2015/06/02 16:27:46, chrishtr wrote: > > > > > > > On 2015/06/02 at 16:23:06, fs wrote: > > > > > > > > On 2015/06/02 15:51:05, chrishtr wrote: > > > > > > > > > You should also be able to test this CL in a similar way to > > > > > > > > > https://codereview.chromium.org/1158193011 (position right at a > tile > > > > > > > boundary, > > > > > > > > > then move the SVG object in the next frame to demonstrate a > paint > > > > > > > invalidation > > > > > > > > > bug. > > > > > > > > > > > > > > > > This method is not used during paint invalidation - this is > related to > > > the > > > > > > > below (and the fact that we need to track/compute transforms during > > > repaint > > > > > for > > > > > > > SVG.) > > > > > > > > > > > > > > How does paint invalidation for this type of object work then? Does > it > > > > > > > invalidate only at the root? > > > > > > > > > > > > In a way I guess you could say so, yes. For LayoutSVG* objects there's > > > > > paintInvalidationRectInLocalCoordinates() method that's used, which is > (sort > > > of) > > > > > intended as the equivalent of visualOverflowRect(). Together with that, > the > > > CTM > > > > > within the SVG root is computed, and then mapped to a final paint > > > invalidation > > > > > rect via the root's mapRectToPaintInvalidationBacking(). > > > > > > > > > > How does mapRectToPaintInvalidationBacking in this case include outline? > Is > > > it > > > > > inherited behavior from LayoutObject or LayoutSVGModelObject? > > > > > > > > Currently it's included via the > > > SVGLayoutSupport::clippedOverflowRectForPaintInvalidation helper (which uses > the > > > paintInvalidationRectInLocalCoordinates method, and inflates with the > > > outline-width). I'll try to refactor that a bit though to do it in the > > > LayoutObject override instead. (And also fix the paint inv. rects for when > > > there's outline-offset != 0.) > > > > > > Thanks. > > > > Looked some more at this today, and realized that the two use (require) > different coordinate spaces, so we end up translating back and forth depending > on if it's paint inv. or paint (which is where visualOverflowRect is used in > this case.) > > What are the two coordinate spaces? Could you give a brief explanation? visualOverflowRect returns a rect relative to the border-box, while paintInvalidationRectInLocalCoordinates is more an equivalent of the frameRect (actually the same in the <text>-case.)
https://codereview.chromium.org/1158323005/diff/1/Source/core/layout/svg/Layo... File Source/core/layout/svg/LayoutSVGBlock.cpp (right): https://codereview.chromium.org/1158323005/diff/1/Source/core/layout/svg/Layo... Source/core/layout/svg/LayoutSVGBlock.cpp:39: LayoutRect LayoutSVGBlock::visualOverflowRect() const Is this only ever used in BoxPainter when painting the LayoutSVGRoot? If so, the coordinate spaces would be the same, and we can also assert.
I've transplanted the fix to the layout-side, PTAL. https://codereview.chromium.org/1158323005/diff/1/Source/core/layout/svg/Layo... File Source/core/layout/svg/LayoutSVGBlock.cpp (right): https://codereview.chromium.org/1158323005/diff/1/Source/core/layout/svg/Layo... Source/core/layout/svg/LayoutSVGBlock.cpp:39: LayoutRect LayoutSVGBlock::visualOverflowRect() const On 2015/06/03 22:12:53, chrishtr wrote: > Is this only ever used in BoxPainter when painting the LayoutSVGRoot? No, AFAIK it's only used in BlockPainter (overflowRectForPaintRejection; paint). LayoutSVGRoot is a different story (inherits from LayoutReplaced) and already uses LayoutBox::m_overflow. LayoutSVGBlock is only used for <text> and <foreignObject>. > If so, the coordinate spaces would be the same, and we can also assert. Not sure what you mean by this, but maybe it's not relevant given the above (and the new PS).
https://codereview.chromium.org/1158323005/diff/20001/Source/core/layout/svg/... File Source/core/layout/svg/LayoutSVGText.cpp (right): https://codereview.chromium.org/1158323005/diff/20001/Source/core/layout/svg/... Source/core/layout/svg/LayoutSVGText.cpp:400: addVisualEffectOverflow(); So now LayoutSVGBlock inherits behavior from LayoutBlockFlow and gets its overflow from there?
https://codereview.chromium.org/1158323005/diff/20001/Source/core/layout/svg/... File Source/core/layout/svg/LayoutSVGText.cpp (right): https://codereview.chromium.org/1158323005/diff/20001/Source/core/layout/svg/... Source/core/layout/svg/LayoutSVGText.cpp:400: addVisualEffectOverflow(); On 2015/06/04 16:57:03, chrishtr wrote: > So now LayoutSVGBlock inherits behavior from LayoutBlockFlow and gets its > overflow from there? If the derived class does not override that in any way - yes. LayoutSVGText overrides this (does not call parent-class layout), while LayoutSVGForeignObject doesn't override (and did update m_overflow before, but didn't use it in visualOverflowRect - which might seem a bit odd.)
The CQ bit was checked by chrishtr@chromium.org
lgtm
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1158323005/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://src.chromium.org/viewvc/blink?view=rev&revision=196591 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
