Brett Koonce Reporter
	Description • 23 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.

-Brett

I'm sorry if this is a dupe...I couldn't find any matches in bugzilla.

	Boris Zbarsky [:bzbarsky]
	Comment 1 • 23 years ago

confirming rfe

	Jesse Ruderman
	Comment 2 • 23 years ago

See bug 66285 for some related discussion.

	Aaron Leventhal
	Comment 3 • 22 years ago

-> Akkana

	Akkana Peck Assignee
	Comment 4 • 22 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.

	Akkana Peck Assignee
	Comment 5 • 22 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.

	Aaron Leventhal
	Comment 7 • 22 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.

	Aaron Leventhal
	Comment 8 • 22 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.

	Akkana Peck Assignee
	Comment 9 • 22 years ago

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.

	Akkana Peck Assignee
	Comment 10 • 22 years ago

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

	Aaron Leventhal
	Comment 11 • 22 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?

	Akkana Peck Assignee
	Comment 12 • 22 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
frustrating.

I'm open to discussion if other Unix users disagree with me.

	Brian Ryner (not reading)
	Comment 13 • 22 years ago

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.

	Simon Fraser [no longer active]
	Comment 14 • 22 years ago

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

	Brett Koonce Reporter
	Comment 15 • 22 years ago

Yeah, we would.

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

Thanks for the effort, guys.

-Brett

	Akkana Peck Assignee
	Comment 16 • 22 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).

	Akkana Peck Assignee
	Comment 17 • 22 years ago

Transferring dependency list too.

	Akkana Peck Assignee
	Comment 18 • 22 years ago

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

	Akkana Peck Assignee
	Comment 19 • 22 years ago

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 ...

	Brian Ryner (not reading)
	Comment 20 • 22 years ago

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? :)

	sairuh (rarely reading bugmail)
	Comment 21 • 22 years ago

nominating.

	Akkana Peck Assignee
	Comment 22 • 22 years ago

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.

	Brian Ryner (not reading)
	Comment 23 • 22 years ago

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

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

	Simon Fraser [no longer active]
	Comment 24 • 22 years ago

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

sr=sfraser

	Akkana Peck Assignee
	Comment 25 • 22 years ago

It's in!  Happy tabbing. :-)

	sairuh (rarely reading bugmail)
	Comment 26 • 22 years ago

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

	Christopher Blizzard (:blizzard)
	Comment 27 • 22 years ago

Hey, who broke my tabbing order?!??!

	David Baron :dbaron:
	Comment 28 • 22 years ago

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.

	sairuh (rarely reading bugmail)
	Comment 29 • 22 years ago

check out bug 169767... there are also bug[s] for requesting a frontend to
control these --see the dep list for bug 169045.

	Randell Jesup [:jesup] (needinfo me)
	Comment 30 • 22 years ago

FOCUS_FORM_ELEMENTS is interesting to us

	Akkana Peck Assignee
	Comment 31 • 22 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.

	Brendan Eich [:brendan]
	Comment 32 • 22 years ago

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

	sairuh (rarely reading bugmail)
	Comment 33 • 22 years ago

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.

	Simon Fraser [no longer active]
	Comment 34 • 22 years ago

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