Last Comment Bug 201499 - Tree widgets should support editable content within hierarchical data structures (inline edit)
: Tree widgets should support editable content within hierarchical data structu...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: All All
: -- enhancement with 4 votes (vote)
: ---
Assigned To: Neil Deakin
:
Mentors:
http://wiki.mozilla.org/XUL:Tree
Depends on: 96384 469487 218642 777832
Blocks: 350346 350347 392216 347772 350332 350333 350335 350350
  Show dependency treegraph
 
Reported: 2003-04-10 08:50 PDT by Josh Johnson
Modified: 2012-07-26 11:46 PDT (History)
15 users (show)
enndeakin: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Workaround by ndeakin (1.94 KB, application/vnd.mozilla.xul+xml)
2004-06-10 05:58 PDT, Cédric Chantepie
no flags Details
Workaround by ndeakin (2.19 KB, application/octet-stream)
2004-06-10 14:45 PDT, Cédric Chantepie
no flags Details
workaround edittree (3.15 KB, application/octet-stream)
2006-04-04 12:49 PDT, Willy also
no flags Details
Support inline text editing (49.11 KB, patch)
2006-05-15 09:19 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
Address review comments. (47.61 KB, patch)
2006-06-05 11:40 PDT, Neil Deakin
jvarga: review+
neil: superreview-
Details | Diff | Splinter Review
address review comments (50.34 KB, patch)
2006-06-14 12:42 PDT, Neil Deakin
jvarga: review+
neil: superreview+
Details | Diff | Splinter Review

Description Josh Johnson 2003-04-10 08:50:50 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0.1) Gecko/20020823
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.2.1) Gecko/20021130

Currently, the only content available in a tree is plain label text.  XUL tree
widgets should support input controls including the textbox and the menulist. 
This would allow advanced UI functions such as being able to click a text
selection and edit the text content, or select another entry from a menulist. 
This would seem to be a common-sense request which would make developers lives
easier and strengthen the overall capabilities of XUL.

Reproducible: Always

Steps to Reproduce:
n/a
Actual Results:  
n/a

Expected Results:  
n/a
Comment 1 Jan Varga [:janv] 2003-04-10 09:44:47 PDT
dupe of 96384 ?
Comment 2 Josh Johnson 2003-04-10 10:03:29 PDT
If the popup widget specified in 96384 can support both a textbox and a menulist
(or any other XUL widget for that matter), then yes this could be marked as a dupe.
Comment 3 bugzilla.mozilla.org 2003-07-15 14:19:33 PDT
just voted for this RFE

it would make the tree a killer feature of XUL applications

Some people at work try to implement something like this in HTML (for MSIE) but
it's horrible work. 
Comment 4 Cédric Chantepie 2004-06-10 05:58:20 PDT
Created attachment 150432 [details]
Workaround by ndeakin
Comment 5 Cédric Chantepie 2004-06-10 14:45:52 PDT
Created attachment 150476 [details]
Workaround by ndeakin
Comment 6 Nickolay_Ponomarev 2006-02-11 10:03:59 PST
(to get the workaround code to work you need to change line 88 of edittree.xml to |var pos = col.index;| because of the tree changes in Gecko 1.8.
Comment 7 Willy also 2006-04-04 12:49:59 PDT
Created attachment 217188 [details]
workaround edittree

Workaround edittree
It solves some problems with the blur_event, allowes doubleclick for editing, get the right textbox-width, allowes to change the value in the callback and uses properties="cedit_true", so you can easy combine the editable treecells with styles, pictures, ....

Surely you know that this works for Content_Trees and RDF_Content_Trees only, which gives you access over the nsITreeContentView.
Before I start to write a binding for RDF_Trees with 'dont-build-content' and end up in an infinite loop:
Is it possible? I'm not sure.
Comment 8 Neil Deakin 2006-04-18 18:23:52 PDT
I'm now working on an improved version of tree editing now that caret changes have landed.
Comment 9 Willy also 2006-05-12 09:32:21 PDT
Now it works for trees with dont-build-content in the following way:

xml-binding:
doubleclick -> get the object from desired triple ->
write a space into the object -> unhide textbox with value = object.

user press CR -> send textbox.value,the predicate,subject + the type of the used tree-datasource to the server.
server changes the desired value in the database.
server checks the client-information about the used datasource.
if(in-memory-datasource || (xml-datasource == URI file://....)
{server writes back XML-RPC to client which has to do an update_triple_object;}
else
{server writes the new object into desired RDF-file and sends a XML-RPC to the client, that he has to do a datasource.Refresh();}

It works, but it is unuseable.
Since version Mozilla Firefox/1.5.0.3 you can see the cursor everywhere, even on a normal XUL or HTML page. But the cursor is still not in the <xul:textbox.

greetings also
Comment 10 Neil Deakin 2006-05-15 09:19:54 PDT
Created attachment 222043 [details] [diff] [review]
Support inline text editing

Patch adds simple support for text editing in trees. Additional features may be added by follow up bugs.
Comment 11 neil@parkwaycc.co.uk 2006-05-17 05:32:09 PDT
Comment on attachment 222043 [details] [diff] [review]
Support inline text editing

Just a quick scan through for anything "obvious" ;-)

>+tree[editing] {
>+  -moz-user-select: text;
>+}
What happens if you don't have this?

>+      <method name="startEditing">
>+        <parameter name="row"/>
>+        <parameter name="column"/>
>+        <body>
>+          <![CDATA[
>+            if (row < 0 || row >= this.itemCount)
>+              return;
Shouldn't you at least null-check column here?

>+            if (this._editingRow >= 0)
>+              this.stopEditing();
I have a slight preference for if (this._editingColumn) ...

>+            var left = outx.value;
>+            input.left = left - 4;
>+            input.top = outy.value - 1;
>+            coords = box.getCoordsForCellItem(row, column, "cell",
>+                                              outx, outy, outwidth, outheight);
>+            input.width = outwidth.value - (left - outx.value);
>+            input.height = outheight.value + 1;
I do love those magic constants... NOT! Is it not possible to tweak the margins on the textbox instead?

I don't understand your height calculation. I understand that you want to use the available width of the cell rather than the measured width but perhaps you should tweak getCoordsForCellItem for a new element type.

>+            input.value = this.view.getCellText(row, column);
>+            var selectText = function selectText() {
>+              input.select();
>+              input.inputField.focus();
>+            }
>+            setTimeout(selectText, 0);
Is this timeout needed because the element was originally hidden rather than collapsed? If so, would it be too expensive to collapse it?

>+            var tree = this;
>+            var blurStop = function blurStop() {
>+              input.inputField.removeEventListener("blur", blurStop, true);
>+              tree.stopEditing(true);
>+            }
>+            input.inputField.addEventListener("blur", blurStop, true);
I notice that for some events you added the listener to the tree, perhaps you could do that for this too?

>+            input.value = "";
Ah, does hiding it clear the undo history, which collapsing would not do?

>+      <handler event="keypress" keycode="vk_enter">
>+        <![CDATA[
>+          if (this._editingRow != -1 &&
>+              event.originalTarget == this.inputField.inputField) {
I'm just wondering what you were expecting the original target to be ;-)

>+            event.preventBubble();
What is it that you don't want the event to do? Whatever it is, preventBubble won't do it.

>+      <handler event="change">
>+        if (this.editable)
>+          this.stopEditing(true);
>+      </handler>
Shouldn't your keypress and blur listeners suffice?


>       <handler event="keypress" keycode="vk_right">
>         <![CDATA[
>+         if (this._editingRow != -1)
>+           return;
phase="target" would probably help filtering out these events.
Comment 12 Neil Deakin 2006-05-18 13:55:19 PDT
(In reply to comment #11)
> >+tree[editing] {
> >+  -moz-user-select: text;
> >+}
> What happens if you don't have this?

Without it, the text in the textbox cannot be selected. And it doesn't work to put the property on the textbox.

> I don't understand your height calculation. I understand that you want to use
> the available width of the cell rather than the measured width but perhaps you
> should tweak getCoordsForCellItem for a new element type.

Are you asking about width or height?


> Is this timeout needed because the element was originally hidden rather than
> collapsed? If so, would it be too expensive to collapse it?

The timeout is needed because the input needs to do some initialization before text can be selected. Collapsed doesn't work either.

> I notice that for some events you added the listener to the tree, perhaps you
> could do that for this too?

I think I want it to fire when the input field is blurred.

> 
> >+            input.value = "";
> Ah, does hiding it clear the undo history, which collapsing would not do?

Yes
Comment 13 neil@parkwaycc.co.uk 2006-05-25 16:12:38 PDT
(In reply to comment #12)
>>I don't understand your height calculation. I understand that you want to use
>>the available width of the cell rather than the measured width but perhaps you
>>should tweak getCoordsForCellItem for a new element type.
>Are you asking about width or height?
I'm asking about the height. I already figured out that the width is the right coord of the cell minus the left coord of the text.

>Collapsed doesn't work either.
Fair enough.

>>I notice that for some events you added the listener to the tree, perhaps you
>>could do that for this too?
>I think I want it to fire when the input field is blurred.
Assuming it remains a capturing event listener I don't see an issue here.
Comment 14 Jan Varga [:janv] 2006-06-05 08:40:55 PDT
Neil D, don't you need selectable cells anymore?
Anyway, I'm going to review this patch.
Comment 15 Neil Deakin 2006-06-05 09:24:05 PDT
Yes, selectable cells would be needed for accessibility at least.

I will be making a new version of the patch that addresses Neil's comments if you want to wait a day or two.
Comment 16 Neil Deakin 2006-06-05 11:40:30 PDT
Created attachment 224455 [details] [diff] [review]
Address review comments.

I couldn't find a way to remove the extra 1 added to the height and make the textbox the right size.
Comment 17 Jan Varga [:janv] 2006-06-08 08:07:42 PDT
Comment on attachment 224455 [details] [diff] [review]
Address review comments.

>+            if (accept) {
>+              var value = input.value;
>+              this.view.setCellText(this._editingRow, this._editingColumn, value);
>+              this.treeBoxObject.invalidateCell(this._editingRow, this._editingColumn);
>+            }

a suggestion, what about doing invalidation in setCellText() ?
see http://lxr.mozilla.org/seamonkey/source/extensions/sql/base/src/mozSqlResult.cpp#925

only column reording code uses invalidation methods in tree.xml, everything else is done in tree view impls

>+      <handler event="dblclick">
>+        if (this.editable) {
>+          var row = {}, col = {}, obj = {};
>+          this.treeBoxObject.getCellAt(event.clientX, event.clientY, row, col, obj);
>+          this.startEditing(row.value, col.value);
>+        }

move to treebody dblclick handler?

>+
>+    nsIContent* baseContent = GetBaseElement();
>+    if (baseContent && baseContent->HasAttr(kNameSpaceID_None, nsXULAtoms::editing))
>+      mScratchArray->AppendElement(nsXULAtoms::editing);

hmm, this doesn't look right to me
If I understand it correctly, you're setting special CSS properties in tree.css for row(s) that are currently selected and the tree has the "editable" attribute.
In theory, one could edit a row w/o selecting it, in that case this wouldn't work. This already happens in "text" selection mode (a row is selected after a double click only if the user clicked on text in cell) 
The editable property should be appended only if aRow==mEditingRow, but then you would have to implement startEditing() partially in C++.
It's a not big deal for now, but please file a new bug and rework it when you have time.

otherwise r=jan
Comment 18 neil@parkwaycc.co.uk 2006-06-11 07:37:58 PDT
Comment on attachment 224455 [details] [diff] [review]
Address review comments.

OK, so I think I discovered the source of the final pixel adjustment; there appears to be a bug in nsTreebodyFrame.cpp where it incorrectly calculates the text coordinates; they are not vertically centred when they should be. You can see this more clearly by specifying an increased row height. Ideally you would correct the coordinates and use them for the height, and only use the cell for its width.

I think that neither cycler, checkbox nor progressmeter cells should be inline editable. Checkboxes of course have their own "editing" system that also checks the column, so I think you need to do that too.

The dblclick handler has no CDATA but you were going to move it anyway, right?

Do we need the existing tree {
-moz-user-select: none; } rule in xul.css? I would prefer to remove that rather than adding the tree[editing] override. Speaking of CSS, the Modern theme should of course not use any system colours. I also guess you'll need to update the CSS to cope with Jan's checkin.

For some odd reason the tree's natural height seems to change ever so slightly during editing, although the width is OK.
Comment 19 neil@parkwaycc.co.uk 2006-06-11 07:39:24 PDT
Comment on attachment 224455 [details] [diff] [review]
Address review comments.

Actually I think a new patch would be a good idea not just because of the bitrot but I think you left off the atom list change.
Comment 20 Neil Deakin 2006-06-14 12:42:53 PDT
Created attachment 225608 [details] [diff] [review]
address review comments

Added some changes to nsTreeBodyFrame::GetCoordsForCellItem, and fix other review issues
Comment 21 Willy also 2006-06-14 13:45:28 PDT
Thx for this Patch, but it does still not all what user needs.
What I did is to use windows with hidechrome="true" or background-color="transparent", modal or dependent.
They are placed exactly over the treecell and gives every possibility.
Treecells now can have textboxes with autocomplete, editable menulist and whatever you need, depending on the cell-datatype. The server knows it and opens the window over XML-RPC, the UI places the window over the cell depending on the informations in the triple got from server. It works great and offers everything we need.

But there is a bug if we open the windows without titlebar in chrome.
In non-chrome it works perfect with titlebar="no".

The problem with windows in chrome without titlebar looks like this:
If we use any editable xul-elements inside the window, we have to take hidechrome="true", cause window -> background-color="transparent" makes the editable item unusable. Ok, I understand the problem. But if we use hidechrome="true", it is not possible anymore to adjust the window height to the height of a eg. textbox. The window will always have height + the height of the hidden titlebar.

Please support the exact height of windows in chrome with the attribute hidechrome="true".
It would give all the possibilities which are necessary for editable treecells.

greetings also
Comment 22 Neil Deakin 2006-06-14 13:58:36 PDT
That doesn't have anything to do with this bug. Please file a different bug about whatever issue you are having with transparent windows.

Comment 23 neil@parkwaycc.co.uk 2006-06-15 04:38:47 PDT
Comment on attachment 225608 [details] [diff] [review]
address review comments

>+  margin: 0 0 0 -4px;
>+  margin: 0 0 0 -4px;
These should be -3px as per the toolkit themes.

>+            var style = window.getComputedStyle(input, "");
>+            var topadj = parseInt(style.borderTopWidth) + parseInt(style.paddingTop);
Ah, I forgot borders and padding mess up sizing. I wonder whether they should be based on margins (that you would set in the CSS) instead, this would also save you from adjusting the top i.e. margin: -2px 0 -2px -3px; would automatically move the input up 2px so you only need to adjust the height.

>+              var value = input.value;
>+              this.view.setCellText(this._editingRow, this._editingColumn, value);
Hardly worth the temporary ;-)

>+      <handler event="blur" phase="capturing"
>+               action="if (event.originalTarget == this.inputField) this.stopEditing(true);"/>
Strange, I don't remember seeing a problem with this before (fallout from agnostic branch perhaps?) but I wasn't seeing editing stop on blur today.

>       <!-- double-click -->
>       <handler event="click" clickcount="2">
>       <![CDATA[
>         if (this.parentNode.disabled)
>           return;
>         var b = this.parentNode.treeBoxObject;
>         var row = b.view.selection.currentIndex;
>-        if (row == -1 || !b.view.isContainer(row))
>+        if (row == -1)
>           return;
> 
>          var col = {};
>          var obj = {};
>          b.getCellAt(event.clientX, event.clientY, {}, col, obj);
> 
>+         if (this.parentNode.editable)
>+           this.parentNode.startEditing(row, col.value);
>+
>+         if (!b.view.isContainer(row))
>+           return;
>+
>          // Cyclers and twisties respond to single clicks, not double clicks
>          if (!col.value.cycler && obj.value != "twisty")
>            this.parentNode.changeOpenState(row);
>       ]]>
>       </handler>
Isn't there a risk that this will both change the open state and edit?
Comment 24 Neil Deakin 2006-06-16 11:50:30 PDT
(In reply to comment #23)
> These should be -3px as per the toolkit themes.
> 
I'm not sure what you see wrong here. Windows uses 4px while Max uses 3px.

> Ah, I forgot borders and padding mess up sizing. I wonder whether they should
> be based on margins (that you would set in the CSS) instead, this would also
> save you from adjusting the top i.e. margin: -2px 0 -2px -3px; would
> automatically move the input up 2px so you only need to adjust the height.

Unfortunately, it causes the height to be truncated, although maybe I was trying something different.

> 
> >+              var value = input.value;
> >+              this.view.setCellText(this._editingRow, this._editingColumn, value);
> Hardly worth the temporary ;-)

Fixed this

> >          // Cyclers and twisties respond to single clicks, not double clicks
> >          if (!col.value.cycler && obj.value != "twisty")
> >            this.parentNode.changeOpenState(row);
> >       ]]>
> >       </handler>
> Isn't there a risk that this will both change the open state and edit?

Fixed this
Comment 25 neil@parkwaycc.co.uk 2006-06-16 13:35:24 PDT
(In reply to comment #24)
>(In reply to comment #23)
>>These should be -3px as per the toolkit themes.
>I'm not sure what you see wrong here. Windows uses 4px while Max uses 3px.
Well on linux I'm seeing the text move left 1px when you edit it, and the textbox extends 1px to the left of the cell, overlapping the tree's left border if it's the leftmost cell that's being edited.

>Unfortunately, it causes the height to be truncated, although maybe I was
>trying something different.
My guess is that you'll still need to adjust the height for the sum of the margins, but you won't have to adjust the top because the margin handles that.
Comment 26 Jan Varga [:janv] 2006-07-05 02:36:48 PDT
Comment on attachment 225608 [details] [diff] [review]
address review comments

looks good
Comment 27 Neil Deakin 2006-07-07 08:18:52 PDT
This is fixed. Another bug should be filed for editing using a menulist
Comment 28 Nickolay_Ponomarev 2006-08-26 17:11:24 PDT
Note: an example and some docs are available at http://wiki.mozilla.org/XUL:Tree and in attachment 235591 [details].
Comment 29 yonatan goraly 2006-12-06 14:06:16 PST
(In reply to comment #27)
> This is fixed. Another bug should be filed for editing using a menulist
> 

Is it final that this bug will not track menulist editing? Was another bug filed  to track that?
Comment 30 Neil Deakin 2007-08-28 13:03:52 PDT
Tests are in bug 377677.
Comment 31 Sergey Zh 2007-11-05 22:09:01 PST
I filed a bug about unexpected and unwanted row hierarchy expand/collapse when starting edit a cell.  https://bugzilla.mozilla.org/show_bug.cgi?id=402655

Note You need to log in before you can comment on or make changes to this bug.