Introduce new API orientation() to expose aria-orientation correctly.

Introduce new API orientation() to expose aria-orientation correctly.

Changes included in this CL:

1. Introduce new API in WebAXObject to return correct orientation enum for specific roles.

2. In ARIA 1.1, the default value for aria-orientation changed from horizontal to undefined.

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

[shreeramk, 2014-11-20 13:54:32]

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

[shreeramk, 2014-11-20 13:54:32]

PTAL!

Patch error is coming because it need to rebased.

Chromium side change https://codereview.chromium.org/744623002/

Thanks!!

[dmazzoni, 2014-11-20 16:48:06]

https://codereview.chromium.org/730113003/diff/20001/Source/modules/accessibi...
File Source/modules/accessibility/AXRenderObject.cpp (right):

https://codereview.chromium.org/730113003/diff/20001/Source/modules/accessibi...
Source/modules/accessibility/AXRenderObject.cpp:850: if (ariaRoleAttribute() ==
ComboBoxRole
You're only checking the aria role, but some of these have native controls that
should return an orientation. In particular:

splitter
slider
combobox
listbox
scrollbar

I know for sure that all of those roles can come from native HTML elements and
we should return an orientation from all of those. I think it'd make sense to
just check the role - not the aria role.

Use switch statements when there's more than 2 or 3 tests.

Instead of putting nearly the whole body of a function inside an "if" block,
reverse the order of the test - i.e. after the first switch, if the role is NOT
one that needs an orientation, return AXObject::orientation - then the rest of
the function doesn't need to be indented.

[shreeramk, 2014-11-20 17:16:38]

On 2014/11/20 16:48:06, dmazzoni wrote:
>
https://codereview.chromium.org/730113003/diff/20001/Source/modules/accessibi...
> File Source/modules/accessibility/AXRenderObject.cpp (right):
> 
>
https://codereview.chromium.org/730113003/diff/20001/Source/modules/accessibi...
> Source/modules/accessibility/AXRenderObject.cpp:850: if (ariaRoleAttribute()
==
> ComboBoxRole
> You're only checking the aria role, but some of these have native controls
that
> should return an orientation. In particular:
> 
> splitter
> slider
> combobox
> listbox
> scrollbar
> 
> I know for sure that all of those roles can come from native HTML elements and
> we should return an orientation from all of those. I think it'd make sense to
> just check the role - not the aria role.

Actually I was in doubt about whether to include native HTML elements with this
role. Thanks for the feedback and clearing my doubt.

> Use switch statements when there's more than 2 or 3 tests.
> 
> Instead of putting nearly the whole body of a function inside an "if" block,
> reverse the order of the test - i.e. after the first switch, if the role is
NOT
> one that needs an orientation, return AXObject::orientation - then the rest of
> the function doesn't need to be indented.

Ok. Thanks for this important feedback. I was also not feeling good with those
ifs.

I ll update the CL tomorrow.

[shreeramk, 2014-11-21 11:07:48]

The update dumped AX tree for aria-orientation can be seen here
https://codereview.chromium.org/744623002/

https://codereview.chromium.org/730113003/diff/40001/Source/modules/accessibi...
File Source/modules/accessibility/AXRenderObject.cpp (right):

https://codereview.chromium.org/730113003/diff/40001/Source/modules/accessibi...
Source/modules/accessibility/AXRenderObject.cpp:869: case TreeGridRole:
Here, TreeGridRole case is not true.

Reason: roleValue() for role="treegrid" is coming TableRole(111).

My proposed Solution:
ariaRoleAttribute() for TreeGridRole can be checked in the ifs.

e.g
if (equalIgnoringCase(ariaOrientation, "horizontal") || ariaRoleAttribute() ==
TreeGridRole)

Whats your view on this?

[dmazzoni, 2014-11-24 17:37:00]

https://codereview.chromium.org/730113003/diff/40001/Source/modules/accessibi...
File Source/modules/accessibility/AXRenderObject.cpp (right):

https://codereview.chromium.org/730113003/diff/40001/Source/modules/accessibi...
Source/modules/accessibility/AXRenderObject.cpp:869: case TreeGridRole:
On 2014/11/21 11:07:47, shreeramk wrote:
> Here, TreeGridRole case is not true.
> 
> Reason: roleValue() for role="treegrid" is coming TableRole(111).
> 
> My proposed Solution:
> ariaRoleAttribute() for TreeGridRole can be checked in the ifs.
> 
> e.g
> if (equalIgnoringCase(ariaOrientation, "horizontal") || ariaRoleAttribute() ==
> TreeGridRole)
> 
> Whats your view on this?

The logic sounds great.

To make the code more readable, I'd maybe put the TreeGridRole on its own line
with a comment.

if (equalIgnoringCase(ariaOrientation, "horizontal"))
    ...

// Comment explaining why this overrides...
if (ariaRoleAttribute() == TreeGridRole)
    axorientation = AccessibilityOrientationHorizontal;

[shreeramk, 2014-11-25 08:58:37]

PTAL!! Thanks.

https://codereview.chromium.org/730113003/diff/40001/Source/modules/accessibi...
File Source/modules/accessibility/AXRenderObject.cpp (right):

https://codereview.chromium.org/730113003/diff/40001/Source/modules/accessibi...
Source/modules/accessibility/AXRenderObject.cpp:869: case TreeGridRole:
On 2014/11/24 17:36:59, dmazzoni wrote:
> On 2014/11/21 11:07:47, shreeramk wrote:
> > Here, TreeGridRole case is not true.
> > 
> > Reason: roleValue() for role="treegrid" is coming TableRole(111).
> > 
> > My proposed Solution:
> > ariaRoleAttribute() for TreeGridRole can be checked in the ifs.
> > 
> > e.g
> > if (equalIgnoringCase(ariaOrientation, "horizontal") || ariaRoleAttribute()
==
> > TreeGridRole)
> > 
> > Whats your view on this?
> 
> The logic sounds great.
> 
> To make the code more readable, I'd maybe put the TreeGridRole on its own line
> with a comment.
> 
> if (equalIgnoringCase(ariaOrientation, "horizontal"))
>     ...
> 
> // Comment explaining why this overrides...
> if (ariaRoleAttribute() == TreeGridRole)
>     axorientation = AccessibilityOrientationHorizontal;

Added code for TreeGridRole.

[shreeramk, 2014-11-26 18:40:51]

@Dominic: PTAL. Thanks!!

[dmazzoni, 2014-11-26 18:51:46]

The CQ bit was checked by dmazzoni@chromium.org

[dmazzoni, 2014-11-26 18:51:47]

lgtm

https://codereview.chromium.org/730113003/diff/60001/Source/modules/accessibi...
File Source/modules/accessibility/AXScrollbar.cpp (right):

https://codereview.chromium.org/730113003/diff/60001/Source/modules/accessibi...
Source/modules/accessibility/AXScrollbar.cpp:73: return
AccessibilityOrientationHorizontal;
I think this should be AccessibilityOrientationVertical, too - since the default
for a scroll bar is always vertical - but it doesn't really matter since this is
only for the case where the scroll bar has been deleted but the AX object still
exists.

[commit-bot: I haz the power, 2014-11-26 18:52:44]

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

[shreeramk, 2014-11-26 19:05:38]

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

[shreeramk, 2014-11-26 19:05:38]

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

Thanks!!

[commit-bot: I haz the power, 2014-11-26 19:48:21]

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

[commit-bot: I haz the power, 2014-11-26 19:48:22]

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

[Mike West, 2014-11-27 09:05:30]

public/ LGTM.

[Mike West, 2014-11-27 09:05:51]

On 2014/11/27 09:05:30, Mike West (OOO) wrote:
> public/ LGTM.

web/ as well. ;)

[shreeramk, 2014-11-27 09:40:39]

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

[commit-bot: I haz the power, 2014-11-27 09:41:08]

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

[commit-bot: I haz the power, 2014-11-27 09:57:07]

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