Bug 94858 - AX: Regression (r126369) - toggle buttons no longer return accessible titles
Summary: AX: Regression (r126369) - toggle buttons no longer return accessible titles
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-23 14:37 PDT by Dominic Mazzoni
Modified: 2012-09-17 05:52 PDT (History)
6 users (show)

See Also:


Attachments
Fixes the bug (9.21 KB, patch)
2012-08-29 11:34 PDT, Alejandro Piñeiro
cfleizach: review-
Details | Formatted Diff | Diff
Fixes the bug (9.12 KB, patch)
2012-09-11 04:26 PDT, Alejandro Piñeiro
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Suggested expected results on Mac (392 bytes, text/plain)
2012-09-11 09:28 PDT, Dominic Mazzoni
no flags Details
Suggested expected results on Chromium (398 bytes, text/plain)
2012-09-11 09:28 PDT, Dominic Mazzoni
no flags Details
Fixes the bug (11.14 KB, patch)
2012-09-12 04:02 PDT, Alejandro Piñeiro
cfleizach: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fixes the bug (11.20 KB, patch)
2012-09-13 03:00 PDT, Alejandro Piñeiro
no flags Details | Formatted Diff | Diff
Patch (10.58 KB, patch)
2012-09-17 02:32 PDT, Alejandro Piñeiro
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Mazzoni 2012-08-23 14:37:13 PDT
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()?
Comment 1 chris fleizach 2012-08-23 14:44:25 PDT
yea it sounds like we should audit isButton() usage and it should be

return ButtonRole || PopUpButtonRole || ToggleButton;
Comment 2 Dominic Mazzoni 2012-08-23 14:50:55 PDT
(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.
Comment 3 chris fleizach 2012-08-23 14:58:12 PDT
that sounds like a good plan
Comment 4 Alejandro Piñeiro 2012-08-24 03:50:42 PDT
(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.
Comment 5 Alejandro Piñeiro 2012-08-29 11:34:34 PDT
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?
Comment 6 Dominic Mazzoni 2012-08-29 13:28:06 PDT
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.
Comment 7 Alejandro Piñeiro 2012-08-30 07:06:14 PDT
(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.
Comment 8 chris fleizach 2012-08-30 19:17:49 PDT
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 ...
Comment 9 Dominic Mazzoni 2012-09-10 14:13:00 PDT
Ping - Alejandro, can you upload another patch? This looks pretty close once you fix that one logic bug.
Comment 10 Alejandro Piñeiro 2012-09-11 02:26:18 PDT
(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.
Comment 11 Alejandro Piñeiro 2012-09-11 04:26:48 PDT
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.
Comment 12 WebKit Review Bot 2012-09-11 05:37:29 PDT
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
Comment 13 chris fleizach 2012-09-11 09:04:18 PDT
looks ok except for failing failing test
Comment 14 Alejandro Piñeiro 2012-09-11 09:10:49 PDT
(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
Comment 15 Dominic Mazzoni 2012-09-11 09:22:07 PDT
Sure. Patched in now, will have the results in a few minutes.
Comment 16 Dominic Mazzoni 2012-09-11 09:28:10 PDT
Created attachment 163382 [details]
Suggested expected results on Mac
Comment 17 Dominic Mazzoni 2012-09-11 09:28:42 PDT
Created attachment 163383 [details]
Suggested expected results on Chromium
Comment 18 Alejandro Piñeiro 2012-09-12 04:02:13 PDT
Created attachment 163578 [details]
Fixes the bug

The same patch but with the expected files provided by Dominic (thanks!)
Comment 19 WebKit Review Bot 2012-09-13 00:30:59 PDT
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
Comment 20 Alejandro Piñeiro 2012-09-13 02:29:14 PDT
(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.
Comment 21 Alejandro Piñeiro 2012-09-13 03:00:49 PDT
Created attachment 163824 [details]
Fixes the bug

Updated patch required after the changes made by http://trac.webkit.org/changeset/128318
Comment 22 WebKit Review Bot 2012-09-13 10:23:14 PDT
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
Comment 23 Alejandro Piñeiro 2012-09-17 02:32:06 PDT
Created attachment 164360 [details]
Patch
Comment 24 WebKit Review Bot 2012-09-17 05:51:55 PDT
Comment on attachment 164360 [details]
Patch

Clearing flags on attachment: 164360

Committed r128748: <http://trac.webkit.org/changeset/128748>
Comment 25 WebKit Review Bot 2012-09-17 05:52:00 PDT
All reviewed patches have been landed.  Closing bug.