Bug 32571 – Support the aria-checked attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Dominic Mazzoni]

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.

[Dominic Mazzoni]

Created attachment 44902 [details]
Patch to add support for the aria-checked attribute.

[Darin Adler]

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.

[Darin Adler]

Thanks for tackling this. I think this is probably good fix for all platforms, not just MSAA on Windows.

[chris fleizach]

yea, we'd want this for all platforms, although Mac does not rely on isChecked for checkbox state (it uses intValue() instead)

[Eric Seidel (no email)]

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!

[Dominic Mazzoni]

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?

[Darin Adler]

(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?

[Dominic Mazzoni]

Created attachment 45017 [details]
Improved patch, adds an isChecked method to AccessibilityUIElement

[Dominic Mazzoni]

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?

[WebKit Review Bot]

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

[chris fleizach]

your methodology looks pretty good to me.

[Dominic Mazzoni]

Created attachment 45105 [details]
All tests pass; source synced.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45105 [details] without any errors.

[Dominic Mazzoni]

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

[Dominic Mazzoni]

I'm syncing and re-uploading the patch now.

[Dominic Mazzoni]

Created attachment 45153 [details]
Re-synced and updated patch

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45153 [details] without any errors.

[Maciej Stachowiak]

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

[Dominic Mazzoni]

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.

[Dominic Mazzoni]

Created attachment 45846 [details]
Makes native checkedness take precedence when checking for aria-checked. Now tests all permutations of checked and aria-checked.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45846 [details] without any errors.

[Darin Adler]

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

[Dominic Mazzoni]

Created attachment 45850 [details]
Address review comments

[Dominic Mazzoni]

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.

[chris fleizach]

> Because the Mac implementation calls intValue, which isn't const.

seems like intValue() could be const.

[Dominic Mazzoni]

Created attachment 45853 [details]
Easier than I thought.  Made both intValue and isChecked const.

[WebKit Review Bot]

style-queue ran check-webkit-style on attachment 45853 [details] without any errors.

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.