Fire accessible state change events when tree item expands/collapses

RESOLVED FIXED

Status

()

Core
Disability Access APIs
P1
normal
RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: Aaron Leventhal, Assigned: Aaron Leventhal)

Tracking

({access, sec508})

Trunk
access, sec508
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

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

Comment 1

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

Updated

14 years ago
Depends on: 201922

Comment 2

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

Comment 3

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

Comment 4

14 years ago
Created 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
(Assignee)

Comment 5

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

Updated

14 years ago
Priority: -- → P1

Comment 6

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

Comment 7

14 years ago
Created attachment 150627 [details] [diff] [review]
New patch based on Neil's comments

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
(Assignee)

Updated

14 years ago
Attachment #150627 - Flags: review?
(Assignee)

Updated

14 years ago
Attachment #150627 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

14 years ago
Attachment #150627 - Flags: review?

Comment 8

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

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

Comment 10

14 years ago
> >+        <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?

(Assignee)

Comment 11

14 years ago
Created attachment 150805 [details] [diff] [review]
Fix all of Neil's comments except selection of first child for right arrow on open container
(Assignee)

Updated

14 years ago
Attachment #150627 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #150627 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 12

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

Updated

14 years ago
Attachment #150805 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 13

14 years ago
Created attachment 150817 [details] [diff] [review]
Also fix first child navigation to do nothing when children not yet populated
Attachment #150805 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #150817 - Flags: review?(neil.parkwaycc.co.uk)

Comment 14

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

Comment 15

14 years ago
Created attachment 150889 [details] [diff] [review]
Fixes more of Neil's comments
Attachment #150817 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #150817 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

14 years ago
Attachment #150889 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 16

14 years ago
Created attachment 150890 [details] [diff] [review]
Forgot to save file before creating last patch
Attachment #150889 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #150889 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

14 years ago
Attachment #150890 - Flags: review?
(Assignee)

Comment 17

14 years ago
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 18

14 years ago
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+
(Assignee)

Updated

14 years ago
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?

Comment 20

14 years ago
(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 :-/
(Assignee)

Comment 21

14 years ago
Neil, what's the bug # on the customize sidebar problem?

Comment 22

14 years ago
(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+
(Assignee)

Comment 24

14 years ago
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
Last Resolved: 14 years ago
Resolution: --- → FIXED

Updated

14 years ago
Blocks: 250306

Comment 25

14 years ago
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 → ---

Updated

14 years ago
Blocks: 248577
(Assignee)

Comment 26

14 years ago
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
Last Resolved: 14 years ago14 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.