Bug 83651 – Ellipse and rect hit testing is clipped by the shape's bounding box

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Philip Rogers]

Created attachment 136594 [details]
Example of broken hit test results.

Before http://trac.webkit.org/changeset/113781 we were not fully testing shapeDependentStrokeContains of RenderSVGRect and RenderSVGEllipse. Now that we are flexing the correct code a bug has been exposed because stroke hit testing is clipped by the object's bounding box.

[Philip Rogers]

Created attachment 136602 [details]
Fix bug where stroke's bounding box was clipped

[Dirk Schulze]

Comment on attachment 136602 [details]
Fix bug where stroke's bounding box was clipped

View in context: https://bugs.webkit.org/attachment.cgi?id=136602&action=review

r=me with little changes.

> Source/WebCore/ChangeLog:20
> +        I also added a FIXME to move the marker dependent code out of
> +        RenderSVGShape entirely (which would have prevented part 2 of this
> +        bug). Markers are only needed on a small subset of RenderSVGShape's
> +        subclasses so that functionality should be moved out of
> +        RenderSVGShape at some point.

And create another class that increases the call of virtual tables? Actually there are more classes that need it than classes that don't need it. I don't think that this fix me should go in. Rather create a new bug where we can discuss this topic.

> Source/WebCore/rendering/svg/RenderSVGShape.h:136
> +    // FIXME: marker-dependant code should be moved into RenderSVGShapeWithMarkers or subclasses

Like I said, I am against adding this fix me. We should discuss it on a bug report instead of adding fixmes that are not really fixmes.

> Source/WebCore/rendering/svg/RenderSVGShape.h:137
> +    //        that support markers (RenderSVGPath, RenderSVGLine, RenderSVGPolygon, RenderSVGPolyLine)

no spaces here.

[Philip Rogers]

Created attachment 136664 [details]
Remove fixme

Thanks for the review! I removed the premature fixme and will put up a separate bug to discuss it further.

[WebKit Review Bot]

Comment on attachment 136664 [details]
Remove fixme

Rejecting attachment 136664 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
t/git/webkit-commit-queue/Source/WebKit/chromium/v8 --revision 11253 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
47>At revision 11253.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/12386396

[Philip Rogers]

Created attachment 136685 [details]
Update so the patch applies cleanly

[WebKit Review Bot]

Comment on attachment 136685 [details]
Update so the patch applies cleanly

Rejecting attachment 136685 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/LayoutTests/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/12382913

[Philip Rogers]

Created attachment 136687 [details]
Adding Dirk to reviewers line.

[WebKit Review Bot]

Comment on attachment 136687 [details]
Adding Dirk to reviewers line.

Clearing flags on attachment: 136687

Committed r113879: <http://trac.webkit.org/changeset/113879>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.