Fix pointer-events:all when stroke="none"

Fix pointer-events:all when stroke="none"

pointer-events:all should make event processing unaffected by the values
of the 'stroke' and 'fill' properties. However, when the 'stroke' of a
shape was set to "none", hit detection would fail for points that would
be in the shape's stroke area if the 'stroke' were not "none".

Originally, two CLs were committed implementing a fix for the issue, but
they were reverted due to a 200%-225% regression of a performance test:
PerformanceTests/SVG/SvgHitTesting.html
The performance regression was due to the change in strokeContains() that
skipped the check whether the stroke bounding box contained the point if
!requiresStroke.

The modified fix implemented by this commit has three parts:
1. m_innerStrokeRect/m_outerStrokeRect are deleted from RenderSVGRect.
   This is because shapeDependentStrokeContains(), which was the only
   user of these FloatRect objects, should ignore whether the shape
   actually has a stroke, but m_innerStrokeRect and m_outerStrokeRect
   were equal to the m_fillBoundingBox if the <rect> had no stroke.
2. The notion of the hit test stroke bounding box is introduced and a
   new RenderSVGShape member is added: m_hitTestStrokeBoundingBox. The
   hit test stroke bounding box of a shape is considered to be the
   bounding box of the stroke, regardless of whether the shape has a
   stroke. The stroke bounding box is usually m_hitTestStrokeBoundingBox
   if the shape has a stroke; otherwise, it's m_fillBoundingBox. In the
   case of RenderSVGPath, though, the stroke bounding box includes the
   markerRect.
3. RenderSVGShape::strokeContains() was changed so that if !requiresStroke,
   then we check whether the point is within the hit test stroke bounding
   box. If requiresStroke, then we check whether the point is within the
   stroke bounding box.

BUG=350338, 445410
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=188888

[dtrebbien, 2015-01-04 23:28:32]

dtrebbien@gmail.com changed reviewers:
+ ed@opera.com, fs@opera.com, pdr@chromium.org

[fs, 2015-01-05 10:08:14]

https://codereview.chromium.org/810343003/diff/20001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGShape.h (right):

https://codereview.chromium.org/810343003/diff/20001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.h:111: virtual FloatRect
strokeBoundingBox() const override final { return actualStrokeBoundingBox(); }
This redefines the semantics of this method in this sub-tree of the class
hierarchy (and hence transitively affects containers.) Could we do the opposite
instead - ie. keep strokeBoundingBox as-is and add a new method to be used for
hit-testing?

[fs, 2015-01-05 10:17:08]

On 2015/01/05 10:08:14, fs wrote:
>
https://codereview.chromium.org/810343003/diff/20001/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGShape.h (right):
> 
>
https://codereview.chromium.org/810343003/diff/20001/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGShape.h:111: virtual FloatRect
> strokeBoundingBox() const override final { return actualStrokeBoundingBox(); }
> This redefines the semantics of this method in this sub-tree of the class
> hierarchy (and hence transitively affects containers.) Could we do the
opposite
> instead - ie. keep strokeBoundingBox as-is and add a new method to be used for
> hit-testing?

FTR: I realize this has other consequences, and maybe this is the way we want to
roll in the end - I'm not sure we want to roll there in this way though. (It's
somewhat unpleasant that collision geometry != visible geometry...)

[dtrebbien, 2015-01-05 17:10:14]

On 2015/01/05 10:17:08, fs wrote:
> On 2015/01/05 10:08:14, fs wrote:
> >
>
https://codereview.chromium.org/810343003/diff/20001/Source/core/rendering/sv...
> > File Source/core/rendering/svg/RenderSVGShape.h (right):
> > 
> >
>
https://codereview.chromium.org/810343003/diff/20001/Source/core/rendering/sv...
> > Source/core/rendering/svg/RenderSVGShape.h:111: virtual FloatRect
> > strokeBoundingBox() const override final { return actualStrokeBoundingBox();
}
> > This redefines the semantics of this method in this sub-tree of the class
> > hierarchy (and hence transitively affects containers.) Could we do the
> opposite
> > instead - ie. keep strokeBoundingBox as-is and add a new method to be used
for
> > hit-testing?
> 
> FTR: I realize this has other consequences, and maybe this is the way we want
to
> roll in the end - I'm not sure we want to roll there in this way though. (It's
> somewhat unpleasant that collision geometry != visible geometry...)

