Closed Bug 201499 Opened 21 years ago Closed 18 years ago

Tree widgets should support editable content within hierarchical data structures (inline edit)

Categories

(Core :: XUL, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: joshua.johnson, Assigned: enndeakin)

References

(Depends on 2 open bugs, )

Details

Attachments

(1 file, 5 obsolete files)

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
dupe of 96384 ?
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.
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. 
Attached file Workaround by ndeakin (obsolete) —
Attached file Workaround by ndeakin (obsolete) —
Attachment #150432 - Attachment is obsolete: true
(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.
Depends on: 96384
OS: Windows 2000 → All
Hardware: PC → All
Summary: [RFE] Tree widgets should support editable content within hierarchical data structures → Tree widgets should support editable content within hierarchical data structures (inline edit)
Depends on: 218642
Attached file workaround edittree (obsolete) —
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.
I'm now working on an improved version of tree editing now that caret changes have landed.
Assignee: Jan.Varga → enndeakin
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
Attached patch Support inline text editing (obsolete) — Splinter Review
Patch adds simple support for text editing in trees. Additional features may be added by follow up bugs.
Attachment #222043 - Flags: superreview?(neil)
Attachment #222043 - Flags: review?(Jan.Varga)
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.
(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
(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.
Neil D, don't you need selectable cells anymore?
Anyway, I'm going to review this patch.
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.
Status: NEW → ASSIGNED
Attached patch Address review comments. (obsolete) — Splinter Review
I couldn't find a way to remove the extra 1 added to the height and make the textbox the right size.
Attachment #150476 - Attachment is obsolete: true
Attachment #217188 - Attachment is obsolete: true
Attachment #222043 - Attachment is obsolete: true
Attachment #224455 - Flags: superreview?(neil)
Attachment #224455 - Flags: review?(Jan.Varga)
Attachment #222043 - Flags: superreview?(neil)
Attachment #222043 - Flags: review?(Jan.Varga)
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
Attachment #224455 - Flags: review?(Jan.Varga) → review+
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 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.
Attachment #224455 - Flags: superreview?(neil) → superreview-
Added some changes to nsTreeBodyFrame::GetCoordsForCellItem, and fix other review issues
Attachment #224455 - Attachment is obsolete: true
Attachment #225608 - Flags: superreview?(neil)
Attachment #225608 - Flags: review?(Jan.Varga)
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
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 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?
Attachment #225608 - Flags: superreview?(neil) → superreview+
(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
(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 on attachment 225608 [details] [diff] [review]
address review comments

looks good
Attachment #225608 - Flags: review?(Jan.Varga) → review+
This is fixed. Another bug should be filed for editing using a menulist
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
Blocks: 347772
Blocks: 350332
Blocks: 350333
Blocks: 350335
Blocks: 350346
Blocks: 350347
Note: an example and some docs are available at http://wiki.mozilla.org/XUL:Tree and in attachment 235591 [details].
Blocks: 350350
(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?
Tests are in bug 377677.
Flags: in-testsuite-
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
Blocks: 392216
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: shrir → xptoolkit.widgets
Depends on: 469487
Depends on: 777832
You need to log in before you can comment on or make changes to this bug.