Open Bug 864678 Opened 11 years ago Updated 1 year ago

do we need to fire focus events when moving between options in a select?

Categories

(Core :: Disability Access APIs, defect)

defect

Tracking

()

People

(Reporter: tbsaunde, Unassigned, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=c++])

Attachments

(5 files)

we certainly did at one point, but it appears that now if you have
<select>
<option>opt 1</option>
<option>opt 2</option>
</select>
and do opt2.takeFocus() no focus event is fired but a selection change is.
With size=1, you don't if it's collapsed but do if it's expanded. With size>1, you always do, since you can't collapse it.

For collapsed size=1, Gecko used to fire focus, but this changed some time ago; I can't remember the reason. In Windows, firing focus for collapsed size=1 changes is non-standard anyway.
(In reply to James Teh [:Jamie] from comment #1)
> With size=1, you don't if it's collapsed but do if it's expanded. With
> size>1, you always do, since you can't collapse it.
> 
> For collapsed size=1, Gecko used to fire focus, but this changed some time
> ago; I can't remember the reason. In Windows, firing focus for collapsed
> size=1 changes is non-standard anyway.

It seemed so logical at that point :) btw, we fire focus in collapsed selects for JAWS.

(In reply to Trevor Saunders (:tbsaunde) from comment #0)
> we certainly did at one point, but it appears that now if you have
> <select>
> <option>opt 1</option>
> <option>opt 2</option>
> </select>
> and do opt2.takeFocus() no focus event is fired but a selection change is.

I think we still fire it on user input, otherwise somebody reported the bug already. I guess takeFocus is broken.
Presumably we fire a11y focus event on DOMMenuItemActive event. In case of takeFocus it focus the select and change current item of the widget (SetCurrentItem) which change @selected attribute on option. It doesn't fire DOMMenuItemActive event because it's user event.

So takeFocus was supposed to be universal code but perhaps easier way to fix it is to override takeFocus on HTML option accessible and call FocusMgr()->ActiveItemChanged() (and make sure the select is focused).

Jamie, does it hit you that takeFocus doesn't always work on HTML option?
(In reply to alexander :surkov from comment #3)
> Jamie, does it hit you that takeFocus doesn't always work on HTML option?
I'm not sure.
1. I thought the method was called focus(), not takeFocus().
2. I can't make this work and I suspect I'm doing something wrong in my code. Can someone provide a working test case?
3. Do you mean size=1 or size>1? They are quite different in terms of expected behaviour.
size>1, the method is accSelect(SELFLAG_TAKEFOCUS).
Oh, I thought you meant from JavaScript.
I've never had cause to test this before. However, I can definitely reproduce it for both size>1 and size=1 expanded. No focus event is fired and the focused state remains on the old item without being exposed for the new.
So is it a problem for you? I need to understand a priority.
It's low priority for us. Users can trigger this with object navigation, but most (even I) wouldn't bother.
ok, putting it into mentored box then
Whiteboard: [mentor=surkov.alexander@gmail.com][lang=c++]
Hi,I am interested in working on this bug,but it's my first time to work on with debug,can anybody guide me on how to get started with it?Thanks a lot.
I'd like to work on this bug. I'm new to Mozilla and it will be my first bug if assigned to me.
Attached image output from the console
output of running test_takeFocus.html
Comment on attachment 8421890 [details]
output from the console

above is the output after run mochitest on test_takeFocus.html with an added select of size 3. The original file passes the tests as well, which contains a combobox of size 5.
My impression upon reading the comments is that this bug can be reproduced with select nodes of size=1 and size>1.  But neither has been seen ( I previously tested on calling takeFocus() on a collapsed select with size=1, and the test passed as well).
(In reply to Xuezhong Wang from comment #15)
> Comment on attachment 8421890 [details]
> output from the console
> 
> above is the output after run mochitest on test_takeFocus.html with an added
> select of size 3. The original file passes the tests as well, which contains
> a combobox of size 5.
> My impression upon reading the comments is that this bug can be reproduced
> with select nodes of size=1 and size>1.  But neither has been seen ( I
> previously tested on calling takeFocus() on a collapsed select with size=1,
> and the test passed as well).

Correct: the test failed (time out) if calling takeFocus() on a collapsed select node. This is the only failed test case observed so far.
I don't see anything wrong in your test, it looks like we don't have a bug. You may want to enable focus logging via export A11YLOG=focus to double check.

Jamie how did you test it?
Windows specific str:
1. Open this URL: data:text/html,<select size="2"><option>a</option><option>b</option></select>
2. Ensure the "a" option is focused and selected.
3. Get the IAccessible for the "b" option.
4. Call IAccessible::accSelect(SELFLAG_TAKEFOCUS, CHILDID_SELF).
Expected: The "b" option should be focused.
Actual: Nothing happens; the "a" option is still focused.
How to write it into a test? accSelect() is not a js function anymore right?  I tried to test it as with takeFocus() but failed.
(In reply to Xuezhong Wang from comment #19)
> Created attachment 8422405 [details] [diff] [review]
> accSelectTest.patch
> 
> How to write it into a test? accSelect() is not a js function anymore right?
> I tried to test it as with takeFocus() but failed.

do what you do but do not focus document, in other words, click 'a' option and then takeFocus() on 'b' option.
takeFocus() still passes (see attached patch). I thought we should check accSelect() instead of takeFocus() upon James' comment.
Comment on attachment 8422757 [details] [diff] [review]
takeFocusTest.patch

Review of attachment 8422757 [details] [diff] [review]:
-----------------------------------------------------------------

pls attach one patch instead few ones for easier review

::: accessible/tests/mochitest/focus/test_takeFocus.html
@@ +43,2 @@
>  	{
> +	  document.getElementById(bID).click();

it doesn't make what you wanted, it clicks it when this object is created, ie before any test run. you need to have one invoker for each actions like synthClick invoker from events.js plus takeFocus invoker.

@@ +43,3 @@
>  	{
> +	  document.getElementById(bID).click();
> +	  

no whitespaces on empty lines

@@ +58,2 @@
>        }
>  	}

please do right indentation
Attached is the single patch that reproduce the bug. The test to invoke the 'takeFocus' for 'cat' option time out.
Mentor: surkov.alexander
Whiteboard: [mentor=surkov.alexander@gmail.com][lang=c++] → [lang=c++]
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.