I thought about different names for the "stroke bounding box disregarding
whether the shape actually has a stroke" and came up with three ideas:
- virtualStrokeBoundingBox
- outerStrokeBoundingBox
- outerStrokeRect  (mirroring what RenderSVGRect was calling it)

Just to clarify: RenderSVGShape::strokeBoundingBox() should be returning the
same FloatRect as without the patch, but I presume that you are referring to the
confusion between RenderSVGShape::strokeBoundingBox() not being the same
bounding box as m_strokeBoundingBox.

[fs, 2015-01-05 17:50:30]

On 2015/01/05 17:10:14, dtrebbien wrote:
> On 2015/01/05 10:17:08, fs wrote:
> > On 2015/01/05 10:08:14, fs wrote:
> > >
> >
>
https://codereview.chromium.org/810343003/diff/20001/Source/core/rendering/sv...
> > > File Source/core/rendering/svg/RenderSVGShape.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/810343003/diff/20001/Source/core/rendering/sv...
> > > Source/core/rendering/svg/RenderSVGShape.h:111: virtual FloatRect
> > > strokeBoundingBox() const override final { return
actualStrokeBoundingBox();
> }
> > > This redefines the semantics of this method in this sub-tree of the class
> > > hierarchy (and hence transitively affects containers.) Could we do the
> > opposite
> > > instead - ie. keep strokeBoundingBox as-is and add a new method to be used
> for
> > > hit-testing?
> > 
> > FTR: I realize this has other consequences, and maybe this is the way we
want
> to
> > roll in the end - I'm not sure we want to roll there in this way though.
(It's
> > somewhat unpleasant that collision geometry != visible geometry...)
> 
> I thought about different names for the "stroke bounding box disregarding
> whether the shape actually has a stroke" and came up with three ideas:
> - virtualStrokeBoundingBox
> - outerStrokeBoundingBox
> - outerStrokeRect  (mirroring what RenderSVGRect was calling it)
> 
> Just to clarify: RenderSVGShape::strokeBoundingBox() should be returning the
> same FloatRect as without the patch, but I presume that you are referring to
the
> confusion between RenderSVGShape::strokeBoundingBox() not being the same
> bounding box as m_strokeBoundingBox.

Yes, what I was trying to express was that 'actual...' does not really make it
obvious what it actually represents (pun intended.) If we only use it for
hit-testing, that may as well be reflected in the name - the less abstract the
better really. I encourage the other reviewers to chime in on this as well.

[dtrebbien, 2015-01-06 12:52:59]

On 2015/01/05 17:50:30, fs wrote:
> Yes, what I was trying to express was that 'actual...' does not really make it
> obvious what it actually represents (pun intended.) If we only use it for
> hit-testing, that may as well be reflected in the name - the less abstract the
> better really. I encourage the other reviewers to chime in on this as well.

I was talking with my brother about this and he said almost exactly the same
thing, only using the word "vague" instead of "abstract"  :)

