Removes direct checking node on AXLayoutObject::determineAccessibilityRole.

Removes direct checking node on AXLayoutObject::determineAccessibilityRole.

This patch removes direct checking node on AXLayoutObject::determine
AccessibilityRole and moves it to AXNodeObject::determineAccessibility
RoleUtil and makes the code general.

Additionally, The conditions for LinkRole are merged and unnecessary
conditions are removed such as isElementNode() because hasTagName()
checks isHTMLElement() inside it.

BUG=N/A
R=dmazzoni

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

[je_julie(Not used), 2015-03-18 16:05:33]

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

[je_julie(Not used), 2015-03-18 16:07:31]

Hi Dominic,

As to modify AX code on blink, I always think where I should add code.
I remember your comment that AXNodeObject is based on Node
and AXlayoutObject is based on layout and it's the basic rule to me.

From the CL[1], we started to use AXNodeObject::determineAccessibilityRoleUtil
on AXLayoutObject::determineAccessibilityRole and it made the code general.
Now we don't need to keep implicit rule checking with node at both of
AXNodeObject and AXLayoutObject.

This patch removes direct checking node on
AXLayoutObject::determineAccessibilityRole and moves it to
AXNodeObject::determineAccessibilityRoleUtil and makes the code general.
Additionally, I merged the conditions for LinkRole and removed unnecessary
conditions such as isElementNode() because hasTagName checks isHTMLElement() if
we call hasTagName().

[1]https://codereview.chromium.org/762783002/

PTAL.

[je_julie(Not used), 2015-03-30 04:31:07]

Hi Dominic, PTAL.

[dmazzoni, 2015-03-30 06:20:53]

Oops, sorry I missed this review.

This looks excellent - thanks for consolidating this logic.

How much of this code is covered by tests? Could you add tests for any of the
HTML element -> role mappings that are not covered by a test now?

[je_julie(Not used), 2015-03-31 01:17:34]

On 2015/03/30 06:20:53, dmazzoni wrote:
> Oops, sorry I missed this review.
> 
> This looks excellent - thanks for consolidating this logic.
> 
> How much of this code is covered by tests? Could you add tests for any of the
> HTML element -> role mappings that are not covered by a test now?

Thanks :)
I added test cases for implicitly assigning role.
I know we have tests in content to verify them but I think it would be great if
we can have them in blink level because tests in content could have no specific
role depending on platform. For instance, several roles in blink are mapped to a
group role at platform level.
I added two test cases, element-role-mapping-focusable.html and
element-role-mapping-normal.html because it's better to have short tests to run
tests in parallel.
I verified that we have same results with this patch and without it.

As to trybot, it got failed regardless of this patch. If I need to run trybot
again, please let me know. I'll upload patch and re-run it.
PTAL.

[dmazzoni, 2015-03-31 06:50:53]

lgtm

Thanks for the very comprehensive tests!

https://codereview.chromium.org/1012383002/diff/40001/LayoutTests/accessibili...
File LayoutTests/accessibility/element-role-mapping-normal.html (right):

https://codereview.chromium.org/1012383002/diff/40001/LayoutTests/accessibili...
LayoutTests/accessibility/element-role-mapping-normal.html:40: </main>>
nit: extra > here

[je_julie(Not used), 2015-03-31 11:50:28]

The CQ bit was checked by je_julie.kim@samsung.com

[je_julie(Not used), 2015-03-31 11:50:28]

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

[commit-bot: I haz the power, 2015-03-31 11:50:37]

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

[commit-bot: I haz the power, 2015-03-31 11:55:26]

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

[commit-bot: I haz the power, 2015-03-31 11:55:27]

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

[je_julie(Not used), 2015-03-31 14:47:06]

On 2015/03/31 11:55:27, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/49726)

After rebase, I'm trying to commit it again.

[je_julie(Not used), 2015-03-31 14:47:58]

The CQ bit was checked by je_julie.kim@samsung.com

[je_julie(Not used), 2015-03-31 14:47:59]

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

[commit-bot: I haz the power, 2015-03-31 14:48:24]

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

[commit-bot: I haz the power, 2015-03-31 19:54:32]

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

[commit-bot: I haz the power, 2015-03-31 19:54:33]

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

[je_julie(Not used), 2015-04-01 01:52:37]

On 2015/03/31 19:54:33, I haz the power (commit-bot) wrote:
> Try jobs failed on following builders:
>   mac_blink_rel on tryserver.blink (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/49753)

