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

VERIFIED FIXED in mozilla1.8alpha6

Status

()

defect
VERIFIED FIXED
17 years ago
2 months ago

People

(Reporter: bugzilla, Assigned: mano)

Tracking

({platform-parity})

Trunk
mozilla1.8alpha6
PowerPC
macOS
Points:
---
Dependency tree / graph
Bug Flags:
blocking-aviary1.0mac +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

v3
12.18 KB, patch
aaronlev
: review+
Details | Diff | Splinter Review
1.51 KB, patch
mano
: review+
Details | Diff | Splinter Review
2.45 KB, patch
Details | Diff | Splinter Review
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...
Reporter

Comment 1

17 years ago
how doable would this be?
No longer blocks: 169489
Reporter

Updated

17 years ago
Blocks: 169489

Comment 2

17 years ago
> 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

17 years ago
Who wants it -- Kathy,  Simon, Akkana or Brian?
Assignee: aaronl → brade
Reporter

Comment 4

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

Comment 5

16 years ago
adt: nsbeta1-
Keywords: nsbeta1nsbeta1-

Comment 6

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

Comment 7

15 years ago
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

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

Comment 9

15 years ago
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.
*** Bug 242831 has been marked as a duplicate of this bug. ***
Flags: blocking-aviary1.0mac?

Comment 11

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

Updated

15 years ago
Flags: blocking-aviary1.0mac? → blocking-aviary1.0mac+
*** Bug 262878 has been marked as a duplicate of this bug. ***

Updated

15 years ago
Blocks: macmeta
*** Bug 266502 has been marked as a duplicate of this bug. ***
*** Bug 266907 has been marked as a duplicate of this bug. ***

Updated

15 years ago
Summary: Mac: tabbing navigation should respect keyboard system preference setting → Follow "full keyboard access" setting in System Preferences for tabbing navigation
moving blocking-aviary1.0mac+ bugs to Firefox1.1 and Mozilla 1.8a6 Target
Milestones.
Target Milestone: --- → mozilla1.8alpha6
*** Bug 273491 has been marked as a duplicate of this bug. ***
*** Bug 273614 has been marked as a duplicate of this bug. ***
*** Bug 273701 has been marked as a duplicate of this bug. ***
Posted patch patch v1 (obsolete) — Splinter Review
Aaron, my r= request is for the XP part only. I will ask pedemonte for the
Widget:Mac part.
Assignee: brade → bugs.mano
Status: NEW → ASSIGNED
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.
Attachment #169333 - Flags: review?(aaronleventhal)

Comment 21

15 years ago
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.
Attachment #169333 - Flags: review?(aaronleventhal) → review+
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.
Posted patch v1.1 (obsolete) — Splinter Review
fix the typo
Attachment #169333 - Attachment is obsolete: true
Comment on attachment 169374 [details] [diff] [review]
v1.1

Requestig r= from pedemonte for the mac part.

Neil, please see comment 22.
Attachment #169374 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169374 - Flags: review?(jhpedemonte)
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"));
(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 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)
Attachment #169401 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169401 - Flags: review?(aaronleventhal)

Updated

15 years ago
Attachment #169401 - Flags: review?(aaronleventhal) → review+
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.
Attachment #169374 - Flags: review?(jhpedemonte) → review+
Attachment #169374 - Attachment is obsolete: true
Attachment #169374 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169401 - Attachment is obsolete: true
Attachment #169401 - Flags: superreview?(neil.parkwaycc.co.uk)
Posted patch v1.2 (obsolete) — Splinter Review
Addressing Javier's comments.

moving r= from Aaaron & Javier
Attachment #169470 - Flags: review+
Attachment #169470 - Flags: superreview?(neil.parkwaycc.co.uk)
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.
Attachment #169470 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview-
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

15 years ago
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

15 years ago
Neil, to clarify: I don't think we need any tabbing prefs expose in our UI for
Mac. Just follow system prefs there.
Posted patch v2 (obsolete) — Splinter Review
Attachment #169470 - Attachment is obsolete: true
Attachment #169501 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169501 - Flags: review?(aaronleventhal)
Comment on attachment 169501 [details] [diff] [review]
v2

Hmm... shouldn't you move the registerCallback line under the if (!/Mac/.test
Comment on attachment 169501 [details] [diff] [review]
v2

