Summary tag is exposed with wrong IA2 Role

Summary tag is exposed with wrong IA2 Role

Correct IA2 role for summary tag is ROLE_SYSTEM_PUSHBUTTON

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

[shreeramk, 2014-09-25 12:57:29]

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

[shreeramk, 2014-09-25 12:57:30]

Please review.

Thanks!

[shreeramk, 2014-09-25 13:05:02]

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

[Mike West, 2014-09-25 13:08:01]

Code change LGTM, but please wait for Dominic to confirm the role you've chosen.
It seems a little strange for the summary attribute to take on a
DisclosureTriangle role, but I might be misunderstanding the impact of that
choice on assistive technologies.

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

https://codereview.chromium.org/594843004/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXRenderObject.cpp:284: if
(isHTMLDetailsElement(m_renderer->parent()->node()))
I think you need to null-check parent() here. Otherwise detached nodes will
crash.

[Mike West, 2014-09-25 13:08:32]

Also: can you add a test to lock in the new behavior?

[dmazzoni, 2014-09-25 15:19:49]

lgtm

I believe you're already working on DumpAccessibilityTree test for this, so
that's okay with me - no need for a layout test also.

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

https://codereview.chromium.org/594843004/diff/1/Source/core/accessibility/AX...
Source/core/accessibility/AXRenderObject.cpp:284: if
(isHTMLDetailsElement(m_renderer->parent()->node()))
On 2014/09/25 13:08:01, Mike West wrote:
> I think you need to null-check parent() here. Otherwise detached nodes will
> crash.

Also, I think it'd be more correct to check node->parentElement rather than
m_renderer->parent()->node.

The render tree can differ from the node tree in subtle ways. If the details
element has multiple children and some are inline and some are block, the render
tree may have an anonymous block in-between, so the details would not
necessarily be the direct parent of the summary element.

[shreeramk, 2014-09-25 16:15:40]

On 2014/09/25 15:19:49, dmazzoni wrote:
> I believe you're already working on DumpAccessibilityTree test for this, so
> that's okay with me - no need for a layout test also.

Yes I am still working on DumpAccessibilityTree tests.

[shreeramk, 2014-09-26 05:05:09]

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

[commit-bot: I haz the power, 2014-09-26 05:06:19]

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

[commit-bot: I haz the power, 2014-09-26 07:37:02]

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

[commit-bot: I haz the power, 2014-09-26 07:37:03]

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

[shreeramk, 2014-09-26 08:28:30]

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

[commit-bot: I haz the power, 2014-09-26 08:29:17]

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

[commit-bot: I haz the power, 2014-09-26 08:29:48]

Committed patchset #2 (id:20001) as 182740