277706 - Need pref UI to control tabbing navigation between form elements and web page links

Status: VERIFIED FIXED

(Camino Graveyard :: Preferences, defect, P3)

Description

[sairuh (rarely reading bugmail)]

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

[sairuh (rarely reading bugmail)]

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

[Asaf Romano (gone)]

err, was tabfocus=7 worked for links in previous camino builds?

Comment 3

[sairuh (rarely reading bugmail)]

no, the accessibility.tabfocus pref was absent in both the old and new profiles
I tested.

Comment 4

[sairuh (rarely reading bugmail)]

oh whups, comment 3 is in reference to using today's 2005010808-trunk build.
I'll go compare with earlier bits.

Comment 5

[Asaf Romano (gone)]

I mean if you set it manualy, in older builds. If it wasn't, it's not a ui bug.

Comment 6

[sairuh (rarely reading bugmail)]

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

[Asaf Romano (gone)]

We first need to remove this set.

Comment 8

[Asaf Romano (gone)]

I didn't find all-camino.js in lxr.

Comment 9

[sairuh (rarely reading bugmail)]

http://lxr.mozilla.org/mozilla/source/camino/resources/application/all-camino.js

Comment 10

[Asaf Romano (gone)]

my bad!

Comment 11

[Asaf Romano (gone)]

Attachment: make "Full keyboard access" patch apply camino

Comment 12

[Mike Pinkerton (not reading bugmail)]

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

[Josh Aas]

Comment on attachment 170775 [details] [diff] [review]
make "Full keyboard access" patch apply camino

sure

Comment 14

[Josh Aas]

Landed on trunk. Is this bug fixed now?

Comment 15

[Simon Fraser [no longer active]]

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

[sairuh (rarely reading bugmail)]

(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

[Asaf Romano (gone)]

Simon has lanaded a fix (Thanks!) in the orginal bug, please test the next build.

Comment 18

[sairuh (rarely reading bugmail)]

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

[Simon Fraser [no longer active]]

Comment on attachment 170775 [details] [diff] [review]
make "Full keyboard access" patch apply camino

Patch landed.

Comment 20

[Samuel Sidler (old account; do not CC)]

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

[Simon Fraser [no longer active]]

Maybe

Comment 22

[Mike Pinkerton (not reading bugmail)]

i wouldn't hold 1.0 for this.

Comment 23

[Mark Mentovai]

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

Comment 24

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

[froodian (Ian Leue)]

Attachment: -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

[Simon Fraser [no longer active]]

I think a "links only" mode would be very weird.

Comment 28

[jonathan chetwynd]

#27 Simon,

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

cheers

Comment 29

[froodian (Ian Leue)]

Attachment: 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

[froodian (Ian Leue)]

Attachment: 2-checkbox Patch (-w, not for checkin)

Comment 31

[froodian (Ian Leue)]

Attachment: 2-checkbox nib

Comment 32

[froodian (Ian Leue)]

Attachment: Screenshot of 2-checkbox nib

Comment 33

[Håkan Waara]

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

[Håkan Waara]

Personally I'd prefer #2 (the popupmenu approach) which is cleaner and takes less space.

Comment 35

[froodian (Ian Leue)]

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

[Simon Fraser [no longer active]]

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

[froodian (Ian Leue)]

(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

[froodian (Ian Leue)]

See also the WONTFIXed bug 338433 (which was wontfixed because this bug should provide UI for it)

Comment 39

[froodian (Ian Leue)]

Sorry, more appropriate is the WONTFIXed Camino bug 316339 (which also says expose it in this pref).  Sorry for the bugspam.

Comment 40

[Håkan Waara]

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

[Simon Fraser [no longer active]]

(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

[Håkan Waara]

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

[froodian (Ian Leue)]

Attachment: 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

[froodian (Ian Leue)]

Attachment: 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

[Simon Fraser [no longer active]]

(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

[Håkan Waara]

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

[froodian (Ian Leue)]

Attachment: 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

[froodian (Ian Leue)]

Attachment: 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

[froodian (Ian Leue)]

Attachment: Screenshot of new nib

Comment 50

[froodian (Ian Leue)]

Attachment: Patch (-w)

Uses |= and &= instead of doing it the long way.

Comment 51

[Håkan Waara]

Comment on attachment 226410 [details] [diff] [review]
Patch (-w)

Code change looks good.

Comment 52

[jonathan chetwynd]

#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

[Chris Lawson (gone)]

(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

[jonathan chetwynd]

#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

[Chris Lawson (gone)]

(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

[jonathan chetwynd]

#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

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

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

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: rip-off of hyatt's forms test page for easy testing

Comment 59

[Mike Pinkerton (not reading bugmail)]

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

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

> 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

[froodian (Ian Leue)]

We're gonna keep going with the current patch (conceptually - it's still up for r and sr) here per pinkerton. :)

Comment 62

[froodian (Ian Leue)]

Attachment: r=smokey nib

Does what smokey wanted; also fixes a few things to follow our guidelines better.

Comment 63

[froodian (Ian Leue)]

Attachment: Screenshot of r=smokey nib

Comment 64

[Nick Kreeger]

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

[froodian (Ian Leue)]

Attachment: r=kreeger Patch

Comment 66

[froodian (Ian Leue)]

Attachment: Same WebFeatures window with polished "exceptions" sheet

This leaves the main prefpane alone, and polishes the pop-up exceptions sheet.

Comment 67

[froodian (Ian Leue)]

Attachment: Screenshot of exceptions sheet

Comment 68

[froodian (Ian Leue)]

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

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 227571 [details] [diff] [review]
r=kreeger Patch

sr=pink

Comment 70

[Nick Kreeger]

Checked into trunk, awaiting an synched nib from the branch before landing there

Comment 71

[Nick Kreeger]

Ok, now landed on the branch.

Comment 72

[jonathan chetwynd]

'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

[Stuart Morgan]

(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

[jonathan chetwynd]

#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

[jonathan chetwynd]

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.