According to revert https://codereview.chromium.org/1045693002/,
I needed to rebase it again.
I'm trying to commit it again.

[je_julie(Not used), 2015-04-01 01:52:51]

The CQ bit was checked by je_julie.kim@samsung.com

[je_julie(Not used), 2015-04-01 01:52:52]

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

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

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

[commit-bot: I haz the power, 2015-04-01 06:42:21]

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

[commit-bot: I haz the power, 2015-04-01 06:42:22]

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

[je_julie(Not used), 2015-04-05 07:29:05]

Hi Dominic,

The test from this patch had a specific result of platforms.
So, I updated test case without dump ax tree.
while I worked on it, I found details element role was missed for blink layout
test.
I uploaded https://codereview.chromium.org/1066503002/ as well.
That should be merged before landing this patch.

PTAL. Thanks.

[dmazzoni, 2015-04-06 06:05:45]

https://codereview.chromium.org/1012383002/diff/160001/LayoutTests/accessibil...
File LayoutTests/accessibility/element-role-mapping-focusable.html (right):

https://codereview.chromium.org/1012383002/diff/160001/LayoutTests/accessibil...
LayoutTests/accessibility/element-role-mapping-focusable.html:75:
shouldBe("axElement.role", "'AXRole: AXLink'");
If this were to fail and I was trying to debug it, it'd be a bit hard to debug
this way, with element numbers that don't mean anything and a big switch like
this - and the expected results don't show me what element id corresponds to
each result.

Look at canvas-fallback-content.html for an example, try to do something like
that. There's a helper function that does the checks and it prints out the
element id before each set of checks.

https://codereview.chromium.org/1012383002/diff/160001/LayoutTests/accessibil...
LayoutTests/accessibility/element-role-mapping-focusable.html:115: case 17:
I'm assuming the missing ones are the ones that differ based on platform?

There are several options here - you can have a different expectation file for
each platform (see LayoutTests/platform/mac/... for some examples), or for those
few cases you could assert that the role is one out of a list of roles that
covers all platforms.

[je_julie(Not used), 2015-04-07 10:59:28]

On 2015/04/06 06:05:45, dmazzoni wrote:
>
https://codereview.chromium.org/1012383002/diff/160001/LayoutTests/accessibil...
> LayoutTests/accessibility/element-role-mapping-focusable.html:115: case 17:
> I'm assuming the missing ones are the ones that differ based on platform?
> 
> There are several options here - you can have a different expectation file for
> each platform (see LayoutTests/platform/mac/... for some examples), or for
those
> few cases you could assert that the role is one out of a list of roles that
> covers all platforms.

Hi Dominic,
Thanks for the information.
The platform specific part is below.
    AXRole: AXDateTimeField AXValue: 
    ....
                     AXRole: AXStaticText AXValue: , 
                         AXRole: AXInlineTextBox AXValue: , 
                     AXRole: AXSpinButton AXValue: 
It's related part to input time formatting such as "DD,MM,Year".
Because we can check AX role consistently for all platforms,
I used AX role comparison referring to 'canvas-fallback-content.html'.
PTAL.

[dmazzoni, 2015-04-07 16:01:23]

lgtm

https://codereview.chromium.org/1012383002/diff/180001/LayoutTests/accessibil...
File LayoutTests/accessibility/element-role-mapping-focusable.html (right):

https://codereview.chromium.org/1012383002/diff/180001/LayoutTests/accessibil...
LayoutTests/accessibility/element-role-mapping-focusable.html:80: function
hasRole(id,expectedRole) {
nit: space after comma

[je_julie(Not used), 2015-04-08 03:50:40]

Thanks. I updated it.

https://codereview.chromium.org/1012383002/diff/180001/LayoutTests/accessibil...
File LayoutTests/accessibility/element-role-mapping-focusable.html (right):

https://codereview.chromium.org/1012383002/diff/180001/LayoutTests/accessibil...
LayoutTests/accessibility/element-role-mapping-focusable.html:80: function
hasRole(id,expectedRole) {
On 2015/04/07 16:01:22, dmazzoni wrote:
> nit: space after comma

Done.

[je_julie(Not used), 2015-04-08 04:03:13]

The CQ bit was checked by je_julie.kim@samsung.com

[je_julie(Not used), 2015-04-08 04:03:13]

The patchset sent to the CQ was uploaded after l-g-t-m from
dmazzoni@chromium.org
Link to the patchset: https://codereview.chromium.org/1012383002/#ps200001
(title: "Update nits")

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

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

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

Committed patchset #11 (id:200001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=193324