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.

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

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

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

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

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

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.

We first need to remove this set.

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

my bad!

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 on attachment 170775 [details] [diff] [review] make "Full keyboard access" patch apply camino sure

Landed on trunk. Is this bug fixed now?

No, this bug isnt' fixed. The bug requests prefs UI to control whether links are tabbed to when 'full keyboard access' is in effect.

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

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

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 on attachment 170775 [details] [diff] [review] make "Full keyboard access" patch apply camino Patch landed.

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.

Maybe

i wouldn't hold 1.0 for this.

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

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.

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 ;)

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?

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

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

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

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 ]

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

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.

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

(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)"

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

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

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?

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

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

(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. ;)

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

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.

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)

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

Comment on attachment 226410 [details] [diff] [review] Patch (-w) Code change looks good.

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

(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

#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

(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

#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 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 ;)

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

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

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

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

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.

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

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 on attachment 227571 [details] [diff] [review] r=kreeger Patch sr=pink

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

Ok, now landed on the branch.

'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?

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

#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 ~:"

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.