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

VERIFIED FIXED in Camino1.5

Status

Camino Graveyard
Preferences
P3
normal
VERIFIED FIXED
13 years ago
11 years ago

People

(Reporter: sairuh (rarely reading bugmail), Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1})

unspecified
Camino1.5
PowerPC
Mac OS X
fixed1.8.1
Dependency tree / graph
Bug Flags:
camino1.0 -

Details

Attachments

(5 attachments, 13 obsolete attachments)

5.89 KB, text/html
Details
72.05 KB, image/png
Details
11.08 KB, patch
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
12.77 KB, application/zip
Smokey Ardisson (offline for a while; not following bugs - do not email)
: review+
Details
41.80 KB, image/png
Details
(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
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...)
(Reporter)

Updated

13 years ago
Summary: need to be able to tab to web page links → need a pref for include tabbing navigation between web page links
err, was tabfocus=7 worked for links in previous camino builds?
(Reporter)

Comment 3

13 years ago
no, the accessibility.tabfocus pref was absent in both the old and new profiles
I tested.
(Reporter)

Comment 4

13 years ago
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.
(Reporter)

Comment 6

13 years ago
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.
(Reporter)

Updated

13 years ago
OS: All → MacOS X
I didn't find all-camino.js in lxr.
(Reporter)

Comment 9

13 years ago
http://lxr.mozilla.org/mozilla/source/camino/resources/application/all-camino.js
my bad!
Created attachment 170775 [details] [diff] [review]
make "Full keyboard access" patch apply camino
Attachment #170775 - Flags: review?(joshmoz)
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.
Attachment #170775 - Flags: superreview+

Comment 13

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

sure
Attachment #170775 - Flags: review?(joshmoz) → review+

Comment 14

12 years ago
Landed on trunk. Is this bug fixed now?

Comment 15

12 years ago
No, this bug isnt' fixed. The bug requests prefs UI to control whether links are
tabbed to when 'full keyboard access' is in effect.
Summary: need a pref for include tabbing navigation between web page links → Need pref UI to control tabbing navigation between web page links
(Reporter)

Comment 16

12 years ago
(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.
(Reporter)

Comment 18

12 years ago
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

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

Patch landed.
Attachment #170775 - Attachment is obsolete: true

Updated

12 years ago
Priority: -- → P3
Target Milestone: --- → Camino1.0
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

12 years ago
Maybe
Flags: camino1.0?
i wouldn't hold 1.0 for this.
Flags: camino1.0? → camino1.0-
Target Milestone: Camino1.0 → Camino1.1

Comment 23

11 years ago
*** Bug 326374 has been marked as a duplicate of this bug. ***
QA Contact: preferences
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.
Summary: Need pref UI to control tabbing navigation between web page links → Need pref UI to control tabbing navigation between form elements and web page links
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 ;)
(Assignee)

Updated

11 years ago
Assignee: mikepinkerton → stridey
(Assignee)

Comment 26

11 years ago
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

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

Comment 28

11 years ago
#27 Simon,

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

cheers
(Assignee)

Comment 29

11 years ago
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.
(Assignee)

Comment 30

11 years ago
Created attachment 226000 [details] [diff] [review]
2-checkbox Patch (-w, not for checkin)
Attachment #225969 - Attachment is obsolete: true
Attachment #226000 - Flags: review?(hwaara)
(Assignee)

Comment 31

11 years ago
Created attachment 226002 [details]
2-checkbox nib
Attachment #226000 - Attachment is obsolete: true
Attachment #226002 - Flags: review?(alqahira)
Attachment #226000 - Flags: review?(hwaara)
(Assignee)

Comment 32

11 years ago
Created attachment 226003 [details]
Screenshot of 2-checkbox nib
Attachment #225998 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #226000 - Attachment is obsolete: false
Attachment #226000 - Flags: review?(hwaara)

Comment 33

11 years ago
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

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

Comment 35

11 years ago
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

11 years ago
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).
(Assignee)

Comment 37

11 years ago
(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)"
(Assignee)

Comment 38

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

Comment 39

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

Comment 40

11 years ago
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

