187508 - Follow "full keyboard access" setting in System Preferences for tabbing navigation

Status: VERIFIED FIXED

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

Description

[sairuh (rarely reading bugmail)]

spun off from bug 169489 comment 23. for mozilla (and, methinks, other mac embed
apps), we should respect keyboard system preference setting in Mac OS X.

not sure how fixing this would affect bugs like bug 169767 and bug 170833...

Comment 1

[sairuh (rarely reading bugmail)]

how doable would this be?

Comment 2

[Aaron Leventhal]

> how doable would this be?
As long as we can get the setting from nsILookAndFeel, it's doable.
- First we need to remove the checkbox corresponding to "Buttons, radio buttons,
check boxes and lists" from the Keyboard Navigation prefs panel (remove on mac
only). This checkbox is removed from there, because we can get that value from
the mac system pref. The other checkbox (for links) can stay, because users may
still want to control that.
- Then we need to replace the bit corresponding to non-text form controls in
esm's sTabFocusModel with the boolean value from the mac keyboard nav system
prefs. Here is sample code for that:

void nsEventStateManager::ResetObservedPrefs()
{
  // Tab focus mode is constant across all windows.
  // It would be nicer if we could make sure this was called only
  // once, each time the pref changed, instead of once per esm

  sTabFocusModel = eTabFocus_any;
  nsresult rv = getPrefBranch();

  // XXX todo: On the mac, only links tabbing should be configurable in Mozilla
  //           Whether other form controls are tabbable is a system setting
  //           that we should adhere to via look and feel.
  // Something like this:
  // sTabFocusModel &= eTabFocus_linksMask;
  // nsCOMPtr<nsILookAndFeel> lookNFeel(do_GetService(kLookAndFeelCID));
  // PRInt32 tabToNonTextControls = 0;
  // lookNFeel->GetMetric(nsILookAndFeel::eMetric_TabToNonTextControls,
  //                      tabToNonTextControls);
  // if (tabToNonTextControls)
  //   sTabFocusModel |= eTabFocus_formElementsMask;
  // else
  //   sTabFocusModel = eTabFocus_textControlsMask;  // Only tab to text fields
  // This assumes that when the setting says "only text controls"
  // that links shouldn't be tabbable.
  // Also, we currently ignore the pref for XUL dialogs
  // but we should pay attention to the mac setting there

  if (NS_SUCCEEDED(rv))
    mPrefBranch->GetIntPref("accessibility.tabfocus", &sTabFocusModel);
}

Note that we don't currently pay attention to this setting for XUL dialogs, but
I think we should be consistent. I think we ignore it in XUL because Akkana she
doesn't want to tab to buttons in HTML content, but wants to in dialogs.

It shouldn't affect either bug 169767 and bug 170833, which Sarah mentioned above.

Comment 3

[Aaron Leventhal]

Who wants it -- Kathy,  Simon, Akkana or Brian?

Comment 4

[sairuh (rarely reading bugmail)]

see also bug 123617, where keybd nav in aqua/classic theme is broken.

Comment 5

[Samir Gehani]

adt: nsbeta1-

Comment 6

[HARUNAGA Hirotoshi]

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

Comment 7

[Devin Bryant]

Camino and Safari both currently follow the behavior described in the OSX
Keyboard preferences as they should as OSX applications, Firefox .9.1 still does
not. This is a UI inconsitency and a common user complaint, I am unclear as to
why the issue hasn't been touched on here in over a year.

Comment 8

[Aaron Leventhal]

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

Comment 9

[Aaron Leventhal]

According to info in bug 223328:
> In "~/Library/Preferences/.GlobalPreferences.plist" look at 
> "AppleKeyboardUIMode", bit 1 
> toggles Full Keyboard Access. If the user's selected full keyboard access, 
> then tab to all links, otherwise only tab to text fields.

Comment 10

[Asaf Romano (gone)]

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

Comment 11

[Kathleen :Brade]

