WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
89425
[Forms] Move isKeyboardFocusable and isMouseFocusable to InputType from HTMLInputElement
https://bugs.webkit.org/show_bug.cgi?id=89425
Summary
[Forms] Move isKeyboardFocusable and isMouseFocusable to InputType from HTMLI...
yosin
Reported
2012-06-18 23:02:29 PDT
For introducing time fields UI and code cleanup, input type class should decide of keyboard/mouse focus-able rather than HTMLInputElement. == Current == bool HTMLInputElement::isKeyboardFocusable(KeyboardEvent* event) const { if (isTextField()) return HTMLTextFormControlElement::isFocusable(); return HTMLTextFormControlElement::isKeyboardFocusable(event) && m_inputType->isKeyboardFocusable(); } == Proposed == bool HTMLInputElement::isKeyboardFocusable(KeyboardEvent* event) const { return m_inputType->isKeyboardFocusable(event); } bool InputType::isKeyboardFocusable(KeyboardEvent* event) const { return element()->isTextFormControlKeyboardFocusable(event); // call new method in HTMLInputElement class } bool TextFieldInputType::isKeyboardFocusable(KeyboardEvent* event) const { return element()->isTextFormControlFocusable(); // call new method in HTMLInputElement class }
Attachments
Patch 1
(8.64 KB, patch)
2012-06-18 23:36 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 2
(8.70 KB, patch)
2012-06-19 00:12 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Patch 3
(8.69 KB, patch)
2012-06-19 00:27 PDT
,
yosin
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
yosin
Comment 1
2012-06-18 23:36:07 PDT
Created
attachment 148261
[details]
Patch 1
yosin
Comment 2
2012-06-18 23:36:32 PDT
Comment on
attachment 148261
[details]
Patch 1 Could you review this patch? Thanks in advance.
Kent Tamura
Comment 3
2012-06-18 23:47:08 PDT
Comment on
attachment 148261
[details]
Patch 1 View in context:
https://bugs.webkit.org/attachment.cgi?id=148261&action=review
> Source/WebCore/html/HTMLInputElement.h:267 > + virtual bool isTextFormControlFocusable() const; > + virtual bool isTextFormControlKeyboardFocusable(KeyboardEvent*) const; > + virtual bool isTextFormControlMouseFocusable() const;
They shouldn't be virtual. Also, please put them just below setValueInternal() because it is also a helper for *InputType.
> Source/WebCore/html/RadioInputType.cpp:119 > + if (!element()->isTextFormControlKeyboardFocusable(event))
if (!InputType::isKeyboardFocusable(event)) is better.
yosin
Comment 4
2012-06-19 00:12:52 PDT
Created
attachment 148270
[details]
Patch 2
yosin
Comment 5
2012-06-19 00:15:02 PDT
Comment on
attachment 148270
[details]
Patch 2 Could you review this patch? Thanks in advance. = Changes since last review == * Remove "virtual" from isTextFormControl*Focusable method declarations ** as suggested * Call InputType::isKeyboardFocusable() in RadioInputType::isKeyboardFocusable ** as suggested
Build Bot
Comment 6
2012-06-19 00:24:29 PDT
Comment on
attachment 148270
[details]
Patch 2
Attachment 148270
[details]
did not pass mac-ews (mac): Output:
http://queues.webkit.org/results/12972717
yosin
Comment 7
2012-06-19 00:27:56 PDT
Created
attachment 148274
[details]
Patch 3
yosin
Comment 8
2012-06-19 00:30:02 PDT
Comment on
attachment 148274
[details]
Patch 3 Could you review this patch? Thanks in advance. = Changes since last review == * Remove "virtual" from isTextFormControl*Focusable method declarations ** as suggested * Call InputType::isKeyboardFocusable() in RadioInputType::isKeyboardFocusable ** as suggested * Remove unused prameter in TextFieldInputType::isKeyboardFocusable ** Failed on Mac build
Kent Tamura
Comment 9
2012-06-19 01:09:16 PDT
Comment on
attachment 148274
[details]
Patch 3 ok
WebKit Review Bot
Comment 10
2012-06-19 01:57:32 PDT
Comment on
attachment 148274
[details]
Patch 3 Clearing flags on attachment: 148274 Committed
r120695
: <
http://trac.webkit.org/changeset/120695
>
WebKit Review Bot
Comment 11
2012-06-19 01:57:37 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug