Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(5)

Issue 16194013: Mouse press should focus on any types of form controls. (Closed)

Created:
4 years, 5 months ago by tkent
Modified:
4 years, 4 months ago
CC:
blink-reviews, dglazkov+blink, eae+blinkwatch, adamk+blink_chromium.org, jchaffraix+rendering, leviw+renderwatch
Visibility:
Public.

Description

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

Patch Set 1 : #

Total comments: 6

Patch Set 2 : rebase, common->default, etc. #

Total comments: 4

Patch Set 3 : Use querySelector #

Patch Set 4 : test cleanup #

Patch Set 5 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+410 lines, -145 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 2 chunks +14 lines, -0 lines 0 comments Download
D LayoutTests/fast/events/click-focus-control.html View 1 chunk +0 lines, -90 lines 0 comments Download
D LayoutTests/fast/events/click-focus-control-expected.txt View 1 chunk +0 lines, -9 lines 0 comments Download
A LayoutTests/fast/forms/button/button-reset-focus-by-mouse.html View 1 2 3 1 chunk +22 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/button/button-reset-focus-by-mouse-expected.html View 1 2 3 1 chunk +18 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/button/button-reset-focus-by-mouse-then-keydown.html View 1 2 3 1 chunk +11 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/button/button-reset-focus-by-mouse-then-keydown-expected.html View 1 2 3 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/checkbox/checkbox-focus-by-mouse.html View 1 2 3 1 chunk +22 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/checkbox/checkbox-focus-by-mouse-expected.html View 1 2 3 1 chunk +19 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/checkbox/checkbox-focus-by-mouse-then-keydown.html View 1 2 3 1 chunk +11 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/checkbox/checkbox-focus-by-mouse-then-keydown-expected.html View 1 2 3 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/radio/radio-focus-by-mouse.html View 1 2 3 1 chunk +21 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/radio/radio-focus-by-mouse-expected.html View 1 2 3 1 chunk +19 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/radio/radio-focus-by-mouse-then-keydown.html View 1 2 3 1 chunk +11 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/radio/radio-focus-by-mouse-then-keydown-expected.html View 1 2 3 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/range/range-focus-by-mouse.html View 1 2 3 1 chunk +22 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/range/range-focus-by-mouse-expected.html View 1 2 3 1 chunk +19 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/range/range-focus-by-mouse-then-keydown.html View 1 2 3 1 chunk +11 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/range/range-focus-by-mouse-then-keydown-expected.html View 1 2 3 1 chunk +9 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/submit/submit-focus-by-mouse.html View 1 2 3 1 chunk +22 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/submit/submit-focus-by-mouse-expected.html View 1 2 3 1 chunk +19 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/submit/submit-focus-by-mouse-then-keydown.html View 1 2 3 1 chunk +11 lines, -0 lines 0 comments Download
A LayoutTests/fast/forms/submit/submit-focus-by-mouse-then-keydown-expected.html View 1 2 3 1 chunk +9 lines, -0 lines 0 comments Download
M Source/core/dom/EventDispatcher.cpp View 1 2 3 4 2 chunks +2 lines, -0 lines 0 comments Download
M Source/core/dom/Node.h View 1 2 3 4 2 chunks +3 lines, -0 lines 0 comments Download
M Source/core/dom/Node.cpp View 1 2 3 4 2 chunks +10 lines, -0 lines 0 comments Download
M Source/core/html/BaseChooserOnlyDateAndTimeInputType.h View 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/html/BaseChooserOnlyDateAndTimeInputType.cpp View 1 chunk +0 lines, -5 lines 0 comments Download
M Source/core/html/BaseDateAndTimeInputType.h View 1 2 3 4 1 chunk +1 line, -0 lines 0 comments Download
M Source/core/html/BaseDateAndTimeInputType.cpp View 1 2 3 4 1 chunk +5 lines, -0 lines 0 comments Download
M Source/core/html/BaseMultipleFieldsDateAndTimeInputType.h View 1 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/html/BaseMultipleFieldsDateAndTimeInputType.cpp View 1 2 3 4 2 chunks +1 line, -6 lines 0 comments Download
M Source/core/html/HTMLFormControlElement.h View 1 2 3 2 chunks +5 lines, -2 lines 0 comments Download
M Source/core/html/HTMLFormControlElement.cpp View 1 2 3 2 chunks +26 lines, -1 line 0 comments Download
M Source/core/html/HTMLInputElement.h View 1 2 3 4 2 chunks +1 line, -2 lines 0 comments Download
M Source/core/html/HTMLInputElement.cpp View 1 2 3 4 2 chunks +2 lines, -7 lines 0 comments Download
M Source/core/html/HTMLSelectElement.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/HTMLSelectElement.cpp View 1 2 3 4 1 chunk +2 lines, -4 lines 0 comments Download
M Source/core/html/HTMLTextAreaElement.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/HTMLTextAreaElement.cpp View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/html/InputType.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/InputType.cpp View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/html/TextFieldInputType.h View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/TextFieldInputType.cpp View 1 2 3 4 1 chunk +2 lines, -2 lines 0 comments Download
M Source/core/page/EventHandler.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/core/page/FocusDirection.h View 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/rendering/RenderInline.cpp View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M Source/core/rendering/RenderObject.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M Source/core/rendering/RenderTheme.h View 1 2 2 chunks +3 lines, -2 lines 0 comments Download
M Source/core/rendering/RenderTheme.cpp View 1 2 2 chunks +17 lines, -1 line 0 comments Download

Messages

Total messages: 16 (0 generated)
eseidel
Your description makes no mention of underlying platform differences. Are FF/Opera behaviors consistent across all ...
4 years, 5 months ago (2013-05-30 07:18:17 UTC) #1
tkent
On 2013/05/30 07:18:17, eseidel wrote: > Your description makes no mention of underlying platform differences. ...
4 years, 5 months ago (2013-05-31 01:07:35 UTC) #2
tkent
4 years, 5 months ago (2013-05-31 04:05:20 UTC) #3
tkent
Everyone, this is an alternative approach of https://codereview.chromium.org/13888009/ . Please review this.
4 years, 5 months ago (2013-05-31 04:06:48 UTC) #4
ojan
This looks good overall. Just a couple questions. https://codereview.chromium.org/16194013/diff/10001/Source/core/html/HTMLFormControlElement.cpp File Source/core/html/HTMLFormControlElement.cpp (right): https://codereview.chromium.org/16194013/diff/10001/Source/core/html/HTMLFormControlElement.cpp#newcode341 Source/core/html/HTMLFormControlElement.cpp:341: void ...
4 years, 5 months ago (2013-05-31 22:48:15 UTC) #5
tkent
https://codereview.chromium.org/16194013/diff/10001/Source/core/html/HTMLFormControlElement.cpp File Source/core/html/HTMLFormControlElement.cpp (right): https://codereview.chromium.org/16194013/diff/10001/Source/core/html/HTMLFormControlElement.cpp#newcode341 Source/core/html/HTMLFormControlElement.cpp:341: void HTMLFormControlElement::willHandleKeyDownEvent() On 2013/05/31 22:48:16, ojan wrote: > Could ...
4 years, 5 months ago (2013-06-02 23:52:14 UTC) #6
tkent
ping reviewers.
4 years, 5 months ago (2013-06-07 01:36:03 UTC) #7
arv (Not doing code reviews)
What about HTMLAnchorElement? They should behave the same way.
4 years, 5 months ago (2013-06-10 14:12:19 UTC) #8
arv (Not doing code reviews)
LGTM Please wait for one more LGTM. https://chromiumcodereview.appspot.com/16194013/diff/31001/LayoutTests/fast/forms/button/button-reset-focus-by-mouse-expected.html File LayoutTests/fast/forms/button/button-reset-focus-by-mouse-expected.html (right): https://chromiumcodereview.appspot.com/16194013/diff/31001/LayoutTests/fast/forms/button/button-reset-focus-by-mouse-expected.html#newcode9 LayoutTests/fast/forms/button/button-reset-focus-by-mouse-expected.html:9: <div id="console"></div> ...
4 years, 5 months ago (2013-06-10 14:25:17 UTC) #9
tkent
https://chromiumcodereview.appspot.com/16194013/diff/31001/LayoutTests/fast/forms/button/button-reset-focus-by-mouse-expected.html File LayoutTests/fast/forms/button/button-reset-focus-by-mouse-expected.html (right): https://chromiumcodereview.appspot.com/16194013/diff/31001/LayoutTests/fast/forms/button/button-reset-focus-by-mouse-expected.html#newcode9 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 ...
4 years, 5 months ago (2013-06-13 00:53:06 UTC) #10
keishi
LGTM. I tried the patch, and I think people who use the space bar to ...
4 years, 5 months ago (2013-06-16 13:41:15 UTC) #11
tkent
On 2013/06/10 14:12:19, arv wrote: > What about HTMLAnchorElement? They should behave the same way. ...
4 years, 5 months ago (2013-06-19 01:37:55 UTC) #12
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tkent@chromium.org/16194013/87001
4 years, 4 months ago (2013-06-26 00:34:54 UTC) #13
commit-bot: I haz the power
Retried try job too often on blink_presubmit for step(s) presubmit http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=blink_presubmit&number=2313
4 years, 4 months ago (2013-06-26 01:32:04 UTC) #14
tkent
Committed patchset #5 manually as r153039 (presubmit successful).
4 years, 4 months ago (2013-06-26 02:02:22 UTC) #15
Manish
4 years, 4 months ago (2013-07-17 15:27:15 UTC) #16
Message was sent while issue was closed.
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.

Powered by Google App Engine
This is Rietveld efc10ee0f