403210 - extra focus (hidden) event fired when tabbing away from combobox, resulting in unidentified state for screen readers.

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P3)

Description

[Marco Zehe (:MarcoZ)]

Steps to reproduce:
1. Start JAWS and Minefield.
2. Go to Tools/Options.
3. Select "Main" page.
4. Tab to the "When Minefield starts:" combobox. By default. it is set to "Show my home page".
5. Tab again to the "Homepage" text field.

Expected: JAWS should speak and braille the prompt and text for the text field.
Actual result: The prompt and text are briefly shown on the braille display, but then, the prompt goes away, and only the highlighted text of the edit field is shown. Speech sometimes may pick up the label in time, but a subsequent of INSERT+TAB, which is a keystroke to speak currently focused item, will result in silence.

Upon investigation, using AccEvent32, I found that, after the Focus event is fired for the textfield, a Focus (hidden) event is fired for a NULL object. No name, value, or any other information is shown for that object.

6. Shift+Tab back to the combobox.
7. Tab forward again to the "Home page" edit field.

Result: This time, the prompt persists for both speech and braille, and AccEvent32 shows that the Focus (hidden) event is not fired again.

This is reproducible 100% of the time for me on both XP and Vista on two different machines. It affects all comboboxes in XUL dialogs.
To prove: Close and reopen Tools/Options and repeat the above steps. When tabbing away from the combobox the first time, that Focus (hidden) event will be fired again.

Note that you do not need to interact at all with the combobox. You just have to tab away from it the first time. Also, if one combobox has been passed by twice, other comboboxes will still exhibit this behaviour.

Comment 1

[Damon Sicore (:damons)]

Is this a regression?

Comment 2

[Marco Zehe (:MarcoZ)]

