804040 - Selection event not fired when selection of ARIA tab changes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[James Teh [:Jamie]]

Attachment: Test case.

Str:
1. Open the attached test case.
2. Press the "Select tab" button.
Expected: A state change event should be fired on the tab.
Actual: No state change event is fired.
Impact: Screen readers such as NVDA that maintain their own representation of the document are not notified when tab selection changes and thus report this incorrectly to the user.

Comment 1

[alexander :surkov (:asurkov)]

Shouldn't we fire selection event instead? We do that for XUL tabs and we don't fire state change events for them.

Comment 2

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #1)
> Shouldn't we fire selection event instead?
My apologies. Yes, it should be a selection event. The only problem is that this wouldn't signal to AT when a given tab is unselected. Hmm.

Comment 3

[alexander :surkov (:asurkov)]

(In reply to James Teh [:Jamie] from comment #2)
> (In reply to alexander :surkov from comment #1)
> > Shouldn't we fire selection event instead?
> My apologies. Yes, it should be a selection event. The only problem is that
> this wouldn't signal to AT when a given tab is unselected. Hmm.

selection event is fired when single item is selected (all others are unselected) within a container, otherwise we fire selection_add/remove or within events.

Why do you need to know an item was unselected?

Btw, aria implementation guide is unclear:

5.8.1. State and Property Change Events (http://www.w3.org/WAI/PF/aria-implementation/#mapping_events_state-change):

aria-selected (state):
EVENT_OBJECT_SELECTIONADD
EVENT_OBJECT_SELECTIONREMOVE

5.8.3. Selection (http://www.w3.org/WAI/PF/aria-implementation/#mapping_events_selection):

Toggle aria-selected:
EVENT_OBJECT_SELECTIONADD/EVENT_OBJECT_SELECTIONREMOVE on the current container + EVENT_OBJECT_STATECHANGE on the item

Selection follows focus:
EVENT_OBJECT_SELECTION then EVENT_OBJECT_STATECHANGE on newly focused item, but arrange events so state change does not occur on focused item, to avoid extra selection change announcements

Select or deselect many items at once:
EVENT_OBJECT_SELECTIONWITHIN is all that is necessary. The state change events MAY be trimmed out for performance.

Comment 4

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #3)
> Why do you need to know an item was unselected?
So that NVDA's representation can be updated such that selected is no longer reported for the tab that was just unselected. This is why I originally thought state change, but selection does make more sense. Ug.

Comment 5

[alexander :surkov (:asurkov)]

I'm fine to follow any reasonable idea. Btw, ARIA impl guide says to fire selection and state change events both. Does make sense for you? Do you know what does IE/others do?

Comment 6

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #5)
> I'm fine to follow any reasonable idea. Btw, ARIA impl guide says to fire
> selection and state change events both. Does make sense for you?
Ideally, we need an event to be fired when selected and an event to be fired when unselected. I guess selection makes sense for selection and state change for unselection, though that's obviously asymmetric. It just seems silly to fire two events for a single action.

> Do you know
> what does IE/others do?
I'm not sure. For IE, due to the major deficiencies in its accessibility implementation, we have to watch the DOM directly using a different API, so we don't really take much interest in its MSAA events. :)

Comment 7

[Trevor Saunders (:tbsaunde)]

(In reply to James Teh [:Jamie] from comment #6)
> (In reply to alexander :surkov from comment #5)
> > I'm fine to follow any reasonable idea. Btw, ARIA impl guide says to fire
> > selection and state change events both. Does make sense for you?
> Ideally, we need an event to be fired when selected and an event to be fired
> when unselected. I guess selection makes sense for selection and state
> change for unselection, though that's obviously asymmetric. It just seems
> silly to fire two events for a single action.

I would agree there's some dupplication, but I think firing both selection and selected state change events would be correct.  Otherwise someone who only tracks states might not know one has changed.

Comment 8

[alexander :surkov (:asurkov)]

Currently we fire events (in case of Firefox tabs):

    EVENT_OBJECT_SELECTION when single item was selected, event is targeted to item.

    EVENT_OBJECT_SELECTIONADD and EVENT_OBJECT_SELECTIONREMOVE when item(s) was unselected and/or items were selected, events are targeted to changed items.

    EVENT_OBJECT_SELECTIONWITHIN when selection within control was changed much, for example, when all items were selected. In this case screen readers can announce that selection was changed rather than announce every changed item. Event target is control.

if we fire state_change events then they dupe selectionadd/selectionremove events. Perhaps it's ok if they are used for different proposes like selection/selectionadd/selectionremove to announce the change, state change events to update the buffer.

Anyway, it's not in sync with ARIA spec what puts us into interesting state.

David, is ARIA on your plate?

Comment 9

[David Bolter [:davidb] (NeedInfo me for attention)]

(In reply to alexander :surkov from comment #8)

> David, is ARIA on your plate?

Yes I can take this one. (I need a bigger plate)

Are we all in agreement we should fire a state change event for selection? (I've put my vote in the whiteboard)

Comment 10

[James Teh [:Jamie]]

FWIW, Chrome fires selectionAdd on a tab which gets selected and selectionRemove on a tab which gets unselected. Technically, this is a bit strange, since tab controls are a single selection control, but it does solve the problem without duplicate events.

Comment 11

[alexander :surkov (:asurkov)]

Jamie, in that case it seems AT should learn how to differentiate following cases on single selection list:
1) selection were moved: selection remove + selection add events
2) selection was lost: selection remove event

(case of listbox)

Comment 12

[James Teh [:Jamie]]

(In reply to alexander :surkov from comment #11)
> Jamie, in that case it seems AT should learn how to differentiate following
> cases on single selection list:
> 1) selection were moved: selection remove + selection add events
> 2) selection was lost: selection remove event
NVDA should handle that fine, but some might argue it seems odd for single selection list boxes. Maybe we should just add the stateChange as suggested elsewhere. Either way, we probably need to check/test with other ATs.

Comment 13

[alexander :surkov (:asurkov)]

so we deal with two problems here:
1) ARIA tabs aren't subject of selection events
2) there's no way to know unselected item for single selection container

Comment 14

[alexander :surkov (:asurkov)]

I filed bug 810268 for 2nd item, let's keep this bug for 1st issue

Comment 15

[alexander :surkov (:asurkov)]

Attachment: wip

Trev, I'm thinking to combine
1) AccTypes.h and protected Accessible::AccessibleTypes
2) make mFlags 64bit unit
3) introduce public Accessible::IsOfType(unit64_t aAccTypes) and protected SetAccType() which take into account shift of StateFlags and ChildrenFlags.

