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

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=123889





--- Comment #5 from Krzysztof Czech <k.czech at samsung.com>  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.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.