Expose multiline attribute from blink

Expose multiline attribute from blink

This CL adds a method to expose multiline attr to chromium.

This CL is starting patch to Get rid of separate TextField and TextArea AX roles
and support a single role TextField with multiline attribute. More CLs will follow this.

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

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

[shreeramk, 2015-03-30 06:07:09]

shreeram.k@samsung.com changed reviewers:
+ aboxhall@chromium.org, dmazzoni@chromium.org

[shreeramk, 2015-03-30 06:07:10]

PTAL. Thanks!!

[shreeramk, 2015-03-30 06:14:04]

shreeram.k@samsung.com changed reviewers:
+ mkwst@chromium.org

[shreeramk, 2015-03-30 06:14:04]

mkwst@chromium.org: Please review changes in web/

[dmazzoni, 2015-03-30 06:17:54]

https://codereview.chromium.org/1045693002/diff/20001/Source/modules/accessib...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1045693002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:521: bool AXObject::isMultiline()
const
Let's do two more things while you're at it so that this is totally accurate

* If it's contenteditable (node->hasEditableStyle()), it should return true by
default 
* If it's not an editable text node at all (if isNativeTextControl() and
isNonNativeTextControl() both return false), it should always return false -
i.e. we shouldn't allow aria-multiline on a button or a div with no role

[shreeramk, 2015-03-30 06:36:28]

https://codereview.chromium.org/1045693002/diff/20001/Source/modules/accessib...
File Source/modules/accessibility/AXObject.cpp (right):

https://codereview.chromium.org/1045693002/diff/20001/Source/modules/accessib...
Source/modules/accessibility/AXObject.cpp:521: bool AXObject::isMultiline()
const
On 2015/03/30 06:17:54, dmazzoni wrote:
> Let's do two more things while you're at it so that this is totally accurate
> 
> * If it's contenteditable (node->hasEditableStyle()), it should return true by
> default 
> * If it's not an editable text node at all (if isNativeTextControl() and
> isNonNativeTextControl() both return false), it should always return false -
> i.e. we shouldn't allow aria-multiline on a button or a div with no role

You mean to say this??
if (isNativeTextControl() && isNonNativeTextControl())
        return false;

Because the implementation of both are like if one is true other would be false
and vice-versa.

[dmazzoni, 2015-03-30 07:04:43]

On 2015/03/30 06:36:28, shreeramk wrote:
>
https://codereview.chromium.org/1045693002/diff/20001/Source/modules/accessib...
> File Source/modules/accessibility/AXObject.cpp (right):
> 
>
https://codereview.chromium.org/1045693002/diff/20001/Source/modules/accessib...
> Source/modules/accessibility/AXObject.cpp:521: bool AXObject::isMultiline()
> const
> On 2015/03/30 06:17:54, dmazzoni wrote:
> > Let's do two more things while you're at it so that this is totally accurate
> > 
> > * If it's contenteditable (node->hasEditableStyle()), it should return true
by
> > default 
> > * If it's not an editable text node at all (if isNativeTextControl() and
> > isNonNativeTextControl() both return false), it should always return false -
> > i.e. we shouldn't allow aria-multiline on a button or a div with no role
> 
> You mean to say this??
> if (isNativeTextControl() && isNonNativeTextControl())
>         return false;
> 
> Because the implementation of both are like if one is true other would be
false
> and vice-versa.

No, both should be false if it's not any kind of text control - like a button, a
paragraph, etc.

[Mike West, 2015-03-30 07:11:26]

mkwst@chromium.org changed required reviewers:
+ dmazzoni@chromium.org

[Mike West, 2015-03-30 07:11:26]

web/ LGTM, but please do not land the patch until Dominic is happy with the
details.

[shreeramk, 2015-03-30 07:12:56]

@dmazzoni:
Updated patch. PTAL.

[shreeramk, 2015-03-30 11:21:12]

On 2015/03/30 07:12:56, shreeramk wrote:
> @dmazzoni:
> Updated patch. PTAL.

Following tests are failing

AriaMultiline
AriaTextbox
InputPassword
InputText

TextArea test is passing.

I logged and find out that from blink side, TextField role is returned but some
problem seems to be in chromium side I think.

Here is the chromium side CL for this patch
https://codereview.chromium.org/1046883002/

[dmazzoni, 2015-03-30 13:35:02]

Remember that Blink and Chromium update independently - any change that lands in
Blink will "roll" into Chromium about 24 hours later, and when it does, it needs
to not cause any tests to fail.

I'm okay with starting out with a Chromium patch to disable all tests that have
to do with editable text that might fail.

If so, this first Blink patch should probably remove every line of code that
references TextArea so we'll be totally done with it.

[dmazzoni, 2015-03-30 13:38:28]

lgtm

Actually I took another look and I think this change looks good now. i thought
you were missing a couple of cases that still returned TextAreaRole but I think
they're all covered. Create a change to disable all of those failing Chromium
tests, land it, then land this one.

