Bug 84117 - SVG hit testing on scaled paths is broken.
Summary: SVG hit testing on scaled paths is broken.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Philip Rogers
URL:
Keywords:
: 84452 (view as bug list)
Depends on:
Blocks:
 
Reported: 2012-04-16 19:01 PDT by Philip Rogers
Modified: 2012-04-26 11:24 PDT (History)
9 users (show)

See Also:


Attachments
Testcase (1.17 KB, text/html)
2012-04-16 19:01 PDT, Philip Rogers
no flags Details
First-pass at fixing SkPathContainsPoint to work with sub-pixel accuracy (9.58 KB, patch)
2012-04-24 10:51 PDT, Philip Rogers
eric: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (6.05 MB, application/zip)
2012-04-24 21:54 PDT, WebKit Review Bot
no flags Details
Update to make this patch pass tests (9.71 KB, patch)
2012-04-26 08:14 PDT, Philip Rogers
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Philip Rogers 2012-04-16 19:01:47 PDT
Created attachment 137455 [details]
Testcase

If we scale a very small (sub 0.5px) path up to a visible size, hit testing will not work properly.

Original bug: http://code.google.com/p/chromium/issues/detail?id=65238
Note: this only affects Skia-backed platforms.
Comment 1 Philip Rogers 2012-04-16 19:24:55 PDT
After some investigation, this problem stems from doing hit testing in object-space instead of screen-space (aka pixels). The problem is only surfaced on Skia-backed platforms because Skia doesn't handle sub-pixel path hit tests.

I'd like to propose a fairly large change to SVG's hit testing code that switches to doing hit testing in screen space. This will probably mean doing away with nodeAtFloatPoint and we will no longer transform the pointer location into each node's local coordinate space because we can rely on bounding boxes.

I don't have a patch ready yet but I wanted to put this idea up for comments as I will be out for the rest of the week (WebKit contributors meeting!)
Comment 2 Eric Seidel (no email) 2012-04-16 20:19:06 PDT
Why would we want to move away from object-space testing?
Comment 3 Philip Rogers 2012-04-20 12:06:29 PDT
*** Bug 84452 has been marked as a duplicate of this bug. ***
Comment 4 Philip Rogers 2012-04-24 10:51:55 PDT
Created attachment 138596 [details]
First-pass at fixing SkPathContainsPoint to work with sub-pixel accuracy

Attached is a first-pass at solving this issue in Skia, comments appreciated!

I earlier stated that hit testing in screen-space would be better but after discussing this f2f at the contrib meeting, my idea was flawed for several reasons. My new patch updates Skia to just scale the path up before doing raster hit testing.
Comment 5 Eric Seidel (no email) 2012-04-24 12:18:26 PDT
Comment on attachment 138596 [details]
First-pass at fixing SkPathContainsPoint to work with sub-pixel accuracy

Seems reasonable to me.
Comment 6 WebKit Review Bot 2012-04-24 21:54:50 PDT
Comment on attachment 138596 [details]
First-pass at fixing SkPathContainsPoint to work with sub-pixel accuracy

Attachment 138596 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12523478

New failing tests:
platform/chromium/virtual/gpu/canvas/philip/tests/2d.path.isPointInPath.edge.html
canvas/philip/tests/2d.path.isPointInPath.edge.html
platform/chromium/virtual/gpu/fast/canvas/pointInPath.html
svg/custom/stroke-width-click.svg
fast/canvas/pointInPath.html
Comment 7 WebKit Review Bot 2012-04-24 21:54:58 PDT
Created attachment 138737 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 8 Philip Rogers 2012-04-26 08:14:33 PDT
Created attachment 139007 [details]
Update to make this patch pass tests

Thanks for the review Eric! The linux test failure was real and I've updated the patch so it should pass all tests now. There are two changes from the previous patch:
1) Skia falls over with coords above 32767 and we were not taking into account that the mouse pointer may be above this in our scaled-up space. I'm now taking into account the mouse coordinates when determining the scale value.
2) Instead of taking the floor of the mouse point when converting to integer values, we need to round. floorf(0.5f + point) accomplishes this.

Do you mind taking one more look Eric?
Comment 9 Eric Seidel (no email) 2012-04-26 09:55:35 PDT
Comment on attachment 139007 [details]
Update to make this patch pass tests

Clever.
Comment 10 WebKit Review Bot 2012-04-26 11:24:04 PDT
Comment on attachment 139007 [details]
Update to make this patch pass tests

Clearing flags on attachment: 139007

Committed r115335: <http://trac.webkit.org/changeset/115335>
Comment 11 WebKit Review Bot 2012-04-26 11:24:17 PDT
All reviewed patches have been landed.  Closing bug.