334370 - Pane icons in options dialog should display visible focus rectangle

Status: RESOLVED WORKSFORME

(Firefox :: Keyboard Navigation, defect)

Description

[Mark Pilgrim (inactive)]

There is no way to tell when focus is on one of the pane icons (General, Privacy, etc.) in the options/preferences dialog.  The icon is highlighted when you're somewhere in that pane, the icon is highlighted, but when you're focused on the icon itself, there's no focus rectangle or anything that would indicate where the focus is.

Comment 1

[Mark Pilgrim (inactive)]

Attachment: implement focus rectangle in winstripe theme

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

I'm away from my PC for the next few days. Can a brother get a screencap? ;)

Comment 3

[Mark Pilgrim (inactive)]

Attachment: "before" screenshot (no focus indicator at all)

Comment 4

[Mark Pilgrim (inactive)]

Attachment: "after" screenshot (focus indicator around Advanced)

Comment 5

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 219761 [details] [diff] [review]
implement focus rectangle in winstripe theme

Assuming that, like with tabs, visible focus isn't given to the pane icon onClick (but is on a subsequent click or on a cursor movement), I'm OK with this. Otherwise we'd get what's a pretty ugly focus indicator every time someone switches prefpanes.

Comment 6

[Asaf Romano (gone)]

Comment on attachment 219761 [details] [diff] [review]
implement focus rectangle in winstripe theme

Minusing per beltzner's comment as this isn't the current behavior (we're using a radiogroup).

Comment 7

[Mark Pilgrim (inactive)]

I spoke with beltzer about this and the focus indicator is not the problem.  The focus indicator itself is fine.  But he feels that, when you *click* on a prefpane icon, focus should move to the first focusable element within the pane.  (Tabs in a tabbox work this way.)  Then if you pressed shift-tab, focus would shift back to the prefpane icon and you would see the focus indicator as expected.

So the only change needed to this patch is an additional check onclick of the prefpane icon, to automatically focus the first focusable element within the prefpane.

Comment 8

[Asaf Romano (gone)]

what you really want to add is something like this in preferences.css

radio[pane] {
  -moz-user-focus: none;
}

radio[pane][selected="true"] {
  -moz-user-focus: normal
}

Comment 9

[Asaf Romano (gone)]

with a semicolon :)

Comment 10

[Mark Pilgrim (inactive)]

*** Bug 345862 has been marked as a duplicate of this bug. ***

Comment 11

[Aaron Leventhal]

Mano, I tried this out and it doesn't seem to do anything. What's it supposed to do?
Do you have a problem if we take Mark's approach and then have an onclick handler which moves focus to the first focusable item inside the pane?

(In reply to comment #8)
> what you really want to add is something like this in preferences.css
> radio[pane] {
>   -moz-user-focus: none;
> }
> radio[pane][selected="true"] {
>   -moz-user-focus: normal
> }
> 

Comment 12

[Mike Beltzner [:beltzner, not reading bugmail]]

From IRC this evening:

<@aaronlev> i like the idea about advancing focus if you click on the icon, like tabs but what about when the prefwindow first comes up, where should focus be?
<@aaronlev> i mean right now when you enter that window, you can start arrowing right away to get to a different pane so it will be different
<@beltzner> can we hide the focus rect until the first keypress?
<@beltzner> that'd work too, really
<@aaronlev> beltzner: that would be okay with me
<@beltzner> gold star

Aaron's right; people will be used to being able to left/right arrow their way between panes when they first flip it open. So while the click-on-pane should automatically give focus to the first item in the list, when the pref window first opens, focus should be (invisibly) given to the pane item. Left/right moves it along, tab highlights the first field, a mouse click selects-and-forwards focus. Everybody goes home happy.

Comment 13

[Aaron Leventhal]

Attachment: More complex than I thought it would be, see comments

1) On dialog launch, start out with focus on the current pane icon just as it is now
2) Shows focus on the pane icons only if it's clear the user is navigating that strip. This happens after arrowing or after clicking on the currently selected icon
3) When an icon for a pane other than the current one is clicked on, window.advanceFocusIntoSubtree is flagged to true. Then the focus is advanced when the pane is ready (either in showPane if already loaded or in the paneload handler if not). The flag is reset after focus has been advanced.
4) The focus on a paneicon is only visible if the containing radiogroup has the attribute showfocus="true"

I suggest we target this just for trunk right now, unless we can devise a simpler approach, which I could not.

Comment 14

[Aaron Leventhal]

What CSS file should the rules be in so that the addons dialog is also affected? That is big 345862.

Comment 15

[Asaf Romano (gone)]

Aaron, could we handle this the way we handle tabs? i.e. something like:

prefpane {
 -moz-user-focus: none;
}

prefpande[selected="true"] {
 -moz-user-focus: normal;
}

Comment 16

[Aaron Leventhal]

