Last Comment Bug 140612 - [RFE] Tab usage prefs: either "to all links" or "to text fields"
: [RFE] Tab usage prefs: either "to all links" or "to text fields"
Status: VERIFIED FIXED
[KEYBASE+][wgate]
: topembed
Product: Core
Classification: Components
Component: Keyboard: Navigation (show other bugs)
: Trunk
: All All
: -- enhancement (vote)
: mozilla1.2beta
Assigned To: Akkana Peck
: sairuh (rarely reading bugmail)
: Andrew Overholt [:overholt]
Mentors:
: 66285 160434 (view as bug list)
Depends on:
Blocks: 169045 47282 58712 127253 160387
  Show dependency treegraph
 
Reported: 2002-04-27 11:50 PDT by Brett Koonce
Modified: 2002-09-26 10:00 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
do it (6.07 KB, patch)
2002-08-22 16:55 PDT, Akkana Peck
no flags Details | Diff | Splinter Review
Patch addressing Aaron's comments (7.61 KB, patch)
2002-08-29 18:40 PDT, Akkana Peck
no flags Details | Diff | Splinter Review
New patch addressing bryner's comments (15.61 KB, patch)
2002-09-13 18:59 PDT, Akkana Peck
no flags Details | Diff | Splinter Review
diff -uw, with "Mask" and "(1<<n)" (10.70 KB, patch)
2002-09-16 16:15 PDT, Akkana Peck
bryner: review+
sfraser_bugs: superreview+
Details | Diff | Splinter Review

Description Brett Koonce 2002-04-27 11:50:53 PDT
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.

-Brett

I'm sorry if this is a dupe...I couldn't find any matches in bugzilla.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2002-04-27 14:30:37 PDT
confirming rfe
Comment 2 Jesse Ruderman 2002-05-07 00:45:15 PDT
See bug 66285 for some related discussion.
Comment 3 Aaron Leventhal 2002-07-16 10:09:26 PDT
-> Akkana
Comment 4 Akkana Peck 2002-07-16 11:31:15 PDT
Setting milestone based on a hope that the backend for this already exists.  If
focus code has to be rewritten, it will take longer.
Comment 5 Akkana Peck 2002-08-22 16:54:24 PDT
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 Akkana Peck 2002-08-22 16:55:17 PDT
Created attachment 96378 [details] [diff] [review]
do it
Comment 7 Aaron Leventhal 2002-08-23 14:52:17 PDT
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 Aaron Leventhal 2002-08-23 14:57:33 PDT
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 Akkana Peck 2002-08-29 18:40:49 PDT
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.
Comment 10 Akkana Peck 2002-09-06 10:04:45 PDT
Aaron suggested bryner should probably look at this since it affects focus
handling and someone experienced with focus needs to review it.
Comment 11 Aaron Leventhal 2002-09-09 18:42:55 PDT
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 Akkana Peck 2002-09-12 12:52:07 PDT
> 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
frustrating.

I'm open to discussion if other Unix users disagree with me.
Comment 13 Brian Ryner (not reading) 2002-09-12 23:50:44 PDT
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 Simon Fraser 2002-09-13 11:38:57 PDT
Most mac users would want the pref at '2' by default, I think.
Comment 15 Brett Koonce 2002-09-13 12:40:55 PDT
Yeah, we would.

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

Thanks for the effort, guys.

-Brett
Comment 16 Akkana Peck 2002-09-13 13:14:43 PDT
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).
Comment 17 Akkana Peck 2002-09-13 13:21:41 PDT
Transferring dependency list too.
Comment 18 Akkana Peck 2002-09-13 13:23:05 PDT
*** Bug 66285 has been marked as a duplicate of this bug. ***
Comment 19 Akkana Peck 2002-09-13 18:59:54 PDT
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 ...
Comment 20 Brian Ryner (not reading) 2002-09-14 20:26:19 PDT
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? :)
Comment 21 sairuh (rarely reading bugmail) 2002-09-16 14:14:06 PDT
nominating.
Comment 22 Akkana Peck 2002-09-16 16:15:58 PDT
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.
Comment 23 Brian Ryner (not reading) 2002-09-18 03:53:32 PDT
Comment on attachment 99442 [details] [diff] [review]
diff -uw, with "Mask" and "(1<<n)"

r=bryner (or sr=, if you want)
Comment 24 Simon Fraser 2002-09-18 14:18:32 PDT
Comment on attachment 99442 [details] [diff] [review]
diff -uw, with "Mask" and "(1<<n)"

sr=sfraser
Comment 25 Akkana Peck 2002-09-18 14:36:34 PDT
It's in!  Happy tabbing. :-)
Comment 26 sairuh (rarely reading bugmail) 2002-09-19 15:20:09 PDT
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
Comment 27 Christopher Blizzard (:blizzard) 2002-09-20 13:41:55 PDT
Hey, who broke my tabbing order?!??!
Comment 28 David Baron :dbaron: ⌚️UTC-10 2002-09-20 13:42:18 PDT
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.
Comment 29 sairuh (rarely reading bugmail) 2002-09-20 13:43:57 PDT
check out bug 169767... there are also bug[s] for requesting a frontend to
control these --see the dep list for bug 169045.
Comment 30 Randell Jesup [:jesup] 2002-09-20 13:49:55 PDT
FOCUS_FORM_ELEMENTS is interesting to us
Comment 31 Akkana Peck 2002-09-20 14:29:59 PDT
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.
Comment 32 Brendan Eich [:brendan] 2002-09-23 18:17:44 PDT
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.

/be
Comment 33 sairuh (rarely reading bugmail) 2002-09-24 12:51:42 PDT
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 Simon Fraser 2002-09-26 10:00:38 PDT
*** Bug 160434 has been marked as a duplicate of this bug. ***

Note You need to log in before you can comment on or make changes to this bug.