Issue 983693002: Hover not working correctly on image with border-radius.

Issue 983693002: Hover not working correctly on image with border-radius. (Closed)

Created:
5 years, 9 months ago by Abhijeet Kandalkar Slow

Modified:
5 years, 9 months ago

Reviewers:
tkent, leviw_travelin_and_unemployed, esprehn, davve, vivekg

CC:
blink-reviews, blink-reviews-rendering

Base URL:
https://chromium.googlesource.com/chromium/blink.git@master

Target Ref:
refs/heads/master

Project:
blink

Visibility:
Public.

More Reviews

Description

Hittest for inlinebox should take care of border-radius. Current behavior of hittesting works properly for LayoutBlock having border style but doesn't take care of inlineBox with border style and hence returns wrong element. This patch adds implementation for border style handling for inlineBox. BUG=451116 Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=192175

Patch Set 1 #

Total comments: 1

Patch Set 2 : #

Total comments: 8

Patch Set 3 : #

Created: 5 years, 9 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+41 lines, -0 lines)			Patch
A	LayoutTests/fast/borders/border-hittest.html	View	1	1 chunk	+21 lines, -0 lines	0 comments	Download
A	LayoutTests/fast/borders/border-hittest-expected.txt	View	1 2	1 chunk	+11 lines, -0 lines	0 comments	Download
M	Source/core/layout/line/InlineBox.cpp	View	1 2	2 chunks	+9 lines, -0 lines	0 comments	Download

Messages

Total messages: 14 (6 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

rwlbuis

On 2015/03/05 14:39:11, Abhijeet Kandalkar wrote: > mailto:abhijeet.k@samsung.com changed reviewers: > + mailto:abarth@chromium.org, mailto:esprehn@chromium.org, mailto:leviw@chromium.org, ...

5 years, 9 months ago (2015-03-09 21:29:48 UTC) #2

Abhijeet Kandalkar Slow

Rebased patch. Renamed renderer() to layoutObject(). Please review patch.

5 years, 9 months ago (2015-03-11 12:06:06 UTC) #4

Abhijeet Kandalkar Slow

On 2015/03/11 12:06:06, Abhijeet Kandalkar wrote: > Rebased patch. Renamed renderer() to layoutObject(). > Please ...

5 years, 9 months ago (2015-03-18 10:56:42 UTC) #6

davve

Non-owner LGTM from me with some attached nits. https://codereview.chromium.org/983693002/diff/1/LayoutTests/fast/borders/border-hittest.html File LayoutTests/fast/borders/border-hittest.html (right): https://codereview.chromium.org/983693002/diff/1/LayoutTests/fast/borders/border-hittest.html#newcode6 LayoutTests/fast/borders/border-hittest.html:6: img{ ...

5 years, 9 months ago (2015-03-18 15:20:40 UTC) #8

Abhijeet Kandalkar Slow

Incorporated David's comments. Please review patch. https://codereview.chromium.org/983693002/diff/20001/LayoutTests/fast/borders/border-hittest-expected.txt File LayoutTests/fast/borders/border-hittest-expected.txt (left): https://codereview.chromium.org/983693002/diff/20001/LayoutTests/fast/borders/border-hittest-expected.txt#oldcode1 LayoutTests/fast/borders/border-hittest-expected.txt:1: polish On 2015/03/18 ...

5 years, 9 months ago (2015-03-19 13:38:15 UTC) #9

Incorporated David's comments. Please review patch.

https://codereview.chromium.org/983693002/diff/20001/LayoutTests/fast/borders...
File LayoutTests/fast/borders/border-hittest-expected.txt (left):

https://codereview.chromium.org/983693002/diff/20001/LayoutTests/fast/borders...
LayoutTests/fast/borders/border-hittest-expected.txt:1: polish
On 2015/03/18 15:20:40, David Vest wrote:
> You may avoid these dummy diffs by adding --no-find-copies to your git cl
upload
> command:
> 
> git cl upload --no-find-copies

Done.

https://codereview.chromium.org/983693002/diff/20001/Source/core/layout/line/...
File Source/core/layout/line/InlineBox.cpp (right):

https://codereview.chromium.org/983693002/diff/20001/Source/core/layout/line/...
Source/core/layout/line/InlineBox.cpp:24: #include "core/layout/HitTestResult.h"
On 2015/03/18 15:20:40, David Vest wrote:
> Why do you need to add this include? None of the added code touches the hit
test
> result, as far as I can see.

locationInContainer is of type HitTestLocation, so added HitTestLocation.h
and removed other two include

https://codereview.chromium.org/983693002/diff/20001/Source/core/layout/line/...
Source/core/layout/line/InlineBox.cpp:213: if
(layoutObject().style()->hasBorderRadius()) {
On 2015/03/18 15:20:40, David Vest wrote:
> While adding this code addresses the test in the bug, a slightly contrived
case
> such as:
> 
> <!doctype html>
>   <style>
>     body { margin: 2em; }
>     span {
>         background-color: green;
>         border-radius: 50px;
>         padding: 1em;
>     }
>     span:hover {
>         background-color:red;
>     }
> </style>
> <span>
>   <button>dummy</button>
> </span>
> 
> won't work, since the border-radius will end up on a RootInlineBox, not
> InlineBox, presumably. I note that this particular case works fine in Firefox.
> 
> I don't necessarily find this a big problem but wanted to bring it up anyway.


We can raise separate bug for this special use case and handle it there.

https://codereview.chromium.org/983693002/diff/20001/Source/core/layout/line/...
Source/core/layout/line/InlineBox.cpp:215: LayoutPoint
adjustedLocation(accumulatedOffset);
On 2015/03/18 15:20:40, David Vest wrote:
> Not sure there is value in the adjustedLocation variable. Seems fine to me to
> just put accumulatedOffset info moveBy directly.

Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/983693002/60001

5 years, 9 months ago (2015-03-19 14:00:00 UTC) #13

commit-bot: I haz the power

5 years, 9 months ago (2015-03-19 15:27:39 UTC) #14

Message was sent while issue was closed.

Committed patchset #3 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192175

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages