Mouse press should focus on any types of form controls.

Mouse press should focus on any types of form controls.

* Background
In WebKit, form controls except text fields and <select> are not
mouse-focusable. This behavior is based on Cocoa native controls.
Though Firefox on Mac behaves same as WebKit, IE, Firefox (non-Mac),
and Opera(Presto) set focus by mouse press.

The WebKit behavior is not Web-compatible.

* Behavior change
We change the behavior so that mouse press sets focus to form controls
to improve web compatibility. However, We haven't set focus until now and
this behavior change might make existing users uncomfortable with focus
rings which appear on click. So, we don't draw focus rings if a form control
was focused by mouse press.

* Implementation
The main part of this CL is to remove HTMLFormControlElement::isMouseFocusable.
It means we use Node::isMouseFocusable, which is just isFocusable. We
can remove many isMouseFocusable implementations of
HTMLFormControlElement subclasses.

We change the followings to hide focus rings on mouse-focus:
 - Introduce FocusDirectionMouse so that form control can distinguish
   mouse focus from others.
 - HTMLFormControlElement has m_wasFocusedByMouse flag. It is cleared
   when a keydown event is delivered.
 - Introduce HTMLFormControlElement::shouldShowFocusRingOnMouseFocus,
    which represents differences between button controls and text/select
    controls.
 - RenderTheme, RenderObject, RenderInline don't draw focus rings if
    conditions match.

We introduced Node::willCallDefaultEventHandler to re-show focus rings
after keyboard operations.

Test changes:
 - Add many tests for new behavior.
 - Remove fast/events/click-focus-control.html because now it is covered
   by existing and new tests.
 - Need rebaseline for afast/repaint/slider-thumb-float.html. We show
    a focus ring for a control without -webkit-appearance.

BUG=89708
R=arv@chromium.org, keishi@chromium.org

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

[eseidel, 2013-05-30 07:18:17]

Your description makes no mention of underlying platform differences.  Are
FF/Opera behaviors consistent across all OSes they run on in this regard?  My
understanding is that WebKit's behavior origininates out of the desire to match
Mac OS X behavior.  (I'm not suggesting we should keep that behavior, just
offering the history.)

[tkent, 2013-05-31 01:07:35]

On 2013/05/30 07:18:17, eseidel wrote:
> Your description makes no mention of underlying platform differences.  Are
> FF/Opera behaviors consistent across all OSes they run on in this regard?  My
> understanding is that WebKit's behavior origininates out of the desire to
match
> Mac OS X behavior.  (I'm not suggesting we should keep that behavior, just
> offering the history.)

ok, I'll write more details in the description.

[tkent, 2013-05-31 04:06:48]

Everyone, this is an alternative approach of
https://codereview.chromium.org/13888009/ .
Please review this.

[ojan, 2013-05-31 22:48:15]

This looks good overall. Just a couple questions.

https://codereview.chromium.org/16194013/diff/10001/Source/core/html/HTMLForm...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/16194013/diff/10001/Source/core/html/HTMLForm...
Source/core/html/HTMLFormControlElement.cpp:341: void
HTMLFormControlElement::willHandleKeyDownEvent()
Could this instead be HTMLFormControlElement::defaultEventHandler? Then you
wouldn't need to sprinkle willHandleKeyDownEvent calls in all the other classes.

https://codereview.chromium.org/16194013/diff/10001/Source/core/html/HTMLForm...
Source/core/html/HTMLFormControlElement.cpp:345: m_wasFocusedByMouse = false;
Do you also need to handle blur and clear this bool?

https://codereview.chromium.org/16194013/diff/10001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTheme.cpp (right):

https://codereview.chromium.org/16194013/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderTheme.cpp:695: bool
RenderTheme::shouldDrawCommonFocusRing(RenderObject* renderer) const
I'm not really sure what "Common" means here.

[tkent, 2013-06-02 23:52:14]

https://codereview.chromium.org/16194013/diff/10001/Source/core/html/HTMLForm...
File Source/core/html/HTMLFormControlElement.cpp (right):

https://codereview.chromium.org/16194013/diff/10001/Source/core/html/HTMLForm...
Source/core/html/HTMLFormControlElement.cpp:341: void
HTMLFormControlElement::willHandleKeyDownEvent()
On 2013/05/31 22:48:16, ojan wrote:
> Could this instead be HTMLFormControlElement::defaultEventHandler?

It's hard because there are many "if (event->defaultHandled()) return" in
sub-classes. We need to fix every such instances.  I understand calling
willHandleKeyDownEvent manually is not good. New patch introduces
Node::willCallDefaultEventHandler.

https://codereview.chromium.org/16194013/diff/10001/Source/core/html/HTMLForm...
Source/core/html/HTMLFormControlElement.cpp:345: m_wasFocusedByMouse = false;
On 2013/05/31 22:48:16, ojan wrote:
> Do you also need to handle blur and clear this bool?

It's unnecessary. The flag should be referred only if focused().  I added some
ASSERTs in the new patch.

https://codereview.chromium.org/16194013/diff/10001/Source/core/rendering/Ren...
File Source/core/rendering/RenderTheme.cpp (right):

https://codereview.chromium.org/16194013/diff/10001/Source/core/rendering/Ren...
Source/core/rendering/RenderTheme.cpp:695: bool
RenderTheme::shouldDrawCommonFocusRing(RenderObject* renderer) const
On 2013/05/31 22:48:16, ojan wrote:
> I'm not really sure what "Common" means here.

We have three types of focus rings.
 - Dedicated to an appearance  (RenderTheme::supportsFocusRing==true)
 - outline:auto
 - non-auto outline (it might not be a focus ring)

I called the second "common."  I renamed it to "default" though I'm not sure
what's the best word.

[tkent, 2013-06-07 01:36:03]

ping reviewers.

[arv (Not doing code reviews), 2013-06-10 14:12:19]

What about HTMLAnchorElement? They should behave the same way.

[arv (Not doing code reviews), 2013-06-10 14:25:17]

LGTM

Please wait for one more LGTM.

https://chromiumcodereview.appspot.com/16194013/diff/31001/LayoutTests/fast/f...
File LayoutTests/fast/forms/button/button-reset-focus-by-mouse-expected.html
(right):

https://chromiumcodereview.appspot.com/16194013/diff/31001/LayoutTests/fast/f...
LayoutTests/fast/forms/button/button-reset-focus-by-mouse-expected.html:9: <div
id="console"></div>
You don't need to manually have a console element.

https://chromiumcodereview.appspot.com/16194013/diff/31001/LayoutTests/fast/f...
File LayoutTests/fast/forms/button/button-reset-focus-by-mouse-then-keydown.html
(right):

https://chromiumcodereview.appspot.com/16194013/diff/31001/LayoutTests/fast/f...
LayoutTests/fast/forms/button/button-reset-focus-by-mouse-then-keydown.html:7:
clickElement(document.getElementsByTagName('button')[0]);
querySelector('button') is shorter

[tkent, 2013-06-13 00:53:06]

https://chromiumcodereview.appspot.com/16194013/diff/31001/LayoutTests/fast/f...
File LayoutTests/fast/forms/button/button-reset-focus-by-mouse-expected.html
(right):

https://chromiumcodereview.appspot.com/16194013/diff/31001/LayoutTests/fast/f...
LayoutTests/fast/forms/button/button-reset-focus-by-mouse-expected.html:9: <div
id="console"></div>
On 2013/06/10 14:25:18, arv wrote:
> You don't need to manually have a console element.

We need to put the console element here. If it is automatically created, it is
inserted before the button element. It moves the button position and we have
platform-dependent results.

https://chromiumcodereview.appspot.com/16194013/diff/31001/LayoutTests/fast/f...
File LayoutTests/fast/forms/button/button-reset-focus-by-mouse-then-keydown.html
(right):

https://chromiumcodereview.appspot.com/16194013/diff/31001/LayoutTests/fast/f...
LayoutTests/fast/forms/button/button-reset-focus-by-mouse-then-keydown.html:7:
clickElement(document.getElementsByTagName('button')[0]);
On 2013/06/10 14:25:18, arv wrote:
> querySelector('button') is shorter

Done.

[keishi, 2013-06-16 13:41:15]

LGTM.

I tried the patch, and I think people who use the space bar to scroll will be
startled. But I guess its worth it for web app compatibility.

[tkent, 2013-06-19 01:37:55]

On 2013/06/10 14:12:19, arv wrote:
> What about HTMLAnchorElement? They should behave the same way.

<a> is out of scope.  I haven't seen a bug about mouse-focus on <a>, and this
way can't be applied to <a>.

[commit-bot: I haz the power, 2013-06-26 00:34:54]

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

[commit-bot: I haz the power, 2013-06-26 01:32:04]

Retried try job too often on blink_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_pres...

[tkent, 2013-06-26 02:02:22]

Committed patchset #5 manually as r153039 (presubmit successful).

[Manish, 2013-07-17 15:27:15]

On 2013/06/26 02:02:22, tkent wrote:
> Committed patchset #5 manually as r153039 (presubmit successful).

Hi,

This issue is marked as closed but I don't see it fixed in Chrome version
28.0.1500.72m. Is this coming in a future release?

thanks,
Manish.