Last Comment Bug 414302 - Incorrect selection events in HTML, XUL and ARIA (originally filed because of XUL tabs)
: Incorrect selection events in HTML, XUL and ARIA (originally filed because of...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: All All
: P2 normal with 1 vote (vote)
: mozilla10
Assigned To: alexander :surkov
:
Mentors:
Depends on: 701669 754377
Blocks: aria 414721 a11ynext 526227
  Show dependency treegraph
 
Reported: 2008-01-27 21:31 PST by Eitan Isaacson [:eeejay]
Modified: 2012-05-11 21:54 PDT (History)
15 users (show)
mtschrep: wanted‑next+
mbeltzner: blocking1.9.2-
mtschrep: blocking1.9-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP. Needs testing. (21.96 KB, patch)
2008-01-31 19:38 PST, Aaron Leventhal
no flags Details | Diff | Splinter Review
WIP. Still not right but a good start. (29.80 KB, patch)
2008-02-01 13:52 PST, Aaron Leventhal
no flags Details | Diff | Splinter Review
Still WIP (26.82 KB, patch)
2008-02-01 14:28 PST, Aaron Leventhal
no flags Details | Diff | Splinter Review
updated aaron WIP (29.51 KB, patch)
2009-09-15 12:10 PDT, David Bolter [:davidb]
no flags Details | Diff | Splinter Review
wip2 (31.34 KB, patch)
2009-09-17 13:25 PDT, David Bolter [:davidb]
no flags Details | Diff | Splinter Review
wip2 (with test included) (34.13 KB, patch)
2009-09-17 13:26 PDT, David Bolter [:davidb]
no flags Details | Diff | Splinter Review
patch 1 (34.80 KB, patch)
2009-09-21 07:48 PDT, David Bolter [:davidb]
no flags Details | Diff | Splinter Review
patch 2 (36.31 KB, patch)
2009-09-21 11:29 PDT, David Bolter [:davidb]
mzehe: review+
neil: review+
Details | Diff | Splinter Review
adds tests and comments (41.29 KB, patch)
2009-11-03 12:40 PST, David Bolter [:davidb]
no flags Details | Diff | Splinter Review
patch (48.64 KB, patch)
2011-10-04 23:35 PDT, alexander :surkov
mzehe: feedback-
Details | Diff | Splinter Review
patch2 (49.62 KB, patch)
2011-10-05 07:13 PDT, alexander :surkov
mzehe: feedback+
Details | Diff | Splinter Review
patch (51.04 KB, patch)
2011-10-12 23:21 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2008-01-27 21:31:03 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.11) Gecko/20071204 Ubuntu/7.10 (gutsy) Firefox/2.0.0.11
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008012704 Minefield/3.0b3pre

There is no indication via AT-SPI that tabs have been switched. There are 4 indirect ways of determining if a tab has been switched:

1. "object:visible-data-changed" emitted from the top-level frame. This will happen only if the two tabs are on different pages, if they point to identical URLs, then we won't get this. Orca today uses this method, it is problematic performance-wise since it is a very high-traffic event, and listening for it slows Orca.
2. "object:property-change:accessible-name" emitted from top-level frame. This is similar to the visible-data-changed event above, of course it will only be emitted if the documents in the two tabs have different titles.
3. "object:state-changed:focused" emitted from the relevant document frame. This is probably the most useful and straightforward event for ATs to use, the problem is that it only seems to be emitted when the tab change happens when the focus is in the document. if the focus is anywhere else, say the location bar, this event is not emitted.

Reproducible: Always




I think the proper event would be "object:selection-changed". It should be emitted from the tab list. This is how GAIL does it today. Reaching the relevant document frame should be easy since the tabs carry relationships of label-for with the document frame's scroll pane.
Comment 1 Aaron Leventhal 2008-01-31 18:16:33 PST
Right now you'd have to watch focus, but that should work.

We should fire selection change events as well.

Oddly, we never fire selection change for anything right now -- XUL, HTML or ARIA. We only fire state change events for the selected bit. Is that enough?
Comment 2 Eitan Isaacson [:eeejay] 2008-01-31 18:27:39 PST
Focus is fine if it were truly dependable, but like I wrote above, it only happens when focus is inside the document.

Ideally we would have selection changed events - just like GAIL, but state changed events would definitely be enough for Orca's purposes.
Comment 3 Aaron Leventhal 2008-01-31 19:38:21 PST
Created attachment 300812 [details] [diff] [review]
WIP. Needs testing.

Fixes selection events in general.

Selection change, add, remove and within events are supposed to occur on the container.
The state change occurs on the item.

The within event is a special event that means "many changes may have occured". In ATK we'll map that to a normal selection change.
Comment 4 Aaron Leventhal 2008-01-31 19:41:10 PST
It works with XUL tabs at least. Could use more testing with various XUL, HTML and ARIA widgets to ensure the events are right.
Comment 5 Aaron Leventhal 2008-01-31 19:44:05 PST
One tweak. For ARIA tabs we calculate the selected state by finding the panel with focus inside of it and following labelledby to the tab.

So that means whenever focus changes we should check to see if we need to fire a selection event of some kind.

There might be other ARIA single selects where we just expose the selected state for the focus (lists?).

However I suggest that any additional dealing with ARIA selection events based on focus should be a separate bug.
Comment 6 Aaron Leventhal 2008-02-01 13:52:32 PST
Created attachment 300927 [details] [diff] [review]
WIP. Still not right but a good start.

This is not simple. 

Internal events (newly reconstructed):
EVENT_SELECTION_CHANGE_IN_CONTAINER
EVENT_SELECTION_ADD_IN_CONTAINER
EVENT_SELECTION_REMOVE_IN_CONTAINER
EVENT_SELECTION_CHANGES_IN_CONTAINER
ATK: these all map to object::selection_changed
MSAA these map to EVENT_OBJECT_SELECTION, EVENT_OBJECT_SELECTIONADD, EVENT_OBJECT_SELECTIONREMOVE and EVENT_OBJECT_SELECTIONWITHIN

Widgets:
1) XUL tree (single and multiple)
2) XUL tabs
3) HTML <select> (single and multiple)
4) ARIA lists and trees (single and multiple)

Scenarios:
A) Toggle selection on the current item -
   SELECTION_ADD_IN_CONTAINER or SELECTION_REMOVE_IN_CONTAINER on container, then state change event on current item
B) Move focus with arrow keys, where selection follows focus
   SELECTION_IN_CONTAINER then state change event on newly focused item (must happen after focus event)
C) Select or deselect many items at once:
   SELECTIONS_IN_CONTAINER for container, no state change events (would be too much of a flood)
Comment 7 Aaron Leventhal 2008-02-01 14:28:23 PST
Created attachment 300934 [details] [diff] [review]
Still WIP

In the case of an unmodified arrow/home/end/page key, do we need a state change event for the old item since selection is cleared. IE doesn't fire that, and it sort of gets in the way. It's not really useful. Mick Curran says it's not necessary and I agree.

What about for ATK? Do we really need that? I'd prefer to leave it out since it does complicate things.
Comment 8 Eitan Isaacson [:eeejay] 2008-02-01 14:45:07 PST
Here are the equivalent scenarios with a GAIL treeview.

