REGRESSION: Click on focused link should not remove focus on it.

REGRESSION: Click on focused link should not remove focus on it.

This is a regression by wkrev.com/72796.

The first isMouseFocusable call in EventHandler::dispatchMouseEvent 
should be isFocusable because we need to do different things for
focusable-but-non-mouse-focusable elements.

Also fix wrong usage of isMouseFocusable in text shadow elements and
SVG elements. In general isFocusable must return true whenever
isMouseFocusable is true. Some layout tests fail without these changes.

BUG=264575
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=155083

[dglazkov, 2013-07-26 13:27:22]

lgtm

[Stephen Chennney, 2013-07-26 14:31:14]

Not ready yet.

https://codereview.chromium.org/20670002/diff/7001/Source/core/svg/SVGStyledE...
File Source/core/svg/SVGStyledElement.cpp (right):

https://codereview.chromium.org/20670002/diff/7001/Source/core/svg/SVGStyledE...
Source/core/svg/SVGStyledElement.cpp:474: Element* eventTarget =
const_cast<SVGStyledElement *>(this);
This should be toSVGStyledElement, but is it even necessary since you're storing
an Element?

Also, SVGStyledElement is going away, so if possible all this should be in
SVGElement.

[tkent, 2013-07-28 23:42:18]

pdr, can you take a loot at SVG changes because Stephen is OOO?

https://codereview.chromium.org/20670002/diff/7001/Source/core/svg/SVGStyledE...
File Source/core/svg/SVGStyledElement.cpp (right):

https://codereview.chromium.org/20670002/diff/7001/Source/core/svg/SVGStyledE...
Source/core/svg/SVGStyledElement.cpp:474: Element* eventTarget =
const_cast<SVGStyledElement *>(this);
On 2013/07/26 14:31:14, Stephen Chenney wrote:
> This should be toSVGStyledElement, but is it even necessary since you're
storing
> an Element?
> 
> Also, SVGStyledElement is going away, so if possible all this should be in
> SVGElement.

The purpose of this case is to get non-const |this| because hasEventListeners is
not a const function. We don't have toSVG*Element function for const_cast.

Ok, I moved these functions to SVGElement.

[tkent, 2013-07-28 23:43:17]

> The purpose of this case is to get non-const |this| because hasEventListeners
is

this case -> this cast

[pdr., 2013-07-29 00:17:47]

LGTM with nits.

https://codereview.chromium.org/20670002/diff/16001/LayoutTests/fast/events/f...
File LayoutTests/fast/events/focus-click-on-non-mouse-focusable-element.html
(right):

https://codereview.chromium.org/20670002/diff/16001/LayoutTests/fast/events/f...
LayoutTests/fast/events/focus-click-on-non-mouse-focusable-element.html:7:
description('Click on a focused anchor element should not change focus, and
click on a non-focused anchor element should remove focus of another elemnt.');
elemnt -> element

https://codereview.chromium.org/20670002/diff/16001/Source/core/svg/SVGElemen...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/20670002/diff/16001/Source/core/svg/SVGElemen...
Source/core/svg/SVGElement.cpp:645: SVGElement* mutableThis =
const_cast<SVGElement*>(this);
I've been bitten by cost_cast in the past. If this is truly safe, can
hasEventListeners be made const and this const_cast moved there?

[tkent, 2013-07-29 03:31:06]

https://chromiumcodereview.appspot.com/20670002/diff/16001/LayoutTests/fast/e...
File LayoutTests/fast/events/focus-click-on-non-mouse-focusable-element.html
(right):

https://chromiumcodereview.appspot.com/20670002/diff/16001/LayoutTests/fast/e...
LayoutTests/fast/events/focus-click-on-non-mouse-focusable-element.html:7:
description('Click on a focused anchor element should not change focus, and
click on a non-focused anchor element should remove focus of another elemnt.');
On 2013/07/29 00:17:47, pdr wrote:
> elemnt -> element

Done.

https://chromiumcodereview.appspot.com/20670002/diff/16001/Source/core/svg/SV...
File Source/core/svg/SVGElement.cpp (right):

https://chromiumcodereview.appspot.com/20670002/diff/16001/Source/core/svg/SV...
Source/core/svg/SVGElement.cpp:645: SVGElement* mutableThis =
const_cast<SVGElement*>(this);
On 2013/07/29 00:17:47, pdr wrote:
> I've been bitten by cost_cast in the past. If this is truly safe, can
> hasEventListeners be made const and this const_cast moved there?

yeah, I think we can.  I added a FIXME comment.

[pdr., 2013-07-29 03:33:48]

On 2013/07/29 03:31:06, tkent wrote:
>
https://chromiumcodereview.appspot.com/20670002/diff/16001/LayoutTests/fast/e...
> File LayoutTests/fast/events/focus-click-on-non-mouse-focusable-element.html
> (right):
> 
>
https://chromiumcodereview.appspot.com/20670002/diff/16001/LayoutTests/fast/e...
> LayoutTests/fast/events/focus-click-on-non-mouse-focusable-element.html:7:
> description('Click on a focused anchor element should not change focus, and
> click on a non-focused anchor element should remove focus of another
elemnt.');
> On 2013/07/29 00:17:47, pdr wrote:
> > elemnt -> element
> 
> Done.
> 
>
https://chromiumcodereview.appspot.com/20670002/diff/16001/Source/core/svg/SV...
> File Source/core/svg/SVGElement.cpp (right):
> 
>
https://chromiumcodereview.appspot.com/20670002/diff/16001/Source/core/svg/SV...
> Source/core/svg/SVGElement.cpp:645: SVGElement* mutableThis =
> const_cast<SVGElement*>(this);
> On 2013/07/29 00:17:47, pdr wrote:
> > I've been bitten by cost_cast in the past. If this is truly safe, can
> > hasEventListeners be made const and this const_cast moved there?
> 
> yeah, I think we can.  I added a FIXME comment.

LGTM

[commit-bot: I haz the power, 2013-07-29 05:16:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tkent@chromium.org/20670002/27001

[commit-bot: I haz the power, 2013-07-29 09:28:44]

Change committed as 155083