(In reply to comment #15)
> Aaron, could we handle this the way we handle tabs? i.e. something like:
I tried what you had in a previous comment, not that I understand how it would work. It didn't seem to affect anything. It would be great if something simple like that worked, but I couldn't follow it.

Comment 17

[Asaf Romano (gone)]

let me make it cleaner, put something like this in preferences.css

radio[pane] {
  -moz-user-focus: none; /* make the pane selectors not-focusable */
}

radio[pane][selected="true"] {
  -moz-user-focus: normal /* make the selected pane selector focusable */
}

Comment 18

[Aaron Leventhal]

(In reply to comment #17)
> let me make it cleaner, put something like this in preferences.css

Right, that's exactly what I tried. It didn't work for me. Maybe you should try it and see what's wrong with it?

Comment 19

[Asaf Romano (gone)]

Comment on attachment 231112 [details] [diff] [review]
More complex than I thought it would be, see comments

>Index: toolkit/content/widgets/preferences.xml
>===================================================================
          
>             var obs = new OverlayLoadObserver(aPaneElement);
>             document.loadOverlay(aPaneElement.src, obs);
>           }
>-          else
>+          else {
>             this._selectPane(aPaneElement);
>+            aPaneElement.advanceFocusIfNecessary();

You could call advanceFocusIfNecessary in _selectPane which is called either way (initial load and when on show).

>+        // Click handler for panebutton will override this so that
>+        // focuses from clicks don't show dotted rectangle,
>+        // unless it was the second click on the same panebutton,
>+        // which causes the panebutton to receive focus
>+        this._selector.setAttribute("showfocus", "true");

The click event is dispatched *before* the command event so this does not make  sense ("Click will override").  How about doing this in a mousedown handler (on  the panebutton binding)? Something like

  if (this.selected) {
    this.parentNode.setAttribute("showfocus", "true");

..and remove the addition to the command handler and the click handler.

>       
>+      <method name="advanceFocusIfNecessary">
>+        <body>
>+        <![CDATA[
>+          if (window.advanceFocusOnPaneLoad) {

please put this flag as a xbl field on the prefwindow element rather than as a js property on the global js object (the prefwindow would be document.getBindingParent(this) at this context).

>+    <handlers>
>+      <handler event="click">
>+        <![CDATA[
>+          if (this.selected)
>+            window.advanceFocusOnPaneLoad = false;
>+          else {
>+            function dontShowFocus(aElement)
>+            {
>+              aElement.removeAttribute("showfocus");
>+            }
>+            window.advanceFocusOnPaneLoad = true;
>+            setTimeout(dontShowFocus, 0, this.parentNode);

why is this setTimeout'ed?

>Index: toolkit/themes/winstripe/global/preferences.css
>===================================================================


>+radiogroup[showfocus="true"]:focus > radio[pane][selected="true"] {
>+  outline: 1px dotted invert;
>+  outline-offset: -1px;
>+}

File a follow-up bug on adding this to Pinstripe.

Comment 20

[Aaron Leventhal]

Let's fix this for Firefox 3. Back to Mark to finish up.

Comment 21

[Jason Spiro]

Thunderbird and Sunbird also lack focus rectangles on their prefs window's top bar. I have just filed bug 363192 for Thunderbird and bug 363193 for Sunbird.

Comment 22

[Mark Pilgrim (inactive)]

Attachment: patch for minefield

Moved click handling to mousedown event, changed _advanceFocusOnPaneLoad to be an XBL field, addressed other nits.

Comment 23

[Mark Pilgrim (inactive)]

Attachment: patch for minefield

Moved click handling to mousedown event, changed _advanceFocusOnPaneLoad to be an XBL field, addressed other nits.

Comment 24

[Asaf Romano (gone)]

Comment on attachment 248424 [details] [diff] [review]
patch for minefield


>Index: toolkit/content/widgets/preferences.xml
>===================================================================

>       
>       <property name="_shouldAnimate">
>         <getter>
>         <![CDATA[
>           var psvc = Components.classes["@mozilla.org/preferences-service;1"]
>@@ -982,16 +984,18 @@
>             params.SetString(1, topic);
>             const ww = Components.classes["@mozilla.org/embedcomp/window-watcher;1"]
>                                  .getService(Components.interfaces.nsIWindowWatcher);
>             ww.openWindow(null, "chrome://help/content/help.xul", "_blank", "chrome,all,alwaysRaised,dialog=no", params);
>           }
>         ]]>
>         </body>
>       </method>
>+
>+      <field name="_advanceFocusOnPaneLoad">true</field>

nit: move this to the fields "section".

>       <handler event="command">
>+        this._selector.setAttribute("showfocus", "true");

This belongs inside the if block:

>         if (event.originalTarget.hasAttribute("pane")) {
>           var pane = document.getElementById(event.originalTarget.getAttribute("pane"));
>           event.originalTarget.parentNode.parentNode.showPane(pane);
>         }
>       </handler>
> 

>+      <method name="advanceFocusIfNecessary">

nit: prefix pseudo-private methods with _.

>+      <handler event="mousedown">
>+        <![CDATA[
>+          var prefWindow = document.getBindingParent(this);
>+          if (this.selected) {
>+            prefWindow._advanceFocusOnPaneLoad = false;
>+            this.parentNode.setAttribute("showfocus", "true");

showfocus is set in the command handler.


>Index: toolkit/themes/winstripe/global/preferences.css
>===================================================================

>+radiogroup[showfocus="true"]:focus > radio[pane][selected="true"] {
>+  outline: 1px dotted invert;
>+  outline-offset: -1px;
>+}

Please use the paneSelector class here.

Looks good otherwise, I didn't test this though.

Comment 28

[skierpage]

From bug 453716, Tools > Add-ons and (new in Firefox 3) Tools > Page Info have the same bug.  This makes keyboard navigation in all three dialogs difficult.

Comment 29

[Asa Dotzler [:asa]]

The latest incarnations of the Options window and the Add-ons window both show a focus outline and a color change for the selected categories. Tested on the latest nightly build on Windows.