362680 - cannot gain focus using keyboard on the only item in a tree

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect)

Description

[Ori Avtalion (salty-horse)]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1) Gecko/20060601 Firefox/2.0 (Ubuntu-edgy)
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.8.1) Gecko/20060601 Firefox/2.0 (Ubuntu-edgy)

Tested with a fresh CVS of 20061203.
When a listbox contains only one item, it is possible to focus on the listbox using the tab key. Then, there is a ghost box surrounding the only item. However, it is not possible to focus on the specific item using the keyboard.

Reproducible: Always

Steps to Reproduce:
1. Open firefox's about:config
2. Filter one specific item.
3. Tab to the list, and try to focus on the item using the keyboard. For example, by pressing the "down" key.

Actual Results:  
No focus is achieved. The ghost selection remains on the item.
Note that even after tabbing away from the list box, the ghost selection remains as if the list box has the focus

Expected Results:  
The down or up keys should cause the item in the list box to gain focus.

bug 230904 *may* be related.

Comment 1

[Bill Gianopoulos [:WG9s]]

Well, you can focus using the keyboard, just not with the up/down arrow keys.  spacebar seems to work for me.

Comment 2

[Ori Avtalion (salty-horse)]

(In reply to comment #1)
> Well, you can focus using the keyboard, just not with the up/down arrow keys. 
> spacebar seems to work for me.
> 

I haven't noticed that before. Still, space is not very intuitive (to me, at least), and is a bit problematic:
In Thunderbird, for example, when I pressed space on such an item, it both focused on it, and sent the space event forward. The result was that it asked me if I wanted to continue reading mail in another folder before I even got a chance to read the current one (I guess that's another bug :)

Comment 3

[Bill Gianopoulos [:WG9s]]

(In reply to comment #2)
> (In reply to comment #1)
> > Well, you can focus using the keyboard, just not with the up/down arrow keys. 
> > spacebar seems to work for me.
> > 
> 
> I haven't noticed that before. Still, space is not very intuitive (to me, at
> least), and is a bit problematic:
> In Thunderbird, for example, when I pressed space on such an item, it both
> focused on it, and sent the space event forward. The result was that it asked
> me if I wanted to continue reading mail in another folder before I even got a
> chance to read the current one (I guess that's another bug :)
> 

Ah,sorry.  Didn't notice you were running Linux.  The Windows convention is that space on a focused item is equivalent to a single mouseclick.

Comment 4

[Ori Avtalion (salty-horse)]

(In reply to comment #3)
> Ah,sorry.  Didn't notice you were running Linux.  The Windows convention is
> that space on a focused item is equivalent to a single mouseclick.
> 

In Windows, the up/down keys also work.

I don't think it's an OS issue (meaning, it shouldn't be customized to work differently on each platform).

Comment 5

[Ori Avtalion (salty-horse)]

Attachment: Testcase with a single-item and multiple-item tree

Attached a testcase. Use the keyboard to focus on the first tree. Then try to select it using the keyboard. Only space and pgup/pgdown will work.

Comment 6

[Ori Avtalion (salty-horse)]

Attachment: Patch allows selection of the single item using the up/down keys, with or without the shift key

Attached a patch that allows selection of the single item with the arrow keys.
Test using the attached testcase (which includes a regular tree to see if I didn't break anything).

Comment 7

[[e-mail]]

I suspect Home and End also work.

Comment 8

[Neil Deakin]

Comment on attachment 247672 [details] [diff] [review]
Patch allows selection of the single item using the up/down keys, with or without the shift key

>-         var c = this.currentIndex - 1;
>-         if (c < 0)
>-           return;
>+         if (this.view.rowCount == 1)
>+           var c = 0;
>+         else {
>+           var c = this.currentIndex - 1;
>+           if (c < 0)
>+             return;
>+         }
> 

This looks weird with c declared twice, and with braces only on the else block. Might be more readable as:

var c = 0;
if (this.view.rowCount > 1) {
  c = this.currentIndex - 1;
  if (c < 0)
    return;
}

>          var cellSelType = this._cellSelType;
>          if (cellSelType) {
>@@ -594,12 +598,16 @@
>            this.treeBoxObject.scrollByLines(1);
>            return;
>          }
>-         var c = this.currentIndex + 1;
>-         if (c == this.view.rowCount)
>-           return;
> 
>-         var cellSelType = this._cellSelType;
>+         if (this.view.rowCount == 1)
>+           var c = 0;
>+         else {
>+           var c = this.currentIndex + 1;
>+           if (c == this.view.rowCount)
>+             return;
>+         }

Similarly, here.

Otherwise, it looks ok

Comment 9

[Ori Avtalion (salty-horse)]

I'll submit a new patch that includes pgup and pgdown. (I thought they worked, but they don't)

Comment 10

[Ori Avtalion (salty-horse)]

Attachment: Revised patch

Attached a new patch.
Is it possible to reuse the keyboard navigation code from listbox.xml? It's much cleaner and is free of many focus problems the tree widget has (although that's not related to keyboard navigation)

Comment 11

[[e-mail]]

(In reply to comment #8)
>This looks weird with ... braces only on the else block.
What really freaks me out is when there are no braces on the else block ;-)
if (cond) {
  stuff();
  more.stuff();
} else
  other.stuff();

Comment 12

[Ori Avtalion (salty-horse)]

(In reply to comment #11)
> What really freaks me out is when there are no braces on the else block ;-)

Are you sure we're both looking at attachment 247969 [details] [diff] [review]? there is no "else" in sight ;;-) (double wink)

Comment 13

[Ori Avtalion (salty-horse)]

Neil has suggested on IRC that the up arrow will select the (outlined) first item in the list even if it's not the only one.

This is not how Windows behaves, but it is how XUL lists behave (as well as GNOME) and the intuitive behavior.

Should I add that to the patch?

Comment 14

[Ori Avtalion (salty-horse)]

Attachment: Another revised patch that affects lists with more than one item

(In reply to comment #13)
I applied Neil's suggestions.

Comment 15

[Neil Deakin]

(In reply to comment #14)
> Created an attachment (id=260481) [details]
> Another revised patch that affects lists with more than one item

What behaviour is different in this patch from the last one? 

Comment 16

[Ori Avtalion (salty-horse)]

(In reply to comment #15)
Now, in a *multiple-item* list, when the first/last items have a outline selection, the up/down keys will select those items.

To see the effect, use the second tree widget in the test case.
Tab to it and press up to see that the first element is selected.
For the last element: Reload, tab to it, hold ctrl and press down until the outline is on the last element and then release the ctrl key and press down again to select the last element.

Comment 17

[Neil Deakin]

Comment on attachment 260481 [details] [diff] [review]
Another revised patch that affects lists with more than one item

No, this will cause a select event to fire when the first or last row is selected and the up/down keys are pressed.

Comment 18

[Ori Avtalion (salty-horse)]

Attachment: Same test-case with onselect alerts

Comment 19

[Ori Avtalion (salty-horse)]

Attachment: New patch - moved handler code to methods

I fixed the problems with the previous patch.

The new one also moves the up/down, pgup/pgdn and home/end into seperate methods.

I also fixed more bugs related to the "selected" event. i.e. home/end now don't raise the event when pressed repeatedly.

Another bug that I noted and fixed happened when you press pgdown after having the last item in the list selected along with other items. pgdown now selects the last item exclusively.

After testing it with the new test-case, you should add more items to the list to check the page scrolling.

Comment 20

[Neil Deakin]

Some comments from a brief look, I didn't review the patch in detail

- separate functions should be used for the shift state and the non-shift state.
- true/false should be used for boolean arguments, not 1/0
- what happens when the keys are used in a tree with no rows? It looks as if the code doesn't handle this case

+      <method name="moveByOffset">
...
+            if (shiftPressed) {
...
+              var c = this.currentIndex;
+              if (c == edge + offset)
+                return;

I don't think this condition can ever be true.

+      <method name="moveByPage">
+        if (this.view.rowCount == 1 && !this.view.selection.isSelected(0)) {
+          this.view.selection.timedSelect(0, this._selectDelay);
+          return;
+        }

when pageUpOrDownMovesSelection is set, the selection shouldn't be changed.


You may wish to try this with the tree tests in bug 368097, although they would need to be modified to handle modified selection behaviour.

After looking at this, I'm not sure it's worth consilidating the code as it doesn't really reduce the amount of code used by much.

Comment 21

[Ori Avtalion (salty-horse)]

(In reply to comment #20)
> - what happens when the keys are used in a tree with no rows? It looks as if
> the code doesn't handle this case
I'll fix it.
* Hmm.. Why does view.rowCount return 1 in these cases?
* I noticed home/end select the "empty" row in the original code.

> After looking at this, I'm not sure it's worth consilidating the code as it
> doesn't really reduce the amount of code used by much.
It may not seem so due to function declarations and a bit more logic that I added, but it does reduce duplication.

I'll address your comments in the new patch

Comment 22

[Ori Avtalion (salty-horse)]

Attachment: Test-case with onselect alerts and an additional empty tree

I updated the test-case with an additional empty tree

Comment 23

[Ori Avtalion (salty-horse)]

Attachment: Yet another patch - see comment

Please ignore my rowCount question in comment #21. I tested it on a tree with a single empty row.

Changes from the previous patch:
* Seperate functions for the shift state.
* Fixed the wrong behavior of shift+pgup/down.
* Fixed the behavior in an empty tree.
* Fixed some wrong triggering of the onselect event when a tree with a single item is already selected.
* Fixed the behavior of ctrl+pgup/down in a multi-element list. It used to scroll the view but did not update the currentIndex.

(In reply to comment #20)
>You may wish to try this with the tree tests in bug 368097, although they would
>need to be modified to handle modified selection behaviour.

I don't know how to run the test suite in mozilla - any guides?

Comment 24

[Neil Deakin]

Comment on attachment 262347 [details] [diff] [review]
Yet another patch - see comment

 
+      <method name="moveByOffset">
...
+            var c = 0;
+            c = this.currentIndex + offset;

var c = this.currentIndex + offset;
 
+            if (offset > 0 ? c > edge : c < edge) {
+              if (!this.view.selection.isSelected(edge) ||
+                   (this.view.selection.isSelected(edge) && this.view.selection.count > 1))
+                c = edge;
+              else
+                return;
+            }

This can be written easier as:

if (offset > 0 ? c > edge : c < edge) {
  if (this.view.selection.isSelected(edge) && this.view.selection.count <= 1)
    return;
  c = edge;
}

+            if (!this._isAccelPressed(event))
+              this.view.selection.timedSelect(c, this._selectDelay);
+            else // Ctrl+Up moves the anchor without selecting

Change the comment here  to Ctrl+Up/Down

+      <method name="moveByOffsetShift">
...
+            if (this.view.selection.single) {
+              this.treeBoxObject.scrollByLines(-1);
+              return;
+            }

What is this for?

+            if (this.view.rowCount == 1 && !this.view.selection.isSelected(0)) {
+              this.view.selection.timedSelect(0, this._selectDelay);
+              return;
+            }
+      
+            var c = this.currentIndex;
+            if (c == edge) {
+              if (this.view.selection.isSelected(c))
+                return;
+            }

What about when currentIndex is -1?

+      <method name="moveByPage">
+        <parameter name="offset"/>
+        <parameter name="edge"/>
+        <parameter name="event"/>
+        <body>
+          <![CDATA[
+            if (this._editingColumn)
+              return;
+
+            if (this.view.rowCount == 1 && !this.view.selection.isSelected(0) &&
+                !(this.pageUpOrDownMovesSelection == this._isAccelPressed(event))) {
+              this.view.selection.timedSelect(0, this._selectDelay);
+              return;
+            }
+
+            var c = this.currentIndex;
+            if (this.view.rowCount == 0 || c == -1)
+              return;

The rowCount == 0 check should just be at the top next to the _editingColumn check

+
+            if (c == edge && this.view.selection.isSelected(c)) {
+              if (this.view.selection.count > 1)
+                if (this.pageUpOrDownMovesSelection == this._isAccelPressed(event))
+                  this.currentIndex = c;
+                else
+                  this.view.selection.timedSelect(c, this._selectDelay);
+

When (this.pageUpOrDownMovesSelection = this._isAccelPressed(event)) is false, the view
should be scrolled only, but the current index or selection should not be changed at all.

And why the call to timedSelect? The first condition has already tested that c is selected.

+              this.treeBoxObject.ensureRowIsVisible(c);
+              return;
+            }
+            var i = this.treeBoxObject.getFirstVisibleRow();

The existing PageDown code sets i to this.treeBoxObject.getFirstVisibleRow() + p - 1

+              if (c <= i) {
+                 var p = this.treeBoxObject.getPageLength();

p is already determined above so this line can be removed

+            if (this.pageUpOrDownMovesSelection == this._isAccelPressed(event))
+              this.currentIndex = i;

Similar issue as above here

+
+      <method name="moveByPageShift">
...
+            if (this._editingColumn)
+              return;

probably should add a rowCount == 0 check here too

+            var i = this.treeBoxObject.getFirstVisibleRow();

Similar as above. The existing PageDown code sets i to this.treeBoxObject.getFirstVisibleRow() + p - 1

+
+      <method name="moveToEdge">
+        <parameter name="edge"/>
+        <parameter name="event"/>
+        <body>
+          <![CDATA[
+            if (this._editingColumn || this.view.rowCount == 0)
+              return;
+
+            if (this.view.rowCount == 1 && !this.view.selection.isSelected(0) &&
+                !this._isAccelPressed(event)) {
+              this.view.selection.timedSelect(0, this._selectDelay);
+              return;
+            }
+

Why is this code needed? Wouldn't the isAccelPressed block below this handle this case?

+      <method name="moveToEdgeShift">
...
+            if (this.view.selection.isSelected(edge))
+              return;
+
+            if (this.view.selection.single)
+              return;
+

Combine into:

if (this.view.selection.single || this.view.selection.isSelected(edge))
  return;

I'll run the tests when a new patch is made. Right now the test fails some of the pageup/pagedown cases but that could be caused by the issues above or expected behaviour changes. The rest of the tests pass ok.

Comment 25

[Ori Avtalion (salty-horse)]

Attachment: Another patch

I fixed another problem introduced with the last patch:
When clicking on home/end+shift when the respective edge was selected, nothing would happen.
Now it correctly selects the focus-to-edge area when the focus is outside of the current selection.
See moveToEdgeShift.

(In reply to comment #24)

>+      <method name="moveByOffsetShift">
>...
>+            if (this.view.selection.single) {
>+              this.treeBoxObject.scrollByLines(-1);
>+              return;
>+            }
>
>What is this for?

It makes "shift" behave like "ctrl" (scroll the list) in a single-selection tree - it's in the original code.
The -1 should be 1 or -1 depending on the context. I fixed that.

>+            if (this.view.rowCount == 1 && !this.view.selection.isSelected(0)) {
>+              this.view.selection.timedSelect(0, this._selectDelay);
>+              return;
>+            }
>+      
>+            var c = this.currentIndex;
>+            if (c == edge) {
>+              if (this.view.selection.isSelected(c))
>+                return;
>+            }
>
>What about when currentIndex is -1?

In that case I'm changing to 0 - in the new patch

>+
>+            if (c == edge && this.view.selection.isSelected(c)) {
>+              if (this.view.selection.count > 1)
>+                if (this.pageUpOrDownMovesSelection == this._isAccelPressed(event))
>+                  this.currentIndex = c;
>+                else
>+                  this.view.selection.timedSelect(c, this._selectDelay);
>+
>
>When (this.pageUpOrDownMovesSelection = this._isAccelPressed(event)) is false,
>the view
>should be scrolled only, but the current index or selection should not be
>changed at all.

(this.pageUpOrDownMovesSelection = this._isAccelPressed(event)) was handled above that code block.
In the new patch I only check if ctrl is pressed.

>And why the call to timedSelect? The first condition has already tested that c
>is selected.

timedSelect() removes the selection from the other elements, leaving just the edge.
I'll remove this for now, as it conflicts with how ctrl+pageup/down is assumed to behave.

>+            if (this.pageUpOrDownMovesSelection ==
>this._isAccelPressed(event))
>+              this.currentIndex = i;
>
>Similar issue as above here

Removing this as well.

Comment 26

[Neil Deakin]

Comment on attachment 265302 [details] [diff] [review]
Another patch

>+      <method name="moveByPage">
(this.pageUpOrDownMovesSelection == this._isAccelPressed(event)) {
>+               this.treeBoxObject.scrollByPages(offset > 0 ? 1 : -1);
>+               return;
>+            }

offset is only ever 1 or -1 so just pass offset directly.

Also, precede all the methods you added with underscores as they aren't intended to be public. (_moveByOffset, etc)

Comment 27

[Ori Avtalion (salty-horse)]

Attachment: Added method underscores and removed redundant 1/-1 conditionals

(In reply to comment #26)
Fixed.

Comment 28

[Neil Deakin]

Bug 377677 has tree tests that can be used to check this bug.
 

Comment 29

[Ori Avtalion (salty-horse)]

All of the tests pass.

Comment 30

[:Gavin Sharp [email: [e-mail]]]

mozilla/toolkit/content/widgets/tree.xml 	1.45

Is this FIXED now?

Comment 31

[Ori Avtalion (salty-horse)]

It is. Thanks for the help!