Incorrect selection events in HTML, XUL and ARIA (originally filed because of XUL tabs)

RESOLVED FIXED in mozilla10

Status

()

Core
Disability Access APIs
P2
normal
RESOLVED FIXED
9 years ago
5 years ago

People

(Reporter: eeejay, Assigned: surkov)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla10
Points:
---
Dependency tree / graph
Bug Flags:
wanted-next +
blocking1.9.2 -
blocking1.9 -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 11 obsolete attachments)

(Reporter)

Description

9 years ago
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

9 years ago
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?
Summary: No AT-SPI signal emitted when switching tabs → No selection change event when switching tabs
(Reporter)

Comment 2

9 years ago
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

9 years ago
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.
Assignee: nobody → aaronleventhal
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true

Comment 4

9 years ago
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

9 years ago
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.

Updated

9 years ago
Blocks: 414721
Status: ASSIGNED → NEW
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
QA Contact: disability.access → accessibility-apis

Comment 6

9 years ago
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)
Attachment #300812 - Attachment is obsolete: true

Comment 7

9 years ago
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.

Updated

9 years ago
Attachment #300927 - Attachment is obsolete: true
(Reporter)

Comment 8

9 years ago
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.
(Assignee)

Updated

9 years ago
Attachment #300934 - Attachment is patch: true
Attachment #300934 - Attachment mime type: application/octet-stream → text/plain

Comment 9

9 years ago
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.
Flags: blocking1.9?
Priority: -- → P1
Target Milestone: --- → mozilla1.9

Updated

9 years ago
Flags: tracking1.9? → blocking1.9?
Flags: blocking1.9? → blocking1.9+

Comment 10

9 years ago
Marco/Aaron we making progress on this - Firefox 3 final ship is fast approaching...

Comment 11

9 years ago
I'm coming back to this soon after dealing with a couple of other issues.

Comment 12

9 years ago
We also need to check on correct selection and focus events in an HTML multiselect listbox.
Progress here?
Target Milestone: mozilla1.9 → mozilla1.9beta5

Comment 14

9 years ago
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

9 years ago
IF that's the case than this will have to miss b5.  If same enough we can take in RC1.
Priority: P1 → P2

Updated

9 years ago
Target Milestone: mozilla1.9beta5 → mozilla1.9

Comment 16

9 years ago
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.   
Flags: wanted-next+
Flags: blocking1.9-
Flags: blocking1.9+

Comment 17

9 years ago
Affects ARIA as well.
Blocks: 343213

Updated

9 years ago
Summary: No selection change event when switching tabs → Incorrect selection events in HTML, XUL and ARIA (originally filed because of XUL tabs)

Updated

9 years ago
Flags: blocking1.9.2?
Mass un-assigning bugs assigned to Aaron.
Assignee: aaronleventhal → nobody
I can't recreate this. Closing as WORKSFORME - please reopen if necessary.
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → WORKSFORME
(Assignee)

Comment 20

8 years ago
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.
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)
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
Will, Joanmarie, do you guys have a workaround in the meantime?
Created attachment 400831 [details] [diff] [review]
updated aaron WIP
Assignee: nobody → bolterbugz
Attachment #300934 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
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.
Attachment #400831 - Attachment is obsolete: true
Created attachment 401271 [details] [diff] [review]
wip2 (with test included)
Attachment #401270 - Attachment is obsolete: true
Tryserver build: https://build.mozilla.org/tryserver-builds/dbolter@mozilla.com-sanify-select-events/
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.
Attachment #401271 - Attachment is obsolete: true
Attachment #401852 - Flags: review?(marco.zehe)
(Note I've changed the uuid on nsIAccessibleEvent locally)
Is the try server build the one that contains this latest patch, or will you kick off a newer one?
Tryserver build is up to date for the things that matter. (New patch adds mochitests and removes some debug trace output).
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.
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.
Version: unspecified → Trunk
(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.
(Note: fixed locally the overriding of EVENT_SELECTION_REMOVE_IN_CONTAINER)
Actually, unsure... we might be able to fix bug 414721 here. Marco did you test with NVDA?
Created attachment 401897 [details] [diff] [review]
patch 2
Attachment #401852 - Attachment is obsolete: true
Attachment #401852 - Flags: review?(marco.zehe)
(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.
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.
OS: Linux → All
Hardware: x86 → All
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.
Attachment #401897 - Flags: review+

Updated

8 years ago
Attachment #401897 - Flags: review?(neil)
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.
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.
Flags: blocking1.9.2? → blocking1.9.2-

Updated

8 years ago
Attachment #401897 - Flags: review?(surkov.alexander)

Updated

8 years ago
Attachment #401897 - Flags: review?(neil) → review+
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.
Thanks Neil, I'll update the patch as soon as I can. Alexander do you need an updated patch before review?
(Assignee)

Comment 44

8 years ago
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 ;)
(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?
(Assignee)

Comment 46

8 years ago
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.
(Assignee)

Comment 47

8 years ago
(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 :)
(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?
(Assignee)

Comment 49

8 years ago
(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 :)
(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.
(Assignee)

Comment 51

8 years ago
(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?
(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?
(Assignee)

Comment 53

8 years ago
(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.
(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.
(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.
(Assignee)

Comment 56

8 years ago
(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.
(Assignee)

Comment 57

8 years ago
(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.
(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.
(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.
OK I finally see what your concern is, and yes I wonder about trees. Let me think about this.

Updated

8 years ago
Blocks: 526227
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.
> 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.
I was thinking of STATE_EXTSELECTABLE
Created attachment 410005 [details] [diff] [review]
adds tests and comments
Attachment #401897 - Attachment is obsolete: true
Attachment #410005 - Flags: review?(surkov.alexander)
Attachment #401897 - Flags: review?(surkov.alexander)
For the record, I do not want this on 1.9.2 :)
(Assignee)

Comment 66

8 years ago
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.
Okay, I'll try reverting to looking for the multiselect; it is too difficult to guess what Aaron was up to :)
... might be easiest to special case only xul tabs.
(Assignee)

Comment 69

7 years ago
Comment on attachment 410005 [details] [diff] [review]
adds tests and comments

cancelling review, waiting for new patch
Attachment #410005 - Flags: review?(surkov.alexander)
Sigh. This patch totally rotted. I'll try to re-capture it later.
(Assignee)

Comment 71

6 years ago
Let's get it fixed for Fx5.
Blocks: 632301
Unassigning myself (for now).
Assignee: bolterbugz → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 73

6 years ago
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
Assignee: nobody → surkov.alexander
Attachment #410005 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #564758 - Flags: review?(trev.saunders)
Attachment #564758 - Flags: feedback?(marco.zehe)
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.
(Assignee)

Comment 75

6 years ago
(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 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.
Attachment #564758 - Flags: feedback?(marco.zehe) → feedback-
Comment on attachment 564758 [details] [diff] [review]
patch

I wonder if we really need to coalesce selection changes?
(Assignee)

Comment 78

6 years ago
Created attachment 564836 [details] [diff] [review]
patch2
Attachment #564758 - Attachment is obsolete: true
Attachment #564836 - Flags: review?(trev.saunders)
Attachment #564836 - Flags: feedback?(marco.zehe)
Attachment #564758 - Flags: review?(trev.saunders)
(Assignee)

Comment 79

6 years ago
(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.
(Assignee)

Comment 80

6 years ago
try server build for patch2: http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-8e2915fbee35
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.
Attachment #564836 - Flags: feedback?(marco.zehe) → feedback+
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.
(Assignee)

Comment 83

6 years ago
(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.
(Assignee)

Comment 84

6 years ago
(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.
(Assignee)

Comment 85

6 years ago
Created attachment 566748 [details] [diff] [review]
patch
Attachment #564836 - Attachment is obsolete: true
Attachment #566748 - Flags: review?(trev.saunders)
Attachment #564836 - Flags: review?(trev.saunders)
(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 ;)
Attachment #566748 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 87

6 years ago
(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.
(Assignee)

Comment 88

6 years ago
inbound land https://hg.mozilla.org/integration/mozilla-inbound/rev/285d90ff1d03
https://hg.mozilla.org/mozilla-central/rev/285d90ff1d03
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9 → mozilla10
(Assignee)

Updated

6 years ago
Depends on: 701669

Updated

5 years ago
Blocks: 754377

Updated

5 years ago
No longer blocks: 754377
Depends on: 754377
You need to log in before you can comment on or make changes to this bug.