Bug 155554 – AX: attributes to retrieve focusable and editable ancestors

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Doug Russell]

Add a simple way to retrieve the focusable or editable element containing an element (possibly that element itself) in order to better make decisions about notifying AT users about focus changes.

[Radar WebKit Bug Importer]

<rdar://problem/25198311>

[Doug Russell]

Created attachment 274211 [details]
Patch

[Build Bot]

Comment on attachment 274211 [details]
Patch

Attachment 274211 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/990314

New failing tests:
accessibility/plugin.html
accessibility/mac/bounds-for-range.html
accessibility/image-link.html
accessibility/table-attributes.html
accessibility/mac/internal-link-anchors.html
accessibility/mac/document-links.html
accessibility/table-sections.html
accessibility/math-multiscript-attributes.html
accessibility/table-one-cell.html
accessibility/table-detection.html
accessibility/table-cells.html
accessibility/image-map2.html
accessibility/table-cell-spans.html
accessibility/table-with-rules.html
accessibility/transformed-element.html
accessibility/mac/aria-columnrowheaders.html
accessibility/internal-link-anchors2.html
accessibility/lists.html

[Build Bot]

Created attachment 274213 [details]
Archive of layout-test-results from ews102 for mac-yosemite

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

[Build Bot]

Comment on attachment 274211 [details]
Patch

Attachment 274211 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/990326

New failing tests:
accessibility/plugin.html
accessibility/mac/bounds-for-range.html
accessibility/image-link.html
accessibility/table-attributes.html
accessibility/mac/internal-link-anchors.html
accessibility/mac/document-links.html
accessibility/table-sections.html
accessibility/math-multiscript-attributes.html
accessibility/table-one-cell.html
accessibility/table-detection.html
accessibility/table-cells.html
accessibility/image-map2.html
accessibility/table-cell-spans.html
accessibility/table-with-rules.html
accessibility/transformed-element.html
accessibility/mac/aria-columnrowheaders.html
accessibility/internal-link-anchors2.html
accessibility/lists.html

[Build Bot]

Created attachment 274215 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 274211 [details]
Patch

Attachment 274211 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/990334

New failing tests:
accessibility/plugin.html
accessibility/mac/bounds-for-range.html
accessibility/image-link.html
accessibility/table-attributes.html
accessibility/mac/internal-link-anchors.html
accessibility/mac/document-links.html
accessibility/table-sections.html
accessibility/math-multiscript-attributes.html
accessibility/table-one-cell.html
accessibility/table-detection.html
accessibility/table-cells.html
accessibility/image-map2.html
accessibility/table-cell-spans.html
accessibility/table-with-rules.html
accessibility/transformed-element.html
accessibility/mac/aria-columnrowheaders.html
accessibility/internal-link-anchors2.html
accessibility/lists.html

[Build Bot]

Created attachment 274218 [details]
Archive of layout-test-results from ews117 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Doug Russell]

Created attachment 274244 [details]
Patch

[chris fleizach]

(In reply to comment #9)
> Created attachment 274244 [details]
> Patch

what is the purpose of this patch? what will it be used for?

this looks like it also add a lot of extra cycles to web retrieval, if these are now three new attributes on every element, that will run up the parent chain independently for every call

[Doug Russell]

(In reply to comment #10)
> (In reply to comment #9)
> > Created attachment 274244 [details]
> > Patch
> 
> what is the purpose of this patch? what will it be used for?
> 
> this looks like it also add a lot of extra cycles to web retrieval, if these
> are now three new attributes on every element, that will run up the parent
> chain independently for every call

The purpose is to simplify the logic that decides if a focus change should be alerted to a user or if we've simply moved between focusable subelements.

[chris fleizach]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Created attachment 274244 [details]
> > > Patch
> > 
> > what is the purpose of this patch? what will it be used for?
> > 
> > this looks like it also add a lot of extra cycles to web retrieval, if these
> > are now three new attributes on every element, that will run up the parent
> > chain independently for every call
> 
> The purpose is to simplify the logic that decides if a focus change should
> be alerted to a user or if we've simply moved between focusable subelements.

and that is happening inside VoiceOver now?

Will VO fetch these on demand or automatically for all elements?

[Doug Russell]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > Created attachment 274244 [details]
> > > > Patch
> > > 
> > > what is the purpose of this patch? what will it be used for?
> > > 
> > > this looks like it also add a lot of extra cycles to web retrieval, if these
> > > are now three new attributes on every element, that will run up the parent
> > > chain independently for every call
> > 
> > The purpose is to simplify the logic that decides if a focus change should
> > be alerted to a user or if we've simply moved between focusable subelements.
> 
> and that is happening inside VoiceOver now?

VoiceOver currently uses some unreliable heuristics based on roles and frames that this is meant to replace.

> 
> Will VO fetch these on demand or automatically for all elements?

Only on demand.

I think the only place where this is adding overhead is in the list of supported attributes growing by three entries and a couple more if statements in accessibilityAttributeValue

[chris fleizach]

Comment on attachment 274244 [details]
Patch

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

some review comments inside

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:253
> +    if (canSetFocusAttribute())

if you start your loop with "this" you can avoid this extra bit of code with no extra cost

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:268
> +    if (isTextControl())

ditto

> Source/WebCore/accessibility/AccessibilityObject.h:1043
>      

these seem like they can all go in AccessibilityObject.cpp and then they don't have to be virtual

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3335
> +    if (!m_object->isWebArea()) {

if its called on WebArea, it will just end up returning nil right? that seems ok doesn't it

[Doug Russell]

(In reply to comment #14)
> Comment on attachment 274244 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274244&action=review
> 
> some review comments inside
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:253
> > +    if (canSetFocusAttribute())
> 
> if you start your loop with "this" you can avoid this extra bit of code with
> no extra cost

Will do

> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:268
> > +    if (isTextControl())
> 
> ditto

Will do

> 
> > Source/WebCore/accessibility/AccessibilityObject.h:1043
> >      
> 
> these seem like they can all go in AccessibilityObject.cpp and then they
> don't have to be virtual

I'll see if I can do that.

> 
> > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:3335
> > +    if (!m_object->isWebArea()) {
> 
> if its called on WebArea, it will just end up returning nil right? that
> seems ok doesn't it

Correct. I can remove that check.

[Doug Russell]

Created attachment 274323 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 274323 [details]
Patch

Clearing flags on attachment: 274323

Committed r198356: <http://trac.webkit.org/changeset/198356>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.