Closed Bug 418521 Opened 16 years ago Closed 14 years ago

Focus ring appears on mouse interactions (as opposed to only when tabbing through items)

Categories

(Core :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: faaborg, Assigned: enndeakin)

References

(Depends on 3 open bugs)

Details

(Keywords: dev-doc-complete, polish, Whiteboard: [polish-hard][polish-visual][polish-p0])

Attachments

(5 files, 6 obsolete files)

The focus ring currently appears on any widget, hyperlink or image that has the focus.  Ideally, the focus ring should only be visible if the user has started to tab through elements, providing feedback on which widget has the focus.

This is (in my opinion) one of the largest polish problems we currently have with XUL.  Web developers also occasionally complain that the focus ring in the content area detracts from the beauty of their carefully crafted designs.

Examples provided in the attachment, my apologies if this is a dupe.
Flags: blocking1.9?
Probably no chance of fixing this in 1.9, though Cc'ing roc to see if he has any quick fix ideas.  I'm pretty sure this isn't just a CSS change, sadly.
Component: Widget → XP Toolkit/Widgets: XUL
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
QA Contact: general → xptoolkit.xul
Version: unspecified → Trunk
we'd probably need a new CSS pseudo-selector to indicate elements that have been keyboard-focused. Definitely not happening for 1.9.
Aaron: do you know how feasible fixing this would be?
This is a pretty big polish problem, especially on Windows. No other application or browser does this, and while I'm a big fan of drawing the focus rings when one starts tabbing, I don't think they need to be drawn on every click. Ends up looking quite messy.
Do other browsers apply :focus styles for mouse-clicked items?
I hear you Mike, but we've always done this, so it's just one more "nice to have" thing for 1.9 and "wanted" status is completely appropriate.
>Do other browsers apply :focus styles for mouse-clicked items?

Safari turns links red like we do, IE7 has no change on mouse click.  I'm in
favor of a visual affordance for down click (although I think red is a bit
much), but the focus ring looks really unpolished, especially when clicking on
images.
I find the focus outline useful for answering the question "which link did I last click?" in bug lists.
FWIW focus changes have a history of causing nasty regressions (not just from me) :)
Fixing this doesn't really require focus changes.
I'm afraid of this breaking a11y of Javascript widgets. A lot of effort has been put into making Dojo's JS widgets accessible. For example, when you click on an item in a group of items it may be using a :focus rule to show the expected indication that the individual item is the active one.
In this screen shot we see how the designer of getmiro.com crafted a beautiful download button, but the focus ring exposes the actual region of the image to the user.  This is an example of the implementation model of the page (the region of the actual image) being incorrectly exposed at the interface level to the user.
It should be noted that, in Windows, this is a UI preference.  Uncheck the hide-keyboard-navigation option (which is checked by default) in the advanced display prefs in XP and then fire up Internet Explorer.  You'll see that, like Firefox, IE will apply a focus ring to everything that gets clicked by the mouse.

Being able to visually verify and check exactly what you clicked on is important for accessibility and usability (especially for images and for links that have been styled to not change color), so if we were to stop this behavior, we must give the user some way to re-instate it, either by honoring the appropriate preferences in Windows or through a Firefox preference (could be hidden) for non-Windows systems that don't have such a global setting
Flags: wanted1.9-
Flags: wanted1.9+
Flags: wanted-next+
Flags: wanted1.9.1?
One option would be to have a -moz-keyboard-focus pseudoclass that is set for the focused element when the focus was set by keyboard, and continue allowing :focus to match any focused element, but change our UA style sheet so that most of our elements use -moz-keyboard-focus. Shouldn't be hard and I don't see that that would break anything. Just need someone to step up.
That sounds like a nice solution for content, but at least in XUL chrome we'd end up with a behaviour that's different from all platforms I've tested on.

Here are the results of my tests:

- Mac OS X chrome:

  On Mac OS X, a focus ring is shown all the time. When opening a new window,
  it's usually on the first focusable element.
  Clicking on a control doesn't change the focus ring's position. Say the focus
  is on the first checkbox in a window. After a click on the fifth checkbox,
  the focus ring is still on the first checkbox, and pressing Tab moves focus
  to the second checkbox.
  There are at least two exceptions to this: Textboxes and lists. Both controls
  grab focus on click because they expect keyboard input - in lists, you can
  change the selection using the arrew keys.
  In chrome, clicking on plain, unfocusable area doesn't reset focus.

- Safari content area:

  In Safari, only clicks on textboxes and <select>s focus the clicked element.
  All other clicks reset the focus to "nowhere"; pressing Tab after such a
  click focuses the first focusable element on the page.

- Windows XP chrome:

  (These tests might be flawed - the preference Kai mentioned in comment 13
  didn't have any effect for me; I might be doing something wrong.)

  Windows XP has a completely different focus model. On Windows, every click
  sets the focus on the clicked control, just like Firefox is doing now.
  However, no focus ring is drawn as long as you haven't hit a keyboard
  navigation key.
  As soon as you hit the tab or arrow keys, a per-window switch is flipped:
  from now on, focused elements in this window will always draw a focus ring,
  even those that have been focused by a mouse click. (However, it looks like
  you can reset the switch when changing to a different tabpanel...)
  It's not possible to reset focus to "nowhere".

- Internet Explorer content area:

  Internet Explorer 6 and 7 behave the same as in chrome - focus follows
  mouse but the focus ring isn't drawn until the keyboard navigation switch is
  flipped.
  The Internet Explorer 8 Beta supports the :focus pseudoclass and has adopted
  Firefox's behaviour - for websites that are in IE 8 mode, the focus ring is
  always drawn, even without "flipping the switch".
  (At least it looks like that's the intended behaviour; sometimes it doesn't
  really work, some focus rings are cut off etc... it's apparently still a
  little buggy.)
  There's another interesting thing: Clicking anywhere on the page doesn't
  reset focus to "nowhere" - instead, the element under the mouse pointer
  (div, p or whatever) is focused (without focus ring), so you can start
  tabbing near the click position.

I haven't tested on linux yet.

I'm still thinking about a model that would work for Gecko, feel native on both platforms, be consistent for web content and not too dangerous to implement.
(In reply to comment #15)
> - Mac OS X chrome:
> 
>   On Mac OS X, a focus ring is shown all the time. When opening a new window,
>   it's usually on the first focusable element.
>   Clicking on a control doesn't change the focus ring's position. Say the focus
>   is on the first checkbox in a window. After a click on the fifth checkbox,
>   the focus ring is still on the first checkbox, and pressing Tab moves focus
>   to the second checkbox.

What are you testing? The native Mac apps I've tried don't ever give focus to checkboxes (on Leopard anyway).

I didn't notice that until just now, though, which suggests maybe it isn't very important.
> What are you testing? The native Mac apps I've tried don't ever give focus to
> checkboxes (on Leopard anyway).

Ah, I forgot to say that. I've turned on "full keyboard access" in the Keyboard Shortcuts section of the Keyboard preferences. That makes these controls focusable by keyboard.
Note bug 437296 for potentially changing the behavior on OS X
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
Depends on: 377320
See also discussion in bug 262578.
Flags: wanted1.9.1?
Flags: wanted1.9.1-
Flags: blocking1.9.1-
this bug is eligible for bug 462080 :)
Whiteboard: [polish-hard][polish-visual]
Whiteboard: [polish-hard][polish-visual] → [polish-hard][polish-visual][polish-high-visibility]
An example of how the focus outline can be useful:
* Go to a page which links to many pages, some or all of which are visited.
* Middle‐click some of the links in order.
* Check the opened tabs, closing some or all of them.
* Go back to the original tab and continue clicking from the place where you have stopped (it is marked by the focus outline).
I rely on focus outlines in the same way Aleksej describes.
Depends on: 462289
At the very least we shouldn't have this issue in chrome.  In terms of content, I agree that the focus ring can be useful on hyperlinks (in addition to changing their color), although we might want to do something like more visually appealing than the current dotted line.
1. any website can disable this focusing with the following simple css || a:focus {outline:none;} ||
2. shouldn't website authors be responsible for visual feedback?
3. how about elements with onclick handlers?
4. IMHO coloring the link red shoulb be more than enough visual feedback...
5. if that's not convincing enough, how about just an underline or something?
What about website controls focused via focus() calls?  Should those be treated as "clicked on" or as "tabbed to" for visual feedback?

I often find that websites will focus some random textfield when I load the site, and the blinking 1-px-wide caret is not very obvious... the focus ring is, though.
(In reply to comment #27)
> What about website controls focused via focus() calls?  Should those be treated
> as "clicked on" or as "tabbed to" for visual feedback?

.focus() needs to provide visual feedback, because authors often have to implement their own keyboard navigation for custom JS widgets like tree views or spreadsheets, etc. They capture a key and focus the next item, when appropriate. So, unless you are going to try and determine why the focus moved for a .focus(), I would just visually show all of those.

Here's an example:
http://archive.dojotoolkit.org/nightly/dojotoolkit/dijit/tests/test_Tree.html
(Tab into the tree and arrow around).
Blocks: 478147
Blocks: 462289
No longer depends on: 462289
Attached patch possible patch (obsolete) — Splinter Review
This patch implements a pseudoclass which can be used for specific focus methods. It applies on top of the focus work patch in bug 178324.

element:-moz-focused-by(mouse) { ... }
element:-moz-focused-by(key) { ... }
element:-moz-focused-by(application) { ... }

A second possible implementation is to instead use:

element:-moz-focus-ring { ... }

which would apply whenever the element was focused and a focusring should appear, which would be determined internally. This would offer less specific control, but would be easier to just use one rule. It would also easily fix bug 377320.

Thoughts?
This tells me that we need :-moz-focus-ring rather than :-moz-focused-by(...):

(In reply to comment #13)
> It should be noted that, in Windows, this is a UI preference.  Uncheck the
> hide-keyboard-navigation option (which is checked by default) in the advanced
> display prefs in XP and then fire up Internet Explorer.
Blocks: 377320
Component: XUL → General
Depends on: 178324
No longer depends on: 377320
QA Contact: xptoolkit.widgets → general
(In reply to comment #30)
> This tells me that we need :-moz-focus-ring rather than :-moz-focused-by(...):
> 
> (In reply to comment #13)
> > It should be noted that, in Windows, this is a UI preference.  Uncheck the
> > hide-keyboard-navigation option (which is checked by default) in the advanced
> > display prefs in XP and then fire up Internet Explorer.

OK, support for the preference is added by bug 25894. I think I'll put together a patch that does :-moz-focus-ring without this preference just to try it out and determine which approach is actually needed.
Attached patch -moz-focusringSplinter Review
This patch implements -moz-focusring which matches when a focus ring should appear. For testing, it matches only when the keyboard is used on all platforms. Really, it would use the setting on Windows as needed.
Much like bug 25894, it's slightly more complex than just a binary pref; see comment 15. The section on OS X is also interesting.
(In reply to comment #33)
> Much like bug 25894, it's slightly more complex than just a binary pref; see
> comment 15. The section on OS X is also interesting.

The behaviour described there makes it sound more complicated that it actually is. This is the actual behaviour:

1. On both Mac and Windows, focus rings do not appear within a window until the Tab key is pressed, at which point they always appear whatever the focus method. On Windows, the Alt key also does this, and also makes accesskeys appear as well.

2. The Windows setting (in the Display control panel -> Effects button) may be used to toggle the behaviour to always showing focus rings, even when the Tab or Alt key is not pressed. This is the behaviour that my Linux machine exhibits by default.

3. On Windows, clicking an element always focuses it. On Mac, clicking an element never focuses it.

4. Textboxes and lists are special on Mac. They may be focused by clicking them and focus rings are always drawn. For this, we should be able to keep the existing focusring styles.

The Mac behaviour for point 3 is not currently implemented and is not this bug (or at least, is a different feature/patch that isn't focusring related)
But without 3, it seems like we'd introduce new non-native behavior on OS X: omitting the focus ring while actually changing focus.
Yes, we should implement that first. I had actually intended to make the -moz-focusring match check windows only and would always match on other platforms until then. But I didn't for now for testing purposes.
(In reply to comment #36)
> I had actually intended to make the
> -moz-focusring match check windows only and would always match on other
> platforms until then.

OK, that seems correct, except for "until then". If focus wouldn't be changed in the first place on OS X, :-moz-focusring would still always match in accordance with :focus.
Blocks: 421949
Blocks: 492452
Sorry I'm coming in late so not sure if this is just noise, it has been a long day and I might have missed a comment or two, but this might be a way to go. No points for style here ;)
I would say ugly focus ring is not a point to not show focused element. I
believe we must indicate somehow focused element because otherwise it's not
possible to say where is the focus and what tab key is supposed to do. I think
this way is definitely keyboard friendly. I think we should replace ugly focus
ring by more gladden-the-eye way. On another hand it's worth to make Firefox
behaving like platform applications do, otherwise it might annoy users. In this
light I would say it's worth to have a preference which points if OS rules or
keyboard friendly rules are used.
Yes. I agree the right way to go is to always indicate where focus is.
>it's worth to make Firefox
>behaving like platform applications do, otherwise it might annoy users. In this
>light I would say it's worth to have a preference

I'm fine with an about:config pref, but by default I would like us to match the behavior of all other operating systems and browsers.
(In reply to comment #42)
> >it's worth to make Firefox
> >behaving like platform applications do, otherwise it might annoy users. In this
> >light I would say it's worth to have a preference
> 
> I'm fine with an about:config pref, but by default I would like us to match the
> behavior of all other operating systems and browsers.

This makes sense I think we should try to correspond to native browser on OS where firefox is used by default. One thing I want is this work should be performed the same time so we won't get usability regressions.
Please resent commenters you are completely missing the point of this bug. The point stated by the reported, and which I agree with, is that the focus SHOULD NOT BE SHOWN if you simply click a link on a page.

Take an example of a frames setup with a navbar in the left hand frame and a document window in the right. When you click a link in the left you get the document appearing in the right. The issue is that clicking that link in the left causes a focus ring to appear (and stay there) in the left panel. Given the user has probably switched their attention to the right hand frame this focus ring is wrong. Please take account of this issue and the OP when thinking about this bug.
Attached patch work in progress (obsolete) — Splinter Review
This patch:
 - makes XUL elements (except lists/textboxes) not focus on mouse clicks on Mac. I haven't changed HTML elements in this patch.
 - focus rings only appear after a key is pressed to focus an element on all platforms (should be Windows only but not for this testing)
 - adds a :-moz-focusring pseudoclass for checking if focus rings should appear

Only tested on Mac.
This bug's priority relative to the set of other polish bugs is:
P0 - There can be only one bug designated P0 at a time, this is the single highest priority active polish bug in existence.

This is basically a polish bug on nearly every single interaction the user has with chrome, the absolutely highest priority polish bug we have right now.
Whiteboard: [polish-hard][polish-visual][polish-high-visibility] → [polish-hard][polish-visual][polish-p0]
Neil, this patch is fantastic!

Some comments:
 - The downloads richlistbox doesn't grab focus on click. It should.
 - layout/style/ua.css should probably use :-moz-focusring instead of :focus?
 - Radio buttons in radio groups, e.g. the buttons in the preferences window,
   still get [focused="true"] on mousedown, but I suppose fixing that isn't
   part of this bug.

I think it's perfectly fine to make the Mac focus model of "don't focus on mousedown" XUL-only.
How do you feel about using the Windows focusring model for HTML content on all platforms? That way, content authors would have a consistent cross-platform focus model.
(In reply to comment #47)
> I think it's perfectly fine to make the Mac focus model of "don't focus on
> mousedown" XUL-only.
> How do you feel about using the Windows focusring model for HTML content on all
> platforms? That way, content authors would have a consistent cross-platform
> focus model.

I think that would be a question best asked of Alex.

The Windows model is:
- click an element to focus it
- focus rings only appear once Tab has been pressed once for that window/dialog. After that, they always appear whatever the focus method (mouse, key, etc).
- the system setting may be changed to enable focus rings always, as well as accesskey underlines.

An open question is whether the 'Tab has been pressed once for that window/dialog' applies per top-level window, per frame, per tab or per document.

Note also that this bug is actually dependent on 25894. I don't think that we should be implementing this without honouring the system setting.
Depends on: 25894
(In reply to comment #48)

I guess he was just referring to the "click always focuses control" rule, not the whole invisible focus ring intricacies brought by Windows 2000.

Now, regarding this idea, I'm not sure I like it, as it wouldn't solve the most visible aspect of the user experience (focus rings floating around when clicking links), unless common hyperlinks (those which do not set tabindex) don't get focused.

Analyzing other browsers, it appears WebKit (Safari 4 and Chrome for Mac preview) does use click-always-focuses behavior for tabindex="0" elements, though with a very specific exception for <a>, whether it's actually a link or not. This last part seems slightly flawed to me.
(In reply to comment #49)
> (In reply to comment #48)
> 
> I guess he was just referring to the "click always focuses control" rule, not
> the whole invisible focus ring intricacies brought by Windows 2000.

No, I was indeed referring to the "invisible focus ring intricacies brought by Windows 2000".
> That way, content authors would have a consistent cross-platform
> focus model.

I think everyone will likely agree that establishing a consistent cross platform focus model for the Web has a lot of advantages, so the question becomes what attributes should that focus model have.

Note that this entire comment is just about the idea of changes to the focus model in the content area (chrome should definitely be platform specific, and match the correct behavior on each platform).

>I think that would be a question best asked of Alex.
>
>The Windows model is:
>- click an element to focus it
>- focus rings only appear once Tab has been pressed once for that
>window/dialog. After that, they always appear whatever the focus method (mouse,
>key, etc).
>- the system setting may be changed to enable focus rings always, as well as
>accesskey underlines.

So think the main points in favor of this approach is:

-Cleaner visual appearance of Web sites (Web developers currently complain that we draw focus rings all over their carefully designed image maps)

-When you tab on OS X, the behavior is different, but we want to violate the platform conventions anyway (details in bug 437296)

Currently the main points against this approach that come to mind are:

-It is sometimes useful to see the focus on hyperlinks, so that you can tell which hyperlink you recently clicked on, and for instance use that information to click on the next hyperlink in a list.

-Drawing the focus on text fields is kind of useful, because it gives you an additional indication beyond the blinking cursor where text is going to show up when you start typing.  Currently Chrome on Windows uses a bright orange highlight on text fields, and aside from being completely non-native, it is actually kind of nice.

So just to throw an idea out there, what if we created a hybrid approach for the Web's focus model on all platforms that had these particular properties:

1) When you start to tab, every control will display visible focus, including things like combo boxes, check marks, and buttons.  This behavior is not standard on OS X, but is nonetheless useful.

2) When you are using the mouse, some controls do actually display visible focus, including text fields and hyperlinks (but not combo boxes, check boxes, radio buttons, buttons, etc.)  This is not standard on Windows, but is nonetheless useful.

3) The focus appearance is a bit fancier than a single dotted line, and can be styled per platform.  So even though the behavior may not be native (depending on the specific control and specific platform), the visual presentation can be customized so that it still integrates well with the surrounding OS, like using the correct color palette, etc.

4) (crazy talk) the focus visual appearance can be customized by the Web author.  I have no idea how much standards work we would need to do ahead of time on this.
Another thing that comes to mind is that I believe Vista is using a different focus model than XP, although I can't confirm since I'm not in the office and only have XP running.  I think on Vista text fields get a different styling when they have the focus (similar to OS X), however this could just be an IE in the content area behavior, I know there are using a different widget styling there (for instance combo boxes look more like text fields as opposed to buttons).

Either way, I'm proposing that even on XP, a platform that has never drawn text fields with a visual focus, we consider drawing text fields with a visual focus (using a color out of the Luna color palette).

Also, since Linux users may quickly comment that I am leaving them out, I honestly don't know anything about the gnome focus model (including things like if it changes based on the OS theme), so any insight you anyone can provide would be really appreciated :)
Does drawing the focus on a hyperlink only when the user goes away from a page and then returns to it (either through a tab switch or a back/forward navigation) a reasonable idea?  You don't really need the focus the very moment you are clicking on the link with the mouse, since it is pretty obvious what you are clicking on given the mouse cursor position.
(In reply to comment #53)
> Does drawing the focus on a hyperlink only when the user goes away from a page
> and then returns to it (either through a tab switch or a back/forward
> navigation) a reasonable idea?  You don't really need the focus the very moment
> you are clicking on the link with the mouse, since it is pretty obvious what
> you are clicking on given the mouse cursor position.

Yes, I think so, because otherwise it would defeat the following goal:

-Cleaner visual appearance of Web sites (Web developers currently complain that
we draw focus rings all over their carefully designed image maps)
yeah, and it would allow us to make the focus more obvious, which is useful since you are returning to the page and are actively looking to see what hyperlink had (I guess now has) the focus.
The Linux behaviour is what is currently implemented. Clicking focuses items, and they always show focus indicators.

> 3) The focus appearance is a bit fancier than a single dotted line, and can be
> styled per platform.

The appearance of focus rings is just specified in the user agent stylesheet (ua.css/html.css/etc) via:

element:focus { ... }

and can be overriden by pages.

Note that the :focus pseudoclass applies whenever the element is focused. We would either want to use the new pseudoclass (:-moz-focusring) or change :focus to only apply when focus rings should be drawn. The former seems better.

Also note that the focus indicator on various elements is drawn by native theme code.

Finally, note that in the patch I have changed the behaviour for XUL and not HTML, and do not distinguish between content and chrome. The former is easy to distinguish in places where focus appearance is handled, but I don't think it's particularly feasible to determine whether in a content or chrome window.
So this is the information I need to finish work on this bug:

Which sets of elements fit within which of the following categories:
 A. Clicking the element focuses it and shows a visible focus ring.
 B. Clicking the element focuses it but does not give a visible indicator.
 C. Clicking the element does not focus it.

I propose the following:

When clicking on XUL elements:
  On Mac, textboxes and lists: A
  On Mac, all other elements: C
  On Windows, when tab or an accesskey has already been pressed in that window: A
  On Windows, with the 'hide underlined letters for keyboard navigation' system setting on: B
  On Windows, with the 'hide underlined letters for keyboard navigation' system setting off: A
  On Linux, all XUL elements: A
When clicking on HTML links, on all platforms: B, but when returning to the page, show the focus indicator.
When clicking on non-link HTML elements, on Windows, with the 'hide underlined letters for keyboard navigation' system setting off: A
When clicking on HTML inputs, on all platforms: A
When clicking on non-link and non-input HTML elements, on all platforms: B
When focusing an element with tab or an accesskey, or by calling the element's focus() method: A

The :focus pseudoclass may be used by authors to set the appearance when the element is focused (This would draw in cases A and B) . The :-moz-focusring pseudoclass may be used to set the appearance only when the element should draw a focus ring (case A only).

Note that the 'Hide underlined letters for keyboard navigation' defaults to on and affects both underline accesskey indicators and focus ring display.
Your proposal sounds perfect to me.
(In reply to comment #57)
> When clicking on HTML links, on all platforms: B, but when returning to the
> page, show the focus indicator.

Keep in mind that links can be opened in new tabs/windows, so "returning to a page" would have to include switching tabs/windows.
Builds are at https://build.mozilla.org/tryserver-builds/neil@mozilla.com-focuscombined/
Assignee: nobody → enndeakin
Attachment #382821 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #388742 - Flags: ui-review?(faaborg)
Should also mention that the patch from bug 504367 should be applied first as it fixes some problems with focus and back/forward navigation.
Two things I didn't change:
 - the appearance of input fields. They will just look as they do now. The specific appearance I think can be done in a followup bug.
 - the focus indicator on links appears when switching back to the tab, or by navigating back in history. The excepions are when the page isn't in the bfcache as there isn't anything to restore anymore, and also, it doesn't get shown when a link opens a new window and the original window is switched back to.
(In reply to comment #62)
>  - the focus indicator on links appears when switching back to the tab, or by
> navigating back in history. The excepions are when the page isn't in the
> bfcache as there isn't anything to restore anymore, and also, it doesn't get
> shown when a link opens a new window and the original window is switched back
> to.

Isnt it better to show outline when alt is pressed. That way the outline will be available only when is requestesd by the user.

No keyboard - no outline.

Hiding the outline when mouse is used and then showing it in some cases will be almost the same situation like is now, but will be seen not so often.
diff --git a/layout/style/forms.css b/layout/style/forms.css
-input[type="image"]:focus {
+input[type="image"]:moz-focusring {

You forgot a dash here.
(In reply to comment #63)
> Isnt it better to show outline when alt is pressed. That way the outline will
> be available only when is requestesd by the user.

I wasn't aware that happened, but I'll add back code to do that.
(In reply to comment #62)
> The excepions are when the page isn't in the
> bfcache as there isn't anything to restore anymore, and also, it doesn't get
> shown when a link opens a new window and the original window is switched back
> to.

BTW maybe this is possible too although im not sure.

The outline could be there all the time just change the color from transparent to what is required. Visually the outline will be invisible but it will be on the exact position when needed.
So when alt is pressed or tab, the focus will appear on place.
> You forgot a dash here.

Please also add an automated test that would catch that.
I'll try to get to this ui-review soon, overall behavior sound great but there are a lot of little details to check are native behavior on each platform.

Does the current patch only effect XUL?  Some of the ideas discussed around Comment #51 should probably be spun into a separate bug since there are a lot of implications to discuss.
(In reply to comment #68)

> Does the current patch only effect XUL?

It affects HTML as well. The patch implements the behaviour described in comment 57, with the caveats mentioned in comment 62.
Sorry about the delay with the ui-review, still catching up on things now that I'm back in the office.
Can we not just get a ui-review on the actual changes as described in comment 57 (if that's what's implemented) and then we can follow up with issues as they're discovered? Or is the issue that you're checking to make sure that what's described in comment 57 is considered native on various platforms?
Target Milestone: --- → mozilla1.9.2a1
I hit a bunch of errors trying to apply the patch, could you check if it might be bitrotten, or post links to new try server builds?

>Can we not just get a ui-review on the actual changes as described in comment
>57 (if that's what's implemented)

The changes sounds right, but I was planning on trying them all out on all platforms for the review.  Otherwise I can just approve the described changes.
Attached file updated patch (obsolete) —
Attachment #388742 - Attachment is obsolete: true
Attachment #394820 - Flags: ui-review?(faaborg)
Attachment #388742 - Flags: ui-review?(faaborg)
So, any news on this bug?

I've been trying the OS X build for the last few weeks, and it seems to work fine -- in fact, it's much better than I had expected, particularly the content behavior (you'll see on comment #49 that I originally thought this approach wouldn't be very good). I just have two quirks to report:

- When changing tabs (or any tab-like widget), focus should never stay on an element from the previous tab. Open the preferences window on the General tab, focus the "Home Page" edit box, switch to another tab (say, Advanced) using the mouse, and press the down arrow. A URL suggestion drop-down will appear, showing the URL box from the General tab is still focused.

- If arrow keys are somehow used to move between elements (basically a chrome window where the focus starts on a checkbox, radio button or button), the focus ring should appear, just as if you used the Tab key. A practical example is the proxy settings window: if you open it and press up/down, the radio button selection will move, but nothing will indicate where your focus actually is. Related to this particular quirk:

    * I'm pretty sure arrow keys under these circumstances also trigger
      the "keyboard mode" on Windows, but I can't test them right now.

    * Maybe we could just always show the focus ring on chrome windows.
      An example where such behavior is clear is the preferences window
      for Safari -- there's always a focus ring somewhere. Windows in
      other apps that don't show a focus ring anywhere seem to actually
      have no focused control (i.e., pressing the spacebar has no
      effect), unlike the behavior in these builds, where we're hiding
      the fact that there's a control with focus.

I was going to try a Linux build now, but the builds seem to be gone.
@@ -549,16 +557,22 @@ nsXULElement::IsFocusable(PRInt32 *aTabI

+#ifdef XP_MACOSX
+  // on Mac, mouse interactions only focus the element if it's a list
+  if (aWithMouse && IsNonList(mNodeInfo))
+    return NS_OK;

This should be return PR_FALSE.
Really sorry about the lag on the ui-review.  I'm now finally back in the office and I can get you a review if you post new tryserver builds, since I want to check this out on all platforms.
Alex, there should be try server builds with the id 'bug418521-focusrings' coming in the next hour or so.
>Alex, there should be try server builds with the id 'bug418521-focusrings'
>coming in the next hour or so.

Hm, not seeing a windows build: https://build.mozilla.org/tryserver-builds/neil@mozilla.com-bug418521-focusrings/
A very recent change that caused a build error. A Windows build is available at https://build.mozilla.org/tryserver-builds/neil@mozilla.com-bug418521-focusrings2/
Comment on attachment 394820 [details]
updated patch

Overall I'm going to give this a ui-r+ since the behavior is a lot better than what we currently have.

I found what I think are 7 possible errors, if it is easy to correct some of these we can consider them nits, otherwise we'll just deal with them in follow up bugs.  This spreadsheet lists what I think the right behavior is, and where the current patch deviates: http://spreadsheets.google.com/pub?key=t8sBdbRxc_OGAZavmb-5F9g&output=html
Attachment #394820 - Flags: ui-review?(faaborg) → ui-review+
This is a long bug, so I'm not sure if this is covered here; but remember that (if we're aiming for full platform support) the "full keyboard access" option on OS X should toggle whether some elements (like trees) can be focusable at all.
(In reply to comment #57)
> When clicking on HTML inputs, on all platforms: A
What about other HTML form controls?
>What about other HTML form controls?

This is a little complicated in that it merges chrome and content controls into a single column, but I think this is the correct approach for each control on each platform.  (would love to get feedback if anyone disagrees with anything):

http://spreadsheets.google.com/pub?key=t8sBdbRxc_OGAZavmb-5F9g&output=html
Attached patch add some tests (obsolete) — Splinter Review
Test changes:
  - window_focus.xul changes because elements are no longer focusable with the mouse on Mac
  - test_focusrings.xul tests whether focus rings appear in various cases
  - /toolkit/content/tests/widgets/test_focus.xul is redundant so I removed it

From the previous patch, this patch changes the windows handling of the WM_CHANGEUISTATE and WM_UPDATEUISTATE messages to be more correct.
Attachment #405117 - Flags: superreview?(neil)
Attachment #405117 - Flags: review?(dbaron)
Comment on attachment 405117 [details] [diff] [review]
add some tests

Theme changes
Attachment #405117 - Flags: review?(dao)
Comment on attachment 405117 [details] [diff] [review]
add some tests

Windows changes
Attachment #405117 - Flags: review?(jmathies)
Comment on attachment 405117 [details] [diff] [review]
add some tests

I find it slightly irritating that clicking a link doesn't give me immediate feedback anymore. Bug 482985 makes this worse.

>--- a/toolkit/themes/winstripe/global/menulist-aero.css
>+++ b/toolkit/themes/winstripe/global/menulist-aero.css

> menulist:focus > .menulist-label-box {
>-  border: 1px dotted ThreeDDarkShadow;
>   background-color: transparent;
>   color: inherit;
> }

This rule can be removed.

>+menulist:-moz-focusring > .menulist-label-box {
>+  border: 1px dotted ThreeDDarkShadow;
>+}

This should use menulist:-moz-focusring:not([open="true"]):not(.menulist-compact) > .menulist-label-box

>--- a/toolkit/themes/winstripe/global/menulist.css
>+++ b/toolkit/themes/winstripe/global/menulist.css

> .menulist-label-box,
>-menulist[open="true"]:focus > .menulist-label-box {
>+menulist[open="true"]:-moz-focusring > .menulist-label-box {
>   border: 1px solid transparent;
>   background-color: transparent;
>   color: inherit;
>   -moz-box-align: center;
> }

The menulist[open="true"]:-moz-focusring > .menulist-label-box selector can be dropped.

> menulist:focus > .menulist-label-box {
>+  background-color: Highlight;
>+  color: HighlightText;
>+}

This should use menulist:focus:not([open="true"]):not(.menulist-compact) > .menulist-label-box

>+menulist:-moz-focusring > .menulist-label-box {
>   border: 1px dotted #F5DB95;
>-  background-color: Highlight;
>-  color: HighlightText;  
> }

This should use :not([open="true"]) too.

> .menulist-compact:focus > .menulist-label-box {
>-  border: 1px dotted;
>   background-color: transparent;
>   color: inherit;
> }

This rule can be dropped.

>+.menulist-compact:-moz-focusring > .menulist-label-box {
>+  border: 1px dotted;
>+}

:not([open="true"]) as well

You also need to adjust menulist > menupopup > menuitem[_moz-menuactive="true"] in menu.css.
Attachment #405117 - Flags: review?(dao) → review+
Was removing test_focus.xul from the makefile intentional?
Yes, as stated in comment 89, that test is redundant. Not sure why the patch diff didn't pick up the deleted file but it is removed locally.
+::SendMessageW(mWnd, WM_CHANGEUISTATE, UIS_INITIALIZE, 0);

+  case WM_CHANGEUISTATE:
+  case WM_UPDATEUISTATE:
+  {
+    PRInt32 action = LOWORD(wParam);
+    PRInt32 flags = HIWORD(wParam);
+    // If the UI state has changed or the state is being initialized, update
+    // the UI state. For initialization, this will set the keyboard cues based
+    // on how the window was opened. For example, a dialog opened via a
+    // keyboard press on a button should enable cues, whereas the same dialog
+    // opened via a mouse click of the button should not.
+    if (msg == WM_UPDATEUISTATE || action == UIS_INITIALIZE) {
+      nsUIStateChangeEvent event(PR_TRUE, NS_UISTATECHANGED, this);
+      if (action == UIS_CLEAR) {
+        if (flags & UISF_HIDEACCEL)
+          event.showAccelerators = UIStateChangeType_Set;
+        if (flags & UISF_HIDEFOCUS)
+          event.showFocusRings = UIStateChangeType_Set;
+      }
+      else if (action == UIS_SET) {
+        if (flags & UISF_HIDEACCEL)
+          event.showAccelerators = UIStateChangeType_Clear;
+        if (flags & UISF_HIDEFOCUS)
+          event.showFocusRings = UIStateChangeType_Clear;
+      }
+      DispatchWindowEvent(&event);
+    }
+    else if (action == UIS_SET || action == UIS_CLEAR) {
+      ::SendMessageW(mWnd, WM_UPDATEUISTATE, wParam, lParam);
+    }
+
+    break;
+  }

Aren't we going to generate an bad event here when we send WM_CHANGEUISTATE w/UIS_INITIALIZE to mWnd? If I'm reading this right, when we receive our initial WM_CHANGEUISTATE, we create a nsUIStateChangeEvent, but none of the action flags are set, so we send off an event that isn't initialized properly.

Shouldn't we send WM_CHANGEUISTATE w/UIS_INITIALIZE, let that fall through, and rely on our response to the WM_UPDATEUISTATE broadcast by the system to update?

I'm pulling this from what I read on Chen's blog:

http://blogs.msdn.com/oldnewthing/archive/2005/05/03/414317.aspx
> Shouldn't we send WM_CHANGEUISTATE w/UIS_INITIALIZE, let that fall through, and
> rely on our response to the WM_UPDATEUISTATE broadcast by the system to update?

The issue here is that when receieve WM_CHANGEUISTATE we send WM_UPDATEUISTATE to all children, but the toplevel window itself never receives a WM_UPDATEUISTATE event.

I'll continue to investigate as I've found a minor issue with initializing.
This patch simplifies the handling of the WM_CHANGEUISTATE and WM_UPDATEUISTATE messages, as we can just rely on the system to do most of the work for us.
Attachment #367581 - Attachment is obsolete: true
Attachment #394820 - Attachment is obsolete: true
Attachment #405117 - Attachment is obsolete: true
Attachment #409969 - Flags: superreview?(neil)
Attachment #409969 - Flags: review?(jmathies)
Attachment #405117 - Flags: superreview?(neil)
Attachment #405117 - Flags: review?(jmathies)
Attachment #405117 - Flags: review?(dbaron)
Attachment #409969 - Flags: review?(dbaron)
Attachment #409969 - Attachment is patch: true
Comment on attachment 409969 [details] [diff] [review]
fix up menulist styles, and simplify windows state message handling

r+ on the windows bits.
Attachment #409969 - Flags: review?(jmathies) → review+
Comment on attachment 409969 [details] [diff] [review]
fix up menulist styles, and simplify windows state message handling

>+                     newFocusedElement.getAttributeNS("http://www.w3.org/1999/xlink", "type") == "simple"))
Didn't we drop xlink support, or was I imagining it??

> 
>+
I would have thought one blank line was enough ;-)
Attachment #409969 - Flags: superreview?(neil) → superreview+
> Didn't we drop xlink support, or was I imagining it??

We dropped generic xlink.

Of course that still leaves <svg:a> (which uses xlink) and some MathML stuff (which uses XLink but doesn't work right now).  You could use an instanceof test to detect <svg:a>.  This code also doesn't detect <area> or <link> elements; it probably should, no?

We should really have a single scriptable interface that all links implement...  Even if it's completely empty and just used for instanceof.
While this change was intended primarily for desktop Firefox, Limi correctly points out that for mobile devices, having visual feedback directly on a link press (for instance the iPhone overlays a grey box on the target you touch) is pretty important feedback.  So Fennec will likely want to make some modifications to the work being done here for touch feedback in the content area.
(In reply to comment #91)
> Yes, as stated in comment 89, that test is redundant. Not sure why the patch
> diff didn't pick up the deleted file but it is removed locally.

If it doesn't show up in the patch, then I doubt it will get removed when you push, so you should figure out why it's not.


I'm a little concerned about the handling of dynamic changes with this patch, and also its interaction with the new approach to pseudoclass implementation in bug 525952.

Would it make more sense to have a second event state (NS_EVENT_STATE_FOCUS_RING?) that's an element has whenever both of the following are true:
 * the element has NS_EVENT_STATE_FOCUS
 * window->ShouldShowFocusRing() returns true
and then send appropriate ContentStatesChanged notifications for that state?

I'm curious what bzbarsky thinks about that approach, or about the current one.


Sorry for the delay in getting to this.
This version is similar but includes the removed test changes, switches to using a different state flag and adds some extra calls to ensure that the keyboard indicator state is propagated to child windows.

Note that Windows only seems to fire ui update notifications when a window is opened. Changing the preference in the control panel only affects new windows.
Attachment #409969 - Attachment is obsolete: true
Attachment #428275 - Flags: review?(dbaron)
Attachment #409969 - Flags: review?(dbaron)
Comment on attachment 428275 [details] [diff] [review]
use separate flag

In the future, when you're requesting review on part of a patch, could
you split the patch into appropriate pieces so that you're requesting review
on a complete diff file and not part of it?

nsEventStateManager.cpp:

>+  nsIContent* focusedContent = fm ? fm->GetFocusedContent() : nsnull;
>+  if (aContent == focusedContent) {
>     aState |= NS_EVENT_STATE_FOCUS;
>+
>+    if (focusedContent) {

Since aContent is known to be non-null, and focusedContent is known to
be equal to aContent, you don't need to null-check focusedContent here.

>+      nsIDocument* doc = focusedContent->GetDocument();

GetDocument() is deprecated; please use GetCurrentDoc() or GetOwnerDoc().
(GetCurrentDoc() is the same as GetDocument(), GetOwnerDoc() is
a hair faster, and in this case I think the difference doesn't matter.)

(Same in nsNativeTheme.cpp.)


>+  if ((aState & NS_EVENT_STATE_FOCUS) || (aState & NS_EVENT_STATE_FOCUSRING)) {

if (aState & (NS_EVENT_STATE_FOCUS | NS_EVENT_STATE_FOCUSRING)) {

That said, what does it mean to call SetContentState(foo,
NS_EVENT_STATE_FOCUSRING).  Might it make more sense to assert that
NS_EVENT_STATE_FOCUSRING is not allowed in the bits passed to
SetContentState, but instead always add it when NS_EVENT_STATE_FOCUS is
passed in?  (And undo the corresponding changes to nsFocusManager?) But
I guess that wouldn't work with the nsGlobalWindow changes, so probably
ignore this comment.

nsGlobalWindow.cpp:

+    }
+    else if (aFocusMethod & nsIFocusManager::FLAG_SHOWRING

local style is "} else" on same line

>+      if (childWindow) {
>+               childWindow->SetKeyboardIndicators(aShowAccelerators, aShowFocusRings);
>+      }

Weird indentation here (tabs)?

In nsGlobalWindow::SetKeyboardIndicators, is the change intended to
propagate across chrome/content boundaries?

>+  nsPIDOMWindow* root = GetPrivateRoot();
>+  if (root) {
>+       PRBool showAccelerators, showFocusRings;
>+       root->GetKeyboardIndicators(&showAccelerators, &showFocusRings);
>+       mShowFocusRings = showFocusRings;
>+  }

2 space indent, not 5




I wonder whether we should really be introducing :-moz-focusring, or
whether it would be better to apply the difference to :focus.  I think
I'd actually slightly prefer applying the changes to :focus instead.  It
might be worth seeing what others think (maybe bzbarsky, or perhaps even
the www-style list.)

If we apply the differences to :focus, then Web pages writing :focus
styles will do what matches the UI.  That said, there could be some edge
cases where they really want the distinction.

r=dbaron either way, though, though please do bring up the issue of
:focus vs. :moz-focusring with others and get some additional opinions.
Sorry for the delay in getting to this.
Attachment #428275 - Flags: review?(dbaron) → review+
bz preferred the current approach, so I checked in the patch as is and we can evaluate feedback. I made one minor fix so that default value of showing indicators on mac is false.

http://hg.mozilla.org/mozilla-central/rev/f3abfe490d7f
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Looks like mochitest-plain 3/5 has been orange on Mac since this landed.  I presume somebody will back it out soon unless that's fixed.
Depends on: 560898
> bz preferred the current approach

Specifically, because the common use of :focus I've noticed on the web is for highlighting input fields, and the sites definitely want those highlighted when you click on them.
Can someone explain how to make a theme compatible with both :focus and :-moz-focusring? Just doing both :focus and :-moz-focusring won't work. FF older than 3.7 should use :focus, but >=3.7 should use -moz-focusring if I understand correctly. How to do this in a theme that supports both <FF3.7 and >=FF3.7?
If you click on link, wait until the other page is loaded, then click "Back" browser button and go back to the original page, the focus ring will appear around link. Is it bug or intended behavior?
That's intended behaviour, so that you can see which link you last clicked. For example, see comment 8.
(In reply to comment #107)
> That's intended behaviour, so that you can see which link you last clicked. For
> example, see comment 8.

intended and very useful!
Depends on: 561518
A few problems I've noticed on OS X:

- Issues reported on comment #75 remain.
- Open the proxy settings, click "Manual proxy configuration", then click any of its text boxes. Now, click "Automatic proxy configuration"; the text box will remain focused even though it is no longer enabled. Click the automatic proxy configuration URL text box, the previous text box will keep a focus ring even though it is no longer focused.
- When full keyboard access (System Preferences) is enabled, the focus ring should appear around chrome buttons. They still shouldn't get focus from clicks, but this is important on confirmation sheets. For example, open TextEdit, type something on a blank document, then try to close it. The "Don't Save" button has a focus ring, and this is extremely important so that you know what happens if you press the space bar. The analogous situation in Firefox is when you try to close a window or quit the application, and a confirmation appears. It's important to know that "Cancel" (or "Quit") is the action taken when you press space bar.
Depends on: 435697
Blocks: 435697
No longer depends on: 435697
Don't report additional problems in this bug, otherwise they will get lost. Regressions or followup bugs should be new bugs and be marked as blocking this bug.
Focus rings are suddenly back with the latest build.

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.3a5pre) Gecko/20100425 Minefield/3.7a5pre
Keywords: dev-doc-needed
Target Milestone: mozilla1.9.2a1 → mozilla1.9.3a5
Blocks: 564277
Depends on: 570206
Depends on: 581253
Depends on: 574353
Now documented here:

https://developer.mozilla.org/en/CSS/:-moz-focusring

Let me know if there are any issues with that doc.
Depends on: 601122
No longer blocks: 435697
Depends on: 140562
No longer depends on: 608878
Depends on: 637224
Depends on: 644479
Depends on: 645776
Depends on: 689500
Depends on: 720352
No longer blocks: 377320
See Also: → 1288927
Bug 1437901 is tracking that.
Depends on: 1481424
Depends on: 1487047
See Also: → 1545499
You need to log in before you can comment on or make changes to this bug.