Summary: | hit testing does not respect clip paths | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | jay <jay> | ||||||||||||
Component: | SVG | Assignee: | Dirk Schulze <krit> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | jeffschiller, krit | ||||||||||||
Priority: | P2 | Keywords: | HasReduction | ||||||||||||
Version: | 523.x (Safari 3) | ||||||||||||||
Hardware: | Mac | ||||||||||||||
OS: | OS X 10.4 | ||||||||||||||
Attachments: |
|
Description
jay
2007-09-08 09:07:23 PDT
Created attachment 16228 [details]
clip-path with anchor demo
Both are hit testing issues (one is not seeming to respect clip-paths during hit testing) but probably should be separate bugs. I believe this only applies to SVG objects where pointer-events is set to visiblePainted or painted. In any case since visiblePainted is default, this is very common and should be fixed. Attaching a simpler test case. Created attachment 16403 [details] simple test case (not ready for DRT) Unfortunately due to bug 15289 the clip is not completely empty, mousing over 0, 0 should actually change the cursor... but the test is close enough. :) Created attachment 53543 [details] Another test case for this bug Another test case for this bug - still present in WebKit r57509 Mac OS X 10.6. Behaviour is as expected in Firefox 3.6.3 Created attachment 57681 [details]
Patch
Comment on attachment 57681 [details]
Patch
In general that looks fine, though I'd like to see the PE_FILL stuff encapsulated. How about passing style()->pointerEvents() and HitTestRequest ot PointerEventsHitRules? It could either grab the style()->pointerEvents() or set it to PE_FILL when request.svgClipContent() is true? Several other comments leading to r-:
WebCore/rendering/RenderPath.h:50
+ bool fillContains(const FloatPoint&, bool requiresFill = true, WindRule = RULE_NONZERO) const;
Please use a parameter name here, looks odd.
WebCore/rendering/RenderSVGResourceClipper.cpp:289
+ IntPoint hitPoint = IntPoint();
This is unncessary, no need to call the assignment operator.
WebCore/rendering/RenderSVGResourceClipper.h:58
+ bool hitTestClipContent(const FloatRect&, FloatPoint);
Please pass FloatPoint as const-reference as well.
LayoutTests/svg/dynamic-updates/script-tests/SVGClipPath-influences-hitTesting.js:26
+ var rectElementFG = createSVGElement("rect");
Why not name it foreground/background instead of using abbrevations?
Created attachment 57961 [details]
Patch
Comment on attachment 57961 [details]
Patch
Looks great, r=me. One typo to fix before landing:
LayoutTests/svg/dynamic-updates/script-tests/SVGClipPath-influences-hitTesting.js:34
+ // Two rects are drawn. One in the bckground and one in the foreground. The rect
Typo "background".
Committed r60761: <http://trac.webkit.org/changeset/60761> Comment on attachment 57961 [details]
Patch
Clearing review flag.
|