Include inner border radius in block hit testing

Include inner border radius in block hit testing

This patch fixes a bug in [1] where the a border radius used was used
based on the clip rect instead of an inner border radius based on the
border box. A test has been added covering both the overflow auto
and overflow scroll cases.

This patch also creates a new top-level hittesting directory for tests.

[1] https://src.chromium.org/viewvc/blink?view=rev&revision=175094
BUG=468613
Committed: https://crrev.com/b6749c9b582f70fc9a5dcb364087e30cf45a7306
Cr-Commit-Position: refs/heads/master@{#364134}

[pdr., 2015-12-09 05:54:28]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 05:54:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1513663002/1
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1513663002/1

[pdr., 2015-12-09 06:48:20]

pdr@chromium.org changed reviewers:
+ chrishtr@chromium.org

[pdr., 2015-12-09 06:50:14]

After some more investigation... the !hasSelfPaintingLayer codepath is
definitely needed.. This patch fixes a bug in it, and adds a test.

In a followup I'd like to fix the paint layer version too.

[commit-bot: I haz the power, 2015-12-09 09:57:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 09:57:09]

Dry run: Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[chrishtr, 2015-12-09 17:06:40]

What is the bug you're fixing?

https://codereview.chromium.org/1513663002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/borders/inner-border-radius-hittest.html
(right):

https://codereview.chromium.org/1513663002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/borders/inner-border-radius-hittest.html:2:
<script src="../../resources/js-test.js"></script>
Is there a hit-testing directory? If not, let's create one and put hit-testing
tests there.

[pdr., 2015-12-09 17:42:08]

This is a real bug where we used the wrong border radius, discovered while
investigating the self painting issue we talked about yesterday, where we really
do need border radius code both in LayoutBlock (or, LayoutBox) and
PaintLayerClipper.

My plan is to do a followup patch to unify this code with the PaintLayerClipper
code that doesn't yet exist (border radius doens't currently work properly in
paint layers). Then, a second followup will move the clip-path hit testing logic
from LayoutBlock to paint layer. The combination of all of these patches will
fully fix 468613.

https://codereview.chromium.org/1513663002/diff/1/third_party/WebKit/LayoutTe...
File
third_party/WebKit/LayoutTests/fast/borders/inner-border-radius-hittest.html
(right):

https://codereview.chromium.org/1513663002/diff/1/third_party/WebKit/LayoutTe...
third_party/WebKit/LayoutTests/fast/borders/inner-border-radius-hittest.html:2:
<script src="../../resources/js-test.js"></script>
On 2015/12/09 at 17:06:40, chrishtr wrote:
> Is there a hit-testing directory? If not, let's create one and put hit-testing
tests there.

Done. Moved all border hittesting tests there. There's still a very large group
of hittesting tests in tables/hittesting and svg/hittesting.

[pdr., 2015-12-09 17:43:44]

Description was changed from

==========
Include inner border radius in block hit testing

This patch fixes a bug in [1] where the a border radius used was used
based on the clip rect instead of an inner border radius based on the
border box. A test has been added covering both the overflow auto
and overflow scroll cases.

[1] https://src.chromium.org/viewvc/blink?view=rev&revision=175094
BUG=468613
==========

to

==========
Include inner border radius in block hit testing

This patch fixes a bug in [1] where the a border radius used was used
based on the clip rect instead of an inner border radius based on the
border box. A test has been added covering both the overflow auto
and overflow scroll cases.

This patch also creates a new top-level hittesting directory for tests.

[1] https://src.chromium.org/viewvc/blink?view=rev&revision=175094
BUG=468613
==========

[pdr., 2015-12-09 17:51:26]

The CQ bit was checked by pdr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2015-12-09 17:53:48]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1513663002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1513663002/20001

[chrishtr, 2015-12-09 17:55:58]

Are the other callers of overflowClipRect also wrong then? Does overflowClipRect
need to be fixed?

