300779 - Support multiple selection for xul:tree

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Aaron Leventhal]

We need to support EVENT_SELECTION_WITHIN/ADD/REMOVE for XUL tree widgets.

Comment 1

[Aaron Leventhal]

Attachment: First part of fix -- make Ctrl+Space work correctly in a tree view -- it should always toggle the selection for multi select trees

Comment 2

[Aaron Leventhal]

Attachment: First part of fix -- make Ctrl+Space work correctly in a tree view -- it should always toggle the selection for multi select trees

Comment 3

[[e-mail]]

Comment on attachment 189428 [details] [diff] [review]
First part of fix -- make Ctrl+Space work correctly in a tree view -- it should always toggle the selection for multi select trees

1) On the Mac we suppport Command+Click (metaKey) to toggle a row.
2) I'm not sure that the space key needs to do anything in a single select
tree. This might simplify the code.

Comment 4

[Aaron Leventhal]

(In reply to comment #3)
> (From update of attachment 189428 [details] [diff] [review] [edit])
> 1) On the Mac we suppport Command+Click (metaKey) to toggle a row.
How might that affect this patch? I'm only modifying keypress behavior, not
click behavior.

> 2) I'm not sure that the space key needs to do anything in a single select
> tree. This might simplify the code.
It doesn't. Okay I'll fix that. 

Comment 5

[[e-mail]]