[shreeramk, 2015-03-30 13:57:49]

On 2015/03/30 13:38:28, dmazzoni wrote:
> lgtm
> 
> Actually I took another look and I think this change looks good now. i thought
> you were missing a couple of cases that still returned TextAreaRole but I
think
> they're all covered. Create a change to disable all of those failing Chromium
> tests, land it, then land this one.

I uploaded patch set 4 to cover all cases of TextAreaRole.

Thanks for review. Will upload the patch to disable test cases tmrw as I m at
home.

[shreeramk, 2015-03-31 08:28:56]

The CQ bit was checked by shreeram.k@samsung.com

[shreeramk, 2015-03-31 08:28:56]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1045693002/#ps60001
(title: " ")

[commit-bot: I haz the power, 2015-03-31 08:29:26]

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

[commit-bot: I haz the power, 2015-03-31 08:49:55]

Committed patchset #4 (id:60001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192814

[jianli, 2015-03-31 19:07:54]

A revert of this CL (patchset #4 id:60001) has been created in
https://codereview.chromium.org/1047253003/ by jianli@chromium.org.

The reason for reverting is: A likely culprit for AutomationApiTest.Mixins
failures in many chromium.webkit bots.
https://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Te....

[shreeramk, 2015-04-01 03:37:47]

On 2015/03/31 19:07:54, jianli wrote:
> A revert of this CL (patchset #4 id:60001) has been created in
> https://codereview.chromium.org/1047253003/ by mailto:jianli@chromium.org.
> 
> The reason for reverting is: A likely culprit for AutomationApiTest.Mixins
> failures in many chromium.webkit bots.
>
https://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Te....

@Dominic: Is this really causing the AutomationApiText.mixins failure??

[dmazzoni, 2015-04-01 04:13:07]

On 2015/04/01 03:37:47, shreeramk wrote:
> On 2015/03/31 19:07:54, jianli wrote:
> > A revert of this CL (patchset #4 id:60001) has been created in
> > https://codereview.chromium.org/1047253003/ by mailto:jianli@chromium.org.
> > 
> > The reason for reverting is: A likely culprit for AutomationApiTest.Mixins
> > failures in many chromium.webkit bots.
> >
>
https://build.chromium.org/p/chromium.webkit/builders/Linux%20ChromiumOS%20Te....
> 
> @Dominic: Is this really causing the AutomationApiText.mixins failure??

Yes. The automation API is a new API that lets you access accessibility from
JavaScript. One of the tests is looking for a textarea role:

var textArea = rootNode.find({ role: 'textArea' });

You'll need to disable this test too, and then update it.

[shreeramk, 2015-04-01 10:14:53]

This CL can be committed after this chromium side CL lands
https://codereview.chromium.org/1051713003/

[shreeramk, 2015-04-02 03:02:41]

The CQ bit was checked by shreeram.k@samsung.com

[shreeramk, 2015-04-02 03:02:42]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1045693002/#ps80001
(title: "Reland")

[commit-bot: I haz the power, 2015-04-02 03:02:54]

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

[shreeramk, 2015-04-02 03:03:16]

On 2015/04/01 10:14:53, shreeramk wrote:
> This CL can be committed after this chromium side CL lands
> https://codereview.chromium.org/1051713003/

@Dominic: Review(LGTM) is required again for this CL?

[commit-bot: I haz the power, 2015-04-02 03:09:20]

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

[commit-bot: I haz the power, 2015-04-02 03:09:22]

Try jobs failed on following builders:
  mac_blink_compile_dbg on tryserver.blink (JOB_FAILED,
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_compile_dbg/bu...)

[shreeramk, 2015-04-02 03:22:32]

The CQ bit was checked by shreeram.k@samsung.com

[shreeramk, 2015-04-02 03:22:33]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1045693002/#ps100001
(title: "rebasing")

[commit-bot: I haz the power, 2015-04-02 03:22:53]

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

[commit-bot: I haz the power, 2015-04-02 05:48:13]

Committed patchset #6 (id:100001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=192998

[Justin Novosad, 2015-04-02 15:20:28]

A revert of this CL (patchset #6 id:100001) has been created in
https://codereview.chromium.org/1057923002/ by junov@chromium.org.

The reason for reverting is: Reason for revert: causing test failure on Android.
DumpAccessibilityTreeTest.AccessibilityInputTextValue
The diff shows "multiline" missing from test output.
http://build.chromium.org/p/chromium.webkit/builders/Android%20Tests%20%28dbg....

[shreeramk, 2015-04-06 07:14:05]

The CQ bit was checked by shreeram.k@samsung.com

[shreeramk, 2015-04-06 07:14:06]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org, mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1045693002/#ps120001
(title: "Reland")

[commit-bot: I haz the power, 2015-04-06 07:14:17]

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

[commit-bot: I haz the power, 2015-04-06 09:40:14]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193175