Closed Bug 246063 Opened 21 years ago Closed 21 years ago

Fire accessible state change events when tree item expands/collapses

Categories

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

Product:
Core
Component:
Disability Access APIs
Type:
defect

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

Details

(Keywords: access)

Attachments

(1 file, 5 obsolete files)

Forgot to save file before creating last patch
13.36 KB, patch
neil
: review+
jst
: superreview+
Details | Diff | Splinter Review
When a tree item changes from expanded to collapsed, or vice versa, the state changes. A state change event should be fired to indicate the occurance. This will alow assistive technologies such as screen readers to report the change.
Jan, Neil, anyone any advice on this one? ToggleOpenState() is implemented by many classes and called in many places. Not fun. http://lxr.mozilla.org/seamonkey/search?string=toggleopenstate Not sure where the best place to hook in is.
Depends on: 201922
OK, here's an example. In mail, if you collapse your email account in the folder pane then click on the Read Mail link in Account Central it expands the account so that it can select the Inbox. Would you want a notification for that too?
Neil, ideally we would -- because that's just "right" to expose everything. From a practical point of view it really only matters for the currently focused tree view item. I suppose we could narrow it down to explicity user action (mouse or keyboard) on the currently focused item. Is your recommendation to do that and put the code in tree.xml?
Comment on attachment 150448 [details] [diff] [review] 1) Consolidate item toggling in tree.xml, 2) Fire "OpenStateChange" event when current item toggled, 3) Listen for "OpenStateChange" event in nsRootAccessible.cpp Bah, forgot to ask Neil for r=
Attachment #150448 - Flags: review?(neil.parkwaycc.co.uk)
Priority: -- → P1
Comment on attachment 150448 [details] [diff] [review] 1) Consolidate item toggling in tree.xml, 2) Fire "OpenStateChange" event when current item toggled, 3) Listen for "OpenStateChange" event in nsRootAccessible.cpp >- else if (eventType.EqualsIgnoreCase("CheckboxStateChange")) { >+ else if (eventType.EqualsIgnoreCase("CheckboxStateChange") || >+ eventType.EqualsIgnoreCase("OpenStateChange")) { I don't claim to understand this code, but it strikes me as odd that you check the event type here and again later on... >+ else if (eventType.EqualsIgnoreCase("OpenStateChange")) { // collapsed/expanded changed >+ <method name="setOpenState"> >+ <parameter name="line"/> I think the technical term is "row" :-) Please change. >+ <!-- openState == -1 to toggle, otherwise true or false to set --> In fact, anything except 0 or 1 should do, see below. >+ <parameter name="openState"/> >+ <body><![CDATA[ >+ if (this.currentIndex == -1 || >+ !this.treeBoxObject.view.isContainer(line)) { >+ return false; >+ } I don't think that the test for currentIndex is correct here. Did you mean to test line? >+ if (openState != -1 || >+ this.treeBoxObject.view.isContainerOpen(line) != openState) { This part of the comparison will always fail when openState is anything other than true, false, 1 or 0, so you don't actually need to check for -1 (or whatever value you might prefer). >- <handler event="blur" action="this.treeBoxObject.focused = false;"/> >+ <handler event="blur" action="this.treeBoxObject.focused = false;"/> Whoops, extra spaces :-( >+ <handler event="keypress" keycode="vk_right" action="setOpenState(this.currentIndex, true);"/> Ooh, I forgot to fix this one. (Ideally for an open container it would move to the first child.) >- v.toggleOpenState(row); >+ this.parentNode.setOpenState(row, -1); // -1 == toggle Nit: The variable v is now only used in one other line, so it could be eliminated via inlining.
Attachment #150448 - Flags: review?(neil.parkwaycc.co.uk) → review-
Neil, I fixed everything except for the duplicate "OpenStateChange" check. This is necessary because of the #ifndef MOZ_ACCESSIBILITY_ATK // Code for MSAA events #else // Code for ATK events #endif At some point we may move that code into files built specifically for each platform, but for now we use an #ifndef/#else/#endif.
Attachment #150448 - Attachment is obsolete: true
Attachment #150627 - Flags: review?
Attachment #150627 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #150627 - Flags: review?
(In reply to comment #7) >This is necessary because of the >#ifndef MOZ_ACCESSIBILITY_ATK >// Code for MSAA events >#else >// Code for ATK events >#endif Ah, I was sure there would be a good reason.
Comment on attachment 150627 [details] [diff] [review] New patch based on Neil's comments >+ <method name="setOpenState"> I think I'd prefer the name changeOpenState - assuming that you can successfully use the shortcut of changeOpenState(row) to toggle a row. If that doesn't work then leave it as setOpenState. >+ <parameter name="row"/> >+ <!-- openState == true or false to set, anything else to toggle --> >+ <parameter name="openState"/> >+ <body><![CDATA[ >+ if (this.currentIndex == -1 || I still think that this test is wrong. >+ <handler event="keypress" keycode="vk_enter" action="setOpenState(this.currentIndex, -1);"/> >+ <handler event="keypress" keycode="vk_return" action="setOpenState(this.currentIndex, -1);"/> XBL style is to use this. document. or window. for clarity, as the scope chain is subject to change. >+ // If already opened, select the first child >+ // Don't need to check row+1 < this.treeBoxObject.view.rowCount >+ // because we're on an open container item, the children come next It occurs to me that some trees have lazy expansion so when you press right you'd have to be a lot more careful about selecting the next row. I'm sorry I mentioned this, please use the previous version.
> >+ <parameter name="row"/> > >+ <!-- openState == true or false to set, anything else to toggle --> > >+ <parameter name="openState"/> > >+ <body><![CDATA[ > >+ if (this.currentIndex == -1 || > I still think that this test is wrong. Not sure what you mean. The code I'm patching checked to see if currentIndex was -1 in case nothing is selected. I'm just moving that code here. What do you mean it's wrong? > > >+ // If already opened, select the first child > >+ // Don't need to check row+1 < this.treeBoxObject.view.rowCount > >+ // because we're on an open container item, the children come next > It occurs to me that some trees have lazy expansion so when you press right > you'd have to be a lot more careful about selecting the next row. I'm sorry I > mentioned this, please use the previous version. By previous version, do you mean 1) don't navigate to first child, or do it differently? I think you were right, we should navigate to the first child. Isn't the lazy expansion case what the timed select code takes care of dealing with for us?
Attachment #150627 - Attachment is obsolete: true
Attachment #150627 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 150805 [details] [diff] [review] Fix all of Neil's comments except selection of first child for right arrow on open container Waiting for clarification on the issue of right arrow selecting the first child of an open container.
Attachment #150805 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #150805 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #150817 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 150817 [details] [diff] [review] Also fix first child navigation to do nothing when children not yet populated Looking good, except for some silly spacing errors e.g. row <0 >+ var row = this.currentIndex; >+ if (!this.changeOpenState(row, true) && row >= 0) { Probably slightly neater to check row >= 0 first. >+ var view = this.treeBoxObject.view; >+ if (row+1 < view.rowCount && >+ view.isContainer(row) && This line of the condition probably isn't necessary - we can probably assume that if the next row's parent is us then we're a container :-) >+ view.getParentIndex(row + 1) == row) { >+ // If already opened, select the first child. >+ // The getParentIndex test above ensures that the children >+ // are already populated and ready. >+ this.view.selection.timedSelect(row+1, this._selectDelay); >+ this.treeBoxObject.ensureRowIsVisible(row+1); >+ } >+ } More spacing nits i.e. row + 1 please. Hopefully I can put this patch though its paces tomorrow.
Attached patch Fixes more of Neil's comments (obsolete) — — Splinter Review
Attachment #150817 - Attachment is obsolete: true
Attachment #150817 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #150889 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #150889 - Attachment is obsolete: true
Attachment #150889 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #150890 - Flags: review?
Comment on attachment 150890 [details] [diff] [review] Forgot to save file before creating last patch Neil, please ignore the accidental blank line at the top of tree.xml. I've removed it.
Attachment #150890 - Flags: review? → review?(neil.parkwaycc.co.uk)
Comment on attachment 150890 [details] [diff] [review] Forgot to save file before creating last patch After all that, the stupid customize sidebar dialog doesn't work if you expand the tree with the keyboard :-/
Attachment #150890 - Flags: review?(neil.parkwaycc.co.uk) → review+
Attachment #150890 - Flags: superreview?(jst)
(In reply to comment #18) > (From update of attachment 150890 [details] [diff] [review]) > After all that, the stupid customize sidebar dialog doesn't work if you expand > the tree with the keyboard :-/ > Isn't that something we'd want fixed before this lands?
(In reply to comment #19) >(In reply to comment #18) >>(From update of attachment 150890 [details] [diff] [review]) >>After all that, the stupid customize sidebar dialog doesn't work if you expand >>the tree with the keyboard :-/ >Isn't that something we'd want fixed before this lands? What I meant was that aaron's patch would play nice with the customize sidebar dialog if only the customize sidebar dialog wasn't already broken :-/
Neil, what's the bug # on the customize sidebar problem?
(In reply to comment #21) >Neil, what's the bug # on the customize sidebar problem? Sorry, I forgot to file one before; Bug 248328 filed.
Comment on attachment 150890 [details] [diff] [review] Forgot to save file before creating last patch sr=jst
Attachment #150890 - Flags: superreview?(jst) → superreview+
Checking in accessible/src/base/nsRootAccessible.cpp; /cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v <-- nsRootAccessible.cpp new revision: 1.97; previous revision: 1.96 done Checking in xpfe/global/resources/content/bindings/tree.xml; /cvsroot/mozilla/xpfe/global/resources/content/bindings/tree.xml,v <-- tree.xml new revision: 1.40; previous revision: 1.39 done Checking in toolkit/content/widgets/tree.xml; /cvsroot/mozilla/toolkit/content/widgets/tree.xml,v <-- tree.xml new revision: 1.8; previous revision: 1.7 done
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Blocks: 250306
This is causing one tree item to incorrectly expand/collapse other tree items. In the Firefox bookmarks manager, if you double click a subfolder the very top parent folder is collapsed instead of having the subfolder expanded. See bug 250306 for a mailnews case where double clicking one account incorrectly expands/collapses another account. Backing out the tree.xml changes fixes both cases.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 248577
I'm unable to repro, and no one has confirmed Stephen Walker's report. Stephen if that's still a problem can you open a new bug?
No longer blocks: 248577, 250306
Status: REOPENED → RESOLVED
Closed: 21 years ago → 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: