[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 #4 from Mario Sanchez Prada <mario at webkit.org>  2014-01-15 07:04:51 PST ---
(From update of attachment 221262)
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.

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