Bug 89425

Summary: [Forms] Move isKeyboardFocusable and isMouseFocusable to InputType from HTMLInputElement
Product: WebKit Reporter: yosin
Component: FormsAssignee: yosin
Status: RESOLVED FIXED    
Severity: Normal CC: tkent, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 88970    
Attachments:
Description Flags
Patch 1
none
Patch 2
none
Patch 3 none

Description yosin 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
}
Comment 1 yosin 2012-06-18 23:36:07 PDT
Created attachment 148261 [details]
Patch 1
Comment 2 yosin 2012-06-18 23:36:32 PDT
Comment on attachment 148261 [details]
Patch 1

Could you review this patch?
Thanks in advance.
Comment 3 Kent Tamura 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.
Comment 4 yosin 2012-06-19 00:12:52 PDT
Created attachment 148270 [details]
Patch 2
Comment 5 yosin 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
Comment 6 Build Bot 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
Comment 7 yosin 2012-06-19 00:27:56 PDT
Created attachment 148274 [details]
Patch 3
Comment 8 yosin 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
Comment 9 Kent Tamura 2012-06-19 01:09:16 PDT
Comment on attachment 148274 [details]
Patch 3

ok
Comment 10 WebKit Review Bot 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>
Comment 11 WebKit Review Bot 2012-06-19 01:57:37 PDT
All reviewed patches have been landed.  Closing bug.