Bug 89425 - [Forms] Move isKeyboardFocusable and isMouseFocusable to InputType from HTMLInputElement
Summary: [Forms] Move isKeyboardFocusable and isMouseFocusable to InputType from HTMLI...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: yosin
URL:
Keywords:
Depends on:
Blocks: 88970
  Show dependency treegraph
 
Reported: 2012-06-18 23:02 PDT by yosin
Modified: 2012-06-19 01:57 PDT (History)
2 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
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.