The cancel button that becomes visible when an input element of type 'search' has text in it is not accessible. <rdar://problem/14700018 >
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?
<rdar://problem/14700018>
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?
Comment on attachment 209692 [details] Preliminary patch for feedback. otherwise looking good
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
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
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
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
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.
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
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
Comment on attachment 209796 [details] Updated patch. Attachment 209796 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1619053
Created attachment 209815 [details] Fixing localization.
(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.
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
Comment on attachment 209815 [details] Fixing localization. Attachment 209815 [details] did not pass win-ews (win): Output: http://webkit-queues.appspot.com/results/1591528
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.
Comment on attachment 209905 [details] Updated patch with build fix. Clearing flags on attachment: 209905 Committed r154778: <http://trac.webkit.org/changeset/154778>
All reviewed patches have been landed. Closing bug.
(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>