Bug 121413 – [ATK] Extends atk value interface to return proper checkbox states.

• Component: Accessibility
• Status: UNCONFIRMED

[Krzysztof Czech]

Adds support for returning proper values of aria-checked attribute.

[Radar WebKit Bug Importer]

<rdar://problem/14997442>

[Krzysztof Czech]

Created attachment 211745 [details]
Patch

[WebKit Commit Bot]

Attachment 211745 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/accessibility/mixed-checkbox-expected.txt', u'LayoutTests/accessibility/mixed-checkbox.html', u'LayoutTests/accessibility/native-vs-nonnative-checkboxes-expected.txt', u'LayoutTests/accessibility/native-vs-nonnative-checkboxes.html', u'LayoutTests/platform/mac/accessibility/mixed-checkbox-expected.txt', u'LayoutTests/platform/mac/accessibility/mixed-checkbox.html', u'LayoutTests/platform/mac/accessibility/native-vs-nonnative-checkboxes-expected.txt', u'LayoutTests/platform/mac/accessibility/native-vs-nonnative-checkboxes.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceValue.cpp', u'Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp']" exit_code: 1
Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceValue.cpp:49:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceValue.cpp:48:  Missing space before ( in switch(  [whitespace/parens] [5]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Krzysztof Czech]

Created attachment 211747 [details]
Patch

[Mario Sanchez Prada]

Comment on attachment 211747 [details]
Patch

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

The patch looks good already, yet there are some minor changes that I think we should do before landing

> LayoutTests/ChangeLog:8
> +        Sharing mac tests with other ports (GTK/EFL).

Great!

> Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceValue.cpp:39
> +static float webkitAccessibleValueGetCurrentValue(AccessibilityObject* coreObject)

I would probably call this webkitAccessibleValueValueForAccessibilityObject(), to make it clearer from the caller (and to make it more explicit in the ChangeLog that you added a new function here)

> Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:1038
> +    if (role == SliderRole || role == SpinButtonRole || role == ScrollBarRole || role == ProgressIndicatorRole || role == CheckBoxRole)

I think we should replace this condition with (coreObject->supportsRangeValue() || coreObject->isCheckboxOrRadio())

[Krzysztof Czech]

Created attachment 211765 [details]
Patch

[Krzysztof Czech]

> > Source/WebCore/accessibility/atk/WebKitAccessibleInterfaceValue.cpp:39
> > +static float webkitAccessibleValueGetCurrentValue(AccessibilityObject* coreObject)
> 
> I would probably call this webkitAccessibleValueValueForAccessibilityObject(), to make it clearer from the caller (and to make it more explicit in the ChangeLog that you added a new function here)

Sounds good.
> 
> > Source/WebCore/accessibility/atk/WebKitAccessibleWrapperAtk.cpp:1038
> > +    if (role == SliderRole || role == SpinButtonRole || role == ScrollBarRole || role == ProgressIndicatorRole || role == CheckBoxRole)
> 
> I think we should replace this condition with (coreObject->supportsRangeValue() || coreObject->isCheckboxOrRadio())

This is nice, thanks.

Already corrected

[Mario Sanchez Prada]

Comment on attachment 211765 [details]
Patch

Lgtm. Thanks

[WebKit Commit Bot]

Comment on attachment 211765 [details]
Patch

Clearing flags on attachment: 211765

Committed r155851: <http://trac.webkit.org/changeset/155851>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joanmarie Diggs]

Uh, sorry for being busy and just now seeing this... AtkValue is NOT how one should expose checkbox states. AtkValue [1] for sliders and progressbars and scrollbars and the like. AtkState [2] is for ... states, like checkbox states. In particular see:

ATK_STATE_CHECKED
Indicates this object is currently checked, for instance a checkbox is 'non-empty'.

ATK_STATE_INDETERMINATE
Indicates that a check box is in a state other than checked or not checked. This usually means that the boolean value reflected or controlled by the object does not apply consistently to the entire current context. For example, a checkbox for the "Bold" attribute of text may have STATE_INDETERMINATE if the currently selected text contains a mixture of weight attributes. In many cases interacting with a STATE_INDETERMINATE object will cause the context's corresponding boolean attribute to be homogenized, whereupon the object will lose STATE_INDETERMINATE and a corresponding state-changed event will be fired.


[1] https://developer.gnome.org/atk/unstable/AtkValue.html
[2] https://developer.gnome.org/atk/unstable/atk-AtkState.html

[Joanmarie Diggs]

Reopened because this change should not have been made at all.

Screen readers expect AtkValue to be implemented correctly. So this "fix" is introducing a bug.

[Csaba Osztrogonác]

Comment on attachment 211747 [details]
Patch

Cleared Mario Sanchez Prada's review+ from obsolete attachment 211747 [details] so that this bug does not appear in http://webkit.org/pending-commit.