Bug 11931 - PathCG fails to hit on unclosed-sub-paths (yet we still fill them)
Summary: PathCG fails to hit on unclosed-sub-paths (yet we still fill them)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 420+
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Eric Seidel (no email)
URL:
Keywords:
: 11096 13010 (view as bug list)
Depends on:
Blocks: 13161
  Show dependency treegraph
 
Reported: 2006-12-22 09:30 PST by Eric Seidel (no email)
Modified: 2011-05-17 10:47 PDT (History)
4 users (show)

See Also:


Attachments
example showing hit-test problem (309 bytes, image/svg+xml)
2006-12-22 09:31 PST, Eric Seidel (no email)
no flags Details
better test case (579 bytes, image/svg+xml)
2007-05-10 05:03 PDT, Eric Seidel (no email)
no flags Details
the fix (18.68 KB, patch)
2007-05-10 05:43 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Disable PathCG::contains workaround on leopard (2.00 KB, patch)
2007-12-27 02:28 PST, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Simple test case drawing an SVG path (1.18 KB, text/html)
2008-02-21 09:39 PST, Jayant Sai
no flags Details
Image showing region where mouse over is fired. (15.00 KB, image/png)
2008-02-21 09:41 PST, Jayant Sai
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2006-12-22 09:30:22 PST
Hit-testing issues with complex fills

See the attached example.  The upper-left and lower-right fills do not hit-test properly.
Comment 1 Eric Seidel (no email) 2006-12-22 09:31:01 PST
Created attachment 11969 [details]
example showing hit-test problem
Comment 2 Eric Seidel (no email) 2007-05-10 05:01:20 PDT
Turns out this is an issue with unclosed sub-paths.  If the subpath were actually closed, hit-testing works correctly.  Probably a CG issue we'll have to work around.  Attaching a better example.
Comment 3 Eric Seidel (no email) 2007-05-10 05:03:52 PDT
Created attachment 14458 [details]
better test case
Comment 4 Eric Seidel (no email) 2007-05-10 05:07:49 PDT
Ok, the fix here is to do a better job of closing the paths.   Right now we close the last subpath, but not necessarily any other subpaths.  We really should walk the entire path, making a manual copy and inserting a close subpath before each "move" and at the very end of the path.

bool Path::contains(const FloatPoint &point, WindRule rule) const
{
    // CGPathContainsPoint returns false for non-closed paths, as a work-around, we copy and close the path first.  Radar 4758998 asks for a better CG API to use
    if (!boundingRect().contains(point))
        return false;

    CGMutablePathRef path = CGPathCreateMutableCopy(m_path);
    CGPathCloseSubpath(path);
    bool ret = CGPathContainsPoint(path, 0, point, rule == RULE_EVENODD ? true : false);
    CGPathRelease(path);
    return ret;
}

Comment 5 Eric Seidel (no email) 2007-05-10 05:43:43 PDT
Created attachment 14463 [details]
the fix

It's ugly that we have to do this.  CG should provide us a better API which doesn't require so much work to get a "hit test wherever you would normally fill" behavior.  I'm *sure* we're not the only client which wants. this.
Comment 6 Eric Seidel (no email) 2007-05-10 05:53:30 PDT
Another way to fix this (that we might consider at a later date) is to just allocate a 1x1 CGBitmapImageContext, translate (-testPoint.x, -testPoint.y) and paint the path in question.  I'm not sure if that would be faster or not.
Comment 7 Eric Seidel (no email) 2007-05-10 06:11:04 PDT
Landed on feature-branch (post Safari 3) as r21368.  It was surprisingly painless to switch to the branch (go svn!).
Comment 8 Andreas Neumann 2007-05-10 06:43:17 PDT
will there be nightlies available of the feature branch? (would make it easier for me to test)

I believe that this hit testing is pretty basic (and important) and that this fix should also be included in the main trunk (not only feature branch), once it is made sure that it works fine.
Comment 9 Eric Seidel (no email) 2007-06-05 13:01:33 PDT
*** Bug 11096 has been marked as a duplicate of this bug. ***
Comment 10 Eric Seidel (no email) 2007-10-26 17:11:14 PDT
Reopening this.  Leopard has supposedly fixed this issue, so we should add a workaround, conditional for Leopard (or just remove the Tiger-only hack).
Comment 11 Eric Seidel (no email) 2007-10-26 17:11:49 PDT
Comment on attachment 14463 [details]
the fix

clearing review flag, this patch has already landed, but I've reopened this bug to track removing this workaround under Leopard.
Comment 12 Eric Seidel (no email) 2007-12-27 01:56:21 PST
*** Bug 13010 has been marked as a duplicate of this bug. ***
Comment 13 Eric Seidel (no email) 2007-12-27 02:28:37 PST
Created attachment 18129 [details]
Disable PathCG::contains workaround on leopard

 WebCore/ChangeLog                       |   10 ++++++++++
 WebCore/platform/graphics/cg/PathCG.cpp |    9 +++++++--
 2 files changed, 17 insertions(+), 2 deletions(-)
Comment 14 Jayant Sai 2008-02-21 09:39:58 PST
Created attachment 19258 [details]
Simple test case drawing an SVG path

The simple test case draws a path, & closes it. But still fires a mouse over event outside the path. I will additionally attach an image showing the region where mouse over is fired.
Comment 15 Jayant Sai 2008-02-21 09:41:00 PST
Created attachment 19259 [details]
Image showing region where mouse over is fired.
Comment 16 Jayant Sai 2008-02-21 09:58:52 PST
I am using build 30377 on leapord & safari 3.0.4 (523.12.2). I see the same problem identifies in the 'better test case'. I have worked further and tried to nail it down. The image attached shows (in red) the region where mouse over/out events are fired.
Comment 17 Rob Buis 2011-05-15 13:41:38 PDT
All testcases work fine on Snow Leopard, maybe someone can check Leopard?
Cheers,

Rob.
Comment 18 Eric Seidel (no email) 2011-05-17 10:47:52 PDT
This looks to work as far as I can tell.  If it's leopard only, we don't really care at this point since leopard is on its way out anyway.