Bug 95373 (border-radius)

Summary: Clickable area is incorrect for elements with border-radius
Product: WebKit Reporter: Dan <phoxkid>
Component: CSSAssignee: Xidorn Quan <xidorn-webkit>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, dglazkov, eric, esprehn+autocc, ojan.autocc, rniwa, simon.fraser, tasak, webkit.review.bot, xidorn-webkit
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
URL: http://jsbin.com/ibiciz/1
Attachments:
Description Flags
pic: webkit border-radius bug
none
WIP
none
Patch
none
A test case that Takashi Sakamoto's patch does not work properly.
none
Modified patch
none
Modified patch
webkit.review.bot: commit-queue-
Modified patch
none
Modified patch
morrita: review-
patch
simon.fraser: review-
patch
simon.fraser: review-
patch none

Description Dan 2012-08-29 13:40:38 PDT
Created attachment 161302 [details]
pic: webkit border-radius bug

On Safari and Chrome bug when I hover element that's behind css-circle-box the hover event isn't triggered, but on Firefox and IE9 I don't have that kind of problem.
Comment 1 Takashi Sakamoto 2012-08-31 04:23:55 PDT
Created attachment 161662 [details]
WIP
Comment 2 Takashi Sakamoto 2012-09-17 22:08:44 PDT
Created attachment 164492 [details]
Patch
Comment 3 Dan 2012-09-18 22:33:38 PDT
(In reply to comment #2)
> Created an attachment (id=164492) [details]
> Patch
Hi and excuse me. I am new to bugzilla. You added attachments, but how do I use them? Do I just put the Javascript you gave me inside my file?
Comment 4 Takashi Sakamoto 2013-02-27 14:38:37 PST
*** Bug 110684 has been marked as a duplicate of this bug. ***
Comment 5 Xidorn Quan 2013-03-02 06:27:30 PST
The patch by Takashi Sakamoto works quite well for me, and the merge conflict can be easily resolved. What prevents this patch to be applied?
Comment 6 Xidorn Quan 2013-03-02 10:15:44 PST
Created attachment 191109 [details]
A test case that Takashi Sakamoto's patch does not work properly.
Comment 7 Xidorn Quan 2013-03-02 10:21:35 PST
Created attachment 191111 [details]
Modified patch

To deal with the testcase I submitted just now, I modified the patch initially submitted by Takashi Sakamoto. I also rearranged some of the code so that it can run faster.

This modified patch does not include Changelog and tests which presented in Takashi Sakamoto's patch since I did not know how to merge them properly.
Comment 8 Xidorn Quan 2013-03-03 07:58:50 PST
Created attachment 191134 [details]
Modified patch

Sorry for the previous useless patch.
This patch should work properly.
Comment 9 WebKit Review Bot 2013-03-03 08:02:30 PST
Attachment 191134 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-radius-position-expected.txt', u'LayoutTests/fast/borders/border-radius-position.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/rendering/HitTestResult.cpp', u'Source/WebCore/rendering/HitTestResult.h', u'Source/WebCore/rendering/RenderBlock.cpp']" exit_code: 1
LayoutTests/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Source/WebCore/rendering/HitTestResult.cpp:233:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/ChangeLog:1:  ChangeLog entry has no bug number  [changelog/bugnumber] [5]
Total errors found: 3 in 7 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 10 WebKit Review Bot 2013-03-03 09:55:27 PST
Comment on attachment 191134 [details]
Modified patch

Attachment 191134 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/16908190

New failing tests:
fast/borders/border-radius-position.html
Comment 11 Build Bot 2013-03-03 11:34:14 PST
Comment on attachment 191134 [details]
Modified patch

Attachment 191134 [details] did not pass mac-ews (mac):
Output: http://webkit-commit-queue.appspot.com/results/16901405

New failing tests:
fast/borders/border-radius-position.html
Comment 12 Build Bot 2013-03-03 14:00:01 PST
Comment on attachment 191134 [details]
Modified patch

Attachment 191134 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/16899526

New failing tests:
fast/borders/border-radius-position.html
Comment 13 Xidorn Quan 2013-03-03 18:37:52 PST
Created attachment 191159 [details]
Modified patch

Fix the style error and slightly adjust the additional test.
Comment 14 Xidorn Quan 2013-03-05 20:57:58 PST
Created attachment 191645 [details]
Modified patch

I noticed that code related to class HitTestLocation has been totally
moved to separate files, which makes the previous patch fail to be
applied on the current source tree. So I made this patch to solve this
problem.
Comment 15 Simon Fraser (smfr) 2013-03-05 22:01:27 PST
The title of this bug doesn't indicate that it has anything to do with hit testing.
Comment 16 Hajime Morrita 2013-03-05 23:49:58 PST
Comment on attachment 191645 [details]
Modified patch

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

> Source/WebCore/ChangeLog:1
> +2013-03-03  Takashi Sakamoto  <tasak@google.com>

Please use your name. and mention the original author on the comment.
Comment 17 Xidorn Quan 2013-03-06 02:37:00 PST
Created attachment 191695 [details]
patch
Comment 18 Simon Fraser (smfr) 2013-03-06 11:13:08 PST
Comment on attachment 191695 [details]
patch

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

> Source/WebCore/rendering/HitTestLocation.cpp:217
> +static inline bool isLineIntersectsEllipse(const FloatPoint& center, const FloatSize& size, const FloatPoint& p0, const FloatPoint& p1)
> +{
> +    float x0 = (p0.x() - center.x()) * size.height() / size.width();
> +    float y0 = p0.y() - center.y();
> +    float x1 = (p1.x() - center.x()) * size.height() / size.width();
> +    float y1 = p1.y() - center.y();
> +    float radius2 = size.height() * size.height();
> +    if ((x0 * x0 + y0 * y0) <= radius2 || (x1 * x1 + y1 * y1) <= radius2)
> +        return true;
> +
> +    float a = y0 - y1;
> +    float b = x1 - x0;
> +    float c = x0 * y1 - x1 * y0;
> +    float distance2 = c * c / (a * a + b * b);
> +    // If distance between the center point and the line > the radius,
> +    // the line doesn't cross (or is contained by) the ellipse.
> +    if (distance2 > radius2)
> +        return false;
> +
> +    // The nearest point on the line is between p0 and p1?
> +    float x = - a * c / (a * a + b * b);
> +    float y = - b * c / (a * a + b * b);
> +    return (((x0 <= x && x <= x1) || (x0 >= x && x >= x1))
> +        && ((y0 <= y && y <= y1) || (y1 <= y && y <= y0)));
> +}
> +
> +static inline bool isQuadIntersectsEllipse(const FloatPoint& center, const FloatSize& size, const FloatQuad& quad)
> +{
> +    if (isLineIntersectsEllipse(center, size, quad.p1(), quad.p2())
> +        || isLineIntersectsEllipse(center, size, quad.p2(), quad.p3())
> +        || isLineIntersectsEllipse(center, size, quad.p3(), quad.p4())
> +        || isLineIntersectsEllipse(center, size, quad.p4(), quad.p1()))
> +        return true;
> +    return false;
> +}

Please move this code out into a separate file; it is generally useful. I suggest platform/graphics/GeometryUtilities.h/cpp. That file could also contain the

float findSlope(const FloatPoint& p1, const FloatPoint& p2, float& c);
bool findIntersection(const FloatPoint& p1, const FloatPoint& p2, const FloatPoint& d1, const FloatPoint& d2, FloatPoint& intersection);

functions that are currently in FloatPoint.cpp.

isLineIntersectsEllipse() should be called lineIntersectsEllipse(). isQuadIntersectsEllipse() should be called quadIntersectsEllipse().

Why don't you just add RoundedRect::containsPoint()?
Comment 19 Xidorn Quan 2013-03-06 18:09:14 PST
(In reply to comment #18)
> (From update of attachment 191695 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=191695&action=review
> 
> Please move this code out into a separate file; it is generally useful. I suggest platform/graphics/GeometryUtilities.h/cpp. That file could also contain the
> 
> float findSlope(const FloatPoint& p1, const FloatPoint& p2, float& c);
> bool findIntersection(const FloatPoint& p1, const FloatPoint& p2, const FloatPoint& d1, const FloatPoint& d2, FloatPoint& intersection);
> 
> functions that are currently in FloatPoint.cpp.

Should I move these functions in the same patch? 

IMHO, unrelated changes should be put in separate patches. It might be better that you move the existing functions to that files first, and then I move functions in my patch into them.

> 
> isLineIntersectsEllipse() should be called lineIntersectsEllipse(). isQuadIntersectsEllipse() should be called quadIntersectsEllipse().
> 
> Why don't you just add RoundedRect::containsPoint()?

Containing a point is not a necessary condition for a rounded rect to intersect a quad, since there is possibility that all points of a quad are out of a rounded rect but one edge of the quad cuts an arc on the corner of the rounded rect.

Should I add RoundedRect::intersectsQuad() instead?
Comment 20 Simon Fraser (smfr) 2013-03-06 18:12:53 PST
(In reply to comment #19)
> (In reply to comment #18)
> > (From update of attachment 191695 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=191695&action=review
> > 
> > Please move this code out into a separate file; it is generally useful. I suggest platform/graphics/GeometryUtilities.h/cpp. That file could also contain the
> > 
> > float findSlope(const FloatPoint& p1, const FloatPoint& p2, float& c);
> > bool findIntersection(const FloatPoint& p1, const FloatPoint& p2, const FloatPoint& d1, const FloatPoint& d2, FloatPoint& intersection);
> > 
> > functions that are currently in FloatPoint.cpp.
> 
> Should I move these functions in the same patch? 
> 
> IMHO, unrelated changes should be put in separate patches. It might be better that you move the existing functions to that files first, and then I move functions in my patch into them.

Yes, that would be good.

> > isLineIntersectsEllipse() should be called lineIntersectsEllipse(). isQuadIntersectsEllipse() should be called quadIntersectsEllipse().
> > 
> > Why don't you just add RoundedRect::containsPoint()?
> 
> Containing a point is not a necessary condition for a rounded rect to intersect a quad, since there is possibility that all points of a quad are out of a rounded rect but one edge of the quad cuts an arc on the corner of the rounded rect.
> 
> Should I add RoundedRect::intersectsQuad() instead?

I think both would be useful. I guess we hit test using quads now, but the primitive is also useful.
Comment 21 Xidorn Quan 2013-03-06 18:56:21 PST
(In reply to comment #20)
> (In reply to comment #19)
> > (In reply to comment #18)
> > > (From update of attachment 191695 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=191695&action=review
> > > 
> > > Please move this code out into a separate file; it is generally useful. I suggest platform/graphics/GeometryUtilities.h/cpp. That file could also contain the
> > > 
> > > float findSlope(const FloatPoint& p1, const FloatPoint& p2, float& c);
> > > bool findIntersection(const FloatPoint& p1, const FloatPoint& p2, const FloatPoint& d1, const FloatPoint& d2, FloatPoint& intersection);
> > > 
> > > functions that are currently in FloatPoint.cpp.
> > 
> > Should I move these functions in the same patch? 
> > 
> > IMHO, unrelated changes should be put in separate patches. It might be better that you move the existing functions to that files first, and then I move functions in my patch into them.
> 
> Yes, that would be good.

What is meant by "that would be good"? Does that mean I should move those functions in FloatPoint.cpp to the files in the same patch or you will move them?

> > > isLineIntersectsEllipse() should be called lineIntersectsEllipse(). isQuadIntersectsEllipse() should be called quadIntersectsEllipse().
> > > 
> > > Why don't you just add RoundedRect::containsPoint()?
> > 
> > Containing a point is not a necessary condition for a rounded rect to intersect a quad, since there is possibility that all points of a quad are out of a rounded rect but one edge of the quad cuts an arc on the corner of the rounded rect.
> > 
> > Should I add RoundedRect::intersectsQuad() instead?
> 
> I think both would be useful. I guess we hit test using quads now, but the primitive is also useful.

I'm quite interested in the reason of using quads instead of points.

Although RoundedRect::containsPoint() seems to be useful, it would be a problem if it is added but not called anywhere.
Comment 22 Xidorn Quan 2013-03-07 21:07:32 PST
Created attachment 192139 [details]
patch

According to Simon Fraser's suggestion, I moved some of the code into RoundedRect.cpp and FloatQuad.cpp so that it will be reusable.

I tried to move the function lineIntersectsCircle to the new separate file mentioned by Simon, but I failed to do that because I am not familiar with the build system of WebKit, and cannot get the new file to be compiled. If you can give some hint, I would still like to create the new file.
Comment 23 WebKit Review Bot 2013-03-07 21:10:03 PST
Attachment 192139 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast/borders/border-radius-position-expected.txt', u'LayoutTests/fast/borders/border-radius-position.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/platform/graphics/FloatQuad.cpp', u'Source/WebCore/platform/graphics/FloatQuad.h', u'Source/WebCore/platform/graphics/RoundedRect.cpp', u'Source/WebCore/platform/graphics/RoundedRect.h', u'Source/WebCore/rendering/HitTestLocation.cpp', u'Source/WebCore/rendering/HitTestLocation.h', u'Source/WebCore/rendering/RenderBlock.cpp']" exit_code: 1
Source/WebCore/platform/graphics/RoundedRect.h:31:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/platform/graphics/FloatQuad.cpp:208:  One space before end of line comments  [whitespace/comments] [5]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 24 Xidorn Quan 2013-03-07 21:36:47 PST
Sorry for not checking the style before submitting. However, the style errors seem to be trivial, so I will not submit a new patch before any other problem is reported.
Comment 25 Xidorn Quan 2013-03-12 19:13:59 PDT
ping?
Comment 26 Simon Fraser (smfr) 2013-03-12 21:43:25 PDT
Comment on attachment 192139 [details]
patch

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

> Source/WebCore/platform/graphics/FloatQuad.cpp:181
> +// tests whether the line is contained by or intersected with the circle

Please uppercase Tests, and end with a period.

> Source/WebCore/platform/graphics/FloatQuad.cpp:193
> +    float distance2 = c * c / (a * a + b * b);

You compute (a * a + b * b) possibly 3 times in this function.

Can (a * a + b * b) be 0, resulting in a divide by zero?

>> Source/WebCore/platform/graphics/FloatQuad.cpp:208
>> +    return containsPoint(center)   // the circle may be totally contained by the quad
> 
> One space before end of line comments  [whitespace/comments] [5]

And Sentence case and punctuation please.

> Source/WebCore/platform/graphics/FloatQuad.cpp:217
> +    FloatQuad transformedQuad(m_p1, m_p2, m_p3, m_p4);

Can't you just say FloatQuad transformedQuad(*this)?

> Source/WebCore/platform/graphics/FloatQuad.cpp:222
> +    transformedQuad.move(-center.x(), -center.y());
> +    transformedQuad.scale(size.height(), size.width());
> +
> +    FloatPoint originPoint;
> +    return transformedQuad.intersectsCircle(originPoint, size.height() * size.width());

I'm not really clear on what you're doing here. Please add a comment. How well have you tested this code?

> Source/WebCore/platform/graphics/FloatQuad.h:98
> +    bool intersectsCircle(const FloatPoint&, float) const;
> +    bool intersectsEllipse(const FloatPoint&, const FloatSize&) const;

Please name the parameters. It's not obvious what they are..

> Source/WebCore/platform/graphics/RoundedRect.cpp:180
> +bool RoundedRect::intersectsQuad(const FloatQuad& quad) const

For platforms that don't do area hit-testing, I assume that all corners of the quad are the same point. I think you should add a fast-path for this case.
Comment 27 Xidorn Quan 2013-03-12 23:07:54 PDT
(In reply to comment #26)
> (From update of attachment 192139 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=192139&action=review
> 
> > Source/WebCore/platform/graphics/FloatQuad.cpp:181
> > +// tests whether the line is contained by or intersected with the circle
> 
> Please uppercase Tests, and end with a period.

Done.

> > Source/WebCore/platform/graphics/FloatQuad.cpp:193
> > +    float distance2 = c * c / (a * a + b * b);
> 
> You compute (a * a + b * b) possibly 3 times in this function.

It might be more accurate that c / (a * a + b * b) is computed 3 times. I believe it wouldn't be a problem since the compiler should know how to optimize it.

> Can (a * a + b * b) be 0, resulting in a divide by zero?

If p0 and p1 are different, it will not be 0. It might be a good idea to add a check of whether they are equal. I'll do that.

> >> Source/WebCore/platform/graphics/FloatQuad.cpp:208
> >> +    return containsPoint(center)   // the circle may be totally contained by the quad
> > 
> > One space before end of line comments  [whitespace/comments] [5]
> 
> And Sentence case and punctuation please.

Done.

> > Source/WebCore/platform/graphics/FloatQuad.cpp:217
> > +    FloatQuad transformedQuad(m_p1, m_p2, m_p3, m_p4);
> 
> Can't you just say FloatQuad transformedQuad(*this)?

Yes, I wanted to do this but found no similar code in the current source tree.

> > Source/WebCore/platform/graphics/FloatQuad.cpp:222
> > +    transformedQuad.move(-center.x(), -center.y());
> > +    transformedQuad.scale(size.height(), size.width());
> > +
> > +    FloatPoint originPoint;
> > +    return transformedQuad.intersectsCircle(originPoint, size.height() * size.width());
> 
> I'm not really clear on what you're doing here. Please add a comment. How well have you tested this code?

The method normalizes the ellipse to an origin-centered circle whose radius is the product of major radius and minor radius. So it is necessary to do the same transformation on the quad. I'll add the comment.

> > Source/WebCore/platform/graphics/FloatQuad.h:98
> > +    bool intersectsCircle(const FloatPoint&, float) const;
> > +    bool intersectsEllipse(const FloatPoint&, const FloatSize&) const;
> 
> Please name the parameters. It's not obvious what they are..

Done.

> > Source/WebCore/platform/graphics/RoundedRect.cpp:180
> > +bool RoundedRect::intersectsQuad(const FloatQuad& quad) const
> 
> For platforms that don't do area hit-testing, I assume that all corners of the quad are the same point. I think you should add a fast-path for this case.

There is only minor difference between hit-testing by area and by point, so adding such fast-path may cause significant code redundance, or it will not be fast.

I'll file the new patch soon.

In addition, I have a question that, why the status of this bug stays UNCONFIRMED? Shouldn't it be NEW?
Comment 28 Xidorn Quan 2013-03-12 23:31:39 PDT
Created attachment 192872 [details]
patch
Comment 29 WebKit Review Bot 2013-03-14 20:39:23 PDT
Comment on attachment 192872 [details]
patch

Clearing flags on attachment: 192872

Committed r145870: <http://trac.webkit.org/changeset/145870>
Comment 30 WebKit Review Bot 2013-03-14 20:39:29 PDT
All reviewed patches have been landed.  Closing bug.
Comment 31 Alexey Proskuryakov 2013-04-01 09:56:44 PDT
Looks like this change caused bug 113591.