hitTest should take care if the location is inside rounded border if border-radius is specified

hitTest should take care if the location is inside rounded border if border-radius is specified

BUG=100004, 105447
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175094

[pals, 2014-04-23 09:42:47]

As you pointed out in my last CL, there was a bug related to border-radius. This
CL fixes the issue. PTAL.

[pals, 2014-04-30 05:57:51]

On 2014/04/23 09:42:47, sanjoy.pal wrote:
> As you pointed out in my last CL, there was a bug related to border-radius.
This
> CL fixes the issue. PTAL.

Friendly ping. Please review.

[ojan, 2014-04-30 18:37:47]

Mostly just cleanup comments. The code change itself looks correct. Thanks for
fixing this. Sorry to keep you waiting on the review.

https://codereview.chromium.org/249133002/diff/1/LayoutTests/fast/borders/bor...
File LayoutTests/fast/borders/border-radius-hittest.html (right):

https://codereview.chromium.org/249133002/diff/1/LayoutTests/fast/borders/bor...
LayoutTests/fast/borders/border-radius-hittest.html:18: y =
offset(innerBox).top;
Nit: You can use getBoundingClientRect instead of offset.

https://codereview.chromium.org/249133002/diff/1/LayoutTests/fast/borders/bor...
LayoutTests/fast/borders/border-radius-hittest.html:29: isSuccessfullyParsed();
I don't think you need isSuccessfullyParsed anymore. js-test.js should do that
for you.

https://codereview.chromium.org/249133002/diff/1/LayoutTests/fast/borders/bor...
LayoutTests/fast/borders/border-radius-hittest.html:34: width: 200px; height:
200px;
Nit: we usually put each property on its own line.

https://codereview.chromium.org/249133002/diff/1/LayoutTests/fast/borders/bor...
LayoutTests/fast/borders/border-radius-hittest.html:48: <div id="console"></div>
You don't need this. js-test.js will add it for you.

https://codereview.chromium.org/249133002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/249133002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:2886: bool checkChildren = !useClip ||
(hasControlClip() ?
locationInContainer.intersects(controlClipRect(adjustedLocation)) :
(!isRenderView() && style()->hasBorderRadius() ?
locationInContainer.intersects(style()->getRoundedBorderFor(clipRect)) :
locationInContainer.intersects(clipRect)));
Why the !isRenderView check? I see that the code below also has that check, but
I don't understand why it's necessary (here or below).

Also, this line is getting a bit unwieldy. We generally avoid nested ternaries.
You could you instead write it as a series of if-statements, e.g. something like

bool checkChildren = !useClip;
if (!checkChildren) {
    if (hasControlClip()) {
        checkChildren =
locationInContainer.intersects(controlClipRect(adjustedLocation));
    } else {
        if (!isRenderView() && style()->hasBorderRadius())
            ...
        else
            ...
    }
}

[pals, 2014-05-06 06:33:40]

Please review.

https://codereview.chromium.org/249133002/diff/1/LayoutTests/fast/borders/bor...
File LayoutTests/fast/borders/border-radius-hittest.html (right):

https://codereview.chromium.org/249133002/diff/1/LayoutTests/fast/borders/bor...
LayoutTests/fast/borders/border-radius-hittest.html:18: y =
offset(innerBox).top;
On 2014/04/30 18:37:47, ojan wrote:
> Nit: You can use getBoundingClientRect instead of offset.

Done.

https://codereview.chromium.org/249133002/diff/1/LayoutTests/fast/borders/bor...
LayoutTests/fast/borders/border-radius-hittest.html:29: isSuccessfullyParsed();
On 2014/04/30 18:37:47, ojan wrote:
> I don't think you need isSuccessfullyParsed anymore. js-test.js should do that
> for you.

Done.

https://codereview.chromium.org/249133002/diff/1/LayoutTests/fast/borders/bor...
LayoutTests/fast/borders/border-radius-hittest.html:34: width: 200px; height:
200px;
On 2014/04/30 18:37:47, ojan wrote:
> Nit: we usually put each property on its own line.

Done.

https://codereview.chromium.org/249133002/diff/1/LayoutTests/fast/borders/bor...
LayoutTests/fast/borders/border-radius-hittest.html:48: <div id="console"></div>
On 2014/04/30 18:37:47, ojan wrote:
> You don't need this. js-test.js will add it for you.
I added this to print the console log below the "container" div. Otherwise we
have to call getBoundingClientRect() after each call of shouldBe(). shouldBe()
prints logs which push down the "container", and makes (x,y) calculated at the
beginning invalid.