I didn't realize this bug was assigned to me; I don't have cycles to work on
this now.  :-(

Comment 12

[HARUNAGA Hirotoshi]

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

Comment 13

[Phil Ringnalda (:philor)]

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

Comment 14

[Phil Ringnalda (:philor)]

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

Comment 15

[Asa Dotzler [:asa]]

moving blocking-aviary1.0mac+ bugs to Firefox1.1 and Mozilla 1.8a6 Target
Milestones.

Comment 16

[Asaf Romano (gone)]

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

Comment 17

[Asaf Romano (gone)]

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

Comment 18

[Asaf Romano (gone)]

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

Comment 19

[Asaf Romano (gone)]

Attachment: patch v1

Aaron, my r= request is for the XP part only. I will ask pedemonte for the
Widget:Mac part.

Comment 20

[Asaf Romano (gone)]

Comment on attachment 169333 [details] [diff] [review]
patch v1

Aaron, my r= request is for the XP part only. I will ask pedemonte for the
Widget:Mac part.

Comment 21

[Aaron Leventhal]

Comment on attachment 169333 [details] [diff] [review]
patch v1

- Please spell Focus without the "e".
- put it next to the other focus metric in nsILookAndFeel:
169	eMetric_SelectTextfieldsOnKeyFocus,		      // select
textfields when focused via tab/accesskey?
- I would make sUseSystemTabFocusModel a local, unless you see some reason to
make it a static.

Fix those, and r=aaronlev on non-Mac parts.

Comment 22

[Asaf Romano (gone)]

Thanks for review.

Following from IRC.

(In reply to comment #21)

> - put it next to the other focus metric in nsILookAndFeel:
> 169	eMetric_SelectTextfieldsOnKeyFocus,		      // select
> textfields when focused via tab/accesskey?

It seems the enum values are used as ints somewhere... moving the new value into
this point causes some regressions, I'll file a separate bug to cleanup this stuff.

> - I would make sUseSystemTabFocusModel a local, unless you see some reason to
> make it a static.

I did, it is used in the Observer method.

Comment 23

[Asaf Romano (gone)]

Attachment: v1.1

fix the typo

Comment 24

[Asaf Romano (gone)]

Comment on attachment 169374 [details] [diff] [review]
v1.1

Requestig r= from pedemonte for the mac part.

Neil, please see comment 22.

Comment 25

[jhp (no longer active)]

Mano, what if you pass 'CFSTR(".GlobalPreferences")' for the second param of
CFPreferencesCopyValue()?  If that works, then you could switch to using
CFPreferencesCopyAppValue(CFSTR("AppleKeyboardUIMode"),
CFSTR(".GlobalPreferences"));

Comment 26

[Asaf Romano (gone)]

(In reply to comment #25)
> Mano, what if you pass 'CFSTR(".GlobalPreferences")' for the second param of
> CFPreferencesCopyValue()?  If that works, then you could switch to using
> CFPreferencesCopyAppValue(CFSTR("AppleKeyboardUIMode"),
> CFSTR(".GlobalPreferences"));

doesn't work

Comment 27

[Asaf Romano (gone)]

Attachment: right all.js changes; hide tab navigation ui

Comment 28

[Asaf Romano (gone)]

Comment on attachment 169401 [details] [diff] [review]
right all.js changes; hide tab navigation ui

Please ignore the all.js part in the backend patch.

(FYI: the comment in p*-nav.js wan't true, there is one Full Keyboard Accees
preference on mac which is expected to switch between limtied to everthing
focusability)

Comment 29

[jhp (no longer active)]

Comment on attachment 169374 [details] [diff] [review]
v1.1

+    eMetric_SystemTabFocusModel			  // Mac: what's the
expected tabbing cycle, based on a system preference

Line up comment with the others.

+	 fullKeyboardAccessProperty = CFPreferencesCopyValue
(CFSTR("AppleKeyboardUIMode"),
+					  kCFPreferencesAnyApplication,
kCFPreferencesCurrentUser,
+					  kCFPreferencesAnyHost);

For system functions, please preface them with "::", as in
"::CFPreferencesCopyValue(...)".  Also, please try to fit the code within the
80 char limit.

+static PRBool sUseSystemTabFocusModel;

Should be initialized to something; maybe 0.

+      if (!sUseSystemTabFocusModel)
+	 nsIContent::sTabFocusModel =
+	   nsContentUtils::GetIntPref("accessibility.tabfocus",
+				      nsIContent::sTabFocusModel);

Encapsulate the 'if' in {} to make it more legible.  Also in the next few lines
of code.

Otherwise, looks good.	r=jhpedemonte.

Comment 30

[Asaf Romano (gone)]

Attachment: v1.2

Addressing Javier's comments.

moving r= from Aaaron & Javier

Comment 31

[[e-mail]]

Comment on attachment 169470 [details] [diff] [review]
v1.2

This patch is an abuse of nsILookAndFeel which is to provide access to a set of
OS defaults that can be overridden with user prefs. This explains comment 22.

Comment 32

[[e-mail]]

Now, the look and feel system might still be the way to go here. By all means
define a new look and feel constant, but update nsXPLookAndFeel.cpp so that
accessibility.tabfocus is mapped to it. Also, each platform should default the
value to 7, except for the Mac code which is fine as it is. Finally, the pref
window code needs to be written to have a "Use system default" which clears the
pref if it is checked. Note that the look and feel code caches the pref value so
that you might not need to cache it in the ESM.

Comment 33

[Aaron Leventhal]

Neil, the tab focus pref shouldn't even show up on a Mac. No need to add another
pref for this there. We should just follow system settings when they exist on a
given platform.

Comment 34

[Aaron Leventhal]

Neil, to clarify: I don't think we need any tabbing prefs expose in our UI for
Mac. Just follow system prefs there.

Comment 35

[Asaf Romano (gone)]

Attachment: v2

Comment 36

[[e-mail]]

Comment on attachment 169501 [details] [diff] [review]
v2

Hmm... shouldn't you move the registerCallback line under the if (!/Mac/.test

Comment 37

[Asaf Romano (gone)]

Comment on attachment 169501 [details] [diff] [review]
v2

yeah, my bad.

Comment 38

[Asaf Romano (gone)]

Actually no, it also save linksonly.

Comment 39

[Asaf Romano (gone)]

Attachment: v2.1

handle prefs as required

Comment 40

[[e-mail]]

(In reply to comment #38)
>Actually no, it also save linksonly.
Note that it shouldn't have to; prefs has logic that can do that itself.

(From update of attachment 169548 [details] [diff] [review])
>-function initPrefs()
>+function loadPanel()
Also note that the pref logic automatically calls a function named Startup.

Comment 41

[Aaron Leventhal]

Comment on attachment 169548 [details] [diff] [review]
v2.1

 was confused by "+// On OS X, we use Full Keyboard Access system preference,
until accessibility.tabfocus is created"
How about "unless accessibility.tabfocus is set by the user"

Also, sTabFocusModel no longer needs to be a global. We can just make it an
(optional?) parameter for IsFocusable(). Things that aren't calling
IsFocusable() for tab order info can either not pass it in, or pass in some
other value, 0 or 7 or something.

Comment 42

[Asaf Romano (gone)]

Aaron / Neil: woudn't it be better to check tabFocuseModel in isFocusable?

Comment 43

[Aaron Leventhal]

(In reply to comment #42)
> Aaron / Neil: woudn't it be better to check tabFocuseModel in isFocusable?

Duh. Yeah, that makes sense :)
Then you only need to check it if aTabIndex is non-null. When it's null the
caller doesn't care about tabbing.

To make that work right, could you fix my mistake here:
http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#1883
It should have null instead of &tabIndexUnused

Comment 44

[[e-mail]]

(In reply to comment #42)
>Aaron / Neil: woudn't it be better to check tabFocuseModel in isFocusable?
Won't isFocusable get called fairly often, compared to ShiftFocus?

Comment 45

[Asaf Romano (gone)]

(In reply to comment #44)
> (In reply to comment #42)
> >Aaron / Neil: woudn't it be better to check tabFocuseModel in isFocusable?
> Won't isFocusable get called fairly often, compared to ShiftFocus?

AFAIK tabIndex is not a null only when ShiftFocus fires it.

Comment 46

[Asaf Romano (gone)]

argh, ShiftFocus calls isFocusable at a loop <sigh />

Comment 47

[Asaf Romano (gone)]

Attachment: [wip] v2.2

There are still some open issues, waiting for your comments ;)
-- Should the default value be 7 or null?
-- if null, Do we want to check if aTabFocusModel isn't a null in isFocusable
implementations?
-- There are some calls to IsFocusable in nsEventStateManager (i'm not
reffering to nsIFrame one) outside of ShiftFocusInternal which do call it with
a tabIndex, what about them?
-- I'm not sure if ShiftFocusInternal arguments should have default parameters.

Comment 48

[[e-mail]]

Comment on attachment 169637 [details] [diff] [review]
[wip] v2.2

aTabFocusModel is not a * so don't default it to nsnull. You could be really
sneaky and default it to -1 so you could write if ((aTabFocusModel &
eTabFocus_linksMask) == 0) *aTabIndex = -1; because aTabIndex shouldn't be null
if aTabFocusModel is zero ;-)

Comment 49

[[e-mail]]

(In reply to comment #47)
>-- There are some calls to IsFocusable in nsEventStateManager (i'm not
>referring to nsIFrame one) outside of ShiftFocusInternal which do call
>it with a tabIndex, what about them?
You're talking about GetNextTabbableMapArea but that's called from ShiftFocus
via GetNextTabbableContent, so we'd just have to forward aTabFocusModel.
>-- I'm not sure if ShiftFocusInternal arguments should have default parameters.
Probably not worthwhile as there's only one use of the default parameter.

Comment 50

[Asaf Romano (gone)]

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

Comment 51

[[e-mail]]

As the tabfocus value is only used by a limited number of elements in a method
three calls away from ShiftFocus I think trying to pass the value though a
parameter is more trouble than it's worth.

Comment 52

[Asaf Romano (gone)]

Attachment: v3

per last comment(s), it seems we don't want to drop sTabFocusModel (becuase of
calls from ShiftFocusInternal, and 'sub'-calls).

changes from 2.1:
 - update comments
 - use auto-called startup instead of initpref/initpane

Comment 53

[Asaf Romano (gone)]

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

Comment 54

[[e-mail]]

Comment on attachment 169986 [details] [diff] [review]
v3

(In reply to comment #21)
> - put it next to the other focus metric in nsILookAndFeel:
> 169	eMetric_SelectTextfieldsOnKeyFocus,		      // select
Aaron, does this comment no longer apply?

Comment 55

[Asa Dotzler [:asa]]

Comment on attachment 169986 [details] [diff] [review]
v3

a=asa (on behalf of drivers) for checkin to 1.8a6.

Comment 56

[Asaf Romano (gone)]

checked in by timeless.

Comment 57

[Simon Fraser [no longer active]]

+        CFPropertyListRef fullKeyboardAccessProperty;
+        fullKeyboardAccessProperty = ::CFPreferencesCopyValue
(CFSTR("AppleKeyboardUIMode"),
+                                                              
kCFPreferencesAnyApplication,
+                                                              
kCFPreferencesCurrentUser,
+                                                              
kCFPreferencesAnyHost);
+        PRInt32 fullKeyboardAccessPrefVal;
+        ::CFNumberGetValue(fullKeyboardAccessProperty, kCFNumberIntType,
+                           &fullKeyboardAccessPrefVal);

This code should test to see if fullKeyboardAccessProperty is non-NULL before
calling CFNumberGetValue.

Comment 58

[Asaf Romano (gone)]

Attachment: Add null check

Thanks Simon.

Comment 59

[Asaf Romano (gone)]

Comment on attachment 170539 [details] [diff] [review]
Add null check

Simon, I hope you're still reading.

Comment 60

[Mike Pinkerton (not reading bugmail)]

the null check looks fine to me. i'm assuming we pick this up in cocoa widget
for free?

Comment 61

[Asaf Romano (gone)]

Comment on attachment 170539 [details] [diff] [review]
Add null check

r=pink on irc.

Comment 62

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This looks like it may have turned the boxset thunderbird tinderbox orange.

Comment 63

[Asaf Romano (gone)]

(In reply to comment #62)
> This looks like it may have turned the boxset thunderbird tinderbox orange.

It /may/ be a result of the missing null check.

Comment 64

[Asa Dotzler [:asa]]

Comment on attachment 170539 [details] [diff] [review]
Add null check

a=asa for checkin to 1.8a6

Comment 65

[Asaf Romano (gone)]

checked in

Comment 66

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The orange was fixed by the recent checkin.

Comment 67

[Simon Fraser [no longer active]]

There's still a memory leak here. You need to call
CFRelease(fullKeyboardAccessProperty) if it's not null.

Comment 68

[Asaf Romano (gone)]

I will file a separate bug, we're done here.

Comment 69

[Jared Jacobs]

(In reply to comment #2)
>   // Also, we currently ignore the pref for XUL dialogs
>   // but we should pay attention to the mac setting there
>
> Note that we don't currently pay attention to this setting for XUL
> dialogs, but I think we should be consistent. I think we ignore it
> in XUL because Akkana she doesn't want to tab to buttons in HTML
> content, but wants to in dialogs.

Thanks for fixing this! I'm curious -- does your fix also make dialog boxes
abide by the System pref, such as the one that says "Enter username and
password..." at a website that requires authentication? When the full keyboard
access system pref is set, I should be able to tab to the "Use Password Manager
to remember this password" checkbox and the OK/Cancel buttons. If this is not
fixed yet, I want to open a new bug. Thanks again!

Comment 70

[Asaf Romano (gone)]

no, that is bug 277516.

Comment 71

[sairuh (rarely reading bugmail)]

[tested on Mac OS X 10.3.7.] using 200501090x-trunk builds of Firefox and
Mozilla, this now works:

a. with the system preference ON, tabbing navigation in a web page goes to all
html form controls and links.

b. with the system preference OFF, tabbing navigation in a web page only goes
between input areas (single line textfields and textareas).

when testing with Camino 2005010808-trunk, it seems like tabbing navigation goes
to all form controls but not links, when the system pref is ON. my profile is a
bit old, so let me doublecheck my prefs file...

Comment 72

[Asaf Romano (gone)]

sairuh, if yopu ever changed a*.tabfocus on this profile "Full keyboard access"
is ignored.

(btw, why have you removed "relnote" keyword)

Comment 73

[sairuh (rarely reading bugmail)]

(In reply to comment #72)
> sairuh, if yopu ever changed a*.tabfocus on this profile "Full keyboard access"
> is ignored.

yeah, I checked for that pref, and it turned out that I hadn't changed
accessibility.tabfocus. I filed bug 277706 for this issue --since even Safari
has separate pref UI for tabbing to web page links.

> (btw, why have you removed "relnote" keyword)

since it's now implemented, I thought we no longer need to relnote this issue.
but if you feel we do, go ahead and add it back.

so I'm verifying this as fixed --Asaf, thanks a bunch for implementing this!
remaining issues (like bug 277706) should be opened as separate bugs.

Comment 74

[Simon Fraser [no longer active]]

This isn't working for me. When I have the 'full keyboard access' option turned
on (mac os x 10.3), the 
"AppleKeyboardUIMode" is set to 2, not 3. It's 0 when turned off.

If you read the comments here, you'll see that you need to test bit 1 of the
value. I'll just fix it.

Comment 75

[Simon Fraser [no longer active]]

Attachment: Patch fixing the bug for real.

I checked in this patch, which makes this work on all machines, not just yours.

Comment 76

[Asaf Romano (gone)]

Interestring, what's else stored in AppleKeyboardUIMode (FYI, nothing of this is
documented)?

Comment 77

[Wevah (Nate Weaver)]

"Full keyboard access" is stored in the lowest bit; "tab to any control" is
stored in the next bit.

Comment 78

[Asaf Romano (gone)]

well, on 10.3 there is only one pref, i guess it was different in <10.3.

Comment 79

[Asaf Romano (gone)]

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

Comment 80

[Asaf Romano (gone)]

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

Comment 81

[José Jeria]

I have the "Turn on full keyboard access" enabled in the system prefereces,
using trunk build 20050516 on 10.3.9, and I cant tab trough buttons in the
Firefox preferences, only the checkboxes and dropdowns. 

In other mac apps its possible to tab trough buttons as well. Also in the about
dialog the tabbing does not work. Is there a bug for this? Or am i missing
something?

Comment 82

[Percy]

Asaf,

In the comments for Bug 285639 (which I submitted) you wrote:

This is fixed for the next release, not 1.0.

*** This bug has been marked as a duplicate of 187508 ***

Are you saying this is not going to be fixed in 1.0 at all?  Do I have to wait
for 1.1.x or 2.0?  Because I am using 1.0.4 now and I am still having this
problem.  Just wanted to check if I'm having to wait until the next major
release...  I thought since it is fixed it would have been included in the 1.0
releases.

Comment 83

[Asaf Romano (gone)]

(In reply to comment #81)
> I have the "Turn on full keyboard access" enabled in the system prefereces,
> using trunk build 20050516 on 10.3.9, and I cant tab trough buttons in the
> Firefox preferences, only the checkboxes and dropdowns. 
> 
> In other mac apps its possible to tab trough buttons as well. Also in the about
> dialog the tabbing does not work. Is there a bug for this? Or am i missing
> something?

Note that this bug was about webpages (i.e. form buttons in webpages are
focusable when full-keyboard-access is on). However, It has been enabled for XUL
in bug 277516 (backend) and bug 242831 (frontend). For UI, we have excluded xul
buttons since we can't yet draw the focus ring around them (see bug 203734) due
to an Appearance Manager bug.

Comment 84

[Asaf Romano (gone)]

(In reply to comment #82)

> Are you saying this is not going to be fixed in 1.0 at all?  Do I have to wait
> for 1.1.x or 2.0?

1.1.x, you can also try a nightly build. Please take any further discussion to
the newsgroups/MozillaZine forums.

Comment 85

[Asaf Romano (gone)]

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

Comment 86

[Asaf Romano (gone)]

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

Comment 87

[Asaf Romano (gone)]

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

Comment 88

[Asaf Romano (gone)]

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

Comment 89

[Phil Ringnalda (:philor)]

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

Comment 90

[jonathan chetwynd]

are these 2 bugs related?

https://bugzilla.mozilla.org/show_bug.cgi?id=342295

https://bugzilla.mozilla.org/show_bug.cgi?id=277706

Comment 91

[Adam Guthrie]

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

Comment 95

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

I've filed bug 437296 to recommend that we no longer follow this setting, at least not by default.