Closed Bug 414302 Opened 18 years ago Closed 14 years ago

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

Categories

(Core :: Disability Access APIs, defect, P2)

Product:
Core
Component:
Disability Access APIs
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla10

People

(Reporter: eeejay, Assigned: surkov)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 11 obsolete files)

patch
51.04 KB, patch
tbsaunde
: review+
Details | Diff | Splinter Review
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.
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
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.
Attached patch WIP. Needs testing. (obsolete) — — Splinter Review
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
It works with XUL tabs at least. Could use more testing with various XUL, HTML and ARIA widgets to ensure the events are right.
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.
Blocks: 414721
Status: ASSIGNED → NEW
Component: Disability Access → Disability Access APIs
Product: Firefox → Core
QA Contact: disability.access → accessibility-apis
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
Attached patch Still WIP (obsolete) — — Splinter Review
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.
Attachment #300927 - Attachment is obsolete: true
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.
Attachment #300934 - Attachment is patch: true
Attachment #300934 - Attachment mime type: application/octet-stream → text/plain
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
Flags: tracking1.9? → blocking1.9?
Flags: blocking1.9? → blocking1.9+
Marco/Aaron we making progress on this - Firefox 3 final ship is fast approaching...
I'm coming back to this soon after dealing with a couple of other issues.
We also need to check on correct selection and focus events in an HTML multiselect listbox.
Progress here?
Target Milestone: mozilla1.9 → mozilla1.9beta5
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.
IF that's the case than this will have to miss b5. If same enough we can take in RC1.
Priority: P1 → P2
Target Milestone: mozilla1.9beta5 → mozilla1.9
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+
Affects ARIA as well.
Blocks: aria
Summary: No selection change event when switching tabs → Incorrect selection events in HTML, XUL and ARIA (originally filed because of XUL tabs)
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
Closed: 16 years ago
Resolution: --- → WORKSFORME
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?
Attached patch updated aaron WIP (obsolete) — — Splinter Review
Assignee: nobody → bolterbugz
Attachment #300934 - Attachment is obsolete: true
Status: REOPENED → ASSIGNED
Attached patch wip2 (obsolete) — — Splinter Review
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
Attached patch wip2 (with test included) (obsolete) — — Splinter Review
Attachment #401270 - Attachment is obsolete: true
Attached patch patch 1 (obsolete) — — Splinter Review
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?
Attached patch patch 2 (obsolete) — — Splinter Review
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+
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-
Attachment #401897 - Flags: review?(surkov.alexander)
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?
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?
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.
(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?
(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.
(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?
(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.
(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.
(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.
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
Attached patch adds tests and comments (obsolete) — — Splinter Review
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 :)
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.
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.
Let's get it fixed for Fx5.
Blocks: a11ynext
Unassigning myself (for now).
Assignee: bolterbugz → nobody
Status: ASSIGNED → NEW
Attached patch patch (obsolete) — — Splinter Review
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.
(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?
Attached patch patch2 (obsolete) — — Splinter Review
Attachment #564758 - Attachment is obsolete: true
Attachment #564836 - Flags: review?(trev.saunders)
Attachment #564836 - Flags: feedback?(marco.zehe)
Attachment #564758 - Flags: review?(trev.saunders)
(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 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.
(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.
(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.
Attached patch patch — — Splinter Review
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+
(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.
https://hg.mozilla.org/mozilla-central/rev/285d90ff1d03
Status: ASSIGNED → RESOLVED
Closed: 16 years ago → 14 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9 → mozilla10
Depends on: 701669
No longer blocks: 754377
Depends on: 754377
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: