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.

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

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.

(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.

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

(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

(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"?

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

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

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.

(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 :-[

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

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?

> 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);

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

Oh, and don't forget to bump the iid's on the changed interfaces.

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

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

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?

(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

(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...

> 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?

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?

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?

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).

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)

(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.

(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.

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

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.

My preference would be for IsSelectable().

I'd prefer IsSelectable as well

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.

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 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.

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.

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.

(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?

> > Having a selectCell function would be useful. > > in the XBL binding? hmm, we should fire the select event, did you mean that?

(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.

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

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?

are the regressions on radar? esp bug 338401

Question: what does "text" do? I can't figure it out from the discussion here.

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.

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

> 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 ?

Filed bug about seltype="cell": https://bugzilla.mozilla.org/show_bug.cgi?id=402958