Bug 121413 - [ATK] Extends atk value interface to return proper checkbox states.
Summary: [ATK] Extends atk value interface to return proper checkbox states.
Status: UNCONFIRMED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 121429
Blocks:
  Show dependency treegraph
 
Reported: 2013-09-16 02:37 PDT by Krzysztof Czech
Modified: 2017-03-11 10:46 PST (History)
9 users (show)

See Also:


Attachments
Patch (14.15 KB, patch)
2013-09-16 02:45 PDT, Krzysztof Czech
no flags Details | Formatted Diff | Diff
Patch (14.13 KB, patch)
2013-09-16 02:59 PDT, Krzysztof Czech
no flags Details | Formatted Diff | Diff
Patch (14.08 KB, patch)
2013-09-16 05:03 PDT, Krzysztof Czech
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Krzysztof Czech 2013-09-16 02:37:19 PDT
Adds support for returning proper values of aria-checked attribute.
Comment 1 Radar WebKit Bug Importer 2013-09-16 02:38:02 PDT
<rdar://problem/14997442>
Comment 2 Krzysztof Czech 2013-09-16 02:45:34 PDT
Created attachment 211745 [details]
Patch
Comment 3 WebKit Commit Bot 2013-09-16 02:47:14 PDT
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.
Comment 4 Krzysztof Czech 2013-09-16 02:59:31 PDT
Created attachment 211747 [details]
Patch
Comment 5 Mario Sanchez Prada 2013-09-16 03:34:19 PDT
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())
Comment 6 Krzysztof Czech 2013-09-16 05:03:53 PDT
Created attachment 211765 [details]
Patch
Comment 7 Krzysztof Czech 2013-09-16 05:04:43 PDT
> > 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
Comment 8 Mario Sanchez Prada 2013-09-16 05:15:02 PDT
Comment on attachment 211765 [details]
Patch

Lgtm. Thanks
Comment 9 WebKit Commit Bot 2013-09-16 05:37:46 PDT
Comment on attachment 211765 [details]
Patch

Clearing flags on attachment: 211765

Committed r155851: <http://trac.webkit.org/changeset/155851>
Comment 10 WebKit Commit Bot 2013-09-16 05:37:49 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Joanmarie Diggs (irc: joanie) 2013-09-16 06:45:31 PDT
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
Comment 12 Joanmarie Diggs (irc: joanie) 2013-09-16 06:46:17 PDT
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.
Comment 13 Csaba Osztrogonác 2014-02-13 04:05:18 PST
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.