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

RESOLVED FIXED

Status

()

--
minor
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: ori, Unassigned)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 7 obsolete attachments)

2.45 KB, application/vnd.mozilla.xul+xml
Details
6.58 KB, application/vnd.mozilla.xul+xml
Details
18.07 KB, patch
enndeakin
: review+
Details | Diff | Splinter Review
(Reporter)

Description

12 years ago
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.
Well, you can focus using the keyboard, just not with the up/down arrow keys.  spacebar seems to work for me.
(Reporter)

Comment 2

12 years ago
(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 :)
(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.
(Reporter)

Comment 4

12 years ago
(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).
(Reporter)

Updated

12 years ago
Summary: cannot gain focus using keyboard on the only item in a list box → cannot gain focus using keyboard on the only item in a tree
(Reporter)

Comment 5

12 years ago
Created attachment 247671 [details]
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.
(Reporter)

Comment 6

12 years ago
Created attachment 247672 [details] [diff] [review]
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).
(Reporter)

Updated

12 years ago
Attachment #247672 - Flags: first-review?(mconnor)
(Reporter)

Updated

12 years ago
Attachment #247672 - Flags: first-review?(mconnor) → first-review?(enndeakin)
I suspect Home and End also work.

Comment 8

12 years ago
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
Attachment #247672 - Flags: first-review?(enndeakin) → first-review+
(Reporter)

Comment 9

12 years ago
I'll submit a new patch that includes pgup and pgdown. (I thought they worked, but they don't)
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Reporter)

Comment 10

12 years ago
Created attachment 247969 [details] [diff] [review]
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)
Attachment #247672 - Attachment is obsolete: true
Attachment #247969 - Flags: first-review?(enndeakin)

Updated

12 years ago
Attachment #247969 - Flags: first-review?(enndeakin) → first-review+
(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();
(Reporter)

Comment 12

12 years ago
(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)
(Reporter)

Comment 13

12 years ago
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?
(Reporter)

Comment 14

12 years ago
Created attachment 260481 [details] [diff] [review]
Another revised patch that affects lists with more than one item

(In reply to comment #13)
I applied Neil's suggestions.
Attachment #247969 - Attachment is obsolete: true
Attachment #260481 - Flags: first-review?(enndeakin)

Comment 15

12 years ago
(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? 
(Reporter)

Comment 16

12 years ago
(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

12 years ago
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.
Attachment #260481 - Flags: first-review?(enndeakin) → first-review-
(Reporter)

Comment 18

12 years ago
Created attachment 260584 [details]
Same test-case with onselect alerts
(Reporter)

Comment 19

12 years ago
Created attachment 260631 [details] [diff] [review]
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.
Attachment #260481 - Attachment is obsolete: true

Comment 20

12 years ago
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.
(Reporter)

Comment 21

12 years ago
(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
(Reporter)

Comment 22

12 years ago
Created attachment 262346 [details]
Test-case with onselect alerts and an additional empty tree

I updated the test-case with an additional empty tree
Attachment #260584 - Attachment is obsolete: true
(Reporter)

Comment 23

12 years ago
Created attachment 262347 [details] [diff] [review]
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?
Attachment #260631 - Attachment is obsolete: true
Attachment #262347 - Flags: review?(enndeakin)

Updated

12 years ago
Attachment #262347 - Attachment is patch: true
Attachment #262347 - Attachment mime type: text/x-patch → text/plain

Comment 24

12 years ago
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.
Attachment #262347 - Flags: review?(enndeakin) → review-
(Reporter)

Comment 25

12 years ago
Created attachment 265302 [details] [diff] [review]
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.
Attachment #262347 - Attachment is obsolete: true
Attachment #265302 - Flags: review?(enndeakin)

Comment 26

12 years ago
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)
Attachment #265302 - Flags: review?(enndeakin) → review+
(Reporter)

Comment 27

12 years ago
Created attachment 265576 [details] [diff] [review]
Added method underscores and removed redundant 1/-1 conditionals

(In reply to comment #26)
Fixed.
Attachment #265302 - Attachment is obsolete: true
Attachment #265576 - Flags: review?(enndeakin)

Updated

12 years ago
Attachment #265576 - Flags: review?(enndeakin) → review+

Comment 28

12 years ago
Bug 377677 has tree tests that can be used to check this bug.
 
(Reporter)

Comment 29

12 years ago
All of the tests pass.
(Reporter)

Updated

12 years ago
Whiteboard: [checkin-needed]
Whiteboard: [checkin-needed] → [checkin needed]
mozilla/toolkit/content/widgets/tree.xml 	1.45

Is this FIXED now?
Whiteboard: [checkin needed]
Version: unspecified → Trunk
(Reporter)

Comment 31

12 years ago
It is. Thanks for the help!
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
Blocks: 382504
No longer blocks: 382504
You need to log in before you can comment on or make changes to this bug.