Bug 11931 – PathCG fails to hit on unclosed-sub-paths (yet we still fill them)

• Component: SVG
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

Hit-testing issues with complex fills

See the attached example.  The upper-left and lower-right fills do not hit-test properly.

[Eric Seidel (no email)]

Created attachment 11969 [details]
example showing hit-test problem

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Created attachment 14458 [details]
better test case

[Eric Seidel (no email)]

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;
}

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

Landed on feature-branch (post Safari 3) as r21368.  It was surprisingly painless to switch to the branch (go svn!).

[Andreas Neumann]

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.

[Eric Seidel (no email)]

*** Bug 11096 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Reopening this.  Leopard has supposedly fixed this issue, so we should add a workaround, conditional for Leopard (or just remove the Tiger-only hack).

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

*** Bug 13010 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

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(-)

[Jayant Sai]

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.

[Jayant Sai]

Created attachment 19259 [details]
Image showing region where mouse over is fired.

[Jayant Sai]

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.

[Rob Buis]

All testcases work fine on Snow Leopard, maybe someone can check Leopard?
Cheers,

Rob.

[Eric Seidel (no email)]

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.