AX: Regression (r126369) - toggle buttons no longer return accessible titles

• Component: Accessibility
• Status: RESOLVED FIXED

[Dominic Mazzoni]

I think that adding the toggle button role broke some logic that was assuming that toggle buttons' roles were Button Role.

As one example, AccessibilityRenderObject::title() returns textUnderElement() for a role of ButtonRole, and presumably it should do the same for ToggleButtonRole.

As a result, now sometimes a toggle button ends up without an accessible title.

There are probably other examples - search for any logic that involves ButtonRole and check for ToggleButtonRole and possibly PopUpButtonRole as well. Maybe modify isButton or add a new method isButtonType()?

[chris fleizach]

yea it sounds like we should audit isButton() usage and it should be

return ButtonRole || PopUpButtonRole || ToggleButton;

[Dominic Mazzoni]

(In reply to comment #1)
> yea it sounds like we should audit isButton() usage and it should be
> 
> return ButtonRole || PopUpButtonRole || ToggleButton;

At first glance it looks like we could just modify isButton - it's only used in 3 places and I think they'd all be fine with matching all 3 roles. Then we can use isButton throughout.

[chris fleizach]

that sounds like a good plan

[Alejandro Piñeiro]

(In reply to comment #3)
> that sounds like a good plan

Ok, just in case nobody else is working on that, I will take a look to this.

[Alejandro Piñeiro]

Created attachment 161275 [details]
Fixes the bug

This patch fixes the bug using suggestions on the previous comments. I redefined isButton, but I didn't use it in every case. For example on the example mentioned by Dominic, I just added ToggleButtonRole on the switch, as using isButton and keeping the switch for the rest of the cases seemed non really natural. I also reviewed other cases where ButtonRole was used in order to try to check if ToggleButtonRole should be used.

About the test:
  * It only test the problem related to the title, as it is the only one described at the bug title. Should I add more tests?
  * This is a gtk test, as I used as reference the previous toggle button test, so I include a shouldBe (role), and this is platform specific. Is this a problem? Should be more platform independent?

[Dominic Mazzoni]

Thanks for fixing this! The patch looks fine to me.

A cross-platform test would be nice - or at a minimum, a test that runs on Mac, since right now Mac gives the most WebKit test coverage. See LayoutTests/accessibility/focusable-div.html for one approach I used recently to create a cross-platform test - it just grabs the last character of the title so that it's agnostic to any prefix like 'AXTitle: '.

Alternatively, consider a test that just dumps the role and title of each button using debug(), and we could have different expectations for each platform. You can add the test to LayoutTests/accessibility and only add a gtk-specific expectation (e.g. LayoutTests/platform/gtk/accessibility/aria-toggle-button-with-title-expected.txt) in this patch - then I'd be happy to add Mac & Chromium expectations later.

[Alejandro Piñeiro]

(In reply to comment #6)
> Thanks for fixing this! The patch looks fine to me.

You are welcome. 

> A cross-platform test would be nice - or at a minimum, a test that runs on Mac, since right now Mac gives the most WebKit test coverage. See LayoutTests/accessibility/focusable-div.html for one approach I used recently to create a cross-platform test - it just grabs the last character of the title so that it's agnostic to any prefix like 'AXTitle: '.

Well, the problem with that focusable-div example is that it doesn't take into account the role. As the bug title refers to toggle button, I really think that a role check needs to be included on the test. And as we know, role name is at this moment something platform specific.

> 
> Alternatively, consider a test that just dumps the role and title of each button using debug(), and we could have different expectations for each platform. You can add the test to LayoutTests/accessibility and only add a gtk-specific expectation (e.g. LayoutTests/platform/gtk/accessibility/aria-toggle-button-with-title-expected.txt) in this patch - then I'd be happy to add Mac & Chromium expectations later.

This approach is somewhat similar to what Joanmarie Diggs made in a recent bug:
https://bugs.webkit.org/show_bug.cgi?id=84137#c9

Although I feel that the one used at that bug is somewhat more elaborated.

Taking into account that people prefer a more multi-platform test (Joanmarie also mentioned me that on IRC), meanwhile someone review the rest of the patch, I will take a look to that legend.html bug to see if I can improve the test.

In general, due this kind of things, and after some talking with Joanmarie, it is clear that it would be good to talk about the status of the accessibility tests, good practices, adding stuff to the testing libraries to avoid C&P. In general make easier having general tests, as the tempting easy solution is create platform-specific tests, when in several cases makes sense at a platform independent level.

[chris fleizach]

Comment on attachment 161275 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=161275&action=review

> Source/WebCore/accessibility/AccessibilityObject.h:380
> +    bool isButton() const { return roleValue() == ButtonRole || PopUpButtonRole || ToggleButtonRole; }

this logic is wrong. This will make isButton always return true.
i think we want roleValue() == Button || roleValue() == Popup ...
since it will be a bit longer might as well put it in the .cpp file and cache roleValue

AccessibilityRole role = roleValue()
return role == Button || role == Popup ...

[Dominic Mazzoni]

Ping - Alejandro, can you upload another patch? This looks pretty close once you fix that one logic bug.

[Alejandro Piñeiro]

(In reply to comment #9)
> Ping - Alejandro, can you upload another patch? This looks pretty close once you fix that one logic bug.

Pong - yes, sorry, I have been busy these days, but I plan to upload a new version of my patch soon. Sorry for the delay.

[Alejandro Piñeiro]

Created attachment 163331 [details]
Fixes the bug

Patch updated after review on comment 8.

> Alternatively, consider a test that just dumps the role and title of each button using debug(), and we could have different expectations for each platform. You can add the test to LayoutTests/accessibility and only add a gtk-specific expectation (e.g. LayoutTests/platform/gtk/accessibility/aria-toggle-button-with-title-expected.txt) in this patch - then I'd be happy to add Mac & Chromium expectations later.

For simplicity, in the end I went to this path. As I said IMHO the test should also check the role, so the approach you proposed (focusable-div.html) was not enough. So this patch add the test to LayoutTests/accessibility, and the gtk-specific expectation. Mac & Chromium expectations are missing.

[WebKit Review Bot]

Comment on attachment 163331 [details]
Fixes the bug

Attachment 163331 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13820345

New failing tests:
accessibility/aria-toggle-button-with-title.html

[chris fleizach]

looks ok except for failing failing test

[Alejandro Piñeiro]

(In reply to comment #13)
> looks ok except for failing failing test

ok, I will wait for Dominic answer, as he offered himself to provide the expected results of that test for Mac & Chromium

[Dominic Mazzoni]

Sure. Patched in now, will have the results in a few minutes.

[Dominic Mazzoni]

Created attachment 163382 [details]
Suggested expected results on Mac

[Dominic Mazzoni]

Created attachment 163383 [details]
Suggested expected results on Chromium

[Alejandro Piñeiro]

Created attachment 163578 [details]
Fixes the bug

The same patch but with the expected files provided by Dominic (thanks!)

[WebKit Review Bot]

Comment on attachment 163578 [details]
Fixes the bug

Rejecting attachment 163578 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
ource/WebCore/accessibility/AccessibilityRenderObject.cpp
Hunk #1 FAILED at 485.
Hunk #2 FAILED at 626.
Hunk #3 FAILED at 914.
Hunk #4 FAILED at 1401.
Hunk #5 FAILED at 3448.
Hunk #6 succeeded at 2907 (offset -848 lines).
5 out of 6 hunks FAILED -- saving rejects to file Source/WebCore/accessibility/AccessibilityRenderObject.cpp.rej

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Chris Flei..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://queues.webkit.org/results/13847080

[Alejandro Piñeiro]

(In reply to comment #19)
> (From update of attachment 163578 [details])
> Rejecting attachment 163578 [details] from commit-queue.
> 
> Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2
> 
> Last 500 characters of output:
> ource/WebCore/accessibility/AccessibilityRenderObject.cpp
> Hunk #1 FAILED at 485.
> Hunk #2 FAILED at 626.
> Hunk #3 FAILED at 914.
> Hunk #4 FAILED at 1401.
> Hunk #5 FAILED at 3448.
> Hunk #6 succeeded at 2907 (offset -848 lines).
> 5 out of 6 hunks FAILED -- saving rejects to file Source/WebCore/accessibility/AccessibilityRenderObject.cpp.rej
> 
> Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--force', u'--reviewer', u'Chris Flei..." exit_code: 1 cwd: /mnt/git/webkit-commit-queue
> 
> Full output: http://queues.webkit.org/results/13847080

After a brief look on this, this was caused by the last changes on AccessibilityRenderObject and AccessibilityNodeObject committed yesterday:

http://trac.webkit.org/changeset/128318

So most of the methods I changed now reside on AccessibilityNodeObject. I will update the patch today.

[Alejandro Piñeiro]

Created attachment 163824 [details]
Fixes the bug

Updated patch required after the changes made by http://trac.webkit.org/changeset/128318

[WebKit Review Bot]

Comment on attachment 163824 [details]
Fixes the bug

Rejecting attachment 163824 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

Last 500 characters of output:
/webkit-commit-queue/Tools/Scripts/webkitpy/common/checkout/scm/scm.py", line 76, in run
    decode_output=decode_output)
  File "/mnt/git/webkit-commit-queue/Tools/Scripts/webkitpy/common/system/executive.py", line 407, in run_command
    output = output.decode(self._child_process_encoding())
  File "/usr/lib/python2.6/encodings/utf_8.py", line 16, in decode
    return codecs.utf_8_decode(input, errors, True)
UnicodeDecodeError: 'utf8' codec can't decode bytes in position 199-202: invalid data

Full output: http://queues.webkit.org/results/13839572

[Alejandro Piñeiro]

Created attachment 164360 [details]
Patch

[WebKit Review Bot]

Comment on attachment 164360 [details]
Patch

Clearing flags on attachment: 164360

Committed r128748: <http://trac.webkit.org/changeset/128748>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.