Bug 115491 – Add "VisibleOnly" key to search predicate

• Component: Accessibility
• Status: RESOLVED FIXED

[Greg Hughes]

For assistive products it would be much more effecient if the search predicate accepted a "visibleOnly" key with a boolean so that the search was limited to items that were scrolled into view.

Without this, clients need to constantly ask for the "next" object checking if it is visible. On large pages this can mean tens of thoughts of isVisible requests.

[Greg Hughes]

<rdar://13623364>

[Greg Hughes]

Created attachment 200247 [details]
Patch 13623364.1

[Greg Hughes]

Found a problem with my first patch, working on another

[EFL EWS Bot]

Comment on attachment 200247 [details]
Patch 13623364.1

Attachment 200247 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/377118

[EFL EWS Bot]

Comment on attachment 200247 [details]
Patch 13623364.1

Attachment 200247 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/370127

[Build Bot]

Comment on attachment 200247 [details]
Patch 13623364.1

Attachment 200247 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/353157

[Greg Hughes]

Created attachment 200257 [details]
patch 13623364.2

[Build Bot]

Comment on attachment 200257 [details]
patch 13623364.2

Attachment 200257 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/392060

New failing tests:
platform/mac/accessibility/search-when-element-starts-in-table.html
platform/mac/accessibility/attributed-string-includes-highlighting.html
platform/mac/accessibility/search-predicate.html

[Build Bot]

Created attachment 200303 [details]
Archive of layout-test-results from webkit-ews-05 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-05  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Build Bot]

Comment on attachment 200257 [details]
patch 13623364.2

Attachment 200257 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/360108

New failing tests:
platform/mac/accessibility/search-when-element-starts-in-table.html
platform/mac/accessibility/attributed-string-includes-highlighting.html
platform/mac/accessibility/search-predicate.html

[Build Bot]

Created attachment 200305 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Greg Hughes]

Created attachment 201625 [details]
13623364.3 patch v3

[EFL EWS Bot]

Comment on attachment 201625 [details]
13623364.3 patch v3

Attachment 201625 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/451895

[Greg Hughes]

Created attachment 201627 [details]
Patch v4

[Early Warning System Bot]

Comment on attachment 201627 [details]
Patch v4

Attachment 201627 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/464389

[EFL EWS Bot]

Comment on attachment 201627 [details]
Patch v4

Attachment 201627 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/375883

[EFL EWS Bot]

Comment on attachment 201627 [details]
Patch v4

Attachment 201627 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/346896

[Early Warning System Bot]

Comment on attachment 201627 [details]
Patch v4

Attachment 201627 [details] did not pass qt-wk2-ews (qt-wk2):
Output: http://webkit-queues.appspot.com/results/466353

[Build Bot]

Comment on attachment 201627 [details]
Patch v4

Attachment 201627 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/427960

[Build Bot]

Comment on attachment 201627 [details]
Patch v4

Attachment 201627 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/464390

