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
Patch 2 (8.70 KB, patch)
2012-06-19 00:12 PDT, yosin
no flags
Patch 3 (8.69 KB, patch)
2012-06-19 00:27 PDT, yosin
no flags
yosin
Comment 1 2012-06-18 23:36:07 PDT
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
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
yosin
Comment 7 2012-06-19 00:27:56 PDT
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.