[RFE] Tab usage prefs: either "to all links" or "to text fields"

VERIFIED FIXED in mozilla1.2beta



Keyboard: Navigation
15 years ago
15 years ago


(Reporter: Brett Koonce, Assigned: Akkana Peck)


(Blocks: 1 bug, {topembed})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)




(1 attachment, 3 obsolete attachments)



15 years ago
This is a simple feature straight out of that 'other' browser: the ability to
set tabbing to either jump to each link on page OR to only jump between text
fields.  If we really wanna go crazy, 'alt-tab' or some other key combo should
do the alternate tab usage.

I prefer the latter, but that's me.  It should be an option.


I'm sorry if this is a dupe...I couldn't find any matches in bugzilla.
confirming rfe
Ever confirmed: true
OS: MacOS X → All
Hardware: Macintosh → All

Comment 2

15 years ago
See bug 66285 for some related discussion.

Comment 3

15 years ago
-> Akkana
Assignee: aaronl → akkana

Comment 4

15 years ago
Setting milestone based on a hope that the backend for this already exists.  If
focus code has to be rewritten, it will take longer.
Target Milestone: --- → mozilla1.2alpha

Comment 5

15 years ago
Bryner pointed me in the right direction. and I have a patch.  We discussed
possibly making a new attribute like -moz-user-focus to set in addition to
-moz-user-focus, to make it stronger (or weaker); but the current tabbing code
in nsEventStateManager isn't looking at -moz-user-focus, so I haven't done
anything like that.

Adding Simon to the cc list because I seem to remember him expressing interest
in this issue in another bug.

There are three levels: tab only to text controls; tab to any form control; tab
to all form controls plus links.

I have currently set the defaults this way:
Windows: same as before (tab to everything including links).
Unix: Same as 4.x, tab only to text controls.
Mac: same as before, tab to everything, which is what OS X IE does -- I don't
have a mac version of 4.x to check.  iCab doesn't tab between elements at all,
just between page-as-a-whole and urlbar.

Comment 6

15 years ago
Created attachment 96378 [details] [diff] [review]
do it

Comment 7

15 years ago
Akkana, I think you should use enums for the tab focus model, it will make the
code much easier to understand.

It's not obvious what this does:
else if (mTabFocusModel < 2 && nsHTMLAtoms::button==tag.get()) {

At least add a comment so people don't break your code later.

Comment 8

15 years ago
CC'ing Sun Beijing accessibility team, since this would change the default
tabbing behavior on their platform. Please let us know if this is going to
affect you.

I think this is a big change for Linux users, even though 4.x did it that way.
I'm worried about people not knowing how to get around it, since the pref is not
exposed in the UI. There are serious consequences for people who can't use the
mouse. They will be unable to do anything unless they know enough to edit their
prefs.js file, which most people aren't. I understand this is better for most
users, but please convince me that this isn't a problem.

Comment 9

15 years ago
Created attachment 97260 [details] [diff] [review]
Patch addressing Aaron's comments

That's fair.  I thought about doing enums but didn't because the code is more
complicated with them and the prefs can't use them anyway.  But they're
probably better even so, and I've rewritten the patch to use them.  I've also
added comments; I think most of the unclearness of the code was there before I
got there :-) but perhaps the comment for each clause will make the flow
clearer.  If nothing else, it'll help point out whether I've misunderstood the
existing code.
Attachment #96378 - Attachment is obsolete: true

Comment 10

15 years ago
Aaron suggested bryner should probably look at this since it affects focus
handling and someone experienced with focus needs to review it.

Comment 11

15 years ago
I hope we do expose this pref at some point - if that's the case, would it hurt
anything to add a pref listener for this?

// text is always in the tab order
Nit: when I read "text" I don't think of text inputs, I think of text nodes

Why would would you want the default to be text inputs only on UNIX. How is it
useful to skip past buttons and selects?

Comment 12

15 years ago
> Nit: when I read "text" I don't think of text inputs, I think of text nodes

Good point.  I've changed that comment to say "text controls are always in the
tab order".

> Why would would you want the default to be text inputs only on UNIX. How is it
> useful to skip past buttons and selects?

That's what we always did in the past on Unix netscape, and my guess is that it
matches most people's habits: I tab to get to a text field because I'm about to
type so I want my hands on the keyboard, but for things like popup menus I'm
more likely to use the mouse so I don't need to be able to tab there, and having
to tab through a lot of other form elements to get to the one text field is

I'm open to discussion if other Unix users disagree with me.
Comment on attachment 97260 [details] [diff] [review]
Patch addressing Aaron's comments

+  // Tab focus models: 0 (default) focuses links and form elements,
+  // 1 focuses form elements but not links, 2 focuses only text controls.
+  enum nsTabFocusModel {
+    eTabFocus_linksFormsText=0,
+    eTabFocus_formsText=1,
+    eTabFocus_textOnly=2,
+    eTabFocus_unset=-1
+  };

I almost wonder if this would be better as a bitfield... that would simplify
some of the comparisons below.

>@@ -263,6 +271,9 @@
>   // So we don't have to keep checking accessibility.browsewithcaret pref
>   PRBool mBrowseWithCaret;
>+  // Which types of elements are in the tab order?
>+  PRInt32 mTabFocusModel;

This is going to be global, isn't it?  I'd make it a static class variable.

>+      // Tab focus mode is constant across all windows.
>+      // It would be nicer if ESM had a prefs callback,
>+      // so we could store this and change behavior when it changes.
>+      // But as long as the pref isn't exposed, that doesn't matter.
>+      // 0=form controls and links, 1=all form controls, 2=text controls.
>+      if (mTabFocusModel == eTabFocus_unset) {
>+        mTabFocusModel = eTabFocus_linksFormsText;
>+        nsresult rv = getPrefService();
>+        if (NS_SUCCEEDED(rv))
>+          mPrefService->GetIntPref("accessibility.tabfocus", &mTabFocusModel);
>+      }

... and then we wouldn't have to fetch this pref again for each document.

>       child->GetTag(*getter_AddRefs(tag));
>       nsCOMPtr<nsIDOMHTMLElement> htmlElement(do_QueryInterface(child));
>       if (htmlElement) {
>@@ -3280,7 +3293,14 @@
>             nsAutoString type;
>             nextInput->GetType(type);
>-            if (type.EqualsIgnoreCase("hidden")) {
>+            if (type.EqualsIgnoreCase("text")) {
>+              disabled = PR_FALSE;  // textfields are always in the tab order
>+            }
>+            else if (mTabFocusModel == eTabFocus_textOnly) {
>+              // tab focus model specifies only text, and we're not.
>+              disabled = PR_TRUE;
>+            }
>+            else if (type.EqualsIgnoreCase("hidden")) {
>               hidden = PR_TRUE;
>             }
>             else if (type.EqualsIgnoreCase("file")) {
>@@ -3288,13 +3308,17 @@
>             }
>           }
>         }
>-        else if (nsHTMLAtoms::select==tag.get()) {
>+        // Select counts as form but not as text
>+        else if ((mTabFocusModel == eTabFocus_linksFormsText
>+                   || mTabFocusModel == eTabFocus_formsText)
>+                 && nsHTMLAtoms::select==tag.get()) {

Go ahead and remove the unneeded .get() on tag while you're here (and other
places where you're changing these lines).

>Index: modules/libpref/src/unix/unix.js
>RCS file: /cvsroot/mozilla/modules/libpref/src/unix/unix.js,v
>retrieving revision 3.74
>diff -u -r3.74 unix.js
>--- modules/libpref/src/unix/unix.js	25 Jul 2002 05:10:46 -0000	3.74
>+++ modules/libpref/src/unix/unix.js	30 Aug 2002 01:30:57 -0000
>@@ -67,6 +67,10 @@
> pref("browser.urlbar.clickSelectsAll", false);
>+// Tab focus models: 0 focuses links and form elements,
>+// 1 focuses form elements but not links, 2 focuses only text controls.
>+pref("accessibility.tabfocus", 2);
> // override double-click word selection behavior.
> pref("layout.word_select.stop_at_punctuation", false);

I suspect this would be desired behavior for Mac as well, but you should talk
to a Mac guy about it.

Comment 14

15 years ago
Most mac users would want the pref at '2' by default, I think.

Comment 15

15 years ago
Yeah, we would.

That's why I reported this in the first place. ;-)

Thanks for the effort, guys.


Comment 16

15 years ago
Transferring keywords from bug 66285 prior to duping it to this one.

I'll attach a new patch soon with the changes bryner suggested (and with mac as
well as unix defaulting to text fields).
Keywords: topembed
Whiteboard: [KEYBASE+]

Comment 17

15 years ago
Transferring dependency list too.
Blocks: 47282, 58712, 127253

Comment 18

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

Comment 19

15 years ago
Created attachment 99181 [details] [diff] [review]
New patch addressing bryner's comments

Here's a new patch addressing Bryner's comments.  This is actually much clearer
-- I'm glad you suggested the bit fields, as it makes the code much more
readable and also gives the user more control (not that anyone is likely to
want to tab to links but not text fields, say, but should they want to, it
works).  I've removed all the .get()'s I saw, and I made mac and unix both
default to text fields only, windows default to everything.

Seeking review ...
Attachment #97260 - Attachment is obsolete: true
Comment on attachment 99181 [details] [diff] [review]
New patch addressing bryner's comments

Usually in Mozilla we use #defines for bitfields, i.e.:

#define TAB_FOCUS_TEXT_CONTROLS (1 << 0)
#define TAB_FOCUS_FORM_ELEMENTS (1 << 1)
#define TAB_FOCUS_LINKS (1 << 2)

To me, that seems cleaner than a non-contiguous enum.

Also, could I convince you to attach a patch with -w? :)
Keywords: nsbeta1
Blocks: 169045

Comment 22

15 years ago
Created attachment 99442 [details] [diff] [review]
diff -uw, with "Mask" and "(1<<n)"

I doublechecked with Simon since I'll want his sr (to me the enum seems cleaner
but I don't feel strongly about it).  He agreed about the (1<<n), but preferred
an enum, partly because it guarantees that the compiler will handle it at
compile time rather than runtime (though most modern compilers should calculate
constants at compile time anyway), but he suggested cleaning it up by adding
Mask to each of the enum symbols:
eTabFocus_linksMask = (1 << 2)
I hope that's okay with you, Brian.  I've done that (I left the Mask part off
unset, since that's a value, not a mask) and made a new patch with -uw as
bryner requested.
Attachment #99181 - Attachment is obsolete: true


15 years ago
Target Milestone: mozilla1.2alpha → mozilla1.2beta
Attachment #99442 - Flags: review+
Comment on attachment 99442 [details] [diff] [review]
diff -uw, with "Mask" and "(1<<n)"

r=bryner (or sr=, if you want)

Comment 24

15 years ago
Comment on attachment 99442 [details] [diff] [review]
diff -uw, with "Mask" and "(1<<n)"

Attachment #99442 - Flags: superreview+

Comment 25

15 years ago
It's in!  Happy tabbing. :-)
Last Resolved: 15 years ago
Resolution: --- → FIXED
Blocks: 160387
a couple regressions due to this fix (on linux and mac; win32 okay):

bug 169767: focus/tabbing broken in application windows
bug 169756: password fields should be part of tab cycle
Hey, who broke my tabbing order?!??!
Do we have a bug for complaining that we don't like the new defaults?  I use
tabbing to access radio buttons and checkboxes all the time.  (I'm assuming we
can still tab to file controls.)  I also use keyboard navigation for selects
quite a bit.
check out bug 169767... there are also bug[s] for requesting a frontend to
control these --see the dep list for bug 169045.
FOCUS_FORM_ELEMENTS is interesting to us
Whiteboard: [KEYBASE+] → [KEYBASE+][wgate]

Comment 31

15 years ago
Bug 169767, for getting the pref exposed, is an important one.  But no, we don't
have a bug for complaining about the new defaults.  Lets not use this one --
let's make this be the one that introduced the pref, and you can open a bug
specifically for changing the pref if you think it's warranted, and add a note
here since a lot of linux and mac users cc'ed here felt quite strongly about
wanting it set the new way, while a few other people (but mostly windows users,
I think) felt strongly the other way.
The new default is less usable in every scenario I use daily, especially in
bugzilla.  I can't tab to a submit button and hit space?  I have to use my mouse
to pick a select option for bug component?  Then why did I care to tab among
text inputs only?  Half as much fun, then some mouse-pain to balance yin and yang?

Asa's going to file a bug demanding the default be reverted to what we shipped
in mozilla1.0: bug 170429.

vrfy'ing fixed.

actually bug 169767 isn't to expose the pref, it's to make sure that the pref
only affects web page content (in the browser window). check out bug 169045 for
frontend pref work.

Comment 34

15 years ago
*** Bug 160434 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.