Bug 120322 – AX: Cancel button in search field not accessible.

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

[Samuel White]

The cancel button that becomes visible when an input element of type 'search' has text in it is not accessible.

<rdar://problem/14700018 >

[Samuel White]

Created attachment 209692 [details]
Preliminary patch for feedback.

No review needed, just looking for feedback. Thanks.

This button is rather hard to uniquely identify. For this patch I've used the shadowPseudoId which seems a bit fragile. Would adding isSearchFieldCancelButton() or something similar to Element be acceptable? I see other methods with this purpose:

isFormControlElement
isSpinButtonElement
isTextFormControl

I see that the way accessible text is derived is changing at some point (see the comment "Accessibility Text - (To be deprecated)" in AccessibilityObject.h). I've structured my implementation to satisfy all platforms (I believe) with the thought that the OVERRIDE macro will make sure my implementation is updated when this move finished. Thoughts?

[Babak Shafiei]

<rdar://problem/14700018>

[chris fleizach]

Comment on attachment 209692 [details]
Preliminary patch for feedback.

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

where's the change log?
also you need a layout test
i've submitted to EWS so we'll find out which platforms break by not including the files in their make files

> Source/WebCore/accessibility/AccessibilitySearchFieldButtons.cpp:52
> +    if (!description.isEmpty())

since you know the description won't be empty (since it's a localized string) you could make this one line of code

> Source/WebCore/accessibility/AccessibilitySearchFieldButtons.cpp:58
> +    if (!m_renderer || !m_renderer->node() || !m_renderer->node()->isElementNode())

You should be able to ask for
Node* node = this->node()
directly and AXRenderObject should return m_renderer->node() for you

if you cache that value you'll also save a few calls

> Source/WebCore/accessibility/AccessibilitySearchFieldButtons.cpp:63
> +    element->accessKeyAction(true);

you should probably add a comment explaining why you need to call setHovered

> Source/WebCore/accessibility/AccessibilitySearchFieldButtons.cpp:69
> +    return ButtonRole;

this can be moved into the header

> Source/WebCore/accessibility/AccessibilitySearchFieldButtons.cpp:74
> +    if (!m_renderer || !m_renderer->style() || m_renderer->style()->visibility() != VISIBLE)

is this the default accessibilityIsIgnored value? is there a baseAccessibilityIsIgnored() that you can call?

[chris fleizach]

Comment on attachment 209692 [details]
Preliminary patch for feedback.

otherwise looking good

[EFL EWS Bot]

Comment on attachment 209692 [details]
Preliminary patch for feedback.

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

[EFL EWS Bot]

Comment on attachment 209692 [details]
Preliminary patch for feedback.

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

[kov's GTK+ EWS bot]

Comment on attachment 209692 [details]
Preliminary patch for feedback.

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

[Build Bot]

Comment on attachment 209692 [details]
Preliminary patch for feedback.

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

[Samuel White]

Created attachment 209796 [details]
Updated patch.

Thanks for the feedback Chris. I've updated the patch as requested with one exception about visibility.

It looks to me like accessibilityIsIgnored calls into computeAccessibilityIsIgnored. So this seems like the correct place to make visibility additions.

[chris fleizach]

Comment on attachment 209796 [details]
Updated patch.

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

> Source/WebCore/accessibility/AccessibilitySearchFieldButtons.cpp:69
> +    if (!m_renderer || !m_renderer->style() || m_renderer->style()->visibility() != VISIBLE)

i think this (visibility check) is already handled by
accessibilityIsIgnoredByDefault(), which calls defaultObjectInclusion(), which has the appropriate check in AXRenderObject

> LayoutTests/platform/mac/accessibility/search-field-cancel-button.html:21
> +        accessibilityController.accessibleElementById("search").childAtIndex(1).press();

you should also output the role and role description for this object as part of your test

> LayoutTests/platform/mac/accessibility/search-field-cancel-button.html:25
> +    }

i think you're missing a succesfullyParsed = true; call at the bottom. that should be present in other tests

[EFL EWS Bot]

Comment on attachment 209796 [details]
Updated patch.

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

[EFL EWS Bot]

Comment on attachment 209796 [details]
Updated patch.

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

[Samuel White]

Created attachment 209815 [details]
Fixing localization.

[Samuel White]

(In reply to comment #10)
> (From update of attachment 209796 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=209796&action=review
> 
> > Source/WebCore/accessibility/AccessibilitySearchFieldButtons.cpp:69
> > +    if (!m_renderer || !m_renderer->style() || m_renderer->style()->visibility() != VISIBLE)
> 
> i think this (visibility check) is already handled by
> accessibilityIsIgnoredByDefault(), which calls defaultObjectInclusion(), which has the appropriate check in AXRenderObject

Good catch, thanks.

> 
> > LayoutTests/platform/mac/accessibility/search-field-cancel-button.html:21
> > +        accessibilityController.accessibleElementById("search").childAtIndex(1).press();
> 
> you should also output the role and role description for this object as part of your test
> 

Added.

> > LayoutTests/platform/mac/accessibility/search-field-cancel-button.html:25
> > +    }
> 
> i think you're missing a succesfullyParsed = true; call at the bottom. that should be present in other tests

This is handled in the post script that runs.

[chris fleizach]

Comment on attachment 209815 [details]
Fixing localization.

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

> LayoutTests/platform/mac/accessibility/search-field-cancel-button-expected.txt:8
> +PASS button.roleDescription is 'AXRoleDescription: button'

And i forgot to say you should also output the description to ensure that it stays as cancel. 
pretty much any functionality you add we should test to ensure it doesn't break

[Build Bot]

Comment on attachment 209815 [details]
Fixing localization.

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

[Samuel White]

Created attachment 209905 [details]
Updated patch with build fix.

Updated layout test and fixed build issue (hopefully). Will mark for review if everything is green.

[WebKit Commit Bot]

Comment on attachment 209905 [details]
Updated patch with build fix.

Clearing flags on attachment: 209905

Committed r154778: <http://trac.webkit.org/changeset/154778>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[David Kilzer (:ddkilzer)]

(In reply to comment #18)
> (From update of attachment 209905 [details])
> Clearing flags on attachment: 209905
> 
> Committed r154778: <http://trac.webkit.org/changeset/154778>

iOS build fix in r154812:  <http://trac.webkit.org/r154812>