Bug 163367

Summary: AX: [Mac] better accessibility support for Summary elements
Product: WebKit Reporter: Nan Wang <n_wang>
Component: AccessibilityAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, jcraig, jdiggs, mario, n_wang, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
none
patch
none
patch
none
patch
none
patch
darin: review+
patch for landing none

Description Nan Wang 2016-10-12 16:21:06 PDT
- Summary element should be exposed as some form of AXButton
  Mapping: AXButton/AXSummary/summary
- Summary element should use its text node as AXTitle
Comment 1 Radar WebKit Bug Importer 2016-10-12 16:21:53 PDT
<rdar://problem/28745010>
Comment 2 Nan Wang 2016-10-12 16:39:20 PDT
Created attachment 291415 [details]
patch
Comment 3 chris fleizach 2016-10-12 17:02:39 PDT
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()?
Comment 4 Nan Wang 2016-10-12 17:06:22 PDT
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.
Comment 5 Nan Wang 2016-10-13 00:11:47 PDT
Created attachment 291460 [details]
patch

Added the enclosure for finding the parent object
Comment 6 chris fleizach 2016-10-13 00:21:41 PDT
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    });
Comment 7 Nan Wang 2016-10-13 00:33:36 PDT
Created attachment 291461 [details]
patch

updated style
Comment 8 WebKit Commit Bot 2016-10-13 00:35:58 PDT
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.
Comment 9 Nan Wang 2016-10-13 00:46:14 PDT
Created attachment 291462 [details]
patch

updated style again.
Comment 10 Nan Wang 2016-10-13 00:52:24 PDT
Created attachment 291464 [details]
patch

Oops.. missed something
Comment 11 Darin Adler 2016-10-13 12:20:54 PDT
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&)>&);
Comment 12 Nan Wang 2016-10-13 14:15:43 PDT
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.
Comment 13 WebKit Commit Bot 2016-10-13 15:54:55 PDT
Comment on attachment 291515 [details]
patch for landing

Clearing flags on attachment: 291515

Committed r207314: <http://trac.webkit.org/changeset/207314>
Comment 14 WebKit Commit Bot 2016-10-13 15:55:00 PDT
All reviewed patches have been landed.  Closing bug.