New issue
Advanced search Search tips

Issue 823796 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Sep 28
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug

Blocking:
issue 370012



Sign in to add a comment

SVG does not support rect-based hit tests

Project Member Reported by pdr@chromium.org, Mar 20 2018

Issue description

SVG only uses point-based hit tests which means our hit testing code cannot do rect-based hit tests in svg:
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/layout/svg/LayoutSVGRoot.cpp
 
Status: Available (was: Untriaged)
Is this blocking something?

Comment 2 by pdr@chromium.org, Mar 20 2018

I think this will likely need to be addressed for intersection observer v2.

Comment 3 by f...@opera.com, Mar 21 2018

I take it this may have some overlap (no pun intended...) with issue 370012 (getIntersectionList/checkIntersection)?

Comment 4 by pdr@chromium.org, Mar 21 2018

That does seem like a very similar bug. getIntersectionList seems to be implemented independent of the hit testing code (nodeAtPoint) though.

Comment 5 by f...@opera.com, Mar 21 2018

The current implementation of getIntersectionList et al is somewhat lacking though. From a spec PoV it makes a lot of sense to implement it using the/a hit-testing code path. (And from a code-sharing PoV it makes even more sense.)
Seems that I'm affected by this, IntersectionObserver doesn't work as expected in SVG.

https://jsbin.com/debugawuno/edit?html,css,js,output

`boundingRect`, `intersectionRect` and `rootBounds` have all 0 values.
Owner: chrishtr@chromium.org
Status: Assigned (was: Available)
@c#6: That sounds like something similar in spirit to  issue 847623 , so I'd suggest that you file a separate issue for that. (AFAIU, rect-based hit-tests are not used by IOv1.)
Labels: -Pri-3 Pri-2
Status: Started (was: Assigned)
Plan of action:

1. Refactor the code in PaintLayer that applies transforms into
the HitTestLocation class (not sure how much of it is needed to refactor though).
2. Change all SVG uses of NodeAtFloatPoint to use NodeAtPoint, and delete
NodeAtFloatPoint.

HitTestingTransformState will be used and passed recursively somehow (not sure whether
as additional parameter yet) to avoid re-multiplying matrices on every recursion.

The only concern I can think of with this plan is to preserve the floating-point accuracy
of NodeAtFloatPoint. However, HitTestLocation already has FloatPoint and FloatRect
internally, so it seems that there is no loss of accuracy.

Upping to P2 because this blocks IOv2.

Sounds fine. The use of the floating-point point/rect probably won't be any issue if all new "Intersects" variants that get added use the float query state. (I'd worry a bit about the current Intersects(const FloatRect&) because it seems to do a LayoutRect -> FloatRect conversion for each call.)

I think I feel more worried about all the more or less dead state that HitTestingTransformState + HitTestLocation carry when it comes to hit-testing SVG. Makes not setting them up unnecessarily important I think. (Not sure we have a perf test which has both lots of primitives and transforms.)
Project Member

Comment 11 by bugdroid1@chromium.org, Sep 27

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

commit 10b5fba1fee6b6f4b7ecc3771aeba82c556f67dd
Author: Fredrik Söderquist <fs@opera.com>
Date: Thu Sep 27 18:35:13 2018

Use LayoutRect for frame rect in HitTestResultInFrame

This is consistent with how the frame rect is treated in other cases of
hit-testing (such as LayoutView::HitTestNoLifecycleUpdate). It also
eliminates the need for HitTestLocation::Intersects(const FloatRect&).
(The other "use" of this method is removed since it isn't used.)

Bug:  823796 
Change-Id: I7532f0e685e9960d8afd16f9b41d699ae27bfd11
Reviewed-on: https://chromium-review.googlesource.com/1249142
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594795}
[modify] https://crrev.com/10b5fba1fee6b6f4b7ecc3771aeba82c556f67dd/third_party/blink/renderer/core/input/event_handling_util.cc
[modify] https://crrev.com/10b5fba1fee6b6f4b7ecc3771aeba82c556f67dd/third_party/blink/renderer/core/layout/layout_inline.cc

Blocking: 370012
Project Member

Comment 13 by bugdroid1@chromium.org, Sep 28

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

commit b2551a0955a04bef10932ddff5a069ef25c389dd
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Fri Sep 28 15:41:55 2018

[IOv2] Support rect-based hit testing for SVG and content under foreignObject.

This is needed to support IOv2 occlusion testing via hit test. It also allows
content under foreignObject to receive disambiguation on touch screens.

Bug:  823796 

Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_layout_tests_slimming_paint_v2;master.tryserver.blink:linux_trusty_blink_rel
Change-Id: I80089feb9a30d3f28ce2c68f7276cafc5410adcb
Reviewed-on: https://chromium-review.googlesource.com/1227470
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Reviewed-by: Fredrik Söderquist <fs@opera.com>
Cr-Commit-Position: refs/heads/master@{#595104}
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/BUILD.gn
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/hit_test_location.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/hit_test_location.h
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/layout_object.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/layout_object.h
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_container.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_container.h
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_foreign_object.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_foreign_object.h
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_foreign_object_test.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_hidden_container.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_hidden_container.h
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_image.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_image.h
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_model_object.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_model_object.h
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_resource_clipper.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_root.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_root_test.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_shape.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_shape.h
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_text.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_text.h
[add] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_text_test.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_viewport_container.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/layout_svg_viewport_container.h
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/line/svg_inline_text_box.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/svg_layout_support.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/layout/svg/svg_layout_support.h
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/svg/svg_geometry_element.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/testing/core_unit_test_helper.cc
[modify] https://crrev.com/b2551a0955a04bef10932ddff5a069ef25c389dd/third_party/blink/renderer/core/testing/core_unit_test_helper.h

Status: Fixed (was: Started)
Project Member

Comment 15 by bugdroid1@chromium.org, Oct 2

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

commit 6580c61f1afd42ebfb5b0c4dacce336ca7954361
Author: Chris Harrelson <chrishtr@chromium.org>
Date: Tue Oct 02 17:39:50 2018

[IOv2] Add an occlusion test for foreignObject.

Bug:  823796 

Change-Id: Id2f733f1823c03300c343386ad01573ff0b6d69f
Reviewed-on: https://chromium-review.googlesource.com/1252326
Reviewed-by: Stefan Zager <szager@chromium.org>
Commit-Queue: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595894}
[add] https://crrev.com/6580c61f1afd42ebfb5b0c4dacce336ca7954361/third_party/WebKit/LayoutTests/intersection-observer/v2/simple-occlusion-svg-foreign-object.html

Sign in to add a comment