A) Toggle selection on the current item:
  state-changed:selected (either state added or removed)
  selection-changed

B) Move focus with arrow keys, where selection follows focus:
  state-changed:selected (removed from previously selected node)
  selection-changed
  Note: There is no state-changed:selected added event fired.

C) Select or deselect many items at once:
  Arbitrary state-changed:selected events, seems to be inconsistent, and usually   is only fired by one or two of the nodes in the selection range. It could also no occur at all.
  selection-changed

Note: "state-changed:selected" have the individual selected nodes as the source, and "selection-changed" is the event fired by the selection container.
Another note, in GAIL there seems to be no additional info to selection-changed, meaning it could be an add or a remove.
Comment 9 Marco Zehe (:MarcoZ) on PTO until August 15 2008-02-27 13:42:13 PST
Marking this one blocking-requested since after the fix for bug 406308 lands, in places like the Extensions Manager and some others, this becomes highly visible and causes extra chatter in screen readers. Requesting P1 after Firefox 3 B4.
Comment 10 Mike Schroepfer 2008-03-13 09:40:36 PDT
Marco/Aaron we making progress on this - Firefox 3 final ship is fast approaching...
Comment 11 Aaron Leventhal 2008-03-13 11:34:23 PDT
I'm coming back to this soon after dealing with a couple of other issues.
Comment 12 Aaron Leventhal 2008-03-13 12:18:19 PDT
We also need to check on correct selection and focus events in an HTML multiselect listbox.
Comment 13 Damon Sicore (:damons) 2008-03-18 10:50:28 PDT
Progress here?
Comment 14 Aaron Leventhal 2008-03-19 07:29:58 PDT
I got caught up in regression including one from a security fix, and no I'm packing up for my move to Germany. I probably can't get to this until Monday.
Comment 15 Mike Schroepfer 2008-03-19 09:34:23 PDT
IF that's the case than this will have to miss b5.  If same enough we can take in RC1.
Comment 16 Mike Schroepfer 2008-04-09 21:40:43 PDT
Given that this is a new feature, we are post RC1 code freeze, and there has been no activity for a month this will have to wait for post 3.0.   
Comment 17 Aaron Leventhal 2008-04-23 07:20:11 PDT
Affects ARIA as well.
Comment 18 David Bolter [:davidb] 2009-06-16 11:56:41 PDT
Mass un-assigning bugs assigned to Aaron.
Comment 19 David Bolter [:davidb] 2009-09-14 19:26:37 PDT
I can't recreate this. Closing as WORKSFORME - please reopen if necessary.
Comment 20 alexander :surkov 2009-09-14 19:35:09 PDT
David, Aaron's patch is reorganization of selection events code. There we fire more selection related events than the meantime. Be careful to close this bug.

Please file details how you tried to reproduce the bug.
Comment 21 David Bolter [:davidb] 2009-09-14 19:54:56 PDT
Heh -- mid air collision.

Reopening -- I agree we should expose selection change events.

(I was seeing focus events -- see comment 2 -- and thought that was enough, but
further discussion with Jamie, and recalling the headaches GOK had with
figuring out tab context changes made the light go on)
Comment 22 David Bolter [:davidb] 2009-09-14 20:41:45 PDT
Will, Joanmarie, do you guys have a workaround in the meantime?
Comment 23 David Bolter [:davidb] 2009-09-15 12:10:01 PDT
Created attachment 400831 [details] [diff] [review]
updated aaron WIP
Comment 24 David Bolter [:davidb] 2009-09-17 13:25:29 PDT
Created attachment 401270 [details] [diff] [review]
wip2

Added a test. More to come.

Note: this will clash/conflict with our event cleanup patch. I'll try to finish this first.
Comment 25 David Bolter [:davidb] 2009-09-17 13:26:58 PDT
Created attachment 401271 [details] [diff] [review]
wip2 (with test included)
Comment 26 David Bolter [:davidb] 2009-09-20 09:25:40 PDT
Tryserver build: https://build.mozilla.org/tryserver-builds/dbolter@mozilla.com-sanify-select-events/
Comment 27 David Bolter [:davidb] 2009-09-21 07:48:07 PDT
Created attachment 401852 [details] [diff] [review]
patch 1

This tests what we can from the core, we now need to see if the refined events are what is expected from the windows layer for browser tabs, and beyond.

With this patch we want to see these events mapped properly on Windows:
EVENT_OBJECT_SELECTION
EVENT_OBJECT_SELECTIONADD
EVENT_OBJECT_SELECTIONREMOVE
EVENT_OBJECT_SELECTIONWITHIN

On ATK we should see "selection_changed" fired appropriately.