How does it sound?

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch

we can take this and file follow up based on comment #15 to make accessible types nicer.

Comment 17

[Trevor Saunders (:tbsaunde)]

Comment on attachment 680058 [details] [diff] [review]
patch

do you really need both the friend thing and public in Accessible.h for the enum>

Comment 18

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #17)
> Comment on attachment 680058 [details] [diff] [review]
> patch
> 
> do you really need both the friend thing and public in Accessible.h for the
> enum>

no, I removed it already (artifacts, sorry). thank you for the catch

how do you feel about comment #15?

Comment 19

[Trevor Saunders (:tbsaunde)]

sorry I forgot to answer that, I'm certainly fine with everything other than making mFlags 64 bits.  I'll live with that if we need to, but of course I'd rather not.  Note the mRoleMapEntry poitner is pretty low data for a pointer size object maybe we should make that an index into the array and stuff that in the mFlags blob.

I'd actually sort of like to use packed bit field stuff to avoid the nasty shifting alltogether so we'd have like

uint32_t mTypeFlags : 20;
uint32_t mChildFlags : 2;
uint32_t mRoleMap : 5;
or something the one worrying thing about that is we could make accessibles bigger and not really notice, maybe we can add static asserts?

The other thing I started pondering was creating some seperate mTypeInfo struct for accessibles like mNodeInfo for nodes.

Comment 20

[alexander :surkov (:asurkov)]

filed bug 810572 on this

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/09c4b0d21581