Summary: | Accessibility client cannot navigate to internal links targets on iOS. | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andres Gonzalez <andresg_22> | ||||||
Component: | New Bugs | Assignee: | 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
Andres Gonzalez
2019-08-08 19:46:52 PDT
Created attachment 375884 [details]
Patch
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. Created attachment 375944 [details]
Patch
(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 on attachment 375944 [details] Patch Clearing flags on attachment: 375944 Committed r248513: <https://trac.webkit.org/changeset/248513> All reviewed patches have been landed. Closing bug. |