For an object with an aria role of "checkbox" or "radiobutton", the aria-checked attribute should be used to determine the state of the object. (Currently that attribute is being used to return the value, but not the state. The state is needed for MSAA on Windows.) I've got a patch that I intend to attach shortly.
Created attachment 44902 [details] Patch to add support for the aria-checked attribute.
Comment on attachment 44902 [details] Patch to add support for the aria-checked attribute. We need a regression test for this. We don't take code changes without regression tests. There are lots of accessibility regression tests, although I don't know if they are yet working on the Chrome platform.
Thanks for tackling this. I think this is probably good fix for all platforms, not just MSAA on Windows.
yea, we'd want this for all platforms, although Mac does not rely on isChecked for checkbox state (it uses intValue() instead)
Also: - All changes require a test if possible (which in this case it should be). - Please don't change the "Reviewed by NOBODY(OOPS!)." line, otherwise our scripts will have trouble updating the reviewer when landing this for you. Otherwise a patch which works for all platforms would be awesome!
Thanks for the quick feedback, I will be happy to add a test. Can you clarify what you mean by making this work on all platforms? This fix is in cross-platform code, and should fix the problem in both Safari and Chrome on Windows. As Chris said, the Mac accessibility code doesn't exercise this code path. I thought it would be better to add a cross-platform test. Would you prefer a Windows-specific test at the MSAA level, or both?
(In reply to comment #6) > Can you clarify what you mean by making this work on all platforms? This fix > is in cross-platform code, and should fix the problem in both Safari and Chrome > on Windows. As Chris said, the Mac accessibility code doesn't exercise this > code path. Sounds fine. > I thought it would be better to add a cross-platform test. Would you prefer a > Windows-specific test at the MSAA level, or both? Cross-platform test is great. MSAA test would be a nice plus, but do we have any of those?
Created attachment 45017 [details] Improved patch, adds an isChecked method to AccessibilityUIElement
As it turns out, there isn't a way to get at isChecked() or the object's MSAA state from a LayoutTest currently, so I added a new method to AccessibilityUIElement and separate implementations for win, mac, and gtk. Does this look right or did I miss an easier way? This works on the Mac, and I'm working on testing GTK. I still need to write an equivalent function for chromium's test_shell that I can try to submit in parallel. I'm using VS 2008, so I'm having trouble running the layout tests on Windows. Is there a try bot I can use? Or is someone willing to test this patch on Windows for me?
Attachment 45017 [details] did not pass style-queue: Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1 WebKitTools/DumpRenderTree/win/AccessibilityUIElementWin.cpp:291: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5] Total errors found: 1
your methodology looks pretty good to me.
Created attachment 45105 [details] All tests pass; source synced.
style-queue ran check-webkit-style on attachment 45105 [details] without any errors.
Hi all, I confirmed that this test passes on Windows and Mac OS X. I re-synced and fixed the style guide error. I also have a corresponding patch to Chromium under review, but it doesn't need to be committed in parallel because that whole directory of layout tests is currently skipped under chromium: http://codereview.chromium.org/503050 So, I hope this is ready to go? Please take another look. This is my first webkit patch, so please let me know anything else I should check; otherwise if it looks good, I'd appreciate it if you could tag it to be committed! Thanks, Dominic
I'm syncing and re-uploading the patch now.
Created attachment 45153 [details] Re-synced and updated patch
style-queue ran check-webkit-style on attachment 45153 [details] without any errors.
Comment on attachment 45153 [details] Re-synced and updated patch I think the correct behavior is that if an element both has native checkedness (e.g. because it is an input element of type radio or checkbox), then the checked attribute should take precedence over aria-checked, because <input type=radio> and <input type=checkbox> have "strong native semantics". Your patch seems to do it the other way around. Please fix this and provide an updated patch. Thanks! Reference: http://dev.w3.org/html5/spec/Overview.html#annotations-for-assistive-technology-products-aria
You're right, thanks. I modified the test to check the interaction of <input type="checkbox"> with the "checked" attribute and the "aria-checked" attribute, and confirmed that the Mac version is already working correctly (ignoring aria-checked). I'll make the Windows version match that next week after the holiday. Thanks.
Created attachment 45846 [details] Makes native checkedness take precedence when checking for aria-checked. Now tests all permutations of checked and aria-checked.
style-queue ran check-webkit-style on attachment 45846 [details] without any errors.
Comment on attachment 45846 [details] Makes native checkedness take precedence when checking for aria-checked. Now tests all permutations of checked and aria-checked. > + if (equalIgnoringCase(getAttribute(aria_checkedAttr).string(), "true")) You should omit the .string() here. > bool isRequired() const; > bool isSelected() const; > bool isExpanded() const; > + bool isChecked(); Why not const? > int hierarchicalLevel() const; > double clickPointX(); > double clickPointY(); > +bool AccessibilityUIElement::isChecked() > +{ > + return static_cast<bool>(intValue()); > +} Normally an int to bool conversion would not require an explicit cast. > + return (intValue() == 1); No need for the parentheses here. > + return (vState.lVal & STATE_SYSTEM_CHECKED); Or here. r=me
Created attachment 45850 [details] Address review comments
Thanks! All changes made except: > > bool isRequired() const; > > bool isSelected() const; > > bool isExpanded() const; > > + bool isChecked(); > > Why not const? Because the Mac implementation calls intValue, which isn't const.
> Because the Mac implementation calls intValue, which isn't const. seems like intValue() could be const.
Created attachment 45853 [details] Easier than I thought. Made both intValue and isChecked const.
style-queue ran check-webkit-style on attachment 45853 [details] without any errors.
Comment on attachment 45853 [details] Easier than I thought. Made both intValue and isChecked const. Clearing flags on attachment: 45853 Committed r52809: <http://trac.webkit.org/changeset/52809>
All reviewed patches have been landed. Closing bug.