yeah, my bad.
Attachment #169501 - Attachment is obsolete: true
Attachment #169501 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169501 - Flags: review?(aaronleventhal)
Actually no, it also save linksonly.
Posted patch v2.1 (obsolete) — Splinter Review
handle prefs as required
Attachment #169548 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169548 - Flags: review?(aaronleventhal)
(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

15 years ago
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.
Attachment #169548 - Flags: review?(aaronleventhal) → review-
Attachment #169548 - Flags: superreview?(neil.parkwaycc.co.uk)
Aaron / Neil: woudn't it be better to check tabFocuseModel in isFocusable?

Comment 43

15 years ago
(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
(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?
(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.
argh, ShiftFocus calls isFocusable at a loop <sigh />
Posted patch [wip] v2.2 (obsolete) — Splinter Review
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.
Attachment #169548 - Attachment is obsolete: true
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 ;-)
(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.
*** Bug 276356 has been marked as a duplicate of this bug. ***
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.
Posted patch v3Splinter Review
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
Attachment #169986 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #169986 - Flags: review?(aaronleventhal)
*** Bug 276930 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla1.8alpha6 → mozilla1.8beta

Updated

15 years ago
Attachment #169986 - Flags: review?(aaronleventhal) → review+
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?
Attachment #169986 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Comment on attachment 169986 [details] [diff] [review]
v3

a=asa (on behalf of drivers) for checkin to 1.8a6.
Attachment #169986 - Flags: approval1.8a6? → approval1.8a6+
Target Milestone: mozilla1.8beta → mozilla1.8alpha6
checked in by timeless.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → FIXED
+        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.
Thanks Simon.
Attachment #170539 - Flags: superreview?(neil.parkwaycc.co.uk)
Comment on attachment 170539 [details] [diff] [review]
Add null check

Simon, I hope you're still reading.
Attachment #170539 - Flags: review?(sfraser_bugs)
the null check looks fine to me. i'm assuming we pick this up in cocoa widget
for free?
Comment on attachment 170539 [details] [diff] [review]
Add null check

r=pink on irc.
Attachment #170539 - Flags: review?(sfraser_bugs) → review+

Updated

15 years ago
Attachment #170539 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
This looks like it may have turned the boxset thunderbird tinderbox orange.
(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 on attachment 170539 [details] [diff] [review]
Add null check

a=asa for checkin to 1.8a6
Attachment #170539 - Flags: approval1.8a6? → approval1.8a6+
The orange was fixed by the recent checkin.
There's still a memory leak here. You need to call
CFRelease(fullKeyboardAccessProperty) if it's not null.
I will file a separate bug, we're done here.

Comment 69

15 years ago
(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!
Reporter

Comment 71

15 years ago
[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...
Keywords: relnote
sairuh, if yopu ever changed a*.tabfocus on this profile "Full keyboard access"
is ignored.

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

Comment 73

15 years ago
(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.
Status: RESOLVED → VERIFIED
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.
I checked in this patch, which makes this work on all machines, not just yours.
Interestring, what's else stored in AppleKeyboardUIMode (FYI, nothing of this is
documented)?
"Full keyboard access" is stored in the lowest bit; "tab to any control" is
stored in the next bit.
well, on 10.3 there is only one pref, i guess it was different in <10.3.
*** Bug 285639 has been marked as a duplicate of this bug. ***
*** Bug 260557 has been marked as a duplicate of this bug. ***

Comment 81

14 years ago
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

14 years ago
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.
(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.
(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.
*** Bug 296869 has been marked as a duplicate of this bug. ***
*** Bug 298473 has been marked as a duplicate of this bug. ***
*** Bug 301132 has been marked as a duplicate of this bug. ***
*** Bug 310886 has been marked as a duplicate of this bug. ***
*** Bug 343994 has been marked as a duplicate of this bug. ***

Comment 91

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

Updated

13 years ago
Duplicate of this bug: 367230
Duplicate of this bug: 348315

Updated

12 years ago
Duplicate of this bug: 344272
I've filed bug 437296 to recommend that we no longer follow this setting, at least not by default.

Updated

10 years ago
Duplicate of this bug: 525885
Component: Keyboard: Navigation → User events and focus handling
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.