Add AX attribute for input type

Add AX attribute for input type

When Input type is url, tel, search and email,
AX attributes for them will be exposed.

BUG=323161
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183119

[je_julie(Not used), 2014-09-26 14:24:02]

je_julie.kim@samsung.com changed reviewers:
+ darin@chromium.org, dmazzoni@chromium.org

[je_julie(Not used), 2014-09-26 14:24:23]

Dominic, PTAL.

[dmazzoni, 2014-09-26 15:17:26]

lgtm with changes below

https://codereview.chromium.org/596393004/diff/1/Source/core/accessibility/AX...
File Source/core/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/596393004/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObject.cpp:634: const AtomicString&
AXObject::textInputType() const
This looks great, but please move it to AXNodeObject.

In general, AXObject is supposed to contain logic that applies to all AX
objects, AXNodeObject should handle things that depend on it being a DOM node,
and AXRenderObject for things that may also depend on its RenderObject or
exactly how it's laid out on the screen.

Put an empty implementation here and override it in AXNodeObject.

https://codereview.chromium.org/596393004/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObject.cpp:640: if (!elementNode->isElementNode()
||!isHTMLInputElement(*elementNode))
nit: space between || and !

[je_julie(Not used), 2014-09-27 09:05:46]

https://codereview.chromium.org/596393004/diff/1/Source/core/accessibility/AX...
File Source/core/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/596393004/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObject.cpp:634: const AtomicString&
AXObject::textInputType() const
Thanks. I'll update it.
On 2014/09/26 15:17:26, dmazzoni wrote:
> This looks great, but please move it to AXNodeObject.
> 
> In general, AXObject is supposed to contain logic that applies to all AX
> objects, AXNodeObject should handle things that depend on it being a DOM node,
> and AXRenderObject for things that may also depend on its RenderObject or
> exactly how it's laid out on the screen.
> 
> Put an empty implementation here and override it in AXNodeObject.

https://codereview.chromium.org/596393004/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXObject.cpp:640: if (!elementNode->isElementNode()
||!isHTMLInputElement(*elementNode))
Thanks, I'll update it.
On 2014/09/26 15:17:26, dmazzoni wrote:
> nit: space between || and !

[je_julie(Not used), 2014-09-27 09:08:14]

Hi Dominic,
I updated patch. PTAL.

[dmazzoni, 2014-09-29 05:45:23]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-09-29 05:46:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/596393004/20001

[commit-bot: I haz the power, 2014-09-29 05:57:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-09-29 05:57:18]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[je_julie(Not used), 2014-09-29 06:28:16]

On 2014/09/29 05:57:18, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   blink_presubmit on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

Hi Dominic,
I saw error below with blink_presubmit.

** Presubmit ERRORS **
Missing LGTM from an OWNER for these files:
    Source/web/WebAXObject.cpp
    public/web/WebAXObject.h

Do you have any idea how to handle it?

[dmazzoni, 2014-09-29 07:02:21]

dmazzoni@chromium.org changed reviewers:
+ dglazkov@chromium.org
- darin@chromium.org

[dmazzoni, 2014-09-29 07:02:21]

+dglazkov for owners review of public/web and src/web

[je_julie(Not used), 2014-10-01 02:23:03]

On 2014/09/29 07:02:21, dmazzoni wrote:
> +dglazkov for owners review of public/web and src/web

Hi dglazkov, PTAL.

[dglazkov, 2014-10-01 04:35:59]

lgtm

[dglazkov, 2014-10-01 04:37:11]

https://codereview.chromium.org/596393004/diff/20001/Source/core/accessibilit...
File Source/core/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/596393004/diff/20001/Source/core/accessibilit...
Source/core/accessibility/AXNodeObject.cpp:1052: if (input.type() ==
InputTypeNames::tel || input.type() == InputTypeNames::url
Should this just be input.isTextField()?

[je_julie(Not used), 2014-10-01 04:53:52]

https://codereview.chromium.org/596393004/diff/20001/Source/core/accessibilit...
File Source/core/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/596393004/diff/20001/Source/core/accessibilit...
Source/core/accessibility/AXNodeObject.cpp:1052: if (input.type() ==
InputTypeNames::tel || input.type() == InputTypeNames::url
On 2014/10/01 04:37:11, dglazkov wrote:
> Should this just be input.isTextField()?

I'm referencing to implementation from FireFox for this.
https://bugzilla.mozilla.org/show_bug.cgi?id=924896
They handle text input type with just tel, url, email and search because it's
worthy of exporting to attribute.
If we use isTextField(), it includes email, password, search, tel, text, and URL
types.
Do we need to include all of them?

[dmazzoni, 2014-10-01 05:22:57]

https://codereview.chromium.org/596393004/diff/20001/Source/core/accessibilit...
File Source/core/accessibility/AXNodeObject.cpp (right):

https://codereview.chromium.org/596393004/diff/20001/Source/core/accessibilit...
Source/core/accessibility/AXNodeObject.cpp:1052: if (input.type() ==
InputTypeNames::tel || input.type() == InputTypeNames::url
On 2014/10/01 04:53:52, je_julie.kim wrote:
> On 2014/10/01 04:37:11, dglazkov wrote:
> > Should this just be input.isTextField()?
> 
> I'm referencing to implementation from FireFox for this.
> https://bugzilla.mozilla.org/show_bug.cgi?id=924896
> They handle text input type with just tel, url, email and search because it's
> worthy of exporting to attribute.
> If we use isTextField(), it includes email, password, search, tel, text, and
URL
> types.
> Do we need to include all of them? 

I think isTextField is probably more correct. Search and Password seem useful to
return too, and plus that's probably more future-proof if more are added later.

If one of them is not allowed to be returned on a particular platform, let's
handle it there.

[je_julie(Not used), 2014-10-01 05:28:52]

On 2014/10/01 05:22:57, dmazzoni wrote:
>
https://codereview.chromium.org/596393004/diff/20001/Source/core/accessibilit...
> File Source/core/accessibility/AXNodeObject.cpp (right):
> 
>
https://codereview.chromium.org/596393004/diff/20001/Source/core/accessibilit...
> Source/core/accessibility/AXNodeObject.cpp:1052: if (input.type() ==
> InputTypeNames::tel || input.type() == InputTypeNames::url
> On 2014/10/01 04:53:52, je_julie.kim wrote:
> > On 2014/10/01 04:37:11, dglazkov wrote:
> > > Should this just be input.isTextField()?
> > 
> > I'm referencing to implementation from FireFox for this.
> > https://bugzilla.mozilla.org/show_bug.cgi?id=924896
> > They handle text input type with just tel, url, email and search because
it's
> > worthy of exporting to attribute.
> > If we use isTextField(), it includes email, password, search, tel, text, and
> URL
> > types.
> > Do we need to include all of them? 
> 
> I think isTextField is probably more correct. Search and Password seem useful
to
> return too, and plus that's probably more future-proof if more are added
later.
> 
> If one of them is not allowed to be returned on a particular platform, let's
> handle it there.

OK, I'll update patch here and also update test result at
https://codereview.chromium.org/559343002/.
Text and Password type will also generate text-input-type attribute.

[je_julie(Not used), 2014-10-01 06:42:32]

On 2014/10/01 05:28:52, je_julie.kim wrote:
> On 2014/10/01 05:22:57, dmazzoni wrote:
> >
>
https://codereview.chromium.org/596393004/diff/20001/Source/core/accessibilit...
> > File Source/core/accessibility/AXNodeObject.cpp (right):
> > 
> >
>
https://codereview.chromium.org/596393004/diff/20001/Source/core/accessibilit...
> > Source/core/accessibility/AXNodeObject.cpp:1052: if (input.type() ==
> > InputTypeNames::tel || input.type() == InputTypeNames::url
> > On 2014/10/01 04:53:52, je_julie.kim wrote:
> > > On 2014/10/01 04:37:11, dglazkov wrote:
> > > > Should this just be input.isTextField()?
> > > 
> > > I'm referencing to implementation from FireFox for this.
> > > https://bugzilla.mozilla.org/show_bug.cgi?id=924896
> > > They handle text input type with just tel, url, email and search because
> it's
> > > worthy of exporting to attribute.
> > > If we use isTextField(), it includes email, password, search, tel, text,
and
> > URL
> > > types.
> > > Do we need to include all of them? 
> > 
> > I think isTextField is probably more correct. Search and Password seem
useful
> to
> > return too, and plus that's probably more future-proof if more are added
> later.
> > 
> > If one of them is not allowed to be returned on a particular platform, let's
> > handle it there.
> 
> OK, I'll update patch here and also update test result at
> https://codereview.chromium.org/559343002/.
> Text and Password type will also generate text-input-type attribute.

Dominic, dglazkov, PTAL.

[dmazzoni, 2014-10-01 19:47:14]

still lgtm

[je_julie(Not used), 2014-10-02 00:22:55]

dglazkov, I updated patch with isTextField() as you commented.
Could you take a look one more?

[dmazzoni, 2014-10-02 06:21:59]

On 2014/10/02 00:22:55, je_julie.kim wrote:
> dglazkov, I updated patch with isTextField() as you commented.
> Could you take a look one more?

He already said lgtm with his review so you don't need to wait for a second
lgtm.

[dmazzoni, 2014-10-02 06:22:02]

The CQ bit was checked by dmazzoni@chromium.org

[commit-bot: I haz the power, 2014-10-02 06:23:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/596393004/40001

[commit-bot: I haz the power, 2014-10-02 07:45:57]

Committed patchset #3 (id:40001) as 183119