Bug 174965 - AX: findMatchingObjects doesn't work when the startObject is ignored
Summary: AX: findMatchingObjects doesn't work when the startObject is ignored
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-07-28 19:40 PDT by Nan Wang
Modified: 2017-07-29 03:27 PDT (History)
11 users (show)

See Also:


Attachments
patch (6.44 KB, patch)
2017-07-28 19:46 PDT, Nan Wang
no flags Details | Formatted Diff | Diff
patch (6.39 KB, patch)
2017-07-29 02:39 PDT, Nan Wang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nan Wang 2017-07-28 19:40:58 PDT
When we pass an ignored object as the start object to findMatchingObjects, we are getting either the first element or last element of the container.

<rdar://problem/31823926>
Comment 1 Nan Wang 2017-07-28 19:46:21 PDT
Created attachment 316694 [details]
patch
Comment 2 chris fleizach 2017-07-29 00:38:55 PDT
Comment on attachment 316694 [details]
patch

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:615
> +    if (startObject && searchPosition == WTF::notFound && childrenSize > 0) {

what does childrenSize == 0 mean in this context? it seems like we would still want to go up to find a valid start element

> Source/WebCore/accessibility/AccessibilityObject.cpp:617
> +        if (startObject->isDescendantOfObject(object) && startObject->accessibilityIsIgnored()) {

does it matter that it's ignored? It's already not found in the children list right?

> Source/WebCore/accessibility/AccessibilityObject.cpp:618
> +            AccessibilityObject* searchObject = startObject;

if you made searchObject = startObject->parent(), then your next while loop just looks like

while (searchObject && ...) which is a bit clearer

> Source/WebCore/accessibility/AccessibilityObject.cpp:620
> +            while (searchObject->parentObject() && searchObject->parentObject()->accessibilityIsIgnored())

do you want to stop if searchObject == object? if you go higher than that then searchChildren wouldn't be correct

> Source/WebCore/accessibility/AccessibilityObject.cpp:625
> +            searchPosition = searchObject ? searchChildren.find(searchObject) : WTF::notFound;

this is the same line as 613

I wonder if you could delete this line, move line 613 here and then your first if check is just
if (startObject && startObject->isIgnored()), because we know if it's ignore, searchPos == WTF::NotFound
Comment 3 Nan Wang 2017-07-29 01:59:33 PDT
Comment on attachment 316694 [details]
patch

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

>> Source/WebCore/accessibility/AccessibilityObject.cpp:615
>> +    if (startObject && searchPosition == WTF::notFound && childrenSize > 0) {
> 
> what does childrenSize == 0 mean in this context? it seems like we would still want to go up to find a valid start element

The function is appendChildrenToArray, I think if childrenSize == 0 means we don't want to append anything into this array.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:617
>> +        if (startObject->isDescendantOfObject(object) && startObject->accessibilityIsIgnored()) {
> 
> does it matter that it's ignored? It's already not found in the children list right?

Probably right.

>> Source/WebCore/accessibility/AccessibilityObject.cpp:620
>> +            while (searchObject->parentObject() && searchObject->parentObject()->accessibilityIsIgnored())
> 
> do you want to stop if searchObject == object? if you go higher than that then searchChildren wouldn't be correct

Ok I'll add that

>> Source/WebCore/accessibility/AccessibilityObject.cpp:625
>> +            searchPosition = searchObject ? searchChildren.find(searchObject) : WTF::notFound;
> 
> this is the same line as 613
> 
> I wonder if you could delete this line, move line 613 here and then your first if check is just
> if (startObject && startObject->isIgnored()), because we know if it's ignore, searchPos == WTF::NotFound

Ok
Comment 4 Nan Wang 2017-07-29 02:39:52 PDT
Created attachment 316708 [details]
patch

update from review
Comment 5 WebKit Commit Bot 2017-07-29 03:27:04 PDT
Comment on attachment 316708 [details]
patch

Clearing flags on attachment: 316708

Committed r220042: <http://trac.webkit.org/changeset/220042>
Comment 6 WebKit Commit Bot 2017-07-29 03:27:05 PDT
All reviewed patches have been landed.  Closing bug.