564277 - Fix up focusrings in html documents

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Neil Deakin]

Attachment: patch

This patch makes focusrings work the same in html elements as xul elements. Specifically:

- bug 561518, on Mac for all elements (including XUL), focus rings should actually appear on all elements when focused. This is a bug in the recent focus rings changes.
- another problem with that patch is that the -moz-focusring wasn't correct for textboxes and lists on Mac. This patch corrects that (as now all focused elements have rings)
- on Mac, buttons and other focusable elements don't focus on click, but do clear the current focus (as Safari does) <input type="text"/type="password">, <textarea> and <select> do however focus on click. Are there others that should? - on Windows, focus rings only appear once a key is used or the system setting is enabled
- focus rings always render on Linux when an element is focused.
- the special link handling of focusing but showing no focus ring still applies. It renders when the tab is switched or when navigating back.

Comment 1

[Markus Stange [:mstange]]

(In reply to comment #0)
> - on Mac, buttons and other focusable elements don't focus on click, but do
> clear the current focus (as Safari does)

Do you know why Safari does that clearing thing? I don't really see the point.

Comment 2

[Neil Deakin]

> > - on Mac, buttons and other focusable elements don't focus on click, but do
> > clear the current focus (as Safari does)
> Do you know why Safari does that clearing thing? I don't really see the point.

Nope. I didn't intentionally implement it that way though.

Comment 3

[Neil Deakin]

(In reply to comment #1)
> (In reply to comment #0)
> > - on Mac, buttons and other focusable elements don't focus on click, but do
> > clear the current focus (as Safari does)
> 
> Do you know why Safari does that clearing thing? I don't really see the point.

Ok, I see why this is used. Clicking in the content moves the caret to that spot and could start a selection if the mouse is dragged.

We could prevent this for specific elements but I'm not sure the extra complexity is worth it.

Comment 4

[Markus Stange [:mstange]]

(In reply to comment #3)
> (In reply to comment #1)
> > (In reply to comment #0)
> > > - on Mac, buttons and other focusable elements don't focus on click, but do
> > > clear the current focus (as Safari does)
> > 
> > Do you know why Safari does that clearing thing? I don't really see the point.
> 
> Ok, I see why this is used. Clicking in the content moves the caret to that
> spot and could start a selection if the mouse is dragged.

Ah, makes sense. Probably not worth worrying more about.

Comment 5

[Neil Deakin]

Comment on attachment 443939 [details] [diff] [review]
patch

Builds are at https://build.mozilla.org/tryserver-builds/[e-mail]-focusringshtml/

Comment 6

[Alex Faaborg [:faaborg] (Firefox UX)]

>- bug 561518, on Mac for all elements (including XUL), focus rings should
>actually appear on all elements when focused. This is a bug in the recent focus
>rings changes.

can you cc me on that bug

>- on Mac, buttons and other focusable elements don't focus on click, but do
>clear the current focus (as Safari does) <input type="text"/type="password">,
><textarea> and <select> do however focus on click. Are there others that
>should? - on Windows, focus rings only appear once a key is used or the system
>setting is enabled

Those are the only two that come to mind, just the different types of text areas

>(as now all focused elements have rings)

Do you mean all keyboard focused elements?  If an element is focused with the mouse (aside from the text field exceptions on OS X) it shouldn't draw a ring (unless I'm missing something from 561518).

Comment 7

[Neil Deakin]

(In reply to comment #6)
> Do you mean all keyboard focused elements?  If an element is focused with the
> mouse (aside from the text field exceptions on OS X) it shouldn't draw a ring
> (unless I'm missing something from 561518).

Clicking the mouse on element doesn't change the focus on Mac (except for textboxes and lists), so naturally no ring will appear. The focus and ring will remain on the currently focused element.

Comment 8

[Alex Faaborg [:faaborg] (Firefox UX)]

(sorry about all the confusion, could find an OS X tryserver build) If I'm on maps.google.com, and I focus the text field with the mouse [ring draws around it], and then I focus the "search maps" button with the mouse [no ring draws since it is a button] does the textfield still have the ring drawn at that point?  The correct behavior OS X seems to be:

-only draw the rings on items on the following items that also currently hold the focus:
   -text field
   -password field
   -list area

Comment 9

[Neil Deakin]

That is the issue that Markus brought up above. The patch here follows what Safari does: clicking 'Search Maps' clears the focus, but does not focus the button. Similar to what happens when clicking some other part of the page. This is so that the caret can be updated and the selection started if needed.

We could add exceptions when clicking specific elements though.

Comment 10

[Alex Faaborg [:faaborg] (Firefox UX)]

>The patch here follows what
>Safari does: clicking 'Search Maps' clears the focus, but does not focus the
>button.

Ok, that sounds ok although I guess the only issue might be if the user believes the button now has the focus (since it was last interacted with), and is surprised when they start to hit the tab key and the focus isn't where they think it is.

Comment 11

[Neil Deakin]

Attachment: fix issue with some tests

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 444421 [details] [diff] [review]
fix issue with some tests

IsLink could be in nsContentUtils.
And if we have similar code elsewhere, that could use the nsContentUtils method.

In general I don't like OSX focus behavior, but that is OS level problem I guess.
(Someone should provide reasonable working window manager for OSX)

Comment 13

[Johnny Stenback (:jst)]

What's left here, kinda looks like this is ready to land?

Comment 14

[Neil Deakin]

http://hg.mozilla.org/mozilla-central/rev/e730711e5624