17602 - Arrow keys should change radio buttons, not Tab

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect, P2)

Description

[cpratt]

Build ID: 1999102908
Platform: Windows 98

To reproduce:
- Load the URL above
- Click into the first text input control
- Press Tab to advance to the radio buttons ("How many hours...?")
- Press the right arrow key

Result: Nothing happens

- Press the Tab key

Result: You advance to the next radio button

This is not correct. You should be able to move between the radio buttons using
the cursor keys, pressing Space to select an option. As it is now, you have to
use the Tab key, which is not correct.

Comment 1

[karnaze (gone)]

Eric, I'm not sure if this is yours.

Comment 2

[gerardok]

QA Contact update.

Comment 3

[ekrock's old account (dead)]

Marking M17.

Comment 4

[Kathleen :Brade]

Changing summary from "Cursor" to "Arrow"

Comment 5

[Greg Engel]

Making test case ([e-mail]), also marking 4xp

Comment 6

[Greg Engel]

Attachment: Testcase for bug 17602

Comment 7

[Gervase Markham [:gerv]]

Note that the requested behaviour is that of IE. NS 4.x does things the way Moz 
does them. If, in this case, Moz is following NS rather than IE conventions, 
this bug is INVALID.

Gerv

Comment 8

[Eric Pollmann]

Rescheduling

Comment 9

[Lakespur Roca]

Pollman, the updated Netscape UI for this is speced at 
http://gooey/client/5.0/specs/keyboard/kybdnav2.htm

Note that the Focus behavior has also changed to enhanse the recognition of 
Radio button groups.

Any one outside Netscape please contact me if you need this.

Comment 10

[Eric Pollmann]

This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another 
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration.

Comment 11

[bsharma]

Updating QA contact.

Comment 12

[Vladimir Ermakov]

QA Contact Update

Comment 13

[Blake Ross]

taking.

Comment 14

[Vladimir Ermakov]

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

Comment 15

[Jesse Ruderman]

This is also a problem in prefs with Modern3, but not with Classic.

Comment 16

[Blake Ross]

usability/polish, 0.9.4.

Comment 17

[Peter Trudelle]

adding access keyword, cc'ing aaronl for input.  Is this critical for moz1.0? If
so, should move to earlier milestone, else move to later one.

Comment 18

[Aaron Leventhal]

From an accessibility standpoint, we need to fix this at some point, but after
we fix things that just plain aren't accessible at all.

Comment 19

[Blake Ross]

-> default owner

Comment 20

[Jesse Ruderman]

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

Comment 21

[Aaron Leventhal]

-> John Keiser

Comment 22

[Tim Powell]

This is a consistency issue that affects web-based applications. The XUL dialogs
already handle this as requested in this bug, so Mozilla is inconsistent in
behavior between HTML forms and XUL dialogs.

Comment 23

[Aaron Leventhal]

John, do you even want bugs assigned to you any more?

Comment 24

[Jesse Ruderman]

This bug still exists.  Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a)
Gecko/20040423 Firefox/0.8.0+

Comment 25

[Aaron Leventhal]

Attachment: 1) Only selected radios are tabbable unless nothing selected in current group, 2) Arrow keys change selection and current focus

Next we'll have to file a bug on focus outlines not surrounding the current
radio or checkbox <label>

Comment 26

[Aaron Leventhal]

Attachment: Another test case

This testcase works better with our patch than with IE, which gets confused
about what radio group is current. On the other hand, IE does a better job with
the focus outline (puts it around the labels specified in this test case).

Comment 27

[[e-mail]]

Comment on attachment 151379 [details] [diff] [review]
1) Only selected radios are tabbable unless nothing selected in current group, 2) Arrow keys change selection and current focus

Your navigation code fails in the case where the focused radio button isn't the
current button, this can occur when no button is current, but it can also occur
when a button cancels the click.

Since access keys focus the the element before clicking could you make up/down
do that too?

IE supports shifted arrow keys (well it's one fewer modifer to check... should
you really be using + instead of ||?).

That callback from the input element back to the esm looks ugly :-/

Comment 28

[Aaron Leventhal]

Attachment: Second test case

Comment 29

[Aaron Leventhal]

BTW, the tabindex check for nsIDOMHTMLInputElement is going away. There's going
to be a general tabindex check at the start of esm for all elements, but that's
another patch in bug 171366. It removes all of the dozen plus tabindex checks in
GetNextTabbableContent() in favor of just one. Also, we're going to put tabIndex
on nsIDOMNSHTMLElement.

