Closed
Bug 362680
Opened 18 years ago
Closed 17 years ago
cannot gain focus using keyboard on the only item in a tree
Categories
(Toolkit :: UI Widgets, defect)
Toolkit
UI Widgets
Tracking
()
RESOLVED
FIXED
People
(Reporter: ori, Unassigned)
Details
Attachments
(3 files, 7 obsolete files)
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•18 years ago
|
||
Well, you can focus using the keyboard, just not with the up/down arrow keys. spacebar seems to work for me.
Reporter | ||
Comment 2•18 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 :)
Comment 3•18 years ago
|
||
(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•18 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•18 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•18 years ago
|
||
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•18 years ago
|
||
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•18 years ago
|
Attachment #247672 -
Flags: first-review?(mconnor)
Reporter | ||
Updated•18 years ago
|
Attachment #247672 -
Flags: first-review?(mconnor) → first-review?(enndeakin)
Comment 7•18 years ago
|
||
I suspect Home and End also work.
Comment 8•18 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•18 years ago
|
||
I'll submit a new patch that includes pgup and pgdown. (I thought they worked, but they don't)
Updated•18 years ago
|
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 10•18 years ago
|
||
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•18 years ago
|
Attachment #247969 -
Flags: first-review?(enndeakin) → first-review+
Comment 11•18 years ago
|
||
(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•18 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•17 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•17 years ago
|
||
(In reply to comment #13) I applied Neil's suggestions.
Attachment #247969 -
Attachment is obsolete: true
Attachment #260481 -
Flags: first-review?(enndeakin)
Comment 15•17 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•17 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•17 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•17 years ago
|
||
Reporter | ||
Comment 19•17 years ago
|
||
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•17 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•17 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•17 years ago
|
||
I updated the test-case with an additional empty tree
Attachment #260584 -
Attachment is obsolete: true
Reporter | ||
Comment 23•17 years ago
|
||
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•17 years ago
|
Attachment #262347 -
Attachment is patch: true
Attachment #262347 -
Attachment mime type: text/x-patch → text/plain
Comment 24•17 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•17 years ago
|
||
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•17 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•17 years ago
|
||
(In reply to comment #26) Fixed.
Attachment #265302 -
Attachment is obsolete: true
Attachment #265576 -
Flags: review?(enndeakin)
Updated•17 years ago
|
Attachment #265576 -
Flags: review?(enndeakin) → review+
Comment 28•17 years ago
|
||
Bug 377677 has tree tests that can be used to check this bug.
Reporter | ||
Comment 29•17 years ago
|
||
All of the tests pass.
Reporter | ||
Updated•17 years ago
|
Whiteboard: [checkin-needed]
Updated•17 years ago
|
Whiteboard: [checkin-needed] → [checkin needed]
Comment 30•17 years ago
|
||
mozilla/toolkit/content/widgets/tree.xml 1.45 Is this FIXED now?
Whiteboard: [checkin needed]
Version: unspecified → Trunk
Reporter | ||
Comment 31•17 years ago
|
||
It is. Thanks for the help!
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•