https://codereview.chromium.org/249133002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/249133002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:2886: bool checkChildren = !useClip ||
(hasControlClip() ?
locationInContainer.intersects(controlClipRect(adjustedLocation)) :
(!isRenderView() && style()->hasBorderRadius() ?
locationInContainer.intersects(style()->getRoundedBorderFor(clipRect)) :
locationInContainer.intersects(clipRect)));
On 2014/04/30 18:37:47, ojan wrote:
> Why the !isRenderView check? I see that the code below also has that check,
but
> I don't understand why it's necessary (here or below).
> 
> Also, this line is getting a bit unwieldy. We generally avoid nested
ternaries.
> You could you instead write it as a series of if-statements, e.g. something
like
> 
> bool checkChildren = !useClip;
> if (!checkChildren) {
>     if (hasControlClip()) {
>         checkChildren =
> locationInContainer.intersects(controlClipRect(adjustedLocation));
>     } else {
>         if (!isRenderView() && style()->hasBorderRadius())
>             ...
>         else
>             ...
>     }
> }

Done. I have removed the !isRenderView() checks. There is no LayoutTests
failures but I am not sure of this change.

[ojan, 2014-05-07 03:11:02]

https://codereview.chromium.org/249133002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/249133002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:2886: bool checkChildren = !useClip ||
(hasControlClip() ?
locationInContainer.intersects(controlClipRect(adjustedLocation)) :
(!isRenderView() && style()->hasBorderRadius() ?
locationInContainer.intersects(style()->getRoundedBorderFor(clipRect)) :
locationInContainer.intersects(clipRect)));
Can you look at the history for why this check was added in the first place?
It'd be good to understand that to have confidence that we're not regressing
something. I think removing the checks makes sense, but I just want to be more
sure.

[ojan, 2014-05-07 03:18:27]

https://codereview.chromium.org/249133002/diff/50001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/249133002/diff/50001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:2885: LayoutRect clipRect =
overflowClipRect(adjustedLocation, IncludeOverlayScrollbarSize);
It looks like this is only used in the else clause below. Can you move it there?

https://codereview.chromium.org/249133002/diff/50001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:2891: checkChildren =
style()->hasBorderRadius() ?
locationInContainer.intersects(style()->getRoundedBorderFor(clipRect)) :
locationInContainer.intersects(clipRect);
I think the ternary makes this hard to read with so much happening inside the
ternary. I think this could just be:
LayoutRect clipRect = overflowClipRect(adjustedLocation,
IncludeOverlayScrollbarSize);
if (style()->hasBorderRadius)
    clipRect = style()->getRoundedBorderFor(clipRect);
checkChildren = locationInConatiner(clipRect);

[pals, 2014-05-07 06:31:53]

PTAL.

https://codereview.chromium.org/249133002/diff/1/Source/core/rendering/Render...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/249133002/diff/1/Source/core/rendering/Render...
Source/core/rendering/RenderBlock.cpp:2886: bool checkChildren = !useClip ||
(hasControlClip() ?
locationInContainer.intersects(controlClipRect(adjustedLocation)) :
(!isRenderView() && style()->hasBorderRadius() ?
locationInContainer.intersects(style()->getRoundedBorderFor(clipRect)) :
locationInContainer.intersects(clipRect)));
The check was first added at
http://trac.webkit.org/changeset/8070/trunk/WebCore/khtml/rendering/render_bl...
and modified here
http://trac.webkit.org/changeset/40258/trunk/WebCore/rendering/RenderBlock.cpp
And then this check was carried forward here
http://trac.webkit.org/changeset/147635/trunk/Source/WebCore/rendering/Render...

I think this check was added keeping in mind that root element/RenderView cannot
have border-radius. But i am not sure how we could specify border-radius CSS
property for root element in the first place. So, I think its good to remove
this check.

https://codereview.chromium.org/249133002/diff/50001/Source/core/rendering/Re...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/249133002/diff/50001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:2885: LayoutRect clipRect =
overflowClipRect(adjustedLocation, IncludeOverlayScrollbarSize);
On 2014/05/07 03:18:28, ojan wrote:
> It looks like this is only used in the else clause below. Can you move it
there?

Done.

https://codereview.chromium.org/249133002/diff/50001/Source/core/rendering/Re...
Source/core/rendering/RenderBlock.cpp:2891: checkChildren =
style()->hasBorderRadius() ?
locationInContainer.intersects(style()->getRoundedBorderFor(clipRect)) :
locationInContainer.intersects(clipRect);
On 2014/05/07 03:18:28, ojan wrote:
> I think the ternary makes this hard to read with so much happening inside the
> ternary. I think this could just be:
> LayoutRect clipRect = overflowClipRect(adjustedLocation,
> IncludeOverlayScrollbarSize);
> if (style()->hasBorderRadius)
>     clipRect = style()->getRoundedBorderFor(clipRect);
> checkChildren = locationInConatiner(clipRect);

Done.

[pals, 2014-05-26 10:05:44]

Updated the patch. Please take another look.

[ojan, 2014-05-29 22:58:06]

lgtm

Sorry for taking so long to get back to this. Thanks for fixing it.

[commit-bot: I haz the power, 2014-05-29 22:59:11]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/sanjoy.pal@samsung.com/249133002/70001

[commit-bot: I haz the power, 2014-05-30 00:44:34]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  linux_blink_dbg on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/linux_blink_dbg/builds/1...)

[commit-bot: I haz the power, 2014-05-30 02:05:02]

Change committed as 175094