Anyway, this brings me to "That callback from the input element back to the esm
looks ugly :-/"  For now I'd like to keep that in esm. Ideally all elements
would know if they're tabbable. I'd like nsIDOMNSHTMLElement to have a |readonly
attribute boolean isTabbable| -- perhaps also on nsIDOMXULElement.

However, we're not there quite yet. For now, esm is still a big hack and it uses
the static variable sTabFocusModel directly. In the future I'd like it to ask
the element whether it's focusable. When we do that we'll put the static on
nsGenericHTMLElement or something and support ::GetIsTabbable there.

So for now, to keep these big changes managable I'd like to keep GetTabbable()
where it is, if that's okay. We can file another bug to do the code improvements
I'm talking about, which will be another reasonable incremental step toward
cleaning up esm's tab navigation code. We just can't do it all at once.

Comment 30

[Aaron Leventhal]

Attachment: New patch addressing Neil's comments (other than moving GetTabbable)

Did not move GetTabbable at the moment, for reasons explained in previous
comment.

Comment 31

[Aaron Leventhal]

Comment on attachment 152208 [details] [diff] [review]
New patch addressing Neil's comments (other than moving GetTabbable)

Carrying bryner's r= over.

Comment 32

[Aaron Leventhal]

Attachment: Create GetNextRadioButton method instead of trying to force that functionality into GetCurrentRadioButton

Comment 33

[Aaron Leventhal]

Comment on attachment 152221 [details] [diff] [review]
Create GetNextRadioButton method instead of trying to force that functionality into GetCurrentRadioButton

Carrying bryner's r=

Comment 34

[[e-mail]]

Comment on attachment 152221 [details] [diff] [review]
Create GetNextRadioButton method instead of trying to force that functionality into GetCurrentRadioButton

>   /**
>-   * Set the current radio button in a group
>+   * Get the current radio button in a group
>    * @param aName the group name
>-   * @param aRadio the currently selected radio button [OUT]
I don't think you meant to delete this line.
>    */
>   NS_IMETHOD GetCurrentRadioButton(const nsAString& aName,
>                                    nsIDOMHTMLInputElement** aRadio) = 0;
> 
>   /**
>+   * Set the next/prev radio button in a group
>+   * @param aDirection == -1 to get prev in group, == 1 for next
>+   * @param aStartRadio the radio button to start from
>+   * @param aRadio the currently selected radio button [OUT]
>+   */
>+  NS_IMETHOD GetNextRadioButton(const nsAString& aName,
>+                                const PRBool aPrevious,
>+                                nsIDOMHTMLInputElement*  aFocusedRadio,
>+                                nsIDOMHTMLInputElement** aRadio) = 0;
Fix the comment to reflect reality :-P

Comment 35

[Aaron Leventhal]

Checking in content/html/content/public/nsIRadioGroupContainer.h;
/cvsroot/mozilla/content/html/content/public/nsIRadioGroupContainer.h,v  <-- 
nsIRadioGroupContainer.h
new revision: 1.3; previous revision: 1.2
done
Checking in content/base/src/nsDocument.h;
/cvsroot/mozilla/content/base/src/nsDocument.h,v  <--  nsDocument.h
new revision: 3.235; previous revision: 3.234
done
Checking in content/base/src/nsDocument.cpp;
/cvsroot/mozilla/content/base/src/nsDocument.cpp,v  <--  nsDocument.cpp
new revision: 3.508; previous revision: 3.507
done
Checking in content/html/content/src/nsHTMLFormElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLFormElement.cpp,v  <-- 
nsHTMLFormElement.cpp
new revision: 1.166; previous revision: 1.165
done
Checking in content/html/content/src/nsHTMLInputElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLInputElement.cpp,v  <-- 
nsHTMLInputElement.cpp
new revision: 1.356; previous revision: 1.355
done
Checking in content/events/src/nsEventStateManager.cpp;
/cvsroot/mozilla/content/events/src/nsEventStateManager.cpp,v  <-- 
nsEventStateManager.cpp
new revision: 1.507; previous revision: 1.506
done

Comment 36

[:deanis74]

Aaron, did you file that bug you mentioned in commen 25?

Comment 37

[Aaron Leventhal]

(In reply to comment #36)
> Aaron, did you file that bug you mentioned in comment 25?

Just filed bug 249813 - und ja it was on my todo list :]

Comment 38

[Johnny Stenback (:jst)]

Next time you go an change a DOM interface, make sure to get my or peterv's
approval for the changes before landing them!

Comment 39

[Boris Zbarsky [:bzbarsky]]

This caused bug 253299