Last Comment Bug 277706 - Need pref UI to control tabbing navigation between form elements and web page links
: Need pref UI to control tabbing navigation between form elements and web page...
Status: VERIFIED FIXED
: fixed1.8.1
Product: Camino Graveyard
Classification: Graveyard
Component: Preferences (show other bugs)
: unspecified
: PowerPC Mac OS X
P3 normal (vote)
: Camino1.5
Assigned To: froodian (Ian Leue)
:
:
Mentors:
: 326374 (view as bug list)
Depends on:
Blocks: 331293 344036
  Show dependency treegraph
 
Reported: 2005-01-09 16:14 PST by sairuh (rarely reading bugmail)
Modified: 2006-07-16 00:33 PDT (History)
13 users (show)
mikepinkerton: camino1.0-
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
make "Full keyboard access" patch apply camino (639 bytes, patch)
2005-01-09 17:06 PST, Mano (::mano, needinfo? for any questions; not reading general bugmail)
jaas: review+
mikepinkerton: superreview+
Details | Diff | Splinter Review
-w Patch for review (not for checkin) (4.88 KB, patch)
2006-06-17 00:39 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
Three-checkbox nib, for posterity (12.97 KB, application/zip)
2006-06-17 10:18 PDT, froodian (Ian Leue)
no flags Details
2-checkbox Patch (-w, not for checkin) (4.79 KB, patch)
2006-06-17 10:33 PDT, froodian (Ian Leue)
hwaara: review-
Details | Diff | Splinter Review
2-checkbox nib (12.69 KB, application/zip)
2006-06-17 10:34 PDT, froodian (Ian Leue)
no flags Details
Screenshot of 2-checkbox nib (69.57 KB, image/png)
2006-06-17 10:35 PDT, froodian (Ian Leue)
no flags Details
Uses bitwise operators (-w again) (3.24 KB, patch)
2006-06-19 14:00 PDT, froodian (Ian Leue)
hwaara: review-
Details | Diff | Splinter Review
Screenshot of new proposed nib (69.81 KB, image/png)
2006-06-19 14:06 PDT, froodian (Ian Leue)
no flags Details
Patch (still -w) (3.50 KB, patch)
2006-06-20 13:47 PDT, froodian (Ian Leue)
no flags Details | Diff | Splinter Review
New nib (12.69 KB, application/zip)
2006-06-20 13:51 PDT, froodian (Ian Leue)
alqahira: review+
Details
Screenshot of new nib (71.21 KB, image/png)
2006-06-20 13:52 PDT, froodian (Ian Leue)
no flags Details
Patch (-w) (5.17 KB, patch)
2006-06-20 15:13 PDT, froodian (Ian Leue)
hwaara: review+
nick.kreeger: review+
Details | Diff | Splinter Review
rip-off of hyatt's forms test page for easy testing (5.89 KB, text/html)
2006-06-23 22:48 PDT, Smokey Ardisson (offline for a while; not following bugs - do not email)
no flags Details
r=smokey nib (12.72 KB, application/zip)
2006-06-29 09:44 PDT, froodian (Ian Leue)
no flags Details
Screenshot of r=smokey nib (72.05 KB, image/png)
2006-06-29 09:45 PDT, froodian (Ian Leue)
no flags Details
r=kreeger Patch (11.08 KB, patch)
2006-06-29 10:52 PDT, froodian (Ian Leue)
mikepinkerton: superreview+
Details | Diff | Splinter Review
Same WebFeatures window with polished "exceptions" sheet (12.77 KB, application/zip)
2006-06-29 21:35 PDT, froodian (Ian Leue)
alqahira: review+
Details
Screenshot of exceptions sheet (41.80 KB, image/png)
2006-06-29 21:36 PDT, froodian (Ian Leue)
no flags Details

Description User image sairuh (rarely reading bugmail) 2005-01-09 16:14:30 PST
with bug 187508 now implemented, the system preference for Full Keyboard
Navigation is obeyed. however, with this system pref ON, tabbing within a web
page still skips links. (all html form controls can be tabbed to, although focus
isn't displayed for buttons, bug 264146.)

Safari actually has pref UI for allowing this, under the Advanced Panel
(Universal Access). we should have that choice as well.

note: this is not a problem for Firefox or Mozilla on Mac OS X.
Comment 1 User image sairuh (rarely reading bugmail) 2005-01-09 16:21:47 PST
hm, would implemented this mean bringing back the accessibility.tabfocus pref?
or will another, new pref be needed instead? (my gut tells me the latter...)
Comment 2 User image Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-09 16:27:02 PST
err, was tabfocus=7 worked for links in previous camino builds?
Comment 3 User image sairuh (rarely reading bugmail) 2005-01-09 16:30:51 PST
no, the accessibility.tabfocus pref was absent in both the old and new profiles
I tested.
Comment 4 User image sairuh (rarely reading bugmail) 2005-01-09 16:31:52 PST
oh whups, comment 3 is in reference to using today's 2005010808-trunk build.
I'll go compare with earlier bits.
Comment 5 User image Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-09 16:34:07 PST
I mean if you set it manualy, in older builds. If it wasn't, it's not a ui bug.
Comment 6 User image sairuh (rarely reading bugmail) 2005-01-09 16:51:55 PST
it's been a while since I've looked at Camino prefs --was looking at prefs.js in
my profile folder rather than (also checking) all-camino.js in the package
contents. :-\

"accessibility.tabfocus" is 3 for both 0.8.2 and today's trunk build, which
means all form controls can be tabbed to. so it looks like we'd need UI to
change between 3 and 7 (7 includes links in the tabbing cycle) --analogous to
what Safari currently does.
Comment 7 User image Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-09 16:54:09 PST
We first need to remove this set.
Comment 8 User image Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-09 16:58:26 PST
I didn't find all-camino.js in lxr.
Comment 10 User image Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-09 17:01:06 PST
my bad!
Comment 11 User image Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-09 17:06:02 PST
Created attachment 170775 [details] [diff] [review]
make "Full keyboard access" patch apply camino
Comment 12 User image Mike Pinkerton (not reading bugmail) 2005-01-10 06:19:22 PST
Comment on attachment 170775 [details] [diff] [review]
make "Full keyboard access" patch apply camino

sr=pink, though i still think people are going to bitch about this not working
any more. i don't think people will know to look in the system prefs to set
something just for camino.
Comment 13 User image Josh Aas 2005-01-19 22:35:43 PST
Comment on attachment 170775 [details] [diff] [review]
make "Full keyboard access" patch apply camino

sure
Comment 14 User image Josh Aas 2005-01-19 22:37:48 PST
Landed on trunk. Is this bug fixed now?
Comment 15 User image Simon Fraser 2005-01-19 23:12:36 PST
No, this bug isnt' fixed. The bug requests prefs UI to control whether links are
tabbed to when 'full keyboard access' is in effect.
Comment 16 User image sairuh (rarely reading bugmail) 2005-01-21 16:00:15 PST
(In reply to comment #14)
> Landed on trunk. Is this bug fixed now?

hm, I think this might've regressed keyboard navigation in camino
2005012108-trunk: I've got full keyboard nav turned ON, but now I can only tab
to html text controls --ie, as if accessibility.tabfocus=1. links and other html
form controls are now skipped.
Comment 17 User image Mano (::mano, needinfo? for any questions; not reading general bugmail) 2005-01-22 01:32:26 PST
Simon has lanaded a fix (Thanks!) in the orginal bug, please test the next build.
Comment 18 User image sairuh (rarely reading bugmail) 2005-01-27 11:50:37 PST
tested with 2005012608-trunk camino: tabbing to all html form controls and page
links now works nicely (w/system pref for full kb access ON).
Comment 19 User image Simon Fraser 2005-07-22 12:37:33 PDT
Comment on attachment 170775 [details] [diff] [review]
make "Full keyboard access" patch apply camino

Patch landed.
Comment 20 User image Samuel Sidler (old account; do not CC) 2005-10-25 15:13:47 PDT
Simon, do you think you can whip up a UI change before 1.0 for this? It seems like it'd go in either the Web Features or General pref panel.
Comment 21 User image Simon Fraser 2005-10-25 18:21:41 PDT
Maybe
Comment 22 User image Mike Pinkerton (not reading bugmail) 2005-10-26 19:08:47 PDT
i wouldn't hold 1.0 for this.
Comment 23 User image Mark Mentovai 2006-02-08 16:58:56 PST
*** Bug 326374 has been marked as a duplicate of this bug. ***
Comment 24 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-05-18 15:35:46 PDT
The FKA behavior was backed out a year ago (http://tinyurl.com/qnqde and see also discussion in bug 316339), so we need to expose UI for the entire Gecko pref here, or some reasonable portion of it.
Comment 25 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-06-16 21:51:06 PDT
I think we expose this in Web Features as 3 check-boxes (this is one of those "additive" moz prefs"):

Tab to: [X] text inputs
        [X] other form controls
        [ ] links

...perhaps with more refined language ;)
Comment 26 User image froodian (Ian Leue) 2006-06-17 00:39:09 PDT
Created attachment 225969 [details] [diff] [review]
-w Patch for review (not for checkin)

OK.  This patch does what's described in Comment 25.  The only problem is that even values (2, 4, 6, and possibly 0) for this pref purportedly shouldn't allow textfields to be selected, but they do.  As this occurs on the incendiary mammal (Win and Mac), I've filed this as Core bug 341849.

So the question is, is the solution to wait on that bug, or just not have the first checkbox?  Not a lot of people want to disalbe tabbing to textfields, but personally, I think it would be sweet to be able to just tab through links. Thoughts?
Comment 27 User image Simon Fraser 2006-06-17 08:53:31 PDT
I think a "links only" mode would be very weird.
Comment 28 User image jonathan chetwynd 2006-06-17 09:01:08 PDT
#27 Simon,

if I've understood right, you might like to try Opera, 'tab' for forms and 'a' for links.

cheers
Comment 29 User image froodian (Ian Leue) 2006-06-17 10:18:12 PDT
Created attachment 225998 [details]
Three-checkbox nib, for posterity

(In reply to comment #27)
> I think a "links only" mode would be very weird.

OK.  Since it's likely that very few people would want to be able to use the even-numbered configurations (ie exclude text fields from the tab chain), it sounds like a two-checkbox approach is the way to go for the UI.  This is the nib for the three-checkbox approach, just so it still exists once I zap it on my computer.
Comment 30 User image froodian (Ian Leue) 2006-06-17 10:33:43 PDT
Created attachment 226000 [details] [diff] [review]
2-checkbox Patch (-w, not for checkin)
Comment 31 User image froodian (Ian Leue) 2006-06-17 10:34:53 PDT
Created attachment 226002 [details]
2-checkbox nib
Comment 32 User image froodian (Ian Leue) 2006-06-17 10:35:26 PDT
Created attachment 226003 [details]
Screenshot of 2-checkbox nib
Comment 33 User image Håkan Waara 2006-06-17 11:30:17 PDT
I agree with Simon that having a links-only mode available is quite weird. So here are two revised proposals:

Idea #1: Make the checkbox dependent. I'm sure the wording of the second checkbox can be improved.

Pressing tab focuses:  [X] Form elements
                           [ ] ... and links


Idea #2: Use a popupmenu.

Pressing tab focuses: [ Form elements only      ]
                      [ Links and form elements ]
Comment 34 User image Håkan Waara 2006-06-17 11:32:24 PDT
Personally I'd prefer #2 (the popupmenu approach) which is cleaner and takes less space.
Comment 35 User image froodian (Ian Leue) 2006-06-17 11:44:04 PDT
Just to be clear, my latest patch doesn't have a links-only mode.  Textfields are always tabbable, and the user can choose if they want form elements, links, or both, to be tabbable to in addition.

A popup menu would be bad IMO because you lose the ability to have both tabbable to, and having dependancies is bad IMO because you lose the ability to have links but not form elements tabbable to.
Comment 36 User image Simon Fraser 2006-06-17 15:01:39 PDT
Do we ever want tabbing *not* to go to text boxes? Since we follow the system pref for text boxes vs. other controls, I think the only choice here should be wheher to include links or not (i.e. one checkbox).
Comment 37 User image froodian (Ian Leue) 2006-06-17 15:50:24 PDT
(In reply to comment #36)
> Do we ever want tabbing *not* to go to text boxes?

I don't think so.  My latest patch doesn't provide UI for tabbing not to go to text boxes.

> Since we follow the system
> pref for text boxes vs. other controls, I think the only choice here should be
> wheher to include links or not (i.e. one checkbox).

I don't think we do...  (from http://kb.mozillazine.org/Accessibility.tabfocus) "default is 3 and the OS “Full Keyboard Access” setting has no effect)"
Comment 38 User image froodian (Ian Leue) 2006-06-17 15:54:32 PDT
See also the WONTFIXed bug 338433 (which was wontfixed because this bug should provide UI for it)
Comment 39 User image froodian (Ian Leue) 2006-06-17 15:56:12 PDT
Sorry, more appropriate is the WONTFIXed Camino bug 316339 (which also says expose it in this pref).  Sorry for the bugspam.
Comment 40 User image Håkan Waara 2006-06-17 16:52:28 PDT
I filed bug 338433 and still would prefer to see us follow the system pref for  tabbing on webpages.

If we did that, we wouldn't even need a pref for this at all. Tabbing to links would be enabled at the same time as all other controls were tabbable... Simon, what do you think?
Comment 41 User image Simon Fraser 2006-06-18 09:30:48 PDT
(In reply to comment #40)
> I filed bug 338433 and still would prefer to see us follow the system pref for 
> tabbing on webpages.
> 
> If we did that, we wouldn't even need a pref for this at all. Tabbing to links
> would be enabled at the same time as all other controls were tabbable... Simon,
> what do you think?

I think that you have to break tabbing to links apart from tabbing to non-textfield controls. I, for one, want to tab to radio buttons, but never to links. I think the problem with respecting the FKA pref was that we did tie links and other form controls together.
Comment 42 User image Håkan Waara 2006-06-19 11:51:19 PDT
Comment on attachment 226000 [details] [diff] [review]
2-checkbox Patch (-w, not for checkin)

Perhaps a comment below the "Form elements" checkbox that says that textfields will always be selectable, indicating what "form elements" we're referring to would be in its place here? (I think the pref UI is kind of vague right now.) What do others think?

As for the code:

>+  // set inital values for tabfocus pref.  Internally, it's an additive pref in which
>+  // 1 focuses text controls, 2 focuses other form elements, 4 adds links (and linked images).
>+  // We expose the second two properties only.
>+  int tabFocus = [self getIntPref:"accessibility.tabfocus" withSuccess:&gotPref];
>+
>+  // order matters here.  Check for the larger pref's "signature" first: 
>+  // if it exists, set the checkbox appropriately, and subtract the number from the running total
>+  if (tabFocus >= 4) {
>+    [mTabToLinks setState:TRUE];
>+    tabFocus = tabFocus - 4;
>+  }
>+  if (tabFocus >= 2) {
>+    [mTabToFormElements setState:TRUE];
>+  }

You should really define some bitmasks in this file instead, and then check them by ANDing the different focus bits, like this:

const int kFocusLinks = (1 << 2); // this is equivalent to 4 (100)
const int kFocusForms = (1 << 1); // this is equivalent to binary 2 (010)

Then you can simply init the checkboxes like this:

[mTabToLinks setState:(tabFocusMask & kFocusLinks)];

Nice, huh? 

In the setter, you gather the bits together into a new mask by ORing them (the | operator) and then simply set the pref with the resulting int.
Comment 43 User image froodian (Ian Leue) 2006-06-19 14:00:37 PDT
Created attachment 226210 [details] [diff] [review]
Uses bitwise operators (-w again)

(In reply to comment #42)
> Nice, huh? 

Very sexy. :)

This does pretty much what you said.  I xored them together in the setter instead of or though, so that it can handle unchecking the checkboxes as well.
Comment 44 User image froodian (Ian Leue) 2006-06-19 14:06:47 PDT
Created attachment 226211 [details]
Screenshot of new proposed nib

(In reply to comment #42)
> Perhaps a comment below the "Form elements" checkbox that says that textfields
> will always be selectable, indicating what "form elements" we're referring to
> would be in its place here? (I think the pref UI is kind of vague right now.)
> What do others think?

I agree with that.  Here's another proposal - It makes it obvious what happens in regards to text fields, which is good, but moves all the content to the right, which is debatable.

I think for the form elements we should say "Form menus, buttons, etc etc".  I'm not sure what the etc etc is though, so maybe some form guru could enlighten. ;)
Comment 45 User image Simon Fraser 2006-06-19 21:51:22 PDT
(In reply to comment #42)


> You should really define some bitmasks in this file instead, and then check
> them by ANDing the different focus bits, like this:
> 
> const int kFocusLinks = (1 << 2); // this is equivalent to 4 (100)
> const int kFocusForms = (1 << 1); // this is equivalent to binary 2 (010)
> 
> Then you can simply init the checkboxes like this:
> 
> [mTabToLinks setState:(tabFocusMask & kFocusLinks)];
> 
> Nice, huh? 

Nice, but bad. You can easily end up with non-zero bits set for something that should be a boolean. The test should be:

[mTabToLinks setState:(tabFocusMask & kFocusLinks) != 0];

or 

[mTabToLinks setState:(tabFocusMask & kFocusLinks) ? NSOnState : NSOffState];

or (not recommended):

[mTabToLinks setState:!!(tabFocusMask & kFocusLinks)];
Comment 46 User image Håkan Waara 2006-06-20 00:31:38 PDT
Comment on attachment 226210 [details] [diff] [review]
Uses bitwise operators (-w again)

>+  BOOL gotPref;
>+  int tabFocusMask = [self getIntPref:"accessibility.tabfocus" withSuccess:&gotPref];
>+
>+  if (sender == mTabToLinks)
>+    [self setPref:"accessibility.tabfocus" toInt:tabFocusMask ^ kFocusLinks];
>+  else // sender == mTabToFormElements
>+    [self setPref:"accessibility.tabfocus" toInt:tabFocusMask ^ kFocusForms];
>+}

It's fancy to be able to use the xor operator, no doubt :), but conceptually I'm not so sure. You're relying on whatever value you get for the current pref, and "blindly" flip its bits.

I think it would be better if you just check the states of all checkboxes in this method, by ORing if a checkbox is checked, and then set the pref in one call at the end with the resulting bits. Don't forget to zero-initialize the int first.

You might also want to change the method name along with this change to tabFocusCheckboxesChanged:, or something else.
Comment 47 User image froodian (Ian Leue) 2006-06-20 13:47:27 PDT
Created attachment 226390 [details] [diff] [review]
Patch (still -w)

This addresses smfr and hwaara's comments.

I changed the method name to clickTabFocusCheckboxes (just adding the plural instead of checkbox) because I wanted to keep parity will all other checkbox selectors, which have the clickSomeCheckbox form.
Comment 48 User image froodian (Ian Leue) 2006-06-20 13:51:36 PDT
Created attachment 226394 [details]
New nib

New nib has more explicit description of "form elements".

Also has new selector name, so all other nibs are obsolete (though their designs aren't necessarily)
Comment 49 User image froodian (Ian Leue) 2006-06-20 13:52:10 PDT
Created attachment 226395 [details]
Screenshot of new nib
Comment 50 User image froodian (Ian Leue) 2006-06-20 15:13:50 PDT
Created attachment 226410 [details] [diff] [review]
Patch (-w)

Uses |= and &= instead of doing it the long way.
Comment 51 User image Håkan Waara 2006-06-20 15:16:29 PDT
Comment on attachment 226410 [details] [diff] [review]
Patch (-w)

Code change looks good.
Comment 52 User image jonathan chetwynd 2006-06-20 22:55:35 PDT
#49

Could someone describe the intended options for different operating systems in English?

How for instance will one navigate links without using the tab key?

Opera uses 'a' & 'z' for anchors and tab for forms in default mode independent of operating system. iirc , But that has the distinct disadvantage that letter keys might perhaps have a better use.
Comment 53 User image Chris Lawson (gone) 2006-06-21 04:34:47 PDT
(In reply to comment #52)
> #49
> 
> Could someone describe the intended options for different operating systems in
> English?
> 
> How for instance will one navigate links without using the tab key?

With the mouse, like a normal person.

cl
Comment 54 User image jonathan chetwynd 2006-06-21 05:40:08 PDT
#53

People who are blind or have limited fine motor skills will still need keyboard access to links.

this is a fundamental of accessibility and device independence guidelines.

cheers

NB: CL they wont be able to use a mouse
Comment 55 User image Chris Lawson (gone) 2006-06-21 05:49:50 PDT
(In reply to comment #54)
> #53
> 
> People who are blind or have limited fine motor skills will still need keyboard
> access to links.
> 
> this is a fundamental of accessibility and device independence guidelines.

Please understand -- this patch does not change *any* of the available behaviours. It simply adds a GUI front-end for the two most common. Comment 27 explains that a links-only mode would be weird, but that only means that we have no options where textboxes get skipped. If the "Links and linked images" checkbox is checked, links will always get tabbed to.

cl
Comment 56 User image jonathan chetwynd 2006-06-21 06:53:01 PDT
#55

Thanks for the clarification, this wasn't clear to me from the description.
However there is a real hurdle to be considered.

please note that in Safari keyboard access is enabled in default mode. 
"tab" navigates everything and
"option + tab" navigates forms, but not links. Why not follow this system?

Camino default mode should also enable keyboard users to access links and forms. They already face considerable life problems without having to configure applications before they can use them at all.

The GUI buttons would then be checked in default mode.
is there a good reason this isn't the case?

cheers
Comment 57 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-06-23 22:42:32 PDT
Comment on attachment 226394 [details]
New nib

I'd make it "Form buttons, pop-up..."; that sounds less awkward.

And nit, kill the comma before the "and".

Otherwise, r=me, provided everyone agrees that this is what we want.  

I think this satisfies people who don't want to tab to form controls without involving the silly system pref and satisfies the people who want an easy way to determine if they can tab to links or not.  Frankly, I think the majority of our userbase is perfectly happy with our current default of "doing the right thing", but this proposed nib set-up provides the most flexibility for those who aren't happy with it--and doesn't require any weird keystrokes for toggling modes.

We do need to start fixing the overly-large bottom margins on the other prefPanes soon ;)
Comment 58 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-06-23 22:48:10 PDT
Created attachment 226900 [details]
rip-off of hyatt's forms test page for easy testing
Comment 59 User image Mike Pinkerton (not reading bugmail) 2006-06-28 07:50:59 PDT
this looks fine and will work, but since we're providing some ui to this, shouldn't one of the options be to follow the system prefs? seems a little weird that we have the technology to do the right thing by the user but yet still make them set the same pref in multiple locations. I know we want our default to be different than the rest of the OS, but....

just a thought....
Comment 60 User image Smokey Ardisson (offline for a while; not following bugs - do not email) 2006-06-29 01:42:18 PDT
> shouldn't one of the options be to follow the system prefs?

I don't follow; our default is already textfields+controls, so the user turning on FKA would make no difference.  Or do you mean that turning on FKA would mean tabbing to textfields+controls+links (which Safari doesn't do when you enable FKA) aka checking both boxes in the proposed UI?
Comment 61 User image froodian (Ian Leue) 2006-06-29 09:22:52 PDT
We're gonna keep going with the current patch (conceptually - it's still up for r and sr) here per pinkerton. :)
Comment 62 User image froodian (Ian Leue) 2006-06-29 09:44:24 PDT
Created attachment 227560 [details]
r=smokey nib

Does what smokey wanted; also fixes a few things to follow our guidelines better.
Comment 63 User image froodian (Ian Leue) 2006-06-29 09:45:04 PDT
Created attachment 227562 [details]
Screenshot of r=smokey nib
Comment 64 User image Nick Kreeger 2006-06-29 10:12:34 PDT
Comment on attachment 226410 [details] [diff] [review]
Patch (-w)

   IBOutlet NSButton* mEnableAnnoyanceBlocker;
+
+  IBOutlet NSButton *mTabToFormElements;
+  IBOutlet NSButton *mTabToLinks;

Lets be consistent here, and put our '*' on the side of class, not the side of the variable name.

+  // set inital values for tabfocus pref.  Internally, it's an additive pref in which
+  // 1 focuses text controls, 2 focuses other form elements, 4 adds links (and linked images).
+  // We expose the second two properties only.

Could you be a little clearer with this comment? I had to read it several times to understand it. Also you should capitalize the first word of a sentence :-) Also what ever changes you make here please add them above |clickTabFocusCheckboxes:|.

+  [mTabToLinks setState:(tabFocusMask & kFocusLinks) ? NSOnState : NSOffState];

This would be clearer if the |setState:| arguments were wrapped in parens, especially since you use bitmasking here.

+  if (sender == mTabToLinks) {
+    if ([sender state])
+      tabFocusValue |= kFocusLinks;
+    else
+      tabFocusValue &= ~kFocusLinks;
+  } else { // sender == mTabToFormElements
+    if ([sender state])
+      tabFocusValue |= kFocusForms;
+    else
+      tabFocusValue &= ~kFocusForms;
+  }

Please don't put your else statements on the same line as a closing curly. Since this block of code contains allot of bitmasking, please comment more than the one comment in the code (// sender == mTabToFormElements). 

r=me with these nits taken care of.
Comment 65 User image froodian (Ian Leue) 2006-06-29 10:52:14 PDT
Created attachment 227571 [details] [diff] [review]
r=kreeger Patch
Comment 66 User image froodian (Ian Leue) 2006-06-29 21:35:56 PDT
Created attachment 227656 [details]
Same WebFeatures window with polished "exceptions" sheet

This leaves the main prefpane alone, and polishes the pop-up exceptions sheet.
Comment 67 User image froodian (Ian Leue) 2006-06-29 21:36:28 PDT
Created attachment 227657 [details]
Screenshot of exceptions sheet
Comment 68 User image froodian (Ian Leue) 2006-07-11 18:41:53 PDT
This is blocking a couple bugs I'm trying to work on, so an sr sooner would be better than later.  Basically, what this patch/nib combination does is:

- Provide UI for accessibility.tabfocus in the form of two checkboxes, which trigger the 'form elements' and 'links' bits of that pref.
Comment 69 User image Mike Pinkerton (not reading bugmail) 2006-07-12 07:21:42 PDT
Comment on attachment 227571 [details] [diff] [review]
r=kreeger Patch

sr=pink
Comment 70 User image Nick Kreeger 2006-07-13 08:40:52 PDT
Checked into trunk, awaiting an synched nib from the branch before landing there
Comment 71 User image Nick Kreeger 2006-07-13 09:18:12 PDT
Ok, now landed on the branch.
Comment 72 User image jonathan chetwynd 2006-07-14 14:04:30 PDT
'focus isn't displayed for buttons'

this does not appear to have been resolved, should I reopen this bug or has another bug been opened for this issue?
Comment 73 User image Stuart Morgan 2006-07-14 21:36:14 PDT
(In reply to comment #72)
> 'focus isn't displayed for buttons'

That was bug 264146, as mentioned in the original report; it was fixed for Tiger.
Comment 74 User image jonathan chetwynd 2006-07-15 00:44:50 PDT
#73 Stuart,

thanks for that, my test background was blue, which was masking the effect.
Is it possible to change the color of the button highlight using css? or is blue forbidden for backgrounds ~:"
Comment 75 User image jonathan chetwynd 2006-07-16 00:33:26 PDT
or to put it another way:

could the whole button change colour to show it has focus?

note many applications use this to show which button is the default, for instance in don't save, cancel, save, "save" might be a pulsing blue button.

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