473412 - Editable tree cells cannot be activated by a keystroke

Status: RESOLVED FIXED

(Toolkit :: XUL Widgets, defect)

Description

[Alex Vincent [:WeirdAl]]

We can enter editing of a tree cell by double-clicking on it, but not by any key stroke combinations.

The ENTER or RETURN keys during editing cause an exit from editing mode.  These keys, when not in editing mode, usually toggle whether a tree row is open or not.

I'm willing to write up a patch (though I might need help with creating a mochitest).  I'd suggest SHIFT+ENTER for entering editing mode.  Anyone else have good ideas?

Comment 1

[James Teh [:Jamie]]

On Windows, f2 seems to be the standard keystroke for editing the current item where enter performs some other action. For example, it is used in SysListView32 controls (including in Windows Explorer) to edit the current list item. It is also used in Microsoft Excel to edit the content of the current cell.

Shift+enter or ctrl+enter are often used to insert a line break when editing where enter would normally dismiss the editor. Although we are discussing the keystroke to begin editing here, it *might* be confusing to have the same keystroke to begin editing and subsequently use the same keystroke to insert a line break. (Personally, this probably wouldn't bother me, but it's worth mentioning.)

Alt+enter is normally used to bring up a properties dialog for the current item.

I'd recommend that f2 be used as the keystroke in Windows. If there are other keystrokes for other operating systems and some consistency is desired, perhaps multiple keystrokes could be allowed.

Comment 2

[Alex Vincent [:WeirdAl]]

Attachment: patch using F2 (no test included yet)

I note this file is preprocessed, so if other operating systems have other preferences, it will be easy to add them.

Help wanted writing a test for this, please!

Comment 3

[Marco Zehe (:MarcoZ)]

On OS X, Enter is used in Finder to start a rename of an item. Left and Right close and open collapsed item respectively, and Cmd+O opens the current item.
If we want consistency with the different operating systems, we should perhaps make the keystroke OS specific?

Comment 4

[Alex Vincent [:WeirdAl]]

Mrs. Diggs, your colleagues on #accessibility nominated you to speak for Linux as to the desired key combination.

Comment 5

[Neil Deakin]

Comment on attachment 356908 [details] [diff] [review]
patch using F2 (no test included yet)

You can write a test for this by adding some code into testtag_tree() in toolkit/content/tests/widgets/tree_shared.js. Several tree related xul files share this test script. The synthesizeKey function can be used to simulate keypresses.

Comment 6

[Neil Deakin]

Comment on attachment 356908 [details] [diff] [review]
patch using F2 (no test included yet)


>+      <handler event="keypress" keycode="VK_F2">
>+        <![CDATA[
>+          var cellSelType = this._cellSelType;
>+          if (!cellSelType)
>+            return;
>+          var row = this.currentIndex;
>+          if (row < 0)
>+            return;
>+          var column = this.view.selection.currentColumn;
>+          this.startEditing(row, column);
>+        ]]>
>+      </handler>

This needs to check if the cell is already being edited, and do nothing in this situation.

On Mac, we should use the enter key for editing at least.

Also, this has exposed an underlying bug. The double-click handler checks this.parentNode.editable to see if the tree is editable. The F2 handler doesn't here. In reality, this check should be done inside startEditing. Why don't we fix that while we're at it too.

Comment 7

[Alex Vincent [:WeirdAl]]

Neil: Thanks for review comments.

(In reply to comment #6)
> On Mac, we should use the enter key for editing at least.

- As I understand it, the ENTER key toggles the row's open state, on all platforms.  (Note I haven't tested anything on the Mac in months - I'm going by code inspection on tree.xml.)  Isn't this a conflict of keystrokes?

Comment 8

[Marco Zehe (:MarcoZ)]

(In reply to comment #7)
> - As I understand it, the ENTER key toggles the row's open state, on all
> platforms.

And on all thre platforms it's inconsistent. On Windows TreeViews, Left closes, Right opens. On Linux Tree Tables: The same. In mac tables: The same. No other application besides Thunderbird that I know uses Enter to expand or collapse a tree item. Moreover, in Thunderbird, Enter both expands a Newsgroup thread, for example, and in the same strike opens the selected message in a new window.

If while you're here you could see if you can address that, I believe many people will appreciate that.

Comment 9

[Alex Vincent [:WeirdAl]]

Attachment: XUL reference testcase

I'm attaching this XUL reference testcase to help us all be clear what we're talking about.  There's no keystroke modifications in this test.  The goal is to see how trees react to different keystrokes.

I had considered using the testcase's key event listener to model different behaviors, to see what the expected behavior would be, but I see now that it's already more complex than what I understood.  This will cause me to re-examine this bug's comments in a new light.

Comment 10

[Alex Vincent [:WeirdAl]]

Attachment: patch with ENTER used to turn on editing

My patch for this bug works, but I noticed another problem using the XUL reference testcase.

(1) In the lower right corner, navigate to the Item 1 row and expand it.
(2) Select the cell which says "3".
(3) Hit ENTER to start editing, and set a new value.
(4) Use the DOWN key to select the next cell down.
-- note here the tree selected the next cell down, but focus is no longer on the tree.
(5) Try using the UP key to select the cell you just edited.
-- you find you can't.  Instead, the text below all the trees now has focus.

I think that's a separate bug, but one that should be fixed soon.

Comment 11

[Alex Vincent [:WeirdAl]]

(In reply to comment #10)
> My patch for this bug works, but I noticed another problem using the XUL
> reference testcase.

The behavior I described is a definite regression from FF3.0 - filed as bug 475293.  Please feel free to review independently of that behavior.

Comment 12

[Neil Deakin]

Comment on attachment 358786 [details] [diff] [review]
patch with ENTER used to turn on editing

startEditing already checks the first and last of the conditions so no need to check them again.

I actually meant to use the return/enter key only on Mac and leave F2 the key for other platforms.

Also, I don't think Mozilla ever fires VK_ENTER. The enter key just maps to the same as the return key. But that's not really something to change in this bug.

>+      <method name="_handleEnter">
>+        <parameter name="event"/>
>+        <body><![CDATA[
>+          if (this._editingColumn) {
>+            this.stopEditing(true);
>+            this.focus();
>+            event.stopPropagation();
>+            return;
>+          }
>+
>+          // See if we can edit the cell.
>+          var row = this.currentIndex;
>+          if (this.editable && this._cellSelType && (row >= 0))

Comment 13

[Neil Deakin]

The first line of my comment was supposed to be for the quoted part of the patch.

Comment 14

[Neil Deakin]

Comment on attachment 358786 [details] [diff] [review]
patch with ENTER used to turn on editing

minus for now anyway, but almost looks good.

Comment 15

[Alex Vincent [:WeirdAl]]

Attachment: patch, with F2 for non-Mac, ENTER for Mac

Sorry for the delay in getting this done.

Comment 16

[Neil Deakin]

Comment on attachment 390428 [details] [diff] [review]
patch, with F2 for non-Mac, ENTER for Mac

>+          var cellSelType = this._cellSelType;
>+          if (!cellSelType)
>+            return;
>+          var row = this.currentIndex;
>+          if (row < 0)
>+            return;

No real need to check this here; it's done by startEditing.
 
>-      <handler event="keypress" keycode="VK_RETURN">
...
>-          event.stopPropagation();
>-          event.preventDefault();

You removed the event cancellation without adding it to _handleEnter.

Otherwise it looks ok.

Comment 17

[Alex Vincent [:WeirdAl]]

Attachment: patch, with F2 for non-Mac, ENTER for Mac

Nits fixed.  I left in the _cellSelType check as startEditing didn't check that.

Comment 18

[Neil Deakin]

(In reply to comment #17)
> Created an attachment (id=391190) [details]
> patch, with F2 for non-Mac, ENTER for Mac
> 
> Nits fixed.  I left in the _cellSelType check as startEditing didn't check
> that.

Close, but stopPropagation/preventDefault isn't being called in the same cases as before. Before, it was called in every case except when changeOpenState returned false. I think it would be easiest to have _handleEnter return true in all cases except this one and have the two callers call stopPropagation/preventDefault as needed.

Comment 19

[Alex Vincent [:WeirdAl]]

Neil, could you do me a favor, please?  Since we're so close, but I can't quite see what you're hinting at, could you write up the next patch and seek r/sr from another person?  I've gotten rather sick over the last few days and need to take care of myself.

It almost looks like we could move the stopPropagation/preventDefault calls to the beginning of the handleEnter method - almost.

Comment 20

[Alex Vincent [:WeirdAl]]

I really want to see this make 1.9.2 if feasible.

Comment 21

[Alex Vincent [:WeirdAl]]

Attachment: patch, nit fixed

Sorry for the lengthy delay, folks.

Comment 22

[Neil Deakin]

Comment on attachment 420503 [details] [diff] [review]
patch, nit fixed

Looks good. A test is needed though.

Comment 23

[Alex Vincent [:WeirdAl]]

Comment on attachment 420503 [details] [diff] [review]
patch, nit fixed

Neil: I'm assuming based on your r+ that the testcase can be submitted as a separate patch - please correct me if I'm wrong.

Comment 24

[Alex Vincent [:WeirdAl]]

http://hg.mozilla.org/mozilla-central/annotate/7a7d0da67610/toolkit/content/tests/widgets/tree_shared.js#l625 looks like just about the right place to place a test.

Comment 25

[Robert Strong (they/them - no direct email)]

Comment on attachment 420503 [details] [diff] [review]
patch, nit fixed

Please request sr after the test is written

Comment 26

[Alex Vincent [:WeirdAl]]

Comment on attachment 420503 [details] [diff] [review]
patch, nit fixed

This patch causes regressions in test_tree_hier_cell.xul.  I don't know why yet.

Comment 27

[Alex Vincent [:WeirdAl]]

(In reply to comment #26)
> (From update of attachment 420503 [details] [diff] [review])
> This patch causes regressions in test_tree_hier_cell.xul.

The tree wasn't editable when we called startEditing, at http://hg.mozilla.org/releases/mozilla-1.9.2/annotate/5d7909e22a27/toolkit/content/tests/widgets/tree_shared.js#l68

That's a direct result of this change:
       <method name="startEditing">
         <parameter name="row"/>
         <parameter name="column"/>
         <body>
           <![CDATA[
+            if (!this.editable)
+              return false;

I believe the test is incorrect in this respect:  if the tree isn't editable, startEditing shouldn't do anything.  Neil, you state the same in comment 6.  I'll fix the test as part of the next patch.

Comment 28

[Alex Vincent [:WeirdAl]]

Attachment: tests for keystrokes, also checking what startEditing returns

Comment 29

[Alex Vincent [:WeirdAl]]

Comment on attachment 420503 [details] [diff] [review]
patch, nit fixed

answered requirement from comment 25 with comment 28.

Comment 30

[Alex Vincent [:WeirdAl]]

FYI: I tested by running the following:
python _tests/testing/mochitest/runtests.py --test-path=toolkit/content/tests/widgets/

That way, we hit all the tree tests, not just the one or two I directly looked at.

Comment 31

[Neil Deakin]

Comment on attachment 424444 [details] [diff] [review]
tests for keystrokes, also checking what startEditing returns

> 
>+  is(tree.editable, editable, "editable (continued)");
>+  // currently, the editable flag means that tree editing cannot be invoked
>+  // by the user. However, editing can still be started with a script.
>+  is(tree.editingRow, -1, testid + " initial editingRow (continued)");
>+  is(tree.editingColumn, null, testid + " initial editingColumn (continued)");
>+
>   var ecolumn = tree.columns[0];
>-  tree.startEditing(1, ecolumn);
>+  ok(!tree.startEditing(1, ecolumn), "non-editable trees shouldn't start editing");
>+  

These few lines seem to rely on a previous part of code making the tree non-editable (I think if I'm reading it right). Could you be more specific and set editable to false before hand so we don't rely on other behaviour that might change.

Comment 32

[Alex Vincent [:WeirdAl]]

(In reply to comment #31)
> These few lines seem to rely on a previous part of code making the tree
> non-editable (I think if I'm reading it right). Could you be more specific and
> set editable to false before hand so we don't rely on other behaviour that
> might change.

I can do that.  Currently the scope of the function sets editable = false (tree_shared.js line 34) and never changes it... I'm inclined to think it'd be safer to explicitly replace the editable variable with the false literal (in this function only).

Also, the last line of testtag_tree_UI_editing, after this patch, explicitly forces the tree to be non-editable.  I also eliminated the only early return from this function, so (aside from an exception that would get logged as a failure) there's no way the tree will be editable when we exit the function.

I'll post a new patch this weekend - if you disagree with my comments here, please say so right away.

Comment 33

[Neil Deakin]

Alex, are you still planning to update this patch?

Comment 34

[Alex Vincent [:WeirdAl]]

(In reply to comment #33)
> Alex, are you still planning to update this patch?

Yes.  I got dreadfully sick this past weekend, or I would've had it done by now.  However, if you or anyone else has free cycles, feel free to do the update on your own.

Comment 35

[Alex Vincent [:WeirdAl]]

Attachment: tests for keystrokes, also checking what startEditing returns (v2)

Comment 36

[Neil Deakin]

Comment on attachment 427920 [details] [diff] [review]
tests for keystrokes, also checking what startEditing returns (v2)

>+  // currently, the editable flag means that tree editing cannot be invoked
>+  // by the user. However, editing can still be started with a script.
>+  is(tree.editingRow, -1, testid + " initial editingRow (continued)");
>+  is(tree.editingColumn, null, testid + " initial editingColumn (continued)");
>+
>   var ecolumn = tree.columns[0];
>-  tree.startEditing(1, ecolumn);
>+  ok(!tree.startEditing(1, ecolumn), "non-editable trees shouldn't start editing");

I'd check editingRow and editingColumn as well here just to be sure.


>+  // Test whether a keystroke can enter text entry, and another can exit.
>+  if (tree.selType == "cell")
>+  {
>+    tree.stopEditing(false);
>+    ok(!tree.editingColumn, "Should not be editing tree cell now");
>+    var [prevRow, prevColumn] = [tree.currentIndex, tree.view.selection.currentColumn];
>+    tree.view.selection.currentColumn = ecolumn;
>+    tree.currentIndex = rowIndex;
>+
>+    const isMac = (navigator.platform.indexOf("Mac") >= 0);
>+    const StartEditingKey = isMac ? "VK_ENTER" : "VK_F2";
>+    synthesizeKey(StartEditingKey, {});
>+    ok(tree.editingColumn, "Should be editing tree cell now");

Use is(tree.editingColumn, ecolumn) here instead


>+    synthesizeKey("VK_ESCAPE", {});
>+    ok(!tree.editingColumn, "Should not be editing tree cell now");
>+    [tree.currentIndex, tree.view.selection.currentColumn] = [prevRow, prevColumn];

The row and column shouldn't have changed should it?


>+  if (0) // XXXndeakin disable these tests for now
>+  {
>+    tree.startEditing(0, ecolumn);
>+    inputField.value = "Value for Return";
>+    synthesizeKey("VK_RETURN", {});
>+    is(tree.view.getCellText(0, ecolumn), "Value for Return", testid + "edit cell return");

I'd just remove all the stuff in the if (0) block since you're testing 
above.

Comment 37

[Alex Vincent [:WeirdAl]]

Attachment: tests for keystrokes, also checking what startEditing returns (v3)

Comment 38

[Neil Deakin]

Comment on attachment 429078 [details] [diff] [review]
tests for keystrokes, also checking what startEditing returns (v3)

>+    tree.stopEditing(false);
>+    ok(!tree.editingColumn, "Should not be editing tree cell now");
>+    var [prevRow, prevColumn] = [tree.currentIndex, tree.view.selection.currentColumn];

Don't need this line anymore.

Otherwise, this looks good!

Comment 39

[Robert Strong (they/them - no direct email)]

Comment on attachment 420503 [details] [diff] [review]
patch, nit fixed

>diff --git a/toolkit/content/widgets/tree.xml b/toolkit/content/widgets/tree.xml
>--- a/toolkit/content/widgets/tree.xml
>+++ b/toolkit/content/widgets/tree.xml
>...
>@@ -651,16 +655,41 @@
>             // Extend the selection from the existing pivot, if any.
>             // -1 doesn't work here, so using currentIndex instead
>             this.view.selection.rangedSelect(this.currentIndex, edge, this._isAccelPressed(event));
> 
>             this.treeBoxObject.ensureRowIsVisible(edge);
>           ]]>
>         </body>
>       </method>
>+      <method name="_handleEnter">
>+        <parameter name="event"/>
>+        <body><![CDATA[
>+          if (this._editingColumn) {
>+            this.stopEditing(true);
>+            this.focus();
>+            return true;
>+          }
>+
>+#ifdef XP_MACOSX
>+          // See if we can edit the cell.
>+          var row = this.currentIndex;
>+          if (this._cellSelType)
>+          {
nit: be consistent with the bracing style here and elsewhere

>+            var column = this.view.selection.currentColumn;
>+            var startedEditing = this.startEditing(row, column);
>+            if (startedEditing)
>+            {
nit: no braces for this case seems to be the prevailing style

>+              return true;
>+            }
>+          }
>+#endif
>+          return this.changeOpenState(this.currentIndex);
>+        ]]></body>
>+      </method>
>     </implementation>
>     
>     <handlers>
>       <handler event="DOMMouseScroll">
>         <![CDATA[
>           if (this._editingColumn)
>             return;
>           if (event.axis == event.HORIZONTAL_AXIS)
>@@ -703,44 +732,44 @@
>             var col = this._getNextColumn(this.currentIndex, false);
>             this.view.selection.currentColumn = col;
>           }
>         ]]>
>       </handler>
>       <handler event="blur" action="this.treeBoxObject.focused = false;"/>
>       <handler event="blur" phase="capturing"
>                action="if (event.originalTarget == this.inputField.inputField) this.stopEditing(true);"/>
>+
nit: did you add this blank line on purpose?

>       <handler event="keypress" keycode="VK_ENTER">
>-        <![CDATA[
>-          if (this._editingColumn) {
>-            this.stopEditing(true);
>-            this.focus();
>-          }
>-          else {
>-            if (!this.changeOpenState(this.currentIndex))
>-              return; // don't consume the event if the open state wasn't changed
>-          }
>+        if (this._handleEnter(event))
>+        {
nit: be consistent with the bracing style here and elsewhere

>           event.stopPropagation();
>           event.preventDefault();
>+        }
>+      </handler>
>+      <handler event="keypress" keycode="VK_RETURN">
>+        if (this._handleEnter(event))
>+        {
nit: be consistent with the bracing style here and elsewhere

r+sr=me with changes to be consistent with the prevailing style

Comment 40

[Alex Vincent [:WeirdAl]]

I'll post a patch combining code changes, test, and review nits as soon as possible - likely this weekend.  Thanks for reviews!

Comment 41

[Alex Vincent [:WeirdAl]]

Attachment: final combined patch for check-in

Comment 42

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/268778c073a6