Last Comment Bug 804040 - Selection event not fired when selection of ARIA tab changes
: Selection event not fired when selection of ARIA tab changes
Status: RESOLVED FIXED
[davidb +1]
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86_64 Windows 7
: -- normal (vote)
: mozilla19
Assigned To: alexander :surkov
:
Mentors:
Depends on:
Blocks: aria
  Show dependency treegraph
 
Reported: 2012-10-21 19:07 PDT by James Teh [:Jamie]
Modified: 2012-11-11 04:09 PST (History)
3 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Test case. (284 bytes, text/html)
2012-10-21 19:07 PDT, James Teh [:Jamie]
no flags Details
wip (31.72 KB, patch)
2012-11-09 05:33 PST, alexander :surkov
no flags Details | Diff | Splinter Review
patch (33.07 KB, patch)
2012-11-09 05:46 PST, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description James Teh [:Jamie] 2012-10-21 19:07:18 PDT
Created attachment 673744 [details]
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 2012-10-21 19:51:25 PDT
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] 2012-10-21 21:04:38 PDT
(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 2012-10-21 21:27:58 PDT
(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] 2012-10-21 21:42:55 PDT
(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 2012-10-21 21:48:04 PDT
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] 2012-10-21 22:18:14 PDT
(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) 2012-10-22 10:14:05 PDT
(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 2012-10-22 23:32:00 PDT
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] 2012-10-23 06:30:22 PDT
(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] 2012-10-23 22:16:05 PDT
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 2012-10-25 07:39:03 PDT
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] 2012-10-25 16:28:05 PDT
(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 2012-11-09 04:28:54 PST
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 2012-11-09 04:35:29 PST
I filed bug 810268 for 2nd item, let's keep this bug for 1st issue
Comment 15 alexander :surkov 2012-11-09 05:33:35 PST
Created attachment 680056 [details] [diff] [review]
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 2012-11-09 05:46:57 PST
Created attachment 680058 [details] [diff] [review]
patch

we can take this and file follow up based on comment #15 to make accessible types nicer.
Comment 17 Trevor Saunders (:tbsaunde) 2012-11-09 23:32:48 PST
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 2012-11-09 23:37:53 PST
(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) 2012-11-09 23:47:45 PST
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 2012-11-10 02:08:57 PST
filed bug 810572 on this
Comment 21 Ryan VanderMeulen [:RyanVM] 2012-11-11 04:09:52 PST
https://hg.mozilla.org/mozilla-central/rev/09c4b0d21581

Note You need to log in before you can comment on or make changes to this bug.