[pdr., 2015-12-09 18:00:17]

On 2015/12/09 at 17:55:58, chrishtr wrote:
> Are the other callers of overflowClipRect also wrong then? Does
overflowClipRect need to be fixed?

No, overflowClipRect is correct, it just isn't the rect we should use for border
radius calculations. When drawing border radii, we do not use the overflow clip
rect.

If you make these tests self painting layers, a similar bug exists so there's at
least one other instance of this bug elsewhere. In a followup, I'd like to
address this.

[chrishtr, 2015-12-09 18:01:43]

On 2015/12/09 at 18:00:17, pdr wrote:
> On 2015/12/09 at 17:55:58, chrishtr wrote:
> > Are the other callers of overflowClipRect also wrong then? Does
overflowClipRect need to be fixed?
> 
> No, overflowClipRect is correct, it just isn't the rect we should use for
border radius calculations. When drawing border radii, we do not use the
overflow clip rect.
> 
> If you make these tests self painting layers, a similar bug exists so there's
at least one other instance of this bug elsewhere. In a followup, I'd like to
address this.

Are you sure? Some of the uses of overflowClipRect are about early outing during
paint, which is the equivalent of this code.

[pdr., 2015-12-09 18:11:53]

On 2015/12/09 at 18:01:43, chrishtr wrote:
> On 2015/12/09 at 18:00:17, pdr wrote:
> > On 2015/12/09 at 17:55:58, chrishtr wrote:
> > > Are the other callers of overflowClipRect also wrong then? Does
overflowClipRect need to be fixed?
> > 
> > No, overflowClipRect is correct, it just isn't the rect we should use for
border radius calculations. When drawing border radii, we do not use the
overflow clip rect.
> > 
> > If you make these tests self painting layers, a similar bug exists so
there's at least one other instance of this bug elsewhere. In a followup, I'd
like to address this.
> 
> Are you sure? Some of the uses of overflowClipRect are about early outing
during paint, which is the equivalent of this code.

This patch only removes using overflowClipRect for rounded borders, but we still
use it here and elsewhere for overflow clips because it's the correct rect to
use.

[commit-bot: I haz the power, 2015-12-09 19:40:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-12-09 19:40:27]

Dry run: This issue passed the CQ dry run.

[chrishtr, 2015-12-09 19:46:54]

The CQ bit was checked by chrishtr@chromium.org

[chrishtr, 2015-12-09 19:46:54]

lgtm

[commit-bot: I haz the power, 2015-12-09 19:49:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1513663002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1513663002/20001

[commit-bot: I haz the power, 2015-12-09 19:59:13]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-12-09 20:00:07]

Description was changed from

==========
Include inner border radius in block hit testing

This patch fixes a bug in [1] where the a border radius used was used
based on the clip rect instead of an inner border radius based on the
border box. A test has been added covering both the overflow auto
and overflow scroll cases.

This patch also creates a new top-level hittesting directory for tests.

[1] https://src.chromium.org/viewvc/blink?view=rev&revision=175094
BUG=468613
==========

to

==========
Include inner border radius in block hit testing

This patch fixes a bug in [1] where the a border radius used was used
based on the clip rect instead of an inner border radius based on the
border box. A test has been added covering both the overflow auto
and overflow scroll cases.

This patch also creates a new top-level hittesting directory for tests.

[1] https://src.chromium.org/viewvc/blink?view=rev&revision=175094
BUG=468613

Committed: https://crrev.com/b6749c9b582f70fc9a5dcb364087e30cf45a7306
Cr-Commit-Position: refs/heads/master@{#364134}
==========

[commit-bot: I haz the power, 2015-12-09 20:00:08]

Patchset 2 (id:??) landed as
https://crrev.com/b6749c9b582f70fc9a5dcb364087e30cf45a7306
Cr-Commit-Position: refs/heads/master@{#364134}