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>
Created attachment 316694 [details] patch
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 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
Created attachment 316708 [details] patch update from review
Comment on attachment 316708 [details] patch Clearing flags on attachment: 316708 Committed r220042: <http://trac.webkit.org/changeset/220042>
All reviewed patches have been landed. Closing bug.