[kov's GTK+ EWS bot]

Comment on attachment 201627 [details]
Patch v4

Attachment 201627 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/460837

[Build Bot]

Comment on attachment 201627 [details]
Patch v4

Attachment 201627 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/471001

[Greg Hughes]

Created attachment 201766 [details]
Patch v5, now with layout tests

[Greg Hughes]

Created attachment 202136 [details]
Patch v 5.1

[Greg Hughes]

Created attachment 202137 [details]
Patch v 5.2

[chris fleizach]

Comment on attachment 202137 [details]
Patch v 5.2

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

in the isOnscreen() method I think you can iterate through the parent objects just once and check whether the bounds of the object are within each parent

> Source/WebCore/accessibility/AccessibilityObject.cpp:1733
> +    // Search up the parent chain and create a vector of all scrollable parent objects

Comments should explain why you need to do this (instead of explaining what you're doing)

> Source/WebCore/accessibility/AccessibilityObject.cpp:1738
> +    for (AccessibilityObject* parentObject = this->parentObject(); parentObject; parentObject = parentObject->parentObject()) {

i think you can start the for loop with this, so that you don't have to append this before hand

> Source/WebCore/accessibility/AccessibilityObject.cpp:1743
> +    objects.reverse();

you should iterate from count -> 0 instead of incurring the cost to reverse

> Source/WebCore/accessibility/AccessibilityObject.cpp:1746
> +    // visible bounds of the outer object

comments should end in a period

> Source/WebCore/accessibility/AccessibilityObject.cpp:1752
> +        const IntRect innerRect = pixelSnappedIntRect(inner->isAccessibilityScrollView() ? inner->parentObject()->boundingBoxRect() : inner->boundingBoxRect());

is there a need for using pixelSnappedIntRect?

> Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:921
> +        [parameter setObject:[NSNumber numberWithBool:true] forKey:@"AXVisibleOnly"];    

you should use YES instead of true here

> Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:931
> +        [parameter setObject:[NSNumber numberWithBool:true] forKey:@"AXVisibleOnly"];    

you should use YES instead of true

[Greg Hughes]

(In reply to comment #26)
> (From update of attachment 202137 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=202137&action=review
> 
> in the isOnscreen() method I think you can iterate through the parent objects just once and check whether the bounds of the object are within each parent

No, because in each check you need to check the current scroll view to the next scroll view, that is why the first loop goes through to find all of the scroll views. The code is much more readable this way rather than in one look try to do both tasks.

> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:1733
> > +    // Search up the parent chain and create a vector of all scrollable parent objects
> 
> Comments should explain why you need to do this (instead of explaining what you're doing)

OK

> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:1738
> > +    for (AccessibilityObject* parentObject = this->parentObject(); parentObject; parentObject = parentObject->parentObject()) {
> 
> i think you can start the for loop with this, so that you don't have to append this before hand

No, because we need “this", and then every scrollable view in our stack of objects, that is why “this” is added first to the stack. I will clarify in the comment.

> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:1743
> > +    objects.reverse();
> 
> you should iterate from count -> 0 instead of incurring the cost to reverse
Ok

> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:1746
> > +    // visible bounds of the outer object
> 
> comments should end in a period

OK

> 
> > Source/WebCore/accessibility/AccessibilityObject.cpp:1752
> > +        const IntRect innerRect = pixelSnappedIntRect(inner->isAccessibilityScrollView() ? inner->parentObject()->boundingBoxRect() : inner->boundingBoxRect());
> 
> is there a need for using pixelSnappedIntRect?

Yes, because we want to know if it is onscreen so we have to snap to pixels

> 
> > Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm:921
> > +        [parameter setObject:[NSNumber numberWithBool:true] forKey:@"AXVisibleOnly"];    
> 
> you should use YES instead of true here
OK

> 
> > Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm:931
> > +        [parameter setObject:[NSNumber numberWithBool:true] forKey:@"AXVisibleOnly"];    
> 
> you should use YES instead of true
OK

[Greg Hughes]

Created attachment 202625 [details]
Patch v6

[Greg Hughes]

Created attachment 202626 [details]
Patch v6.1

[WebKit Commit Bot]

Attachment 202626 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/platform/mac/accessibility/attributed-string-includes-highlighting.html', u'LayoutTests/platform/mac/accessibility/search-predicate-expected.txt', u'LayoutTests/platform/mac/accessibility/search-predicate.html', u'LayoutTests/platform/mac/accessibility/search-when-element-starts-in-table.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/AccessibilityObject.cpp', u'Source/WebCore/accessibility/AccessibilityObject.h', u'Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm', u'Tools/ChangeLog', u'Tools/DumpRenderTree/AccessibilityUIElement.cpp', u'Tools/DumpRenderTree/AccessibilityUIElement.h', u'Tools/DumpRenderTree/atk/AccessibilityUIElementAtk.cpp', u'Tools/DumpRenderTree/ios/AccessibilityUIElementIOS.mm', u'Tools/DumpRenderTree/mac/AccessibilityUIElementMac.mm', u'Tools/DumpRenderTree/win/AccessibilityUIElementWin.cpp', u'Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.cpp', u'Tools/WebKitTestRunner/InjectedBundle/AccessibilityUIElement.h', u'Tools/WebKitTestRunner/InjectedBundle/Bindings/AccessibilityUIElement.idl', u'Tools/WebKitTestRunner/InjectedBundle/atk/AccessibilityUIElementAtk.cpp', u'Tools/WebKitTestRunner/InjectedBundle/mac/AccessibilityUIElementMac.mm']" exit_code: 1
Source/WebCore/accessibility/AccessibilityObject.cpp:1740:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 21 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Greg Hughes]

Created attachment 202628 [details]
Patch v7

[chris fleizach]

Comment on attachment 202628 [details]
Patch v7

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

> Source/WebCore/accessibility/AccessibilityObject.cpp:1733
> +    // To figure out if the element is onscreen, we start by boulding of a stack starting with the

boulding -> building ( i presume)

[Greg Hughes]

Created attachment 203073 [details]
Patch v8

[WebKit Commit Bot]

Comment on attachment 203073 [details]
Patch v8

Clearing flags on attachment: 203073

Committed r151179: <http://trac.webkit.org/changeset/151179>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.