Here are two more ideas:
- hitTestStrokeBoundingBox
- maxStrokeBoundingBox  (this was my brother's suggestion)

[fs, 2015-01-07 12:56:22]

On 2015/01/06 12:52:59, dtrebbien wrote:
> On 2015/01/05 17:50:30, fs wrote:
> > Yes, what I was trying to express was that 'actual...' does not really make
it
> > obvious what it actually represents (pun intended.) If we only use it for
> > hit-testing, that may as well be reflected in the name - the less abstract
the
> > better really. I encourage the other reviewers to chime in on this as well.
> 
> I was talking with my brother about this and he said almost exactly the same
> thing, only using the word "vague" instead of "abstract"  :)
> 
> Here are two more ideas:
> - hitTestStrokeBoundingBox
> - maxStrokeBoundingBox  (this was my brother's suggestion)

As long as the method is only used in the hit-testing realm, I'd prefer the
former.

[dtrebbien, 2015-01-10 18:06:04]

On 2015/01/07 12:56:22, fs wrote:
> On 2015/01/06 12:52:59, dtrebbien wrote:
> > On 2015/01/05 17:50:30, fs wrote:
> > > Yes, what I was trying to express was that 'actual...' does not really
make
> it
> > > obvious what it actually represents (pun intended.) If we only use it for
> > > hit-testing, that may as well be reflected in the name - the less abstract
> the
> > > better really. I encourage the other reviewers to chime in on this as
well.
> > 
> > I was talking with my brother about this and he said almost exactly the same
> > thing, only using the word "vague" instead of "abstract"  :)
> > 
> > Here are two more ideas:
> > - hitTestStrokeBoundingBox
> > - maxStrokeBoundingBox  (this was my brother's suggestion)
> 
> As long as the method is only used in the hit-testing realm, I'd prefer the
> former.

A new version of the patch has been uploaded where m_strokeBoundingBox is
renamed to m_hitTestStrokeBoundingBox.  I have also updated the CL description.

[fs, 2015-01-12 13:00:04]

On 2015/01/10 18:06:04, dtrebbien wrote:
...
> A new version of the patch has been uploaded where m_strokeBoundingBox is
> renamed to m_hitTestStrokeBoundingBox.  I have also updated the CL
description.

I can seem to bring myself to like this change... Probably because I feel it's
too much of a stir for what I consider an edge-case. I also suspect that it'd be
better served to split any code re-org from the actual bugfix. I'm also not a
fan of renaming m_strokeBoundingBox, since that creates unnecessary differences
with other SVG RenderObjects. (Note that my previous comment featured the word
'method'.)

https://codereview.chromium.org/810343003/diff/40001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGShape.cpp (right):

https://codereview.chromium.org/810343003/diff/40001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.cpp:104: if (!(requiresStroke ?
strokeBoundingBox() : m_hitTestStrokeBoundingBox).contains(point))
Not particularly fond of this construct... Feels like plain nested ifs would be
a better choice.

[dtrebbien, 2015-01-22 00:57:07]

On 2015/01/12 13:00:04, fs wrote:
> On 2015/01/10 18:06:04, dtrebbien wrote:
> ...
> > A new version of the patch has been uploaded where m_strokeBoundingBox is
> > renamed to m_hitTestStrokeBoundingBox.  I have also updated the CL
> description.
> 
> I can seem to bring myself to like this change... Probably because I feel it's
> too much of a stir for what I consider an edge-case. I also suspect that it'd
be
> better served to split any code re-org from the actual bugfix. I'm also not a
> fan of renaming m_strokeBoundingBox, since that creates unnecessary
differences
> with other SVG RenderObjects. (Note that my previous comment featured the word
> 'method'.)
> 
>
https://codereview.chromium.org/810343003/diff/40001/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGShape.cpp (right):
> 
>
https://codereview.chromium.org/810343003/diff/40001/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGShape.cpp:104: if (!(requiresStroke ?
> strokeBoundingBox() : m_hitTestStrokeBoundingBox).contains(point))
> Not particularly fond of this construct... Feels like plain nested ifs would
be
> a better choice.

Hi fs,

I have uploaded a new Patch Set.  What I have done is added back
m_strokeBoundingBox and also added a new member to RenderSVGShape,
m_hitTestStrokeBoundingBox, to hold the hit test stroke bounding box.

[fs, 2015-01-22 12:15:59]

I think it should be possible to do this without the additional rect (by using
an approximation for the non-convex case, and intersecting the inflated rect in
the convex cases), but let's run with this now (I feel I've backpedaled this
enough by now...sorry for that)

https://codereview.chromium.org/810343003/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGPath.cpp (right):

https://codereview.chromium.org/810343003/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGPath.cpp:54: if (m_markerPositions.isEmpty())
{
The if-branch is a no-op, so suggest reversing this condition (and dropping the
if-branch).

https://codereview.chromium.org/810343003/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGPath.cpp:58:
m_hitTestStrokeBoundingBox.unite(markerRect);
IIRC markers are not included in the hit test, so maybe this should be moved
down after the if at the end instead.

https://codereview.chromium.org/810343003/diff/60001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGShape.cpp (right):

https://codereview.chromium.org/810343003/diff/60001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGShape.cpp:113: if (requiresStroke &&
!SVGPaintServer::existsForRenderer(*this, style(), ApplyToStrokeMode))
Suggest you move this into the commonly predicated if above.

