Closed Bug 187508 Opened 22 years ago Closed 20 years ago

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

Categories

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

PowerPC
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.8alpha6

People

(Reporter: bugzilla, Assigned: asaf)

References

Details

(Keywords: platform-parity)

Attachments

(3 files, 7 obsolete files)

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...
how doable would this be?
No longer blocks: 169489
Blocks: 169489
> 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.
Who wants it -- Kathy, Simon, Akkana or Brian?
Assignee: aaronl → brade
see also bug 123617, where keybd nav in aqua/classic theme is broken.
adt: nsbeta1-
Keywords: nsbeta1nsbeta1-
*** Bug 244118 has been marked as a duplicate of this bug. ***
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.
*** Bug 223328 has been marked as a duplicate of this bug. ***
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?
I didn't realize this bug was assigned to me; I don't have cycles to work on this now. :-(
Keywords: helpwanted
Flags: blocking-aviary1.0mac? → blocking-aviary1.0mac+
*** Bug 262878 has been marked as a duplicate of this bug. ***
Blocks: macmeta
*** Bug 266502 has been marked as a duplicate of this bug. ***
*** Bug 266907 has been marked as a duplicate of this bug. ***
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. ***
Attached 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)
Keywords: helpwanted, nsbeta1-pp
Attachment #169333 - Flags: superreview?(sfraser_bugs)
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.
Attachment #169333 - Flags: superreview?(sfraser_bugs)
Attached 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)
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)
Attached 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.
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.
Neil, to clarify: I don't think we need any tabbing prefs expose in our UI for Mac. Just follow system prefs there.
Attached 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.
Attached 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 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?
(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 />
Attached 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.
Attached 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)
Attachment #169637 - Attachment is obsolete: true
*** Bug 276930 has been marked as a duplicate of this bug. ***
Target Milestone: mozilla1.8alpha6 → mozilla1.8beta
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+
Attachment #169986 - Flags: approval1.8a6?
Keywords: relnote
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
Closed: 20 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.
Attached patch Add null checkSplinter Review
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+
Attachment #170539 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Attachment #170539 - Flags: approval1.8a6?
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+
checked in
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.
Blocks: 277467
(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!
no, that is bug 277516.
[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)
(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. ***
Blocks: deermac
*** Bug 260557 has been marked as a duplicate of this bug. ***
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?
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. ***
*** Bug 349357 has been marked as a duplicate of this bug. ***
I've filed bug 437296 to recommend that we no longer follow this setting, at least not by default.
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: