Closed Bug 300779 Opened 19 years ago Closed 19 years ago

Support multiple selection for xul:tree

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

Details

(Keywords: access)

Attachments

(4 files, 3 obsolete files)

We need to support EVENT_SELECTION_WITHIN/ADD/REMOVE for XUL tree widgets.
Attachment #189427 - Attachment is obsolete: true
Attachment #189428 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #189428 - Flags: review?(neil.parkwaycc.co.uk)
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.
(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. 

(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?
- 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.
Attachment #189428 - Attachment is obsolete: true
Attachment #189992 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #189992 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #189428 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #189428 - Flags: review?(neil.parkwaycc.co.uk)
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?
Neil, I thought about that. I suppose we can do that. Want a new patch?
If you agree to the change, then a new patch would avoid confusion.
Attachment #189992 - Attachment is obsolete: true
Attachment #189992 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #189992 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #190030 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #190030 - Flags: review?(neil.parkwaycc.co.uk)
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.
Attachment #190030 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #190030 - Flags: superreview+
Attachment #190030 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #190030 - Flags: review+
Attachment #190030 - Flags: approval-l10n?
Attachment #190030 - Flags: approval-l10n? → approval1.8b4?
Attachment #190030 - Flags: approval1.8b4? → approval1.8b4+
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
Checking in tabbox.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbox.xml,v  <--  tabbox.xml
new revision: 1.27; previous revision: 1.26
done
Status: NEW → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I had marked the wrong bug.
Attachment #190462 - Flags: superreview?(bzbarsky)
Attachment #190462 - Flags: review?(bzbarsky)
I'm not going to be able to get to this in the foreseeable future.
Attachment #190462 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #190462 - Flags: superreview?(bzbarsky)
Attachment #190462 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #190462 - Flags: review?(bzbarsky)
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.
Attachment #190462 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #190462 - Flags: review?(neil.parkwaycc.co.uk)
Attachment #190476 - Flags: superreview?(bryner) → superreview?(roc)
Attachment #190476 - Flags: superreview?(roc) → superreview+
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
Attachment #190476 - Flags: review?(timeless) → review+
Attachment #190476 - Flags: approval1.8b4?
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
Attachment #190476 - Flags: approval1.8b4? → approval1.8b4+
Attachment #190592 - Flags: superreview?(roc)
Attachment #190592 - Flags: review?(timeless)
Comment on attachment 190592 [details] [diff] [review]
Missing files that still need review. Implementation of nsPLDOMEvent

change "safe time"

perhaps: stable/consistent DOM
Attachment #190592 - Flags: review?(timeless) → review+
Flags: blocking1.8b4+
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.
Attachment #190592 - Flags: superreview?(roc) → superreview+
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
Status: REOPENED → RESOLVED
Closed: 19 years ago19 years ago
Resolution: --- → FIXED
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...
(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?
returnType
classname::methodname(args)
{

(three separate lines there).
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: