AX: [Mac] better accessibility support for Summary elements

[Nan Wang]

- Summary element should be exposed as some form of AXButton
  Mapping: AXButton/AXSummary/summary
- Summary element should use its text node as AXTitle

[Radar WebKit Bug Importer]

<rdar://problem/28745010>

[Nan Wang]

Created attachment 291415 [details]
patch

[chris fleizach]

Comment on attachment 291415 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:2483
> +        for (AccessibilityObject* parentObject = this->parentObject(); parentObject; parentObject = parentObject->parentObject()) {

do we have an enclosure for this?

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1413
> +    if (m_object->supportsExpanded() || m_object->isSummary())

should supportsExpanded() also include isSummary()?

[Nan Wang]

Comment on attachment 291415 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityObject.cpp:2483
>> +        for (AccessibilityObject* parentObject = this->parentObject(); parentObject; parentObject = parentObject->parentObject()) {
> 
> do we have an enclosure for this?

we don't in this class.

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1413
>> +    if (m_object->supportsExpanded() || m_object->isSummary())
> 
> should supportsExpanded() also include isSummary()?

I think it's the detail element that actually has the expanded status. I'm not sure how other platform is doing this, so putting here to only affect mac.

[Nan Wang]

Created attachment 291460 [details]
patch

Added the enclosure for finding the parent object

[chris fleizach]

Comment on attachment 291460 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:463
> +    return AccessibilityObject::matchedParent(const_cast<AccessibilityObject*>(this), false, matchFunc);

can we write these enclosures inline to save space?

return AccessibilityObject::matchedParent(const_cast<AccessibilityObject*>(this), false, [](AccessibilityObject* object) {
 461        return !object->accessibilityIsIgnored();
 462    });

[Nan Wang]

Created attachment 291461 [details]
patch

updated style

[WebKit Commit Bot]

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


ERROR: Source/WebCore/accessibility/AccessibilityObject.cpp:460:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.cpp:1696:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.cpp:1790:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.cpp:2315:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.cpp:2484:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.cpp:2945:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.cpp:3043:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebCore/accessibility/AccessibilityObject.cpp:3049:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 8 in 10 files


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

[Nan Wang]

Created attachment 291462 [details]
patch

updated style again.

[Nan Wang]

Created attachment 291464 [details]
patch

Oops.. missed something

[Darin Adler]

Comment on attachment 291464 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.h:1066
> +    static AccessibilityObject* matchedParent(AccessibilityObject*, bool includeSelf, const std::function<bool(AccessibilityObject*)>&);

I don’t think the types are quite right here. I think we should probably use const versions and take references. The passed in function should take an AccesibilityObject&, not an AccessibilityObject*.

    static const AccessibilityObject* matchedParent(const AccessibilityObject&, bool includeSelf, const std::function<bool(const AccessibilityObject&)>&);

And if we really need a non-const version (I am not sure we do):

    static AccessibilityObject* matchedParent(AccessibilityObject&, bool includeSelf, const std::function<bool(const AccessibilityObject&)>&);

[Nan Wang]

Created attachment 291515 [details]
patch for landing

Updated the types based on the review.
Changed lots of code so uploaded a patch to see if everything still works.

[WebKit Commit Bot]

Comment on attachment 291515 [details]
patch for landing

Clearing flags on attachment: 291515

Committed r207314: <http://trac.webkit.org/changeset/207314>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.