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

Status: NEW

(Core :: Disability Access APIs, defect)

Description

[Trevor Saunders (:tbsaunde)]

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.

Comment 1

[James Teh [:Jamie]]

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.

Comment 2

[alexander :surkov (:asurkov)]

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

Comment 3

[alexander :surkov (:asurkov)]

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?

Comment 4

[James Teh [:Jamie]]

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

Comment 5

[alexander :surkov (:asurkov)]

size>1, the method is accSelect(SELFLAG_TAKEFOCUS).

Comment 6

[James Teh [:Jamie]]

Oh, I thought you meant from JavaScript.

Comment 7

[James Teh [:Jamie]]

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.

Comment 8

[alexander :surkov (:asurkov)]

So is it a problem for you? I need to understand a priority.

Comment 9

[James Teh [:Jamie]]

It's low priority for us. Users can trigger this with object navigation, but most (even I) wouldn't bother.

Comment 10

[alexander :surkov (:asurkov)]

ok, putting it into mentored box then

Comment 11

[MikeLing]

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.

Comment 12

[Xuezhong Wang]

I'd like to work on this bug. I'm new to Mozilla and it will be my first bug if assigned to me.

Comment 13

[Xuezhong Wang]

Attachment: output from the console

output of running test_takeFocus.html

Comment 14

[Xuezhong Wang]

Attachment: addSelectTest.patch

Comment 15

[Xuezhong Wang]

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

Comment 16

[Xuezhong Wang]

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

Comment 17

[alexander :surkov (:asurkov)]

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?

Comment 18

[James Teh [:Jamie]]

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.

Comment 19

[Xuezhong Wang]

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

Comment 20

[alexander :surkov (:asurkov)]

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

Comment 21

[Xuezhong Wang]

Attachment: takeFocusTest.patch

takeFocus() still passes (see attached patch). I thought we should check accSelect() instead of takeFocus() upon James' comment.

Comment 22

[alexander :surkov (:asurkov)]

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

Comment 23

[Xuezhong Wang]

Attachment: bug864678_mochitest.patch

Attached is the single patch that reproduce the bug. The test to invoke the 'takeFocus' for 'cat' option time out.