Bug 123889 - [ATK] "Indeterminate" state and state changes in tri-state checkboxes should be exposed to ATs
Summary: [ATK] "Indeterminate" state and state changes in tri-state checkboxes should ...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-11-06 05:16 PST by Joanmarie Diggs (irc: joanie)
Modified: 2017-03-11 10:46 PST (History)
19 users (show)

See Also:


Attachments
accessible-event listener (666 bytes, text/x-python)
2013-11-06 05:16 PST, Joanmarie Diggs (irc: joanie)
no flags Details
patch (2.71 KB, patch)
2014-01-15 06:00 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff
patch (deleted)
2014-02-06 09:16 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff
patch (13.79 KB, patch)
2014-02-13 06:23 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff
patch (13.86 KB, patch)
2014-02-13 06:33 PST, Krzysztof Czech
no flags Details | Formatted Diff | Diff
patch (8.47 KB, patch)
2014-02-14 03:01 PST, Krzysztof Czech
cfleizach: review-
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-01 for mac-mountainlion (486.21 KB, application/zip)
2014-02-14 04:23 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2 (503.13 KB, application/zip)
2014-02-14 04:53 PST, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion (485.83 KB, application/zip)
2014-02-14 05:21 PST, Build Bot
no flags Details
Modified version of the UIUC checkbox 2 test (7.17 KB, text/html)
2014-02-14 20:29 PST, Joanmarie Diggs (irc: joanie)
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2013-11-06 05:16:12 PST
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
Comment 1 Radar WebKit Bug Importer 2013-11-06 05:16:49 PST
<rdar://problem/15403059>
Comment 2 Krzysztof Czech 2014-01-15 06:00:48 PST
Created attachment 221262 [details]
patch
Comment 3 Krzysztof Czech 2014-01-15 06:09:59 PST
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.
Comment 4 Mario Sanchez Prada 2014-01-15 07:04:51 PST
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.
Comment 5 Krzysztof Czech 2014-01-16 02:52:40 PST
(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.
Comment 6 Krzysztof Czech 2014-02-06 09:16:14 PST
Created attachment 223338 [details]
patch
Comment 7 chris fleizach 2014-02-06 09:48:58 PST
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
Comment 8 Krzysztof Czech 2014-02-13 06:22:11 PST
> > 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.
Comment 9 Krzysztof Czech 2014-02-13 06:23:00 PST
Created attachment 224064 [details]
patch
Comment 10 WebKit Commit Bot 2014-02-13 06:25:03 PST
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.
Comment 11 Krzysztof Czech 2014-02-13 06:33:08 PST
Created attachment 224065 [details]
patch
Comment 12 chris fleizach 2014-02-13 09:25:56 PST
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
Comment 13 Krzysztof Czech 2014-02-14 00:46:26 PST
(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.
Comment 14 Krzysztof Czech 2014-02-14 03:01:50 PST
Created attachment 224190 [details]
patch
Comment 15 Krzysztof Czech 2014-02-14 03:16:43 PST
(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.
Comment 16 Build Bot 2014-02-14 04:23:28 PST
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
Comment 17 Build Bot 2014-02-14 04:23:31 PST
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
Comment 18 Build Bot 2014-02-14 04:53:45 PST
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
Comment 19 Build Bot 2014-02-14 04:53:49 PST
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
Comment 20 Build Bot 2014-02-14 05:21:37 PST
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
Comment 21 Build Bot 2014-02-14 05:21:43 PST
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
Comment 22 Mario Sanchez Prada 2014-02-14 05:24:11 PST
(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
Comment 23 chris fleizach 2014-02-14 09:02:40 PST
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()) {
Comment 24 Joanmarie Diggs (irc: joanie) 2014-02-14 20:29:37 PST
Created attachment 224278 [details]
Modified version of the UIUC checkbox 2 test

(Discussion to follow)
Comment 25 Joanmarie Diggs (irc: joanie) 2014-02-14 21:25:15 PST
(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.
Comment 26 Krzysztof Czech 2014-02-17 03:03:45 PST
(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.
Comment 27 Joanmarie Diggs (irc: joanie) 2014-02-17 03:52:54 PST
(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?
Comment 28 Krzysztof Czech 2014-02-17 05:14:17 PST
(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.
Comment 29 Steve Faulkner 2014-07-01 05:15:03 PDT
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?
Comment 30 chris fleizach 2014-07-01 08:51:46 PDT
(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