Bug 123889 – [ATK] "Indeterminate" state and state changes in tri-state checkboxes should be exposed to ATs

• Component: Accessibility
• Status: NEW

[Joanmarie Diggs]

Created attachment 216169 [details]
accessible-event listener

Steps to reproduce:
  1. (Re)Load http://test.cita.illinois.edu/aria/checkbox/checkbox2.php
  2. Launch the attached accessible-event listener in a terminal
  3. Tab to All Condiments (which initially is partially checked)
  4. Tab to Lettuce
  5. Press Space (All Condiments will lose partially checked state)
  6. Press Space (All Condiments will gain partially checked state)

Expected results:
  $ ./indeterminate.py 
  [check box | All condiments] has indeterminate state: True
  [check box | Lettuce] has indeterminate state: False
  [check box | All condiments] indeterminate state changed to: False
  [check box | All condiments] indeterminate state changed to: True

Actual results with current WebKitGtk MiniBrowser:
  $ ./indeterminate.py 
  [check box | All condiments] has indeterminate state: False
  [check box | Lettuce] has indeterminate state: False

[Radar WebKit Bug Importer]

<rdar://problem/15403059>

[Krzysztof Czech]

Created attachment 221262 [details]
patch

[Krzysztof Czech]

I proposed a patch for exposing tri-state checkboxes. It's a kind of a draft not a final version. I think it would be nice to have a layout test as well. Basically I'd like to hear an opinion about my approach. WebKitGtk MiniBrowser works as expected.

[Mario Sanchez Prada]

Comment on attachment 221262 [details]
patch

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

I like the approach. I only have a couple of comments

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:188
> +    if (coreObject->roleValue() != GroupRole)
> +        axGroup = coreObject->parentObjectUnignored();

Not sure whether this should be a while instead of an if. Would it be possible that the parent with the GroupRole role is not the immediate unignored parent for the object which received the notification?

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:190
> +    AccessibilityObject::AccessibilityChildrenVector children = axGroup->children();

As per recent findings, you should not be copying the list of children here. use const auto& children = axGroup->children() instead.

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:201
> +    // Get tri-state head object

Missing period at the end

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:239
> +        notifyCheckedStateChanged(coreObject);
> +        notifyCheckedStateIndeterminate(coreObject);

I would probably call notifyCheckedStateIndeterminate() from inside notifyCheckedStateChanged(), or even combine both functions into a simple one because in the end, from this specific point, it's all about notifying the "checked state". And in this context, the "indeterminate" checked state seems to be IMHO an internal detail that should not be exposed here.

[Krzysztof Czech]

(In reply to comment #4)
> (From update of attachment 221262 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221262&action=review
> 
> I like the approach. I only have a couple of comments
> 
> > Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:188
> > +    if (coreObject->roleValue() != GroupRole)
> > +        axGroup = coreObject->parentObjectUnignored();
> 
> Not sure whether this should be a while instead of an if. Would it be possible that the parent with the GroupRole role is not the immediate unignored parent for the object which received the notification?

I also have the same doubt and couple of others, regarding this peace of code.
I guess GroupRole objects may not be an immediate parent. I can imagine for example other element between, then tri-state object would not be the first(0) element.

I'm also wondering whether there might be something different than GroupRole element. Basically in this specific test case GroupRole groups those checkboxes and this is obvious for me, but is it possible that other element could do the same thing or maybe checkboxes could not be grouped at all.

Those are things that bothers me.
> 
> > Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:190
> > +    AccessibilityObject::AccessibilityChildrenVector children = axGroup->children();
> 
> As per recent findings, you should not be copying the list of children here. use const auto& children = axGroup->children() instead.
Thanks.
> 
> > Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:201
> > +    // Get tri-state head object
> 
> Missing period at the end
> 
> > Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:239
> > +        notifyCheckedStateChanged(coreObject);
> > +        notifyCheckedStateIndeterminate(coreObject);
> 
> I would probably call notifyCheckedStateIndeterminate() from inside notifyCheckedStateChanged(), or even combine both functions into a simple one because in the end, from this specific point, it's all about notifying the "checked state". And in this context, the "indeterminate" checked state seems to be IMHO an internal detail that should not be exposed here.

Yes, you are right.

[Krzysztof Czech]

Created attachment 223338 [details]
patch

[chris fleizach]

Comment on attachment 223338 [details]
patch

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

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:203
> +    // WebKit notifies it as many times as children changes its states.

I think we should try to resolve the issue of multiple updates being posted rather than storing a boolean value and a list of tri state objects in every AXObject. That seems to paper over the problem which may affect other cases as well as this one

[Krzysztof Czech]

> > Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:203
> > +    // WebKit notifies it as many times as children changes its states.
> 
> I think we should try to resolve the issue of multiple updates being posted rather than storing a boolean value and a list of tri state objects in every AXObject. That seems to paper over the problem which may affect other cases as well as this one

Basically those notifications from children are triggered by a tri-state object when its state is changed, so this is a proper behavior. The thing was that webkit notified them as they came from a tri-state object which was wrong.
This happened because child elements were not accessible.

Regarding this boolean value. I guess there is no other way to know that particular element is a tri-state objects. Well there's an option to check the aria-checked attribute but when the state is different than mixed then it's not possible. So my idea is to store this information in AccessibilityObject every time I find a proper element with a mixed state. Having this value, I'm able to send a signal to ATs when this state is set or not.

[Krzysztof Czech]

Created attachment 224064 [details]
patch

[WebKit Commit Bot]

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


ERROR: Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:210:  One line control clauses should not use braces.  [whitespace/braces] [4]
Total errors found: 1 in 9 files


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

[Krzysztof Czech]

Created attachment 224065 [details]
patch

[chris fleizach]

Comment on attachment 224065 [details]
patch

I don't support this change. I think this can be done without storing a bool value in every object and without making a list of all possible tri-state objects that needs to be checked.
It seems like you just need to know if a radio button's aria-value was mixed -> not mixed, or not mixed -> mixed
That seems like it's possible to know already by monitoring attribute changes

[Krzysztof Czech]

(In reply to comment #12)
> (From update of attachment 224065 [details])
> I don't support this change. I think this can be done without storing a bool value in every object and without making a list of all possible tri-state objects that needs to be checked.
> It seems like you just need to know if a radio button's aria-value was mixed -> not mixed, or not mixed -> mixed
> That seems like it's possible to know already by monitoring attribute changes

Thanks Chris for your comments. I guess I tried to complicate things a bit. I found some valuable information in aria-practices that put some light on things that I wasn't really sure.

[Krzysztof Czech]

Created attachment 224190 [details]
patch

[Krzysztof Czech]

(In reply to comment #14)
> Created an attachment (id=224190) [details]
> patch

w3 says (http://www.w3.org/TR/2013/WD-wai-aria-practices-20130307/) that checkboxes that have logical grouping should be children of a DOM element with group role.

Related to previous Mario's comments, this seems that an immediate parent of the children that received notification is one with group role.

My only concern is that whether we can treat the first element of a group as tri-state object, but I could not imagine other place, so it likely to be the first child.

[Build Bot]

Comment on attachment 224190 [details]
patch

Attachment 224190 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5956747923554304

New failing tests:
accessibility/aria-checkbox-sends-indeterminate-notification.html

[Build Bot]

Created attachment 224198 [details]
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-01  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 224190 [details]
patch

Attachment 224190 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/4908985148768256

New failing tests:
accessibility/aria-checkbox-sends-indeterminate-notification.html

[Build Bot]

Created attachment 224200 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 224190 [details]
patch

Attachment 224190 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6321002623533056

New failing tests:
accessibility/aria-checkbox-sends-indeterminate-notification.html

[Build Bot]

Created attachment 224205 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Mario Sanchez Prada]

(In reply to comment #15)
> (In reply to comment #14)
> > Created an attachment (id=224190) [details] [details]
> > patch
> 
> w3 says (http://www.w3.org/TR/2013/WD-wai-aria-practices-20130307/) that checkboxes that have logical grouping should be children of a DOM element with group role.
> 
> Related to previous Mario's comments, this seems that an immediate parent of the children that received notification is one with group role.
> 
> My only concern is that whether we can treat the first element of a group as 
> tri-state object, but I could not imagine other place, so it likely to be the 
> first child.

I also find strange this thing of emitting the signal always from the first element (what happens if the state has actually changed over one of the others?). In GTK you can get that signal emitted by any checkbox in the group, I think, so my understanding is that this scenario should be more or less similar.

Perhaps Joanie can drop some light in here. After all, she reported the bug and probably knows better what Orca would actually expect from its side

[chris fleizach]

Comment on attachment 224190 [details]
patch

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

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:186
> +    AccessibilityObject* currentObject = coreObject;

probably don't need to make a new currentObject here. you could rename the incoming parameter to just "object" and use that

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:190
> +    const AccessibilityObject::AccessibilityChildrenVector& children = currentObject->children();

const auto& children

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:212
> +    AtkObject* atkObject = accessibilityTriStateObject ? accessibilityTriStateObject->wrapper() : nullptr;

i don't think you need to check if accessibilityTriStateObject == nil here because you already did that on line 205

> Source/WebCore/accessibility/atk/AXObjectCacheAtk.cpp:213
> +    if (atkObject) {

you can probably write this as
if (AtkObject* atkObject = axTriState->wrapper()) {

[Joanmarie Diggs]

Created attachment 224278 [details]
Modified version of the UIUC checkbox 2 test

(Discussion to follow)

[Joanmarie Diggs]

(In reply to comment #15)

> w3 says (http://www.w3.org/TR/2013/WD-wai-aria-practices-20130307/) that checkboxes that have logical grouping should be children of a DOM element with group role.

Indeed it does. If you have a bunch of checkboxes all related to foo, grouping those checkboxes together into a "foo" group makes sense. BUT I don't think that's relevant to the problem at hand.

1. Just because you have a "foo" group of checkboxes does not necessarily mean you have an associated "toggle all foos" uber tri-state checkbox.

2. If you happen to have an associated "toggle all foos" uber tri-state checkbox, there is no specification (that I have found) indicating where that uber checkbox must go. (Nor should there be IMHO.)

> My only concern is that whether we can treat the first element of a group as tri-state object, but I could not imagine other place

Your concern is valid. :) I created a local/stand-alone copy of the UIUC checkbox 2 test and added cheese. :) The "all cheeses" checkbox is the last child; not the first. Firefox does the right thing; your patch does not.

Related aside #1: When I try to interact with my newly-attached test case as an attachment (i.e. link followed in bugzilla), the checkboxes are not interactive. But if I download the test case to my system and open it as a local file, it works as expected. <insert shrug here>

Getting back to the topic at hand, I have what I hope is some good news for you: You should NOT have to do heuristics to figure out where the tri-state object is. When a checkbox-roled element (e.g. Lettuce) changes its state, it is up to the web developer to set the state of that checkbox-roled element. If that state change in turn causes some other checkbox (e.g. All condiments) to also change state, then it is up to the web developer to also set the state of that other checkbox-roled element (i.e. All condiments). It ain't your responsibility to sort out this mess, in other words. :) You should "just have" the checkbox and the state change. Just pass that information along via the platform accessibility API. :)

Related aside #2: It appears that the UIUC checkbox 2 example sets the "mixed" state even when the mixed state has not changed. See adjCheckedCount(). I did not fix this in my updated version of the UIUC example. Apparently Gecko is filtering this non-change out and only emitting state-changed:indeterminate signals if the indeterminate state really has changed. While that is appreciated noise reduction, it is not clear to me whose bug/responsibility it is to only emit state-changed signals when the state has actually changed. I'm leaning towards this being an author error.... Anyhoo, let's separate that out from the originally filed bug here.

My question to you is: Using the modified (and downloaded) new test case, are you seeing WebCore Accessibility events for checkboxes which you can pass along unmodified via ATK? If the answer is "yes", please do so. :) If not, please elaborate.

[Krzysztof Czech]

(In reply to comment #25)
> (In reply to comment #15)
> 
> > w3 says (http://www.w3.org/TR/2013/WD-wai-aria-practices-20130307/) that checkboxes that have logical grouping should be children of a DOM element with group role.
> 
> Indeed it does. If you have a bunch of checkboxes all related to foo, grouping those checkboxes together into a "foo" group makes sense. BUT I don't think that's relevant to the problem at hand.
> 
> 1. Just because you have a "foo" group of checkboxes does not necessarily mean you have an associated "toggle all foos" uber tri-state checkbox.

> 
> 2. If you happen to have an associated "toggle all foos" uber tri-state checkbox, there is no specification (that I have found) indicating where that uber checkbox must go. (Nor should there be IMHO.)

Thank you for your valuable clarification. It seems that there could be more then one tri-state object as well as other checkboxes in a group that are just standalone elements and not associated in any relation.

> Getting back to the topic at hand, I have what I hope is some good news for you: You should NOT have to do heuristics to figure out where the tri-state object is. When a checkbox-roled element (e.g. Lettuce) changes its state, it is up to the web developer to set the state of that checkbox-roled element. If that state change in turn causes some other checkbox (e.g. All condiments) to also change state, then it is up to the web developer to also set the state of that other checkbox-roled element (i.e. All condiments). It ain't your responsibility to sort out this mess, in other words. :) You should "just have" the checkbox and the state change. Just pass that information along via the platform accessibility API. :)

This is right, as soon as state-change comes I'm able to send proper notification via platform accessibility API for example (not-indeterminate->indeterminate) but when (indeterminate->not-indeterminate) comes without knowledge about this specific tri-state checkbox I guess, it's hard to send a notification that I think one should be associated to this tri-state element that (indeterminate is false).
I'm referring to this:
[check box | All condiments] indeterminate state changed to: False
[check box | All condiments] indeterminate state changed to: True

Existing API only provides the information that state has changed. There is no info about the previous state of the element. I guess some little heuristic should be done here to find this element. What do you think ?

> 
> Related aside #2: It appears that the UIUC checkbox 2 example sets the "mixed" state even when the mixed state has not changed. See adjCheckedCount(). I did not fix this in my updated version of the UIUC example. Apparently Gecko is filtering this non-change out and only emitting state-changed:indeterminate signals if the indeterminate state really has changed. While that is appreciated noise reduction, it is not clear to me whose bug/responsibility it is to only emit state-changed signals when the state has actually changed. I'm leaning towards this being an author error.... Anyhoo, let's separate that out from the originally filed bug here.
> 
> My question to you is: Using the modified (and downloaded) new test case, are you seeing WebCore Accessibility events for checkboxes which you can pass along unmodified via ATK? If the answer is "yes", please do so. :) If not, please elaborate.

I'm able to see Accessibility events for checkboxes with new test cases. I could pass them along with an available ATK API such as:
atk_object_notify_state_change(..., ATK_STATE_CHACKED/ATK_STATE_INDETERMINATE,...)
Still bother me this test case I mentioned before.

[Joanmarie Diggs]

(In reply to comment #26)

> Existing API only provides the information that state has changed. There is no info about the previous state of the element. I guess some little heuristic should be done here to find this element. What do you think ?

If you literally mean "find" that element, I think that's a bad idea -- and an unnecessary one: Given a tri-state checkbox foo, any time foo changes state -- for whatever reason, including bar changing state -- foo itself should notify WebKit that its state has changed. In other words, if bar changes state you should not try to find foo. Foo should come to you with its own web-app-emitted signal.

If, on the other hand, all you are told is "foo changed state to checked/unchecked" and you have no way of knowing if foo might be a tri-state that just lost state mixed/indeterminate.... a) That sucks. ;) b) Determining that foo is tri-state *might* require a heuristic.

Make sense?

[Krzysztof Czech]

(In reply to comment #27)
> (In reply to comment #26)
> 
> > Existing API only provides the information that state has changed. There is no info about the previous state of the element. I guess some little heuristic should be done here to find this element. What do you think ?
> 
> If you literally mean "find" that element, I think that's a bad idea -- and an unnecessary one: Given a tri-state checkbox foo, any time foo changes state -- for whatever reason, including bar changing state -- foo itself should notify WebKit that its state has changed. In other words, if bar changes state you should not try to find foo. Foo should come to you with its own web-app-emitted signal.
Exactly, this is how it works. Foo posted its own signal and then to find out what state it has, depends whether Foo is aria checkbox aria-checked attribute should be read (true/false/mixed).
> 
> If, on the other hand, all you are told is "foo changed state to checked/unchecked" and you have no way of knowing if foo might be a tri-state that just lost state mixed/indeterminate.... a) That sucks. ;) b) Determining that foo is tri-state *might* require a heuristic.
> 
> Make sense?

Yes, it makes sense. This is exactly the case that bothers me, how to know if foo might a tri-state object when lost its state mixed/indeterminate.

I guess the only way of knowing mixed/indeterminate is by reading aria-checked attribute that I mentioned before, so monitoring changes of aria-checked attribute will not solve this issue.

[Steve Faulkner]

I have been checking support for HTML indeterminate IDL attribute
test file http://stevefaulkner.github.io/html-mapping-tests/browser-tests/checkbox-states.html

as part of testing the implementation of acc layer in browsers: http://stevefaulkner.github.io/html-mapping-tests/

I have found that <input type=checkbox> indeterminate= true in firefox and Chrome on windows exposes as the MSAA mixed state. Having checked webkit latest build on Mac have found that while aria-checked=mixed is exposed as value=2, indeterminate= true is not.

Is this something I should file a new bug on?

[chris fleizach]

(In reply to comment #29)
> I have been checking support for HTML indeterminate IDL attribute
> test file http://stevefaulkner.github.io/html-mapping-tests/browser-tests/checkbox-states.html
> 
> as part of testing the implementation of acc layer in browsers: http://stevefaulkner.github.io/html-mapping-tests/
> 
> I have found that <input type=checkbox> indeterminate= true in firefox and Chrome on windows exposes as the MSAA mixed state. Having checked webkit latest build on Mac have found that while aria-checked=mixed is exposed as value=2, indeterminate= true is not.
> 
> Is this something I should file a new bug on?

Yes please

[Ferdinand Prantl]

May I asked about the state of this issue, please?

Safari is the only browser, which doesn't include the state "mixed" in the accessibility tree, when the property `indeterminate` is set to true on an input element of the type "checkbox". Both Chrome and Firefox do it and a screen reader can read the checkbox state properly. Safari doesn't and it causes more work for application developers, who have to support Safari by a custom checkbox element.

Can WebKit work the same like other browsers, which use two boolean properties to control the state in the accessibility tree?

    indeterminate = true  =>  "mixed"
    otherwise             =>  checked ? "checked" : "unchecked"