490003 - Invalidation and hit testing does not account for border or patting on outer-<svg>

Status: RESOLVED FIXED

(Core :: SVG, defect, P2)

Description

[Jonathan Watt [:jwatt]]

Invalidation and hit testing does not account for border or patting on outer-<svg>.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: testcase - invalidation

Comment 2

[Jonathan Watt [:jwatt]]

Attachment: testcase - hit testing

Comment 3

[Jonathan Watt [:jwatt]]

Attachment: patch

I'm not sure how to create an automated test for the hit testing. I need a way to dispatch an event to a given point on the screen, not to a particular target.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

For hit-testing, use the elementFromPoint DOM API.

+  nsMargin bp = GetUsedBorderAndPadding();
+  ApplySkipSides(bp);
+  const nsPoint point = aPoint - nsPoint(bp.left, bp.top);
   nsRect thisRect(nsPoint(0,0), GetSize());
-  if (!thisRect.Contains(aPoint)) {
+  if (!thisRect.Contains(point)) {

This can't be right, you're still using the size of the border-rect for your hit-test. Same goes for the other places.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 374491 [details] [diff] [review]
patch

Patch needs to be updated per comment #4

Comment 7

[Boris Zbarsky [:bzbarsky]]

We should really fix this....

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes!

Comment 9

[Robert Longson [:longsonr]]

jwatt, do you mind if I take this?

Comment 10

[Robert Longson [:longsonr]]

Attachment: what I've got

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Instead of calling GetUsedBorderAndPadding and ApplySkipSides, I advise you to just use GetContentRect - GetPosition. Otherwise looks good.

Comment 12

[Robert Longson [:longsonr]]

Attachment: address review comment

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

-  nsRect thisRect(nsPoint(0,0), static_cast<nsSVGOuterSVGFrame*>(mFrame)->GetSize());
+  nsRect thisRect(nsPoint(0,0), outerSVGFrame->GetSize());
   if (!thisRect.Intersects(rectAtOrigin))
     return;

I'm not sure what this check is for, actually. Should we just take it out?

Comment 14

[Robert Longson [:longsonr]]

A check for the point being within the overall bounds for the outer svg element I guess. I guess that might not be valid in the case of overflow: visible but we have bug 378923 for that so how about leaving the code as it is and putting a note in bug 378923 to look more closely into this code as part of that bug. I might even take bug 378923 next.

Comment 15

[Daniel Holbert [:dholbert]]

Pushed final patch on longsonr's behalf:
http://hg.mozilla.org/mozilla-central/rev/fbca3a801c22