[dtrebbien, 2015-01-22 23:04:47]

On 2015/01/22 12:15:59, fs wrote:
> I think it should be possible to do this without the additional rect (by using
> an approximation for the non-convex case, and intersecting the inflated rect
in
> the convex cases), but let's run with this now (I feel I've backpedaled this
> enough by now...sorry for that)

It's no problem :)

I like this idea of possibly computing the hit test stroke bounding box on the
fly.  There might be some trickiness, though, with miter joins at sharp angles,
particularly for very small miter limits.  I will try to think about this and
possibly propose a follow up patch.  By the way, I have been looking into
https://crbug.com/393143, for which I will want to submit a patch first before
tackling a follow up patch.

> 
>
https://codereview.chromium.org/810343003/diff/60001/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGPath.cpp (right):
> 
>
https://codereview.chromium.org/810343003/diff/60001/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGPath.cpp:54: if
(m_markerPositions.isEmpty())
> {
> The if-branch is a no-op, so suggest reversing this condition (and dropping
the
> if-branch).

Done.

> 
>
https://codereview.chromium.org/810343003/diff/60001/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGPath.cpp:58:
> m_hitTestStrokeBoundingBox.unite(markerRect);
> IIRC markers are not included in the hit test, so maybe this should be moved
> down after the if at the end instead.

I don't 100% understand this.  Did I get it right in the new Patch Set?

> 
>
https://codereview.chromium.org/810343003/diff/60001/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGShape.cpp (right):
> 
>
https://codereview.chromium.org/810343003/diff/60001/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGShape.cpp:113: if (requiresStroke &&
> !SVGPaintServer::existsForRenderer(*this, style(), ApplyToStrokeMode))
> Suggest you move this into the commonly predicated if above.

Done.

[fs, 2015-01-23 11:26:48]

LGTM with the issue in RenderSVGPath (below) addressed.

On 2015/01/22 23:04:47, dtrebbien wrote:
> On 2015/01/22 12:15:59, fs wrote:
... 
> I like this idea of possibly computing the hit test stroke bounding box on the
> fly.  There might be some trickiness, though, with miter joins at sharp
angles,
> particularly for very small miter limits.  I will try to think about this and
> possibly propose a follow up patch.

Those kinds of miters (or miter-limits) will indeed make for a very pessimistic
approximate - they are not very common in practice though (miter-limits !=
default is pretty rare I'd say.)

> By the way, I have been looking into
> https://crbug.com/393143, for which I will want to submit a patch first before
> tackling a follow up patch.

Great! =)

> > 
> >
>
https://codereview.chromium.org/810343003/diff/60001/Source/core/rendering/sv...
> > Source/core/rendering/svg/RenderSVGPath.cpp:58:
> > m_hitTestStrokeBoundingBox.unite(markerRect);
> > IIRC markers are not included in the hit test, so maybe this should be moved
> > down after the if at the end instead.
> 
> I don't 100% understand this.  Did I get it right in the new Patch Set?

Not quite. I added another comment.

https://codereview.chromium.org/810343003/diff/80001/Source/core/rendering/sv...
File Source/core/rendering/svg/RenderSVGPath.cpp (right):

https://codereview.chromium.org/810343003/diff/80001/Source/core/rendering/sv...
Source/core/rendering/svg/RenderSVGPath.cpp:60:
m_hitTestStrokeBoundingBox.unite(markerRect);
This line not needed. (Markers not "sensitive" to hit-tests.)

[dtrebbien, 2015-01-23 12:26:21]

