Cell-based selection in trees

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
12 years ago
9 years ago

People

(Reporter: kinger, Assigned: janv)

Tracking

Trunk
x86
All
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

12 years ago
One of our clients has requested the following, and we want to develop it on the
trunk and get it checked in. We feel it would be a useful addition to the tree
widget.

1) The ability to have a cell based selector in a tree, allowing us to navigate
by keyboard across a single row to each adjacent column.
2) The ability to attach an event to the tree (or a cell) that would allow us to
know which cell is currently selected when a key (such as enter) is pressed,
based on its row and column.
Summary: Cell based selection in trees → Cell-based selection in trees
(Assignee)

Comment 1

12 years ago
Created attachment 197793 [details] [diff] [review]
Initial patch

Neil, could you take a look at this patch?
Thanks
(Assignee)

Comment 2

12 years ago
Created attachment 197794 [details]
test case

Updated

12 years ago
Attachment #197794 - Attachment mime type: text/plain → application/vnd.mozilla.xul+xml

Comment 3

12 years ago
I had a quick look at the patch, and it seems to me that you made tree cells
keyboard selectable by default which you don't want in the folder pane.

I wonder whether it's worth optimizing invalidateSelection for cell mode.
(Assignee)

Comment 4

12 years ago
(In reply to comment #3)
> I had a quick look at the patch, and it seems to me that you made tree cells
> keyboard selectable by default which you don't want in the folder pane.
> 

yeah, good catch
we need a new attribute to enable/disable it, any ideas?

> I wonder whether it's worth optimizing invalidateSelection for cell mode.

I don't think it's worth at this point.

Thanks for initial review.
Status: NEW → ASSIGNED

Comment 5

12 years ago
Jan, could you please specify the possible values for seltype and selstyle? Are
they meant to be combined into one? There are places
(classic/global/win/tree.css) where both are used and other places where only
one is used

Also <property name="cellmode"> should be readonly, and I think should be cellMode
(Assignee)

Comment 6

12 years ago
(In reply to comment #5)
> Jan, could you please specify the possible values for seltype and selstyle? Are
> they meant to be combined into one? There are places
> (classic/global/win/tree.css) where both are used and other places where only
> one is used

This is probably a cut and paste problem. I meant to remove selstyle completely.
seltype can have these values: single, cell or text.
I'll fix that.

> 
> Also <property name="cellmode"> should be readonly, and I think should be cellMode
> 

This is just a helper for internal code, we could rename it to _cellMode for now
(Assignee)

Comment 7

12 years ago
(In reply to comment #3)
> I had a quick look at the patch, and it seems to me that you made tree cells
> keyboard selectable by default which you don't want in the folder pane.
> 

There is selstyle="primary" in mailWidgets.xml (without seltype="single"), do
you think it's safe to convert it to seltype="text"?

Comment 8

12 years ago
(In reply to comment #7)
>There is selstyle="primary" in mailWidgets.xml (without seltype="single"), do
>you think it's safe to convert it to seltype="text"?
Yeah, the seltype="single" is enforced by the mail code
(the tree never gets focus, so its code does not apply).
(Assignee)

Comment 9

12 years ago
Created attachment 198052 [details] [diff] [review]
patch for review
Attachment #197793 - Attachment is obsolete: true
Attachment #198052 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Comment 10

12 years ago
Created attachment 198320 [details] [diff] [review]
better patch

changes:
- removed nsITreeColumn.active and nsITreeColumns.getActiveColumn()
- added nsITreeSelection.currentColumn
(Assignee)

Updated

12 years ago
Attachment #198052 - Attachment is obsolete: true
Attachment #198320 - Flags: review?(neil.parkwaycc.co.uk)
(Assignee)

Updated

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

Comment 11

12 years ago
Comment on attachment 198320 [details] [diff] [review]
better patch

>+NS_IMETHODIMP nsTreeSelection::SetCurrentColumn(nsITreeColumn* aCurrentColumn)
>+{
>+  if (mCurrentColumn == aCurrentColumn) {
>+    return NS_OK;
>+  }
>+  if (mCurrentIndex != -1 && mCurrentColumn)
>+    mTree->InvalidateCell(mCurrentIndex, mCurrentColumn);
>+  
>+  mCurrentColumn = aCurrentColumn;
>+  
>+  if (mCurrentIndex != -1 && mCurrentColumn)
>+    mTree->InvalidateCell(mCurrentIndex, mCurrentColumn);
>+
>+  return NS_OK;
>+}
Just skimming this, but note that we don't guarantee that the current column is
the selected column, for example when right-clicking a folder in the folder
pane.

Comment 12

12 years ago
(In reply to comment #11)
>Just skimming this, but note that we don't guarantee that the current column is
>the selected column, for example when right-clicking a folder in the folder pane.
Doh! I meant row, not column :-[
(Assignee)

Comment 13

12 years ago
yeah, I should call invalidateSelection() or an optimized version as you suggested

Comment 14

12 years ago
Comment on attachment 198320 [details] [diff] [review]
better patch

>+      <handler event="focus">
>+        <![CDATA[
>+          this.treeBoxObject.focused = true;
>+          if (this.currentIndex == -1 && this.view.rowCount > 0) {
>+            this.currentIndex = this.treeBoxObject.getFirstVisibleRow();
>+          }
We invented class="focusing" specially to avoid this. It really confuses the
thread pane.

>+           var currentColumn = this.view.selection.currentColumn;
>+           var primary = this.columns.getPrimaryColumn();
>+           if (currentColumn && currentColumn != primary)
if (currentColumn && !currentColumn.primary) should work, which should make it
possible to simplify the check.

>          var c = this.currentIndex;
>          if (c == -1 || c == 0)
>            return;
>+
>+         var cellSelType = this._cellSelType;
>+         if (cellSelType) {
>+           var column = this.view.selection.currentColumn;
>+           if (!column)
>+             return;
>+
>+           while (c > 0 && !this.view.isSelectable(c-1, column))
>+             c--;
>+           if (c == 0)
>+             return;
>+         }
>+
I think this would look neater if you subtracted 1 immediately (i.e. var c =
this.currentIndex = 1;) then optionally searched for a selectable cell (while
(c >= 0) of course).

>             if (c+1 == this.view.rowCount)
>               return;
>          } catch (e) {}
This try/catch is to work around badly written content views. You're now
calling more view methods, so more could break. (The only sure solution is to
move all this stuff into the C++, presumably somewhere in nsTreeSelection.cpp).

>       <!-- double-click -->
>       <handler event="click" clickcount="2">
>       <![CDATA[
>+        if (this.parentNode._cellSelType)
>+          return;
>+
>         var b = this.parentNode.treeBoxObject;
>         var row = b.view.selection.currentIndex;
>         if (row == -1 || !b.view.isContainer(row))
Why no double-clicks on primary cells?
(Assignee)

Comment 15

12 years ago
Created attachment 199181 [details] [diff] [review]
patch v0.1
Attachment #198320 - Attachment is obsolete: true
(Assignee)

Comment 16

12 years ago
> We invented class="focusing" specially to avoid this. It really confuses the
> thread pane.

ok, I removed it from the xpfe version

> if (currentColumn && !currentColumn.primary) should work, which should make it
> possible to simplify the check.

fixed

> I think this would look neater if you subtracted 1 immediately (i.e. var c =
> this.currentIndex = 1;) then optionally searched for a selectable cell (while
> (c >= 0) of course).

fixed

> This try/catch is to work around badly written content views. You're now
> calling more view methods, so more could break. (The only sure solution is to
> move all this stuff into the C++, presumably somewhere in nsTreeSelection.cpp).

you mean the content view or views?

> Why no double-clicks on primary cells?
 
I removed the check

It's now possible to pass a column to invalidateRange()
Anyway, it's not used at the moment, because we support only single cell
selection. I just added this code to SetCurrentColumn()

if (mFirstRange)
  mTree->InvalidateCell(mFirstRange->mMin, mCurrentColumn);
(Assignee)

Updated

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

Comment 17

12 years ago
(In reply to comment #16)
>>This try/catch is to work around badly written content views.
>you mean the content view or views?
Sorry, I mean web content applications which use custom tree views.

>>Why no double-clicks on primary cells?
>I removed the check
I'm fine with that.

>I just added this code to SetCurrentColumn()
A comment might be welcome :-)

Comment 18

12 years ago
Oh, and don't forget to bump the iid's on the changed interfaces.
(Assignee)

Comment 19

12 years ago
(In reply to comment #18)
> Oh, and don't forget to bump the iid's on the changed interfaces.

sure, r=neil with that?

Comment 20

12 years ago
Comment on attachment 199181 [details] [diff] [review]
patch v0.1

You removed all of the constructor, including the bit that sets the first
selected column, which I guess we do need...

I would still like to know your angle on remote xul pages with "badly" written
custom tree views (i.e. don't implement class info or security checked
component).

Comment 21

12 years ago
Oh, and for some reason I can't get any sort of selection to display in the
advanced message search/filter edit folder selector.
(Assignee)

Comment 22

12 years ago
ok, I bumped iid's on nsITreeBoxObject, nsITreeSelection and nsITreeView

>You removed all of the constructor, including the bit that sets the first
>selected column, which I guess we do need...

which bit, xpfe or toolkit?

>I would still like to know your angle on remote xul pages with "badly" written
>custom tree views (i.e. don't implement class info or security checked
>component).

Is there a bug for this, should we file a new one?

(Assignee)

Comment 23

12 years ago
(In reply to comment #21)
> Oh, and for some reason I can't get any sort of selection to display in the
> advanced message search/filter edit folder selector.

fixed in my tree

Comment 24

12 years ago
(In reply to comment #22)
>>You removed all of the constructor, including the bit that sets the first
>>selected column, which I guess we do need...
>which bit, xpfe or toolkit?
xpfe

>>I would still like to know your angle on remote xul pages with "badly" written
>>custom tree views (i.e. don't implement class info or security checked
>>component).
>Is there a bug for this, should we file a new one?
The vk_down handlers were originally checked in with try/catch on rowCount...
(Assignee)

Comment 25

12 years ago
> The vk_down handlers were originally checked in with try/catch on rowCount...

Neil, I just tried to load remote xul with custom tree view implementation. It doesn't work even w/o this patch (I can't get selection), there's "Permission denied to create wrapper for object class UnnamedClass" in JS console. Shouldn't this be discussed in a different bug?

Comment 26

12 years ago
OK, then I guess r=me on patch v0.2 as discussed...
as they don't work you might want to remove the existing try/catch blocks?
(Assignee)

Comment 27

12 years ago
Created attachment 200963 [details] [diff] [review]
patch v0.2
Attachment #199181 - Attachment is obsolete: true
Attachment #200963 - Flags: superreview?(bzbarsky)
Attachment #200963 - Flags: review?(neil.parkwaycc.co.uk)
Comment on attachment 200963 [details] [diff] [review]
patch v0.2

I won't be able to get to this in any sort of reasonable timeframe.  Please try another sr?
Attachment #200963 - Flags: superreview?(bzbarsky)
(Assignee)

Updated

12 years ago
Attachment #200963 - Flags: superreview?(bryner)

Comment 29

12 years ago
Comment on attachment 200963 [details] [diff] [review]
patch v0.2

>-          try {
>-            if (c+1 == this.view.rowCount)
>-              return;
>-         } catch (e) {}
>+         if (c+1 == this.view.rowCount)
>+           return;
Might as well fix this to c + 1 (both copies).
Attachment #200963 - Flags: review?(neil.parkwaycc.co.uk) → review+
(Assignee)

Comment 30

12 years ago
bryner, would you have time to sr it?
Comment on attachment 200963 [details] [diff] [review]
patch v0.2

>--- layout/xul/base/src/tree/public/nsITreeBoxObject.idl	20 Sep 2005 09:30:32 -0000	1.35
>+++ layout/xul/base/src/tree/public/nsITreeBoxObject.idl	27 Oct 2005 06:39:31 -0000
>@@ -156,7 +156,7 @@
>   void invalidateColumn(in nsITreeColumn col);
>   void invalidateRow(in long index);
>   void invalidateCell(in long row, in nsITreeColumn col);
>-  void invalidateRange(in long startIndex, in long endIndex);
>+  void invalidateRange(in long startIndex, in long endIndex, in nsITreeColumn col);

We could preserve script compatibility, in many cases, by leaving the old version of this method (equivalent to col == null), and adding a new version that takes the column parameter (like invalidateColumnRange).

>--- layout/xul/base/src/tree/public/nsITreeView.idl	29 Oct 2004 06:29:49 -0000	1.25
>+++ layout/xul/base/src/tree/public/nsITreeView.idl	27 Oct 2005 06:39:31 -0000
>@@ -198,6 +198,13 @@
>   boolean isEditable(in long row, in nsITreeColumn col);
> 
>   /**
>+   * isSelectable is called to ask the view if the cell is selectable.
>+   * This method is only called if the selection style is |cell| or |text|.
>+   * XXXvarga shouldn't this be called isCellSelectable?
>+   */
>+  boolean isSelectable(in long row, in nsITreeColumn col);

Maybe instead of this, we could have a special property that's returned from getCellProperties to indicate that the cell is selectable?  That way we could avoid requiring any changes to existing TreeView implementations.

>--- layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp	26 Oct 2005 21:46:39 -0000	1.255
>+++ layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp	27 Oct 2005 06:39:32 -0000
>@@ -743,13 +743,19 @@
> }
> 
> NS_IMETHODIMP
>-nsTreeBodyFrame::InvalidateRange(PRInt32 aStart, PRInt32 aEnd)
>+nsTreeBodyFrame::InvalidateRange(PRInt32 aStart, PRInt32 aEnd, nsITreeColumn* aCol)
> {
>   if (mUpdateBatchNest)
>     return NS_OK;
> 
>-  if (aStart == aEnd)
>+  nsTreeColumn* col = NS_STATIC_CAST(nsTreeColumn*, aCol);

I think we really need to be doing better input validation for scriptable methods.  Javascript could crash this just by passing in a nsITreeColumn object of its own.  How about adding a private IID on nsTreeColumn so you can QI to it?

>@@ -1319,9 +1448,33 @@
>     return nsCSSAnonBoxes::moztreeimage;
>   }
> 
>-  // Just assume "text".
>-  // XXX For marquee selection, we'll have to make this more precise and do text measurement.
>-  return nsCSSAnonBoxes::moztreecelltext;
>+  currX += iconRect.width;
>+  remainingWidth -= iconRect.width;    

Can you explain the change to this method?  It's not really clear to me how it ties in with the other changes.

(more later)
(Assignee)

Comment 32

12 years ago
(In reply to comment #31)
> We could preserve script compatibility, in many cases, by leaving the old
> version of this method (equivalent to col == null), and adding a new version
> that takes the column parameter (like invalidateColumnRange).

> Maybe instead of this, we could have a special property that's returned from
> getCellProperties to indicate that the cell is selectable?  That way we could
> avoid requiring any changes to existing TreeView implementations.

Ok, I'll try to rework the patch to address these comments
 
> I think we really need to be doing better input validation for scriptable
> methods.  Javascript could crash this just by passing in a nsITreeColumn object
> of its own.  How about adding a private IID on nsTreeColumn so you can QI to
> it?

There's already a patch for this in bug 309429.

 
> >@@ -1319,9 +1448,33 @@
> >     return nsCSSAnonBoxes::moztreeimage;
> >   }
> > 
> >-  // Just assume "text".
> >-  // XXX For marquee selection, we'll have to make this more precise and do text measurement.
> >-  return nsCSSAnonBoxes::moztreecelltext;
> >+  currX += iconRect.width;
> >+  remainingWidth -= iconRect.width;    
> 
> Can you explain the change to this method?  It's not really clear to me how it
> ties in with the other changes.
 
This change fixes GetCellAt(..., out childElt) to return "text" in childElt for the real text area only (the text is now correctly measured in this method, it was only done in PaintText() before).
The old implementation returns "text" for entire text rect which is a problem when you click on it in "text selection mode". It would be confusing for the user.

Comment 33

12 years ago
(In reply to comment #31)
>Maybe instead of this, we could have a special property that's returned from
>getCellProperties to indicate that the cell is selectable?  That way we could
>avoid requiring any changes to existing TreeView implementations.
Note that properties aren't available to remote XUL.
(Assignee)

Comment 34

12 years ago
Created attachment 208018 [details] [diff] [review]
new approach

Ok, I didn't remove nsITreeView::IsSelectable() yet
There's a new method nsITreeBoxObject::IsCellSelectable() that calls GetCellProperties() and stuff. nsTreeContentView has been updated to append nsXULAtoms::selectable to the property array if the cell is selectable.
toolkit/tree.xml has been updated to call tree.isCellSelectable() instead of view.isSelectable()
Let me know if you like this approach and I'll finish it off.
Thanks
(Assignee)

Comment 35

11 years ago
Neil Deakin and Neil Rashbrook, what do you think, should we add a new method IsSelectable() or use GetCellProperties() to return the "selectable" atom. The problem with the latter is that we already use IsEditable() and stuff for other purposes, so it would be quite inconsistent.

I'd like to check in this patch ASAP.

Comment 36

11 years ago
My preference would be for IsSelectable().

Comment 37

11 years ago
I'd prefer IsSelectable as well
(Assignee)

Updated

11 years ago
Attachment #200963 - Flags: superreview?(bryner)
(Assignee)

Comment 38

11 years ago
Created attachment 224531 [details] [diff] [review]
patch v0.3
Attachment #200963 - Attachment is obsolete: true
Attachment #208018 - Attachment is obsolete: true
Attachment #224531 - Flags: superreview?(neil)
Attachment #224531 - Flags: review?(enndeakin)

Comment 39

11 years ago
Comment on attachment 224531 [details] [diff] [review]
patch v0.3

>+		       selectable="false"/>
No tabs please...

I assume the following issue applies to all themes:

>+tree[seltype="cell"] > treechildren::-moz-tree-cell {
>+  border: 1px solid transparent;
>+  padding: 0px 1px 1px 1px;
>+}
The "text" selection padding keeps the text away from the border. I'm don't think that "cell" padding has the same issue; given that the cell default is 0px 2px 0px 2px then maybe 0px 1px 0px 1px would best allow for the border.

I assume the following nits apply to both versions:

>+              return seltype;
>+            else
Nit: else after return

+                 col.element.getAttribute("selectable") == "false" ||
Shouldn't you create a new property col.selectable for these?

>+         else if (!cellSelType) // Ctrl+Up moves the anchor without selecting
Unnecessary; this case already only happens for a multiselect tree.
Attachment #224531 - Flags: superreview?(neil) → superreview+
(Assignee)

Comment 40

11 years ago
Neil R, I addressed all your comments except the theme issue, I don't understand what's the problem. Could you explain it in more details? Thanks

Comment 41

11 years ago
Comment on attachment 224531 [details] [diff] [review]
patch v0.3

The existing primary selection padding is specific to selected cell text. I don't think it makes sense to apply the same padding when selecting an entire cell. That's why I think the bottom padding for selected cells should be zero.

>+          var col = this.view.selection.currentColumn;
>+          if (col) {
>+            col = left ? col.getPrevious() : col.getNext();
>+          }
>+          else {
>+            col = this.columns.getFirstColumn();
>+          }
It just occurred to me that getKeyColumn might make more sense here.
(Assignee)

Updated

11 years ago
Attachment #224531 - Flags: review?(enndeakin)

Comment 42

11 years ago
Hmmm. I had just finished reviewing this.

>+NS_IMETHODIMP
>+nsTreeBodyFrame::InvalidateColumnRange(PRInt32 aStart, PRInt32 aEnd, nsITreeColumn* aCol)
>+{
>+  if (mUpdateBatchNest)
>+    return NS_OK;
>+
>+  nsTreeColumn* col = NS_STATIC_CAST(nsTreeColumn*, aCol);

Is there a bug on fixing up these direct casts?


>+  if (col) {
>+    if (aStart == aEnd)
>+      return InvalidateCell(aStart, col);
>+
>+    PRInt32 last = GetLastVisibleRow();
>+    if (aEnd < mTopRowIndex || aStart > last)
>+      return NS_OK;
>+
>+    if (aStart < mTopRowIndex)
>+      aStart = mTopRowIndex;
>+
>+    if (aEnd > last)
>+      aEnd = last;

What happens if aStart is greater than aEnd? Just extra painting?


>+NS_IMETHODIMP nsTreeSelection::SetCurrentColumn(nsITreeColumn* aCurrentColumn)
>+{
>+  if (mCurrentColumn == aCurrentColumn) {
>+    return NS_OK;
>+  }
>+
>+  if (mCurrentColumn) {
>+    if (mFirstRange)
>+      mTree->InvalidateCell(mFirstRange->mMin, mCurrentColumn);
>+    if (mCurrentIndex != -1)
>+      mTree->InvalidateCell(mCurrentIndex, mCurrentColumn);
>+  }
>+  

Why is the old selected column being repainted here? Won't the repainting code still think the column is selected?


>       <handler event="keypress" keycode="vk_left">
>         <![CDATA[
>-         if (!this.changeOpenState(this.currentIndex, false)) {
>-            var parentIndex = this.view.getParentIndex(this.currentIndex);
>-           if (parentIndex >= 0) {
>-             this.view.selection.select(parentIndex);
>-             this.treeBoxObject.ensureRowIsVisible(parentIndex);
>+         var row = this.currentIndex;
>+         if (row < 0)
>+           return;
>+
>+         var cellSelType = this._cellSelType;
>+         var checkContainers = true;
>+
>+         var currentColumn;
>+         if (cellSelType) {
>+           currentColumn = this.view.selection.currentColumn;
>+           if (currentColumn && !currentColumn.primary)
>+             checkContainers = false;
>+         }
>+
>+         if (checkContainers) {
>+           if (this.changeOpenState(this.currentIndex, false))
>+             return;

I think pressing left and right in cell selection mode should always move the current cell rather than open containers. Cells can be opened with Enter, and we should have a bug on making + and - do this as well.


>+         if (cellSelType) {
>+           var col = this._getNextColumn(row, true);
>+           if (col) {
>+             this.view.selection.currentColumn = col;
>+             this.treeBoxObject.ensureCellIsVisible(row, col);

Having a selectCell function would be useful.
(Assignee)

Comment 43

11 years ago
Created attachment 224577 [details] [diff] [review]
patch v0.4
Attachment #224531 - Attachment is obsolete: true
Attachment #224577 - Flags: review?(enndeakin)
(Assignee)

Updated

11 years ago
Attachment #224577 - Flags: superreview+

Comment 44

11 years ago
Comment on attachment 224577 [details] [diff] [review]
patch v0.4

One issue I saw after testing was that an overflowing cell was cut off when selected. But, otherwise, it looks good.
Attachment #224577 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 45

11 years ago
(In reply to comment #42)
> Is there a bug on fixing up these direct casts?

yes, bug 309429

> What happens if aStart is greater than aEnd? Just extra painting?

Ah, this was copied from invalidateRange()
The final rect for invalidation will be invalid.

There should be a check:
if (aStart > aEnd)
  return NS_OK;

I'll add it to both methods

> Why is the old selected column being repainted here? Won't the repainting code
> still think the column is selected?

In theory, there may be 2 cells (selected and current).
Anyway, we support only single selection in cell mode.

> I think pressing left and right in cell selection mode should always move the
> current cell rather than open containers. Cells can be opened with Enter, and
> we should have a bug on making + and - do this as well.

Well, I did it this way originally, but the customer wanted to change it. Personally, I like both aproaches.

> Having a selectCell function would be useful.

in the XBL binding?
(Assignee)

Comment 46

11 years ago
> > Having a selectCell function would be useful.
> 
> in the XBL binding?

hmm, we should fire the select event, did you mean that?

Comment 47

11 years ago
(In reply to comment #46)
> > > Having a selectCell function would be useful.
> > 
> > in the XBL binding?
> 
> hmm, we should fire the select event, did you mean that?
> 

It should fire the select event when a cell is changed, yes, but I just meant that a selectCell function would be logical so that a specific cell can be selected.
(Assignee)

Comment 48

11 years ago
Neil D, it seems it's not so trivial as I thought. e.g. view.selection.currentIndex doesn't fire select events.

I checked it in as it is for now.

Thanks
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Comment 49

11 years ago
I think Enn was just looking for a single method that sets the selected column and row at the same time, rather than setting currentColumn and calling select(row) separately.

I agree that changing the currentColumn should fire a select event. But what should it do for row-select-style trees?

Given your (temporary, I hope!) confusion over the currentIndex property perhaps you should change the name of currentColumn to selectedColumn?

Updated

11 years ago
Depends on: 340811
Depends on: 340867
Depends on: 338401
Depends on: 343636
are the regressions on radar?  esp bug 338401
Question: what does "text" do?  I can't figure it out from the discussion here.

Comment 52

10 years ago
Created attachment 277798 [details]
Rough guide to selection styles
Excellent; thank you.  Is "text" a new selection type in Fx3?  It appears to be based on what I'm seeing in the docs, but I'd like to be sure before I tag it as such.

Comment 54

10 years ago
(In reply to comment #53)
>Is "text" a new selection type in Fx3?
Both "text" and "cell" are new selection types. The nearest equivalent that SeaMonkey 1.x supports is selstyle="primary" which is only a visual effect equivalent to seltype="cell" applied to the column with primary="true" whereas the new selection types apply to the currently selected column (new property).
Depends on: 393711

Comment 55

10 years ago
> Anyway, we support only single selection in cell mode.

Why ?? Multiple selection is a basic user interaction feature. That is the default in row selection.

Could you add an option for multiple cell selection ?

Comment 56

10 years ago
Filed bug about seltype="cell": https://bugzilla.mozilla.org/show_bug.cgi?id=402958
No longer depends on: 338401

Updated

9 years ago
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.