(In reply to comment #4)
>I'm only modifying keypress behavior, not click behavior.
Ah, but if you're modifying Ctrl+Space to match Ctrl+Click, wouldn't
it be more consistent to make Command+Space match Command+Click too?

Comment 6

[Aaron Leventhal]

Attachment: Assume that space does nothing in single select

- Unfortunately assuming space does nothing on a single line didn't simplify
the logic.
- I decided to skip doing something for cmd+space on mac because it doesn't
look like we support that for HTML listbox either. In fact, I don't even know
whether cmd+space is correct yet. I also don't know if we support cmd+click for
listbox.  A bug spinnoff should be filed on finding out and then doing the
right thing cmd+click/cmd+space for multiselect widgets on mac.

Comment 7

[Aaron Leventhal]

Comment on attachment 189428 [details] [diff] [review]
First part of fix -- make Ctrl+Space work correctly in a tree view -- it should always toggle the selection for multi select trees

This was the older patch. The newer one should get the review.

Comment 8

[[e-mail]]

Comment on attachment 189992 [details] [diff] [review]
Assume that space does nothing in single select

Would it be worthwhile moving the selection.single test to the previous if() so
that we can use spaces for key navigation in single select trees?

Comment 9

[Aaron Leventhal]

Neil, I thought about that. I suppose we can do that. Want a new patch?

Comment 10

[[e-mail]]

If you agree to the change, then a new patch would avoid confusion.

Comment 11

[Aaron Leventhal]

Attachment: Allow space to be used for inc search in single select

Comment 12

[[e-mail]]

Comment on attachment 190030 [details] [diff] [review]
Allow space to be used for inc search in single select

In the File Bookmark dialog typing "New " now prefers "New Folder" over "Newer
Folder", while in bookmarks manager Ctrl+Space toggles the current item.

Comment 13

[Aaron Leventhal]

Keeping open for accessibility API work.

Checking in toolkit/content/widgets/tree.xml;
/cvsroot/mozilla/toolkit/content/widgets/tree.xml,v  <--  tree.xml
new revision: 1.18; previous revision: 1.17
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.47; previous revision: 1.46
done

Comment 14

[Aaron Leventhal]

Checking in tabbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbox.xml,v  <--  tabbox.xml
new revision: 1.27; previous revision: 1.26
done

Comment 15

[Aaron Leventhal]

I had marked the wrong bug.

Comment 16

[Aaron Leventhal]

Attachment: Finish work for tree view. Now deal with accessibility API event support.

Comment 17

[Boris Zbarsky [:bzbarsky]]

I'm not going to be able to get to this in the foreseeable future.

Comment 18

[Aaron Leventhal]

Comment on attachment 190462 [details] [diff] [review]
Finish work for tree view. Now deal with accessibility API event support.

Will use a PLEvent for safety.

Comment 19

[Aaron Leventhal]

Attachment: Expose via a11y events. We don't need DOMItemSelected after all, they aren't fired for tree. We'll go off of "select" DOM events later for the add/remove a11y events.

Comment 20

[Aaron Leventhal]

Comment on attachment 190476 [details] [diff] [review]
Expose via a11y events. We don't need DOMItemSelected after all, they aren't fired for tree. We'll go off of "select" DOM events later for the add/remove a11y events.

r=timeless from IRC

Comment 21

[Mike Kaply [:mkaply]]

Comment on attachment 190476 [details] [diff] [review]
Expose via a11y events. We don't need DOMItemSelected after all, they aren't fired for tree. We'll go off of "select" DOM events later for the add/remove a11y events.

a=mkaply

Comment 22

[Aaron Leventhal]

Attachment: Missing files that still need review. Implementation of nsPLDOMEvent

Comment 23

[timeless]

Comment on attachment 190592 [details] [diff] [review]
Missing files that still need review. Implementation of nsPLDOMEvent

change "safe time"

perhaps: stable/consistent DOM

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 190592 [details] [diff] [review]
Missing files that still need review. Implementation of nsPLDOMEvent

+  mEventNode->GetOwnerDocument(getter_AddRefs(domDoc));

Please nullcheck domDoc.

+static void PR_CALLBACK HandlePLDOMEvent(nsPLDOMEvent* aEvent)
+{
+  aEvent->HandleEvent();
+}
+
+static void PR_CALLBACK DestroyPLDOMEvent(nsPLDOMEvent* aEvent)
+{
+  delete aEvent;
+}

These should be in the .cpp file, not in the .h file.

Comment 25

[Aaron Leventhal]

Checking in accessible/src/base/nsRootAccessible.cpp;
/cvsroot/mozilla/accessible/src/base/nsRootAccessible.cpp,v  <-- 
nsRootAccessible.cpp
new revision: 1.127; previous revision: 1.126
done
Checking in accessible/src/xul/nsXULTreeAccessible.cpp;
/cvsroot/mozilla/accessible/src/xul/nsXULTreeAccessible.cpp,v  <-- 
nsXULTreeAccessible.cpp
new revision: 1.33; previous revision: 1.32
done
Checking in accessible/src/xul/nsXULTreeAccessible.h;
/cvsroot/mozilla/accessible/src/xul/nsXULTreeAccessible.h,v  <-- 
nsXULTreeAccessible.h
new revision: 1.17; previous revision: 1.16
done
Checking in content/events/public/Makefile.in;
/cvsroot/mozilla/content/events/public/Makefile.in,v  <--  Makefile.in
new revision: 1.11; previous revision: 1.10
done
RCS file: /cvsroot/mozilla/content/events/public/nsPLDOMEvent.h,v
done
Checking in content/events/public/nsPLDOMEvent.h;
/cvsroot/mozilla/content/events/public/nsPLDOMEvent.h,v  <--  nsPLDOMEvent.h
initial revision: 1.1
done
Checking in content/events/src/Makefile.in;
/cvsroot/mozilla/content/events/src/Makefile.in,v  <--  Makefile.in
new revision: 1.53; previous revision: 1.52
done
RCS file: /cvsroot/mozilla/content/events/src/nsPLDOMEvent.cpp,v
done
Checking in content/events/src/nsPLDOMEvent.cpp;
/cvsroot/mozilla/content/events/src/nsPLDOMEvent.cpp,v  <--  nsPLDOMEvent.cpp
initial revision: 1.1
done
Checking in layout/xul/base/src/tree/src/nsTreeSelection.cpp;
/cvsroot/mozilla/layout/xul/base/src/tree/src/nsTreeSelection.cpp,v  <-- 
nsTreeSelection.cpp
new revision: 1.47; previous revision: 1.46
done

Comment 26

[Boris Zbarsky [:bzbarsky]]

Is there a reason nsPLDOMEvent::PostDOMEvent doesn't just do cleanup itself? 
That would make it a lot simpler to use....

Also, is there a reason this code doesn't follow the content/ style conventions?

Finally, is there a reason the nsPLDOMEvent class can only be used for some
types of events?  I would have preferred something that could be used for any
DOM event content wants to fire...

Comment 27

[Aaron Leventhal]

(In reply to comment #26)
> Is there a reason nsPLDOMEvent::PostDOMEvent doesn't just do cleanup itself? 
> That would make it a lot simpler to use....
> 
> Also, is there a reason this code doesn't follow the content/ style conventions?
> 
> Finally, is there a reason the nsPLDOMEvent class can only be used for some
> types of events?  I would have preferred something that could be used for any
> DOM event content wants to fire...

I think it should evolve into a user-friendly general class for doing this.
That's the idea. If there's an optimization or simplification I missed then
let's just fix that.

What style convention did I miss?

Comment 28

[Boris Zbarsky [:bzbarsky]]

returnType
classname::methodname(args)
{

(three separate lines there).

Comment 29

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381