On 2015/01/23 11:26:48, fs wrote:
> LGTM with the issue in RenderSVGPath (below) addressed.
> 
> On 2015/01/22 23:04:47, dtrebbien wrote:
> > On 2015/01/22 12:15:59, fs wrote:
> ... 
> > I like this idea of possibly computing the hit test stroke bounding box on
the
> > fly.  There might be some trickiness, though, with miter joins at sharp
> angles,
> > particularly for very small miter limits.  I will try to think about this
and
> > possibly propose a follow up patch.
> 
> Those kinds of miters (or miter-limits) will indeed make for a very
pessimistic
> approximate - they are not very common in practice though (miter-limits !=
> default is pretty rare I'd say.)
> 
> > By the way, I have been looking into
> > https://crbug.com/393143, for which I will want to submit a patch first
before
> > tackling a follow up patch.
> 
> Great! =)
> 
> > > 
> > >
> >
>
https://codereview.chromium.org/810343003/diff/60001/Source/core/rendering/sv...
> > > Source/core/rendering/svg/RenderSVGPath.cpp:58:
> > > m_hitTestStrokeBoundingBox.unite(markerRect);
> > > IIRC markers are not included in the hit test, so maybe this should be
moved
> > > down after the if at the end instead.
> > 
> > I don't 100% understand this.  Did I get it right in the new Patch Set?
> 
> Not quite. I added another comment.
> 
>
https://codereview.chromium.org/810343003/diff/80001/Source/core/rendering/sv...
> File Source/core/rendering/svg/RenderSVGPath.cpp (right):
> 
>
https://codereview.chromium.org/810343003/diff/80001/Source/core/rendering/sv...
> Source/core/rendering/svg/RenderSVGPath.cpp:60:
> m_hitTestStrokeBoundingBox.unite(markerRect);
> This line not needed. (Markers not "sensitive" to hit-tests.)

Ah!  Makes sense, thanks.

New Patch Set uploaded and CL description updated slightly.

[fs, 2015-01-23 12:41:35]

On 2015/01/23 12:26:21, dtrebbien wrote:
> On 2015/01/23 11:26:48, fs wrote:
> > LGTM with the issue in RenderSVGPath (below) addressed.
> > 
> > On 2015/01/22 23:04:47, dtrebbien wrote:
> > > On 2015/01/22 12:15:59, fs wrote:
> > ... 
> > > I like this idea of possibly computing the hit test stroke bounding box on
> the
> > > fly.  There might be some trickiness, though, with miter joins at sharp
> > angles,
> > > particularly for very small miter limits.  I will try to think about this
> and
> > > possibly propose a follow up patch.
> > 
> > Those kinds of miters (or miter-limits) will indeed make for a very
> pessimistic
> > approximate - they are not very common in practice though (miter-limits !=
> > default is pretty rare I'd say.)
> > 
> > > By the way, I have been looking into
> > > https://crbug.com/393143, for which I will want to submit a patch first
> before
> > > tackling a follow up patch.
> > 
> > Great! =)
> > 
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/810343003/diff/60001/Source/core/rendering/sv...
> > > > Source/core/rendering/svg/RenderSVGPath.cpp:58:
> > > > m_hitTestStrokeBoundingBox.unite(markerRect);
> > > > IIRC markers are not included in the hit test, so maybe this should be
> moved
> > > > down after the if at the end instead.
> > > 
> > > I don't 100% understand this.  Did I get it right in the new Patch Set?
> > 
> > Not quite. I added another comment.
> > 
> >
>
https://codereview.chromium.org/810343003/diff/80001/Source/core/rendering/sv...
> > File Source/core/rendering/svg/RenderSVGPath.cpp (right):
> > 
> >
>
https://codereview.chromium.org/810343003/diff/80001/Source/core/rendering/sv...
> > Source/core/rendering/svg/RenderSVGPath.cpp:60:
> > m_hitTestStrokeBoundingBox.unite(markerRect);
> > This line not needed. (Markers not "sensitive" to hit-tests.)
> 
> Ah!  Makes sense, thanks.
> 
> New Patch Set uploaded and CL description updated slightly.

Still LGTM.

[dtrebbien, 2015-01-23 13:00:43]

The CQ bit was checked by dtrebbien@gmail.com

[commit-bot: I haz the power, 2015-01-23 13:01:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/810343003/100001

[commit-bot: I haz the power, 2015-01-23 14:41:09]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=188888

[dtrebbien, 2015-01-27 15:50:08]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/882443006/ by dtrebbien@gmail.com.

The reason for reverting is: Issue 452284:	10.1%-69.4% regression in
blink_perf.svg at 312837:312949.