Bug 140326 – [Mac] AX: The input element with type="search" has no default focus outline

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

Unlike other types of input elements (e.g. type="text"), input elements with type="search" don’t show a visible default focus outline. The default focus outline is an essential accessibility feature and should be always visible.

<rdar://problem/19398333>

[chris fleizach]

Created attachment 244388 [details]
patch

[Build Bot]

Comment on attachment 244388 [details]
patch

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

New failing tests:
fast/css/focus-ring-exists-for-search-field.html

[Build Bot]

Created attachment 244391 [details]
Archive of layout-test-results from ews100 for mac-mountainlion

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

[chris fleizach]

Created attachment 244392 [details]
patch

[Darin Adler]

Comment on attachment 244392 [details]
patch

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

I’m going to say review+ but I have two substantial concerns:

This seems like a pretty big deal and would change how websites look; we’ve had this behaving like this for a long time. Do we have some examples of websites we looked at to vet this change not only preserves this feature to satisfy accessibility needs but also works well and looks good in the actual websites? I could even imagine contexts in apps or widgets where someone might have wanted to turn this off.

> Source/WebCore/rendering/RenderThemeMac.mm:1605
> +    if (isFocused(o) && o.style().outlineStyleIsAuto())
> +        wkDrawCellFocusRingWithFrameAtTime(search, NSRect(unzoomedRect), documentView, std::numeric_limits<double>::max());

RenderThemeMac::paintMenuList calls setFocusedElementNeedsRepaint. Is there a reason that is correct there but is not needed here?

[chris fleizach]

(In reply to comment #5)
> Comment on attachment 244392 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=244392&action=review
> 
> I’m going to say review+ but I have two substantial concerns:
> 
> This seems like a pretty big deal and would change how websites look; we’ve
> had this behaving like this for a long time. Do we have some examples of
> websites we looked at to vet this change not only preserves this feature to
> satisfy accessibility needs but also works well and looks good in the actual
> websites? I could even imagine contexts in apps or widgets where someone
> might have wanted to turn this off.

I'll do some browsing and post screenshots here of what it looks like

> 
> > Source/WebCore/rendering/RenderThemeMac.mm:1605
> > +    if (isFocused(o) && o.style().outlineStyleIsAuto())
> > +        wkDrawCellFocusRingWithFrameAtTime(search, NSRect(unzoomedRect), documentView, std::numeric_limits<double>::max());
> 
> RenderThemeMac::paintMenuList calls setFocusedElementNeedsRepaint. Is there
> a reason that is correct there but is not needed here?

I'll have to investigate what that does and why it did not seem necessary here

[chris fleizach]

> > 
> > > Source/WebCore/rendering/RenderThemeMac.mm:1605
> > > +    if (isFocused(o) && o.style().outlineStyleIsAuto())
> > > +        wkDrawCellFocusRingWithFrameAtTime(search, NSRect(unzoomedRect), documentView, std::numeric_limits<double>::max());
> > 
> > RenderThemeMac::paintMenuList calls setFocusedElementNeedsRepaint. Is there
> > a reason that is correct there but is not needed here?
> 
> I'll have to investigate what that does and why it did not seem necessary
> here

It looks like we probably need that NeedsRepaint, because of the comments in

[Mac] Allow focus rings to redraw themselves if necessary
https://bugs.webkit.org/show_bug.cgi?id=132593

"Mac allows focus rings to change rendering over time. Expose this
functionality to WebKit by having the rendering code detect that
a focused element has asked to repaint itself, and also have
the FocusController object remember how long it has been since
an element was focused."

[chris fleizach]

Created attachment 246085 [details]
screenshot1

[chris fleizach]

Created attachment 246086 [details]
screenshot2

[chris fleizach]

Created attachment 246087 [details]
screenshot3

[chris fleizach]

Created attachment 246088 [details]
screenshot4

[chris fleizach]

Created attachment 246089 [details]
screenshot5

[chris fleizach]

http://trac.webkit.org/changeset/179796

[Alexey Proskuryakov]

This test fails on all Mavericks bots:

https://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fcss%2Ffocus-ring-exists-for-search-field.html

Looks like a font difference, so I'm going to land platform specific results.