11 years ago
(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.
(Assignee)

Updated

11 years ago
Attachment #226000 - Flags: review?(nick.kreeger)

Comment 42

11 years ago
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.
Attachment #226000 - Flags: review?(hwaara) → review-
(Assignee)

Comment 43

11 years ago
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.
Attachment #226000 - Attachment is obsolete: true
Attachment #226210 - Flags: review?(hwaara)
Attachment #226000 - Flags: review?(nick.kreeger)
(Assignee)

Comment 44

11 years ago
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

11 years ago
(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

11 years ago
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.
Attachment #226210 - Flags: review?(hwaara) → review-
(Assignee)

Comment 47

11 years ago
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.
Attachment #226210 - Attachment is obsolete: true
Attachment #226390 - Flags: review?(hwaara)
(Assignee)

Comment 48

11 years ago
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)
Attachment #226002 - Attachment is obsolete: true
Attachment #226003 - Attachment is obsolete: true
Attachment #226211 - Attachment is obsolete: true
Attachment #226394 - Flags: review?(alqahira)
Attachment #226002 - Flags: review?(alqahira)
(Assignee)

Comment 49

11 years ago
Created attachment 226395 [details]
Screenshot of new nib
(Assignee)

Comment 50

11 years ago
Created attachment 226410 [details] [diff] [review]
Patch (-w)

Uses |= and &= instead of doing it the long way.
Attachment #226390 - Attachment is obsolete: true
Attachment #226410 - Flags: review?(hwaara)
Attachment #226390 - Flags: review?(hwaara)

Comment 51

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

Code change looks good.
Attachment #226410 - Flags: review?(hwaara) → review+
(Assignee)

Updated

11 years ago
Attachment #226410 - Flags: review?(bugzilla)

Comment 52

11 years ago
#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

11 years ago
(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

11 years ago
#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

11 years ago
(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

11 years ago
#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 ;)
Attachment #226394 - Flags: review?(alqahira) → review+
Created attachment 226900 [details]
rip-off of hyatt's forms test page for easy testing
(Assignee)

Updated

11 years ago
Blocks: 331293
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?
(Assignee)

Comment 61

11 years ago
We're gonna keep going with the current patch (conceptually - it's still up for r and sr) here per pinkerton. :)
Status: NEW → ASSIGNED
(Assignee)

Comment 62

11 years ago
Created attachment 227560 [details]
r=smokey nib

Does what smokey wanted; also fixes a few things to follow our guidelines better.
Attachment #226394 - Attachment is obsolete: true
(Assignee)

Comment 63

11 years ago
Created attachment 227562 [details]
Screenshot of r=smokey nib
Attachment #226395 - Attachment is obsolete: true

Comment 64

11 years ago
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.
Attachment #226410 - Flags: review?(bugzilla) → review+
(Assignee)

Comment 65

11 years ago
Created attachment 227571 [details] [diff] [review]
r=kreeger Patch
Attachment #226410 - Attachment is obsolete: true
Attachment #227571 - Flags: superreview?(mikepinkerton)
(Assignee)

Comment 66

11 years ago
Created attachment 227656 [details]
Same WebFeatures window with polished "exceptions" sheet

This leaves the main prefpane alone, and polishes the pop-up exceptions sheet.
Attachment #227560 - Attachment is obsolete: true
Attachment #227656 - Flags: review?(alqahira)
(Assignee)

Comment 67

11 years ago
Created attachment 227657 [details]
Screenshot of exceptions sheet
Attachment #227656 - Flags: review?(alqahira) → review+
(Assignee)

Updated

11 years ago
Blocks: 344036
(Assignee)

Comment 68

11 years ago
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
Attachment #227571 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Updated

11 years ago
Whiteboard: [needs checkin]

Comment 70

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

Comment 71

11 years ago
Ok, now landed on the branch.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: access → fixed1.8.1
Resolution: --- → FIXED
Whiteboard: [needs checkin]
(Assignee)

Updated

11 years ago
Status: RESOLVED → VERIFIED

Comment 72

11 years ago
'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

11 years ago
(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

11 years ago
#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

11 years ago
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.
You need to log in before you can comment on or make changes to this bug.