Bug 200559

Summary: Accessibility client cannot navigate to internal links targets on iOS.
Product: WebKit Reporter: Andres Gonzalez <andresg_22>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, darin, dbates, dmazzoni, ews-watchlist, jcraig, jdiggs, pvollan, samuel_white, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Andres Gonzalez 2019-08-08 19:46:52 PDT
Accessibility client cannot navigate to internal links targets on iOS.
Comment 1 Andres Gonzalez 2019-08-08 20:04:49 PDT
Created attachment 375884 [details]
Patch
Comment 2 Darin Adler 2019-08-09 09:46:02 PDT
Comment on attachment 375884 [details]
Patch

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

Looks good.

> Source/WebCore/accessibility/AccessibilityObject.cpp:545
> +    return WebCore::firstAccessibleObjectFromNode(node, [] (const AccessibilityObject& acc) {
> +        return !acc.accessibilityIsIgnored();
> +    });

WebKit coding style says we should use words, not abbreviations, for variable names. So the name here could be "object", "accessibilityObject", "candidate", or some other word, but shouldn’t be "acc".

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1816
> +    /* AccessibilityObject::linkedUIElements may return an object that is
> +     exposed in other platforms but not on iOS, i.e., grouping or structure
> +     elements like <div> or <p>. Thus find the next accessible object that is
> +     exposed on iOS.
> +     */

WebKit coding style calls for "//" comments, not "/*" comments.

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1820
> +    if (auto linked = firstAccessibleObjectFromNode(linkedElements[0]->node(), [] (const AccessibilityObject& acc) {
> +        return acc.wrapper().isAccessibilityElement;
> +    }))
> +        return linked->wrapper();

Same issue with variable named "acc".

Formatting here makes this hard to read. Might want to rearrange to avoid that. One way to do it is:

    auto linkedElement = firstAccessibleObjectFromNode(linkedElements[0]->node(), [] (const AccessibilityObject& object) {
        return object.wrapper().isAccessibilityElement;
    });
    return linkedElement ? linkedElement->wrapper() : nullptr;

But I’m sure there are other ways to sidestep it too.
Comment 3 Andres Gonzalez 2019-08-09 13:09:25 PDT
Created attachment 375944 [details]
Patch
Comment 4 Andres Gonzalez 2019-08-09 13:20:18 PDT
(In reply to Darin Adler from comment #2)
> Comment on attachment 375884 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=375884&action=review
> 
> Looks good.
> 
Thanks very much Darin, for the prompt review and good points.

> > Source/WebCore/accessibility/AccessibilityObject.cpp:545
> > +    return WebCore::firstAccessibleObjectFromNode(node, [] (const AccessibilityObject& acc) {
> > +        return !acc.accessibilityIsIgnored();
> > +    });
> 
> WebKit coding style says we should use words, not abbreviations, for
> variable names. So the name here could be "object", "accessibilityObject",
> "candidate", or some other word, but shouldn’t be "acc".
> 
Changed it too accessible.

> > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1816
> > +    /* AccessibilityObject::linkedUIElements may return an object that is
> > +     exposed in other platforms but not on iOS, i.e., grouping or structure
> > +     elements like <div> or <p>. Thus find the next accessible object that is
> > +     exposed on iOS.
> > +     */
> 
> WebKit coding style calls for "//" comments, not "/*" comments.
> 
Changed it to // comment. I personally prefer /* */ for multiline comments, so that I don't have to hear "slash slash" before every line of text. But I will of course adhere to the WebKit coding style conventions.

> > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1820
> > +    if (auto linked = firstAccessibleObjectFromNode(linkedElements[0]->node(), [] (const AccessibilityObject& acc) {
> > +        return acc.wrapper().isAccessibilityElement;
> > +    }))
> > +        return linked->wrapper();
> 
> Same issue with variable named "acc".
> 
> Formatting here makes this hard to read. Might want to rearrange to avoid
> that. One way to do it is:
> 
>     auto linkedElement =
> firstAccessibleObjectFromNode(linkedElements[0]->node(), [] (const
> AccessibilityObject& object) {
>         return object.wrapper().isAccessibilityElement;
>     });
>     return linkedElement ? linkedElement->wrapper() : nullptr;
> 
> But I’m sure there are other ways to sidestep it too.
Thanks! agree this way is much more legible. Done.
Comment 5 WebKit Commit Bot 2019-08-10 20:46:16 PDT
Comment on attachment 375944 [details]
Patch

Clearing flags on attachment: 375944

Committed r248513: <https://trac.webkit.org/changeset/248513>
Comment 6 WebKit Commit Bot 2019-08-10 20:46:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 7 Radar WebKit Bug Importer 2019-08-10 20:47:16 PDT
<rdar://problem/54173330>