It needs QA love.
Comment 28 David Bolter [:davidb] 2009-09-21 07:50:32 PDT
(Note I've changed the uuid on nsIAccessibleEvent locally)
Comment 29 Marco Zehe (:MarcoZ) on PTO until August 15 2009-09-21 08:08:04 PDT
Is the try server build the one that contains this latest patch, or will you kick off a newer one?
Comment 30 David Bolter [:davidb] 2009-09-21 08:18:39 PDT
Tryserver build is up to date for the things that matter. (New patch adds mochitests and removes some debug trace output).
Comment 31 Marco Zehe (:MarcoZ) on PTO until August 15 2009-09-21 08:23:38 PDT
Comment on attachment 401852 [details] [diff] [review]
patch 1

One nit before I try anything:
>+         new invokerChecker(nsIAccessibleEvent.EVENT_SELECTION_CHANGE_IN_CONTAINER, getAccessible("tabs")),

Please add a constant in events.js for this one.
Comment 32 Marco Zehe (:MarcoZ) on PTO until August 15 2009-09-21 08:58:12 PDT
First impressions:
1. Selecting a different item inside an HTML:select is *very* sluggish. For example changing the "version" on this bug from "unspecified" to "trunk" takes about a second for JAWS to respond with the try server build, but takes only a fraction of a second to speak with a regular nightly.

2. This patch does not help at all with bug 414721. Don't know if that was expected, but JAWS still gives "unselected tab1, tab2" inside Add-Ons manager.
Comment 33 David Bolter [:davidb] 2009-09-21 09:59:06 PDT
(In reply to comment #32)
> First impressions:
> 1. Selecting a different item inside an HTML:select is *very* sluggish. For
> example changing the "version" on this bug from "unspecified" to "trunk" takes
> about a second for JAWS to respond with the try server build, but takes only a
> fraction of a second to speak with a regular nightly.

Thanks. I'll dig into why this happens.

> 2. This patch does not help at all with bug 414721. Don't know if that was
> expected, but JAWS still gives "unselected tab1, tab2" inside Add-Ons manager.

That's as expected - a different class of problem.
Comment 34 David Bolter [:davidb] 2009-09-21 10:10:52 PDT
(Note: fixed locally the overriding of EVENT_SELECTION_REMOVE_IN_CONTAINER)
Comment 35 David Bolter [:davidb] 2009-09-21 10:43:38 PDT
Actually, unsure... we might be able to fix bug 414721 here. Marco did you test with NVDA?
Comment 36 David Bolter [:davidb] 2009-09-21 11:29:54 PDT
Created attachment 401897 [details] [diff] [review]
patch 2
Comment 37 David Bolter [:davidb] 2009-09-22 06:25:48 PDT
(In reply to comment #32)
> First impressions:
> 1. Selecting a different item inside an HTML:select is *very* sluggish. For
> example changing the "version" on this bug from "unspecified" to "trunk" takes
> about a second for JAWS to respond with the try server build, but takes only a
> fraction of a second to speak with a regular nightly.

I tested this using accprobe to watch EVENT_OBJECT_FOCUS, EVENT_OBJECT_STATECHANGE, and EVENT_OBJECT_SELECTION*. The events seem to be really fast and accurate.
Comment 38 Marco Zehe (:MarcoZ) on PTO until August 15 2009-09-22 07:53:04 PDT
I now cannot reproduce the sluggishness either. Seems to be tied to a different bug that JAWS 11 public beta has which might have been in effect. JAWS 10 and NVDA don't show the sluggish behavior with the tryserver build.
Comment 39 Marco Zehe (:MarcoZ) on PTO until August 15 2009-09-22 08:04:27 PDT
Comment on attachment 401897 [details] [diff] [review]
patch 2

>+const EVENT_SELECTION_CHANGE_IN_CONTAINER = nsIAccessibleEvent.EVENT_SELECTION_CHANGE_IN_CONTAINER;

Nit: Please mind the 80 char boundary per line.

r=me for the tests.
Comment 40 David Bolter [:davidb] 2009-09-22 10:37:36 PDT
Comment on attachment 401897 [details] [diff] [review]
patch 2

Neil, if you have time could you review this? Alexander is away this week.

(Original patch from Aaron)

I'd like to wrap up this bug before our larger event cleanup work.
Comment 41 David Bolter [:davidb] 2009-09-23 06:36:06 PDT
Tryserver builds for patch2: https://build.mozilla.org/tryserver-builds/dbolter@mozilla.com-select_events2/

Note, the select events are improved (AFAICT) but the screen readers might not make use of them yet.

The real test here will be for Orca. Joanmarie, Willy, can one of you give this a try? You should find you now get selection_changed events on the container.
Comment 42 neil@parkwaycc.co.uk 2009-09-27 16:32:53 PDT
Comment on attachment 401897 [details] [diff] [review]
patch 2

>       if (selType.IsEmpty() || !selType.EqualsLiteral("single")) {
Ironically it's not one of the changed lines that's giving me cause for concern ;-)
A tree is multiselect unless the selType is one of the three strings "single", "cell" or "text". See nsTreeSelection::GetSingle.
Comment 43 David Bolter [:davidb] 2009-09-29 13:00:18 PDT
Thanks Neil, I'll update the patch as soon as I can. Alexander do you need an updated patch before review?
Comment 44 alexander :surkov 2009-09-30 22:54:56 PDT
David, please describe the patch. It sounds it is just events constants rename :) Fresh patch version would be nice. Also I would like to see mochitests for every peace of code you touch ;)
Comment 45 David Bolter [:davidb] 2009-10-01 07:04:05 PDT
(In reply to comment #44)
> David, please describe the patch.

The mochitests in the patch pass and fail without the code changes. With this patch we fire events for more kinds of selections in atk -- this will help Orca in particular with catching changes in tab context. I recall GOK had a lot of trouble with this :)

GetSelectFor which no longer requires STATE_MULTISELECTABLE, will work with single selects. We climb the parent chain looking for the highest ancestor with STATE_SELECTABLE.

I like the constants renaming Aaron did so kept them.

> mochitests for every peace of code you touch

This makes sense for where we don't already have coverage. Do you see parts of the code changes that are not covered by our existing tests + the ones I added?
Comment 46 alexander :surkov 2009-10-01 22:16:09 PDT
Comment on attachment 401897 [details] [diff] [review]
patch 2


> already_AddRefed<nsIAccessible>
>-nsAccUtils::GetMultiSelectFor(nsIDOMNode *aNode)
>+nsAccUtils::GetSelectFor(nsIDOMNode *aNode)
> {
>   if (!aNode)
>     return nsnull;
> 
>   nsCOMPtr<nsIAccessible> accessible;
>   nsAccessNode::GetAccService()->GetAccessibleFor(aNode,
>                                                   getter_AddRefs(accessible));
>   if (!accessible)
>     return nsnull;
> 
>-  PRUint32 state = State(accessible);
>-  if (0 == (state & nsIAccessibleStates::STATE_SELECTABLE))
>-    return nsnull;
>-
>-  while (0 == (state & nsIAccessibleStates::STATE_MULTISELECTABLE)) {
>+  nsCOMPtr<nsIAccessibleDocument> docAcc =
>+    nsAccessNode::GetDocAccessibleFor(aNode);
>+  nsCOMPtr<nsIAccessible> endOfSearch = do_QueryInterface(docAcc);
>+  NS_ENSURE_TRUE(endOfSearch, nsnull);
>+  while (State(accessible) & nsIAccessibleStates::STATE_SELECTABLE) {
>     nsIAccessible *current = accessible;
>     current->GetParent(getter_AddRefs(accessible));
>-    if (!accessible ||
>-        nsAccUtils::Role(accessible) == nsIAccessibleRole::ROLE_PANE) {
>+    if (!accessible || accessible == endOfSearch)
>       return nsnull;
>-    }
>-    state = State(accessible);
>   }

The meantime you look for non selectable parent and assume there is couple of selectable items. I don't get what cases do you want to catch here.

> 
>   nsIAccessible *returnAccessible = nsnull;
>   accessible.swap(returnAccessible);
>   return returnAccessible;


nit: accessible.forget() while you're here.

>   PRUint32 state = nsAccUtils::State(this);
>   if (state & nsIAccessibleStates::STATE_SELECTABLE) {
>-    nsCOMPtr<nsIAccessible> multiSelect =
>-      nsAccUtils::GetMultiSelectFor(mDOMNode);
>-    if (!multiSelect) {
>+    nsCOMPtr<nsIAccessible> container = nsAccUtils::GetSelectFor(mDOMNode);
>+    if (!container || 0 == (nsAccUtils::State(container) & nsIAccessibleStates::STATE_MULTISELECTABLE)) {
>       return aSelect ? TakeFocus() : NS_ERROR_FAILURE;

This bothers me, GetMultiSelectFor != GetSelectFor + state & STATE_MULTISELECTABLE.
Comment 47 alexander :surkov 2009-10-01 22:17:16 PDT
(In reply to comment #45)

> This makes sense for where we don't already have coverage. Do you see parts of
> the code changes that are not covered by our existing tests + the ones I added?

Sure, after C++ code review I think I'll be able to understand any possible gaps :)
Comment 48 David Bolter [:davidb] 2009-10-02 08:22:40 PDT
(In reply to comment #46)
> (From update of attachment 401897 [details] [diff] [review])
> >-  while (0 == (state & nsIAccessibleStates::STATE_MULTISELECTABLE)) {
> >+  nsCOMPtr<nsIAccessibleDocument> docAcc =
> >+    nsAccessNode::GetDocAccessibleFor(aNode);
> >+  nsCOMPtr<nsIAccessible> endOfSearch = do_QueryInterface(docAcc);
> >+  NS_ENSURE_TRUE(endOfSearch, nsnull);
> >+  while (State(accessible) & nsIAccessibleStates::STATE_SELECTABLE) {
> >     nsIAccessible *current = accessible;
> >     current->GetParent(getter_AddRefs(accessible));
> >-    if (!accessible ||
> >-        nsAccUtils::Role(accessible) == nsIAccessibleRole::ROLE_PANE) {
> >+    if (!accessible || accessible == endOfSearch)
> >       return nsnull;
> >-    }
> >-    state = State(accessible);
> >   }
> 
> The meantime you look for non selectable parent and assume there is couple of
> selectable items. I don't get what cases do you want to catch here.

I'm trying to channel Aaron here (like a psychic jedi).

This change is about walking up to the outermost/topmost container of the path of selectables -- in the sense of depth. I think this makes sure we get the correct container of the node in question.

> 
> > 
> >   nsIAccessible *returnAccessible = nsnull;
> >   accessible.swap(returnAccessible);
> >   return returnAccessible;
> 
> 
> nit: accessible.forget() while you're here.

OK sure.

> 
> >   PRUint32 state = nsAccUtils::State(this);
> >   if (state & nsIAccessibleStates::STATE_SELECTABLE) {
> >-    nsCOMPtr<nsIAccessible> multiSelect =
> >-      nsAccUtils::GetMultiSelectFor(mDOMNode);
> >-    if (!multiSelect) {
> >+    nsCOMPtr<nsIAccessible> container = nsAccUtils::GetSelectFor(mDOMNode);
> >+    if (!container || 0 == (nsAccUtils::State(container) & nsIAccessibleStates::STATE_MULTISELECTABLE)) {
> >       return aSelect ? TakeFocus() : NS_ERROR_FAILURE;
> 
> This bothers me, GetMultiSelectFor != GetSelectFor + state &
> STATE_MULTISELECTABLE.

Why not?
Comment 49 alexander :surkov 2009-10-04 23:29:06 PDT
(In reply to comment #48)

> I'm trying to channel Aaron here (like a psychic jedi).

That's great. But you cannot be protected by his force ;)

> This change is about walking up to the outermost/topmost container of the path
> of selectables -- in the sense of depth. I think this makes sure we get the
> correct container of the node in question.

Exactly, but why? Can you think of an example where we can have nested selectable children? Now we look for multiselectable state. Should our new algorithm be based on role?

For example, if we have

select
  option group
    option

Which target should be used for selection event for an option? Should it be an option group or should it be a select? Note, the meantime it is a select (because it is multiselectable). After your patch it will be an option group.

> > This bothers me, GetMultiSelectFor != GetSelectFor + state &
> > STATE_MULTISELECTABLE.
> 
> Why not?

because of example above :)
Comment 50 David Bolter [:davidb] 2009-10-13 11:29:52 PDT
(In reply to comment #49)
> (In reply to comment #48)
> > This change is about walking up to the outermost/topmost container of the path
> > of selectables -- in the sense of depth. I think this makes sure we get the
> > correct container of the node in question.
> 
> Exactly, but why? Can you think of an example where we can have nested
> selectable children? Now we look for multiselectable state. Should our new
> algorithm be based on role?
> 
> For example, if we have
> 
> select
>   option group
>     option
> 
> Which target should be used for selection event for an option? Should it be an
> option group or should it be a select? Note, the meantime it is a select
> (because it is multiselectable). After your patch it will be an option group.

I tested this example: http://www.w3schools.com/tags/tryit.asp?filename=tryhtml_optgroup

The option groups are siblings to the options according to the accessible tree view of accprobe, so we should be okay in this case.
Comment 51 alexander :surkov 2009-10-13 18:07:28 PDT
(In reply to comment #50)

> I tested this example:
> http://www.w3schools.com/tags/tryit.asp?filename=tryhtml_optgroup
> 
> The option groups are siblings to the options according to the accessible tree
> view of accprobe, so we should be okay in this case.

This is interesting they are exposed as siblings. What about ARIA analogue?

It would be nice to get an answer on

> Can you think of an example where we can have nested
> selectable children?
Comment 52 David Bolter [:davidb] 2009-10-30 09:22:21 PDT
(In reply to comment #51)
> (In reply to comment #50)
> 
> > I tested this example:
> > http://www.w3schools.com/tags/tryit.asp?filename=tryhtml_optgroup
> > 
> > The option groups are siblings to the options according to the accessible tree
> > view of accprobe, so we should be okay in this case.
> 
> This is interesting they are exposed as siblings. What about ARIA analogue?

I guess that's up to the web developer :(  unless we flatten it.

> 
> It would be nice to get an answer on
> 
> > Can you think of an example where we can have nested
> > selectable children?

No. This was in Aaron's starter patch. Do you think we need it?
Comment 53 alexander :surkov 2009-11-01 16:54:59 PST
(In reply to comment #52)
> (In reply to comment #51)
> > (In reply to comment #50)
> > 
> > > I tested this example:
> > > http://www.w3schools.com/tags/tryit.asp?filename=tryhtml_optgroup
> > > 
> > > The option groups are siblings to the options according to the accessible tree
> > > view of accprobe, so we should be okay in this case.
> > 
> > This is interesting they are exposed as siblings. What about ARIA analogue?
> 
> I guess that's up to the web developer :(  unless we flatten it.

Ok, we don't flatten it and ARIA spec doesn't allow group usage inside of select, nevertheless it does for ARIA trees.

> > 
> > It would be nice to get an answer on
> > 
> > > Can you think of an example where we can have nested
> > > selectable children?
> 
> No. This was in Aaron's starter patch. Do you think we need it?

I prefer to have the code we understand :) I'm not sure Aaron will recall even if we summon him. So let's keep this in mind and fix it when we'll get an example or any evidence we need this.
Comment 54 David Bolter [:davidb] 2009-11-02 05:33:19 PST
(In reply to comment #53)
> (In reply to comment #52)
> > (In reply to comment #51)
> > > (In reply to comment #50)
> > > It would be nice to get an answer on
> > > 
> > > > Can you think of an example where we can have nested
> > > > selectable children?
> > 
> > No. This was in Aaron's starter patch. Do you think we need it?
> 
> I prefer to have the code we understand :) I'm not sure Aaron will recall even
> if we summon him. So let's keep this in mind and fix it when we'll get an
> example or any evidence we need this.

Sounds reasonable. I'll work up another patch.
Comment 55 David Bolter [:davidb] 2009-11-02 11:20:48 PST
(In reply to comment #46)
> (From update of attachment 401897 [details] [diff] [review])
> >   PRUint32 state = nsAccUtils::State(this);
> >   if (state & nsIAccessibleStates::STATE_SELECTABLE) {
> >-    nsCOMPtr<nsIAccessible> multiSelect =
> >-      nsAccUtils::GetMultiSelectFor(mDOMNode);
> >-    if (!multiSelect) {
> >+    nsCOMPtr<nsIAccessible> container = nsAccUtils::GetSelectFor(mDOMNode);
> >+    if (!container || 0 == (nsAccUtils::State(container) & nsIAccessibleStates::STATE_MULTISELECTABLE)) {
> >       return aSelect ? TakeFocus() : NS_ERROR_FAILURE;
> 
> This bothers me, GetMultiSelectFor != GetSelectFor + state &
> STATE_MULTISELECTABLE.

In our implementation STATE_SELECTABLE always accompanies STATE_MULTISELECTABLE, so GetSelectFor covers the superset. We then check this against STATE_MULTISELECTABLE so it should be equivalent.
Comment 56 alexander :surkov 2009-11-02 18:18:01 PST
(In reply to comment #55)

> > This bothers me, GetMultiSelectFor != GetSelectFor + state &
> > STATE_MULTISELECTABLE.
> 
> In our implementation STATE_SELECTABLE always accompanies
> STATE_MULTISELECTABLE, so GetSelectFor covers the superset. We then check this
> against STATE_MULTISELECTABLE so it should be equivalent.

Technically no, I would guess STATE_MULTISELECTABLE accompanies STATE_SELECTABLE, STATE_SELECTABLE doesn't accompanies STATE_MULTISELECTABLE always, however it could.
Comment 57 alexander :surkov 2009-11-02 18:19:39 PST
(In reply to comment #56)

> Technically no, I would guess STATE_MULTISELECTABLE accompanies
> STATE_SELECTABLE, STATE_SELECTABLE doesn't accompanies STATE_MULTISELECTABLE
> always, however it could.

I mean can we get a case when we have a selectable but multiselectable parent when multiselectable grandparent exists.
Comment 58 David Bolter [:davidb] 2009-11-03 06:16:12 PST
(In reply to comment #56)
> (In reply to comment #55)
> 
> > > This bothers me, GetMultiSelectFor != GetSelectFor + state &
> > > STATE_MULTISELECTABLE.
> > 
> > In our implementation STATE_SELECTABLE always accompanies
> > STATE_MULTISELECTABLE, so GetSelectFor covers the superset. We then check this
> > against STATE_MULTISELECTABLE so it should be equivalent.
> 
> Technically no, I would guess STATE_MULTISELECTABLE accompanies
> STATE_SELECTABLE, STATE_SELECTABLE doesn't accompanies STATE_MULTISELECTABLE
> always, however it could.

Not sure, but we might agree. I mean we never have a STATE_MULTISELECTABLE bit set without a STATE_SELECTABLE bit set also.
Comment 59 David Bolter [:davidb] 2009-11-03 06:17:36 PST
(In reply to comment #57)
> (In reply to comment #56)
> 
> > Technically no, I would guess STATE_MULTISELECTABLE accompanies
> > STATE_SELECTABLE, STATE_SELECTABLE doesn't accompanies STATE_MULTISELECTABLE
> > always, however it could.
> 
> I mean can we get a case when we have a selectable but multiselectable parent
> when multiselectable grandparent exists.

If we do, then I think that is a pickle. I'm not sure how we should handle it.
Comment 60 David Bolter [:davidb] 2009-11-03 06:28:22 PST
OK I finally see what your concern is, and yes I wonder about trees. Let me think about this.
Comment 61 David Bolter [:davidb] 2009-11-03 07:55:13 PST
OK as per our chat, I'm going to make sure we don't break trees in this bug, and have filed a follow up for coverage of other roles: bug 526227

We'll keep Aaron's approach of grabbing the first non selectable parent to use as the container as a fallback.
Comment 62 David Bolter [:davidb] 2009-11-03 11:39:01 PST
> Not sure, but we might agree. I mean we never have a STATE_MULTISELECTABLE bit
> set without a STATE_SELECTABLE bit set also.

Ouch forget I said "never" :)

I'm getting somewhere -- I think most of what you'll need will be comments.
Comment 63 David Bolter [:davidb] 2009-11-03 11:41:14 PST
I was thinking of STATE_EXTSELECTABLE
Comment 64 David Bolter [:davidb] 2009-11-03 12:40:49 PST
Created attachment 410005 [details] [diff] [review]
adds tests and comments
Comment 65 David Bolter [:davidb] 2009-11-03 12:41:32 PST
For the record, I do not want this on 1.9.2 :)
Comment 66 alexander :surkov 2009-11-04 04:12:49 PST
David, unfortunately I haven't look at your patch close enough. But I still don't get why do we need 'while' in GetSelectFor. It won't work for ARIA tree because it will return ARIA group if any; it's not needed for ARIA listbox for example because listitem is direct child of listbox.
Comment 67 David Bolter [:davidb] 2009-11-04 05:43:00 PST
Okay, I'll try reverting to looking for the multiselect; it is too difficult to guess what Aaron was up to :)
Comment 68 David Bolter [:davidb] 2009-11-04 05:43:33 PST
... might be easiest to special case only xul tabs.
Comment 69 alexander :surkov 2010-01-18 05:31:37 PST
Comment on attachment 410005 [details] [diff] [review]
adds tests and comments

cancelling review, waiting for new patch
Comment 70 David Bolter [:davidb] 2010-01-18 06:05:53 PST
Sigh. This patch totally rotted. I'll try to re-capture it later.
Comment 71 alexander :surkov 2011-02-14 20:14:15 PST
Let's get it fixed for Fx5.
Comment 72 David Bolter [:davidb] 2011-02-15 07:17:08 PST
Unassigning myself (for now).
Comment 73 alexander :surkov 2011-10-04 23:35:35 PDT
Created attachment 564758 [details] [diff] [review]
patch

No state change events because we need them on linux only and no easy way to do that. We should deal with Linux part in follow up.

For selection/selection_add/selection_remove/selection_within I follow MSAA.

build will be here: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-46592c42fb43
Comment 74 Marco Zehe (:MarcoZ) on PTO until August 15 2011-10-05 02:58:40 PDT
Comment on attachment 564758 [details] [diff] [review]
patch

First, I notice two instances of:
>+      // muliselectable listbox

Missing t in "multi". ;)

Second, shouldn't you be changing the UUID for accessible/public/nsIAccessibleEvent.idl?

Waiting for try build to finish for Win32 so I can give this a manual test go-round.
Comment 75 alexander :surkov 2011-10-05 03:30:51 PDT
(In reply to Marco Zehe (:MarcoZ) from comment #74)
> Comment on attachment 564758 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> First, I notice two instances of:
> >+      // muliselectable listbox
> 
> Missing t in "multi". ;)

doesn't muli look fun ;)

> Second, shouldn't you be changing the UUID for
> accessible/public/nsIAccessibleEvent.idl?

sure

> Waiting for try build to finish for Win32 so I can give this a manual test
> go-round.

I cancelled that one and failed new one - http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-394ca616e763
Comment 76 Marco Zehe (:MarcoZ) on PTO until August 15 2011-10-05 05:42:04 PDT
Comment on attachment 564758 [details] [diff] [review]
patch

This patch completely breaks comboboxes with NVDA. The selected item is not indicated in virtual buffer, and arrowing through one no longer produces any speech.
Comment 77 David Bolter [:davidb] 2011-10-05 05:50:23 PDT
Comment on attachment 564758 [details] [diff] [review]
patch

I wonder if we really need to coalesce selection changes?
Comment 78 alexander :surkov 2011-10-05 07:13:51 PDT
Created attachment 564836 [details] [diff] [review]
patch2
Comment 79 alexander :surkov 2011-10-05 07:16:48 PDT
(In reply to David Bolter [:davidb] from comment #77)
> Comment on attachment 564758 [details] [diff] [review] [diff] [details] [review]
> patch
> 
> I wonder if we really need to coalesce selection changes?

Would it be useful for AT to receive thousands notifications that the user selected all his emails in inbox? There's always a limit after that AT would likely ignore them.
Comment 80 alexander :surkov 2011-10-05 07:21:35 PDT
try server build for patch2: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-8e2915fbee35
Comment 81 Marco Zehe (:MarcoZ) on PTO until August 15 2011-10-07 02:46:34 PDT
Comment on attachment 564836 [details] [diff] [review]
patch2

This patch works as expected. No breackage of lists, comboboxes, aria-select controls like used in Yahoo! Mail, drop downs like in the search engine selector. Text selection also works as it did before. f=me.
Comment 82 Trevor Saunders (:tbsaunde) 2011-10-12 14:53:22 PDT
Comment on attachment 564836 [details] [diff] [review]
patch2

>diff --git a/accessible/public/nsIAccessibleEvent.idl b/accessible/public/nsIAccessibleEvent.idl
>--- a/accessible/public/nsIAccessibleEvent.idl
>+++ b/accessible/public/nsIAccessibleEvent.idl
>@@ -54,17 +54,17 @@ interface nsIDOMNode;
>  * the event and its target. To listen to in-process accessibility invents,
>  * make your object an nsIObserver, and listen for accessible-event by 
>  * using code something like this:
>  *   nsCOMPtr<nsIObserverService> observerService = 
>  *     do_GetService("@mozilla.org/observer-service;1", &rv);
>  *   if (NS_SUCCEEDED(rv)) 
>  *     rv = observerService->AddObserver(this, "accessible-event", PR_TRUE);
>  */
>-[scriptable, uuid(fd1378c5-c606-4a5e-a321-8e7fc107e5cf)]
>+[scriptable, uuid(7f66a33a-9ed7-4fd4-87a8-e431b0f43368)]
> interface nsIAccessibleEvent : nsISupports
> {
>   /**
>    * An object has been created.
>    */
>   const unsigned long EVENT_SHOW = 0x0001;
> 
>   /**
>@@ -275,17 +275,22 @@ interface nsIAccessibleEvent : nsISuppor
>   const unsigned long EVENT_DOCUMENT_ATTRIBUTES_CHANGED = 0x002A;
> 
>   /**
>    * The contents of the document have changed.
>    */
>   const unsigned long EVENT_DOCUMENT_CONTENT_CHANGED = 0x002B;
> 
>   const unsigned long EVENT_PROPERTY_CHANGED = 0x002C;
>-  const unsigned long EVENT_SELECTION_CHANGED = 0x002D;
>+
>+  /**
>+   * A slide changed in a presentation document or a page boundary was
>+   * crossed in a word processing document.
>+   */
>+  const unsigned long EVENT_PAGE_CHANGED = 0x002D;
> 
>   /**
>    * A text object's attributes changed.
>    * Also see EVENT_OBJECT_ATTRIBUTE_CHANGED.
>    */
>   const unsigned long EVENT_TEXT_ATTRIBUTE_CHANGED = 0x002E;
> 
>   /**
>@@ -432,25 +437,19 @@ interface nsIAccessibleEvent : nsISuppor
>   const unsigned long EVENT_HYPERTEXT_NLINKS_CHANGED = 0x0054;
> 
>   /**
>    * An object's attributes changed. Also see EVENT_TEXT_ATTRIBUTE_CHANGED.
>    */
>   const unsigned long EVENT_OBJECT_ATTRIBUTE_CHANGED = 0x0055;
> 
>   /**
>-   * A slide changed in a presentation document or a page boundary was
>-   * crossed in a word processing document.
>-   */
>-  const unsigned long EVENT_PAGE_CHANGED = 0x0056;
>-
>-  /**
>    * Help make sure event map does not get out-of-line.
>    */
>-  const unsigned long EVENT_LAST_ENTRY = 0x0057;
>+  const unsigned long EVENT_LAST_ENTRY = 0x0056;
> 
>   /**
>    * The type of event, based on the enumerated event values
>    * defined in this interface.
>    */
>   readonly attribute unsigned long eventType;
>   
>   /**
>diff --git a/accessible/src/atk/nsAccessibleWrap.cpp b/accessible/src/atk/nsAccessibleWrap.cpp
>--- a/accessible/src/atk/nsAccessibleWrap.cpp
>+++ b/accessible/src/atk/nsAccessibleWrap.cpp
>@@ -1081,20 +1081,34 @@ nsAccessibleWrap::FirePlatformEvent(AccE
>         nsCOMPtr<nsIAccessibleValue> value(do_QueryObject(accessible));
>         if (value) {    // Make sure this is a numeric value
>             // Don't fire for MSAA string value changes (e.g. text editing)
>             // ATK values are always numeric
>             g_object_notify( (GObject*)atkObj, "accessible-value" );
>         }
>       } break;
> 
>-    case nsIAccessibleEvent::EVENT_SELECTION_CHANGED:
>-        MAI_LOG_DEBUG(("\n\nReceived: EVENT_SELECTION_CHANGED\n"));
>-        g_signal_emit_by_name(atkObj, "selection_changed");
>-        break;
>+    case nsIAccessibleEvent::EVENT_SELECTION:
>+    case nsIAccessibleEvent::EVENT_SELECTION_ADD:
>+    case nsIAccessibleEvent::EVENT_SELECTION_REMOVE:
>+    {

>+      MAI_LOG_DEBUG(("\n\nReceived: EVENT_SELECTION_CHANGED\n"));

nit, I've always found starting these messages with \n\n to be really odd, I think the normal thing everyone else does is to make sure messages leave the terminal in a reasonable state, but since its the pattern i don't really care

>+  if (aSelChangeType == eSelectionAdd) {
>+    if (mWidget->GetSelectedItem(1))
>+      mEventType = nsIAccessibleEvent::EVENT_SELECTION_ADD;
>+    else
>+      mEventType = nsIAccessibleEvent::EVENT_SELECTION;
>+  } else {
>+    mEventType = nsIAccessibleEvent::EVENT_SELECTION_REMOVE;
>+  }
>+}

if (aSelType == blah && mWidget->GetSelectedItem(1))
  statement;
else
  blah

I could live with ? there too, but the condition is a little long.

>-class AccSelectionChangeEvent : public AccEvent
>+class AccSelChangeEvent : public AccEvent

btw do we want to keep Acc in the names for these event classes? it seems like it might be nice to get rid of it, but whatever

>+  AccSelChangeEvent(nsAccessible* aWidget, nsAccessible* aItem,
>+                    SelChangeType aSelChangeType);

you don't inline it because you'd need to include more random headers?

>+  virtual unsigned int GetEventGroups() const
>+  {
>+    return AccEvent::GetEventGroups() | (1U << eSelectionChangeEvent);
>+  }

I thought you were against inline vfuncs?  I don't see any reason for it to be inline, but on the other hand its a little silly not to.

>+  nsRefPtr<nsAccessible> mItem;

is this ever different from  mAccessible in AccEvent?

>+        if (thisEvent->mEventRule == tailEvent->mEventRule) {
>+          AccSelChangeEvent* thisSelChangeEvent =
>+            downcast_accEvent(thisEvent);

do you not just immediately try the downcast to save a virtual call in the case its not a selection event?

>+    } break; // eCoalesceSelectionChange

nit, I find this style weird, but shrug


>+NotificationController::CoalesceSelChangeEvents(AccSelChangeEvent* aTailEvent,
>+                                                AccSelChangeEvent* aThisEvent,
>+                                                PRInt32 aThisIndex)

isn'tthe index and the event pointer redundant?  (I'm not sure which is faster to pass the event or to get it again)

>+  if (aTailEvent->mItem == aThisEvent->mItem) {
>+    aThisEvent->mEventRule = AccEvent::eDoNotEmit;
>+    return;
>+  }

for this to be the case we have seen the thing get selected and then unselected.  It would seem to be the right behavior is either fire no event or fire the select and the unselect in whichever order.

>+    if (aThisEvent->mEventType != nsIAccessibleEvent::EVENT_SELECTION_WITHIN) {

if (==)
  return;

>+      for (PRInt32 jdx = aThisIndex - 1; jdx >= 0; jdx--) {

minor nit, I think people usually  go the other way when they don't have a particular reason

>+        AccEvent* prevEvent = mEvents[jdx];
>+        if (prevEvent->mEventRule == aTailEvent->mEventRule) {

aTailEvent->mEventRule is a constant we know right? I think it would be good to just put it there so  we don't need to keep aTailEvent in cache

>+  // Unpack the packed selection change event because we've got one
>+  // more selection add/remove.

why do you want to do this instead of coalesce all three events into one?

>   /**
>-   * Do not emit one of two given reorder events fired for DOM nodes in the case
>-   * when one DOM node is in parent chain of second one.
>+   * Coalesce two selection change events within the same select control.
>    */
>-  void CoalesceReorderEventsFromSameTree(AccEvent* aAccEvent,
>-                                         AccEvent* aDescendantAccEvent);
>+  void CoalesceSelChangeEvents(AccSelChangeEvent* aTailEvent,
>+                               AccSelChangeEvent* aThisEvent,
>+                               PRInt32 aThisIndex);]

what am I missing here? I don't think I saw you remove a method anywhere?

>-  if (aAttribute == nsGkAtoms::selected ||
>+  // ARIA or XUL selection
>+  if (aContent->IsXUL() && aAttribute == nsGkAtoms::selected ||
>       aAttribute == nsGkAtoms::aria_selected) {

add parens please, I think gcc may warn about this, and I certainly don't want to remember presidence for || and &&

>+    gA11yEventDumpToConsole = true; // debuggin

comment before landing ;)

>+    <option id="cb1_item1" value="mushrooms">mushrooms

yuk ;)

>+    <option id="cb1_item5" value="olives">olives

nasty ;-)

>   <div id="testContainer">
>     <iframe id="iframe"></iframe>
>   </div>

is this still used for something?

>+    gA11yEventDumpToConsole = true; // debuggin

comment


would it be possible to to test the case of election changing twice before  before the first selection event is fired, so we deal with the case of unpacking?

also can we test coalescing selection events in html? I don't see why it should be different from xu, but its funcallity we should be sure works.
Comment 83 alexander :surkov 2011-10-12 19:57:36 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #82)

> >+      MAI_LOG_DEBUG(("\n\nReceived: EVENT_SELECTION_CHANGED\n"));
> 
> nit, I've always found starting these messages with \n\n to be really odd, I
> think the normal thing everyone else does is to make sure messages leave the
> terminal in a reasonable state, but since its the pattern i don't really care

that doesn't happen on practice, having extra \n is preferable over not having them at all

> >+  if (aSelChangeType == eSelectionAdd) {
> >+    if (mWidget->GetSelectedItem(1))
> >+      mEventType = nsIAccessibleEvent::EVENT_SELECTION_ADD;
> >+    else
> >+      mEventType = nsIAccessibleEvent::EVENT_SELECTION;
> >+  } else {
> >+    mEventType = nsIAccessibleEvent::EVENT_SELECTION_REMOVE;
> >+  }
> >+}
> 
> if (aSelType == blah && mWidget->GetSelectedItem(1))
>   statement;
> else
>   blah
> 
> I could live with ? there too, but the condition is a little long.

that blah is supposed to be if/else.

> >-class AccSelectionChangeEvent : public AccEvent
> >+class AccSelChangeEvent : public AccEvent
> 
> btw do we want to keep Acc in the names for these event classes? it seems
> like it might be nice to get rid of it, but whatever

yes, I thought of this but didn't get sure. I think consistent obsolete style should be preferable over than in-the-middle-of-the-way style.

> >+  AccSelChangeEvent(nsAccessible* aWidget, nsAccessible* aItem,
> >+                    SelChangeType aSelChangeType);
> 
> you don't inline it because you'd need to include more random headers?

no, it looks a little bit big, do you think to take a perf benefits from inline?

> >+  virtual unsigned int GetEventGroups() const
> >+  {
> >+    return AccEvent::GetEventGroups() | (1U << eSelectionChangeEvent);
> >+  }
> 
> I thought you were against inline vfuncs?  I don't see any reason for it to
> be inline, but on the other hand its a little silly not to.

yes, here's an exception, all event classes are kept in one file and this method for every class is defined inside a class definition.

> >+  nsRefPtr<nsAccessible> mItem;
> 
> is this ever different from  mAccessible in AccEvent?

mAccessible can be either mItem or mWidget

> >+        if (thisEvent->mEventRule == tailEvent->mEventRule) {
> >+          AccSelChangeEvent* thisSelChangeEvent =
> >+            downcast_accEvent(thisEvent);
> 
> do you not just immediately try the downcast to save a virtual call in the
> case its not a selection event?

partially, also it ignores coalesced events of this type

> >+    } break; // eCoalesceSelectionChange
> 
> nit, I find this style weird, but shrug

just follow the style of this file

> >+NotificationController::CoalesceSelChangeEvents(AccSelChangeEvent* aTailEvent,
> >+                                                AccSelChangeEvent* aThisEvent,
> >+                                                PRInt32 aThisIndex)
> 
> isn'tthe index and the event pointer redundant?  (I'm not sure which is
> faster to pass the event or to get it again)

yes, they are, index is enough. I'm not sure either. Which way do you prefer?


> >+  if (aTailEvent->mItem == aThisEvent->mItem) {
> >+    aThisEvent->mEventRule = AccEvent::eDoNotEmit;
> >+    return;
> >+  }
> 
> for this to be the case we have seen the thing get selected and then
> unselected.  It would seem to be the right behavior is either fire no event
> or fire the select and the unselect in whichever order.

makes sense, I'll take a look

> 
> >+    if (aThisEvent->mEventType != nsIAccessibleEvent::EVENT_SELECTION_WITHIN) {
> 
> if (==)
>   return;

Why? It it's selection_within event then it means all previous events were processed. If it's not then we should process them.

> >+      for (PRInt32 jdx = aThisIndex - 1; jdx >= 0; jdx--) {
> 
> minor nit, I think people usually  go the other way when they don't have a
> particular reason

just logical order: we were on 'this' so we go starting from it to first :) I don't mind to have either way though.

> >+        AccEvent* prevEvent = mEvents[jdx];
> >+        if (prevEvent->mEventRule == aTailEvent->mEventRule) {
> 
> aTailEvent->mEventRule is a constant we know right? I think it would be good
> to just put it there so  we don't need to keep aTailEvent in cache

within this 'if' block yes but within method it might be not. But I didn't catch about keeping aTailEvent in cache

> >+  // Unpack the packed selection change event because we've got one
> >+  // more selection add/remove.
> 
> why do you want to do this instead of coalesce all three events into one?

not sure I follow you, can you elaborate please?

> >   /**
> >-   * Do not emit one of two given reorder events fired for DOM nodes in the case
> >-   * when one DOM node is in parent chain of second one.
> >+   * Coalesce two selection change events within the same select control.
> >    */
> >-  void CoalesceReorderEventsFromSameTree(AccEvent* aAccEvent,
> >-                                         AccEvent* aDescendantAccEvent);
> >+  void CoalesceSelChangeEvents(AccSelChangeEvent* aTailEvent,
> >+                               AccSelChangeEvent* aThisEvent,
> >+                               PRInt32 aThisIndex);]
> 
> what am I missing here? I don't think I saw you remove a method anywhere?

a sign of previous life :)

> >-  if (aAttribute == nsGkAtoms::selected ||
> >+  // ARIA or XUL selection
> >+  if (aContent->IsXUL() && aAttribute == nsGkAtoms::selected ||
> >       aAttribute == nsGkAtoms::aria_selected) {
> 
> add parens please, I think gcc may warn about this, and I certainly don't
> want to remember presidence for || and &&

hm, ok

> would it be possible to to test the case of election changing twice before 
> before the first selection event is fired, so we deal with the case of
> unpacking?

I'll add more

> also can we test coalescing selection events in html? I don't see why it
> should be different from xu, but its funcallity we should be sure works.

doesn't make huge sense, otherwise we should add it for every possible control.
Comment 84 alexander :surkov 2011-10-12 22:46:05 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #82)

> >+  if (aTailEvent->mItem == aThisEvent->mItem) {
> >+    aThisEvent->mEventRule = AccEvent::eDoNotEmit;
> >+    return;
> >+  }
> 
> for this to be the case we have seen the thing get selected and then
> unselected.  It would seem to be the right behavior is either fire no event
> or fire the select and the unselect in whichever order.

ok, I think to follow the second option, it's easier and it is an edge case actually. I'll add a test for item selecting and then unselecting.

> would it be possible to to test the case of election changing twice before 
> before the first selection event is fired, so we deal with the case of
> unpacking?

case of unpacking happens select4FirstItems test because the first event is of selection type.
Comment 85 alexander :surkov 2011-10-12 23:21:33 PDT
Created attachment 566748 [details] [diff] [review]
patch
Comment 86 Trevor Saunders (:tbsaunde) 2011-10-25 06:40:07 PDT
(In reply to alexander surkov from comment #83)
> (In reply to Trevor Saunders (:tbsaunde) from comment #82)
> 
> > >+      MAI_LOG_DEBUG(("\n\nReceived: EVENT_SELECTION_CHANGED\n"));
> > 
> > nit, I've always found starting these messages with \n\n to be really odd, I
> > think the normal thing everyone else does is to make sure messages leave the
> > terminal in a reasonable state, but since its the pattern i don't really care
> 
> that doesn't happen on practice, having extra \n is preferable over not
> having them at all

I guess, if we find places that don't end with \n we should fix them though

> 
> > >+  if (aSelChangeType == eSelectionAdd) {
> > >+    if (mWidget->GetSelectedItem(1))
> > >+      mEventType = nsIAccessibleEvent::EVENT_SELECTION_ADD;
> > >+    else
> > >+      mEventType = nsIAccessibleEvent::EVENT_SELECTION;
> > >+  } else {
> > >+    mEventType = nsIAccessibleEvent::EVENT_SELECTION_REMOVE;
> > >+  }
> > >+}
> > 
> > if (aSelType == blah && mWidget->GetSelectedItem(1))
> >   statement;
> > else
> >   blah
> > 
> > I could live with ? there too, but the condition is a little long.
> 
> that blah is supposed to be if/else.

well, what to do in the else case.  The point was you don't need the nested if.

> 
> > >-class AccSelectionChangeEvent : public AccEvent
> > >+class AccSelChangeEvent : public AccEvent
> > 
> > btw do we want to keep Acc in the names for these event classes? it seems
> > like it might be nice to get rid of it, but whatever
> 
> yes, I thought of this but didn't get sure. I think consistent obsolete
> style should be preferable over than in-the-middle-of-the-way style.

ok

> > >+  AccSelChangeEvent(nsAccessible* aWidget, nsAccessible* aItem,
> > >+                    SelChangeType aSelChangeType);
> > 
> > you don't inline it because you'd need to include more random headers?
> 
> no, it looks a little bit big, do you think to take a perf benefits from
> inline?

I'm not really sure, but in general I like to give the compiler as much information as I can.

> > >+NotificationController::CoalesceSelChangeEvents(AccSelChangeEvent* aTailEvent,
> > >+                                                AccSelChangeEvent* aThisEvent,
> > >+                                                PRInt32 aThisIndex)
> > 
> > isn'tthe index and the event pointer redundant?  (I'm not sure which is
> > faster to pass the event or to get it again)
> 
> yes, they are, index is enough. I'm not sure either. Which way do you prefer?

I'm not sure do as you like

> 
> > >+        AccEvent* prevEvent = mEvents[jdx];
> > >+        if (prevEvent->mEventRule == aTailEvent->mEventRule) {
> > 
> > aTailEvent->mEventRule is a constant we know right? I think it would be good
> > to just put it there so  we don't need to keep aTailEvent in cache
> 
> within this 'if' block yes but within method it might be not. But I didn't
> catch about keeping aTailEvent in cache

I'm pretty sure this doesn't actually mater unless we get super unlucky.  However what I mean is consider the case that at some point the memory  where tail event is is flushed from the cache, then you'll have to fetch it from memory.  (this is super micro optamizing ; and best ignored ;)
Comment 87 alexander :surkov 2011-10-27 07:13:48 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #86)
> > that doesn't happen on practice, having extra \n is preferable over not
> > having them at all
> 
> I guess, if we find places that don't end with \n we should fix them though

agree, but that's something outside teh a11y.

> > 
> > > >+  if (aSelChangeType == eSelectionAdd) {
> > > >+    if (mWidget->GetSelectedItem(1))
> > > >+      mEventType = nsIAccessibleEvent::EVENT_SELECTION_ADD;
> > > >+    else
> > > >+      mEventType = nsIAccessibleEvent::EVENT_SELECTION;
> > > >+  } else {
> > > >+    mEventType = nsIAccessibleEvent::EVENT_SELECTION_REMOVE;
> > > >+  }
> > > >+}
> > > 
> > > if (aSelType == blah && mWidget->GetSelectedItem(1))
> > >   statement;
> > > else
> > >   blah
> > > 
> > > I could live with ? there too, but the condition is a little long.
> > 
> > that blah is supposed to be if/else.
> 
> well, what to do in the else case.  The point was you don't need the nested
> if.

maybe code snippet because I afraid I don't follow you.
Comment 88 alexander :surkov 2011-11-02 14:46:49 PDT
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/285d90ff1d03
Comment 89 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-11-03 08:49:41 PDT
https://hg.mozilla.org/mozilla-central/rev/285d90ff1d03

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