(In reply to comment #1)
> Is this a regression?

Yes. This first happened in 3.0a9pre/2007102604. The checkins for the given period are: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2007-10-25+02%3A00&maxdate=2007-10-26+09%3A00&cvsroot=%2Fcvsroot

There were three accessibility-related fixes in that period, one of them having to do with comboboxes.

Comment 3

[Marco Zehe (:MarcoZ)]

I have backed out bug 399128 and bug 399858 from a local tree, but both are not the cause of the problem.

Comment 4

[Aaron Leventhal]

This is in fact caused by bug 399128.

Comment 5

[Aaron Leventhal]

It's weird though, that we're receiving DOMMenuItemActive as we tab away from the XUL combo box. That might be something caused by the menu rearchitecture.

Comment 6

[Aaron Leventhal]

Evan's patch is really not to blame -- it just uncovered the real problem.

Here is the line which causes the extra DOMMenuItemActive event to be fired after a tab key press. In the keypress handler we set the active menu item.
http://mxr.mozilla.org/seamonkey/source/toolkit/content/widgets/menulist.xml#42

Doing the code archeology is sending me back to 2004, so I suspect that because of Neil Deakin's recent menu rearchitecture this extra event is now being fired later than it used to be (now it's after the focus on the item that's tabbed to). It wasn't a delayed event in the past.

Comment 7

[Aaron Leventhal]

I guess we need to fire DOMMenuItemActive asynch now, so we probably need to be smarter in the keypress handler in menulist.xml

The original code to set the active menu item on any keypress was in bug 65157.

Comment 8

[Aaron Leventhal]

Neil Rashbrook wrote the original keypress handler in menulist.xml which sets the active menu item, in bug 64157 (the last comment accidentally had the wrong bug number).

Comment 9

[Aaron Leventhal]

Neil, should the keypress handler have a whitelist or blacklist of keys to check before setting the active menu item? Or do you have another solution in mind?

Comment 10

[Aaron Leventhal]

Attachment: Whitelist for keypresses that move relative to the current item in a combobox

Comment 11

[[e-mail]]

(In reply to comment #9)
>Neil, should the keypress handler have a whitelist or blacklist of keys to
>check before setting the active menu item?
I guess a whitelist would work. You should check for up, down, home, end and backspace. I don't see why you need to check for ctrlKey.

Comment 12

[Aaron Leventhal]

Ctrl key is for case where accesskey modifier is Ctrl (sometimes the case on Unix). I believe Ctrl+A when then produce a charCode of 1, no?

Also, Ctrl+PageUp/Ctrl+Pagedown switches tab panels.

Comment 13

[Aaron Leventhal]

Attachment: Add backspace to whitelist

Comment 14

[[e-mail]]

(In reply to comment #12)
>Ctrl key is for case where accesskey modifier is Ctrl (sometimes the case on
>Unix). I believe Ctrl+A when then produce a charCode of 1, no?
No, we produce a charCode of 97. If you only want to exclude the ctrl key where the accesskey modifier is ctrl then you should probably use modifiers="accel shift any" instead. If you want to completely exclude alt/meta/ctrl keys then you should probably use modifiers="shift any" instead. 

>Also, Ctrl+PageUp/Ctrl+Pagedown switches tab panels.
Menulists don't use PageUp/PageDown.

Comment 15

[Aaron Leventhal]

> Menulists don't use PageUp/PageDown

They are supposed to:
https://bugzilla.mozilla.org/show_bug.cgi?id=250091

I'm concerned that someone will fix that in another bug and not notice this needs to be updated.

Comment 16

[Aaron Leventhal]

Attachment: Use modifiers="shift any". I kept the logic for pgup/pgdn because I believe it will be fixed (currently works that way in HTML and in desktop widgets)

Comment 17

[[e-mail]]

Comment on attachment 288688 [details] [diff] [review]
Use modifiers="shift any". I kept the logic for pgup/pgdn because I believe it will be fixed (currently works that way in HTML and in desktop widgets)

All key presses are relative, so you have to make sure that the the activeChild is set even for characters. (I wonder whether that's best done by a focus event handler?) You don't need to patch xpfe, it's going to go away as soon as Camino work out how to stop building it. Also toolkit doesn't need sr.

Comment 18

[Aaron Leventhal]

Attachment: Corrected patch

Comment 19

[[e-mail]]

Comment on attachment 288726 [details] [diff] [review]
Corrected patch

>           if (event.originalTarget == this) {
>-            this.menuBoxObject.activeChild = this.mSelectedInternal;
>+            if (event.keyCode == KeyEvent.DOM_VK_UP ||
>+                event.keyCode == KeyEvent.DOM_VK_DOWN ||
>+                event.keyCode == KeyEvent.DOM_VK_PAGE_UP ||
>+                event.keyCode == KeyEvent.DOM_VK_PAGE_DOWN ||
>+                event.keyCode == KeyEvent.DOM_VK_HOME ||
>+                event.keyCode == KeyEvent.DOM_VK_END ||
>+                event.keyCode == KeyEvent.DOM_VK_BACK_SPACE ||
>+                event.charCode) {
>+              // Moving relative to an item: start from the currently selected item
>+              this.menuBoxObject.activeChild = this.mSelectedInternal;
>+            }
>             if (this.menuBoxObject.handleKeyPress(event))
>               this.menuBoxObject.activeChild.doCommand();
>           }
All the code needs to be inside the if, not just the one line...

Comment 20

[Aaron Leventhal]

Attachment: Address more comments

Good point Neil :) I'm having more problems getting this simple patch in .... !

Comment 21

[[e-mail]]

Comment on attachment 288840 [details] [diff] [review]
Address more comments

>+              (event.keyCode == KeyEvent.DOM_VK_UP ||
>+              event.keyCode == KeyEvent.DOM_VK_DOWN ||
>+              event.keyCode == KeyEvent.DOM_VK_PAGE_UP ||
>+              event.keyCode == KeyEvent.DOM_VK_PAGE_DOWN ||
>+              event.keyCode == KeyEvent.DOM_VK_HOME ||
>+              event.keyCode == KeyEvent.DOM_VK_END ||
>+              event.keyCode == KeyEvent.DOM_VK_BACK_SPACE ||
>+              event.charCode > 0)) {
Extra space on the continuation lines so that the word event lines up please?

Comment 22

[Aaron Leventhal]

Comment on attachment 288840 [details] [diff] [review]
Address more comments

Will change spacing at checkin time.