Bug 95373 – Clickable area is incorrect for elements with border-radius

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Dan]

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.

[Takashi Sakamoto]

Created attachment 161662 [details]
WIP

[Takashi Sakamoto]

Created attachment 164492 [details]
Patch

[Dan]

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

[Takashi Sakamoto]

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

[Xidorn Quan]

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?

[Xidorn Quan]

Created attachment 191109 [details]
A test case that Takashi Sakamoto's patch does not work properly.

[Xidorn Quan]

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.

[Xidorn Quan]

Created attachment 191134 [details]
Modified patch

Sorry for the previous useless patch.
This patch should work properly.

[WebKit Review Bot]

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.

[WebKit Review Bot]

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

[Build Bot]

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

[Build Bot]

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

[Xidorn Quan]

Created attachment 191159 [details]
Modified patch

Fix the style error and slightly adjust the additional test.

[Xidorn Quan]

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.

[Simon Fraser (smfr)]

The title of this bug doesn't indicate that it has anything to do with hit testing.

[Hajime Morrita]

Comment on attachment 191645 [details]
Modified patch

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

> Source/WebCore/ChangeLog:1
> [phone]  Takashi Sakamoto  <[e-mail]>

Please use your name. and mention the original author on the comment.

[Xidorn Quan]

Created attachment 191695 [details]
patch

[Simon Fraser (smfr)]

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

[Xidorn Quan]

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

[Simon Fraser (smfr)]

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

[Xidorn Quan]

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

[Xidorn Quan]

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.

[WebKit Review Bot]

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.

[Xidorn Quan]

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.

[Xidorn Quan]

ping?

[Simon Fraser (smfr)]

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.

[Xidorn Quan]

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

[Xidorn Quan]

Created attachment 192872 [details]
patch

[WebKit Review Bot]

Comment on attachment 192872 [details]
patch

Clearing flags on attachment: 192872

Committed r145870: <http://trac.webkit.org/changeset/145870>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Looks like this change caused bug 113591.