Type letters to navigate XUL tree

VERIFIED FIXED

Status

()

--
enhancement
VERIFIED FIXED
17 years ago
11 years ago

People

(Reporter: aaronlev, Assigned: yuanyi21)

Tracking

({access})

Trunk
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Reporter)

Description

17 years ago
assume user press 'A', and:
1. the list has neither shortcut key 'A', nor first letter 'A' 
   -- do nothing
2. the list has only one item which has either shortcut key 'A' or first 
letter 'A' 
   -- select it and do its action
3. the list has more than one item that has either shortcut key 'A' or first 
letter 'A' 
   -- go circularly within them but do not act them

For condition 3, we should also support incremental typing (IE's Favorities 
does not support).
(Reporter)

Comment 1

17 years ago
Tree and outliner are undergoing major revision. There is no sense working on
this until that is finished.
Depends on: 110156
(Assignee)

Comment 2

17 years ago
Oh, I've done a no sence patch :(

Should I put any patches here?
(Reporter)

Comment 3

17 years ago
Kyle, it makes sense to wait until bug 110156 is fixed. Until then, you should
not work on any patches for this bug.
Keywords: access

Comment 4

17 years ago
What about bug 104503 ?
(Reporter)

Comment 5

17 years ago
Hmmm, didn't see that one before.

We also have bug 133365 for listbox.

I suggest we make bug 104503 the outliner bug, and keep bug 133365 a separate
bug for listbox.

Kyle Yuan is going to be working on these.

Comment 6

17 years ago
Do outliner items ever have "shortcut keys" or was that inadvertently copied
from the spec for menus?

When an outliner has multiple columns, what should Mozilla do?  Here are several
possibilities:
1. Use the leftmost text column
2. Use the widest text column
3. Use a specific column, specified by the outliner
4. Use the column by which the user has sorted the rows of the outliner
5. Search in all columns

3 and 5 make the most sense to me, but I haven't thought about it much or tried
other programs.

Comment 7

17 years ago
*** Bug 133950 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 8

17 years ago
I think 1 is good enough for the most user requirement. Of course 3 and 4 are 
better if they can be implemented. If the current active(focus) column can be 
determined, we should navigate by this column first. Otherwise, we should use 
the first column.

I dont like 5, it will confuse me where the focus will go next.

Comment 9

17 years ago
I would prefer having 1,3,4.
Also look at mpt's proposal in bug 104503.
(Assignee)

Comment 10

17 years ago
Since bug 110156 fixed, we should change the widget's name.
"outliner" -> "tree"
Summary: Type letters to navigate XUL outliner → Type letters to navigate XUL tree
(Assignee)

Comment 11

17 years ago
Created attachment 77198 [details] [diff] [review]
patch to evaluate


A new method was added into nsITreeBoxObject.idl -- getColumnID.
This ColumnID is used to get cell text.

Currently, I just try to match typing with text in the first column.

This patch is only for evaluation. Any comments are welcome.

Comment 12

17 years ago
mpt's proposal in bug 104503 was basically "3".  I agree with him -- I'd get
confused if selecting bookmarks or mail messages worked differently depending on
how I had sorted them.  mpt also suggested going to the first item starting with
'f' if I hit 'e' and no item starts with 'e', which seems like a good idea to me.

This feature would have to be disabled in the mail thread pane, because mail
uses single-letter shortcuts for things like "next unread message".

Comment 13

17 years ago
*** Bug 104503 has been marked as a duplicate of this bug. ***

Comment 14

17 years ago
> mpt also suggested going to the first item starting with
> 'f' if I hit 'e' and no item starts with 'e', which seems like a good idea to me.

That's contrary to how windows behaves, and doesn't make any sense to me.
  * assume a list with "apple", "orange", and "tomato" in it
  * I type "p" because I'm looking for "pear"
  * "tomato" is selected.  WTF?  I wasn't looking for tomato.

Windows doesn't change the selection if the item doesn't exist.

Comment 15

17 years ago
If pressing 'p' does nothing, you'll think you mistyped the letter.  If it
selects 'tomato', you'll be able to see quickly that there is no item beginning
with 'p'.  I encountered that situation once.

Comment 16

17 years ago
On the other hand, if we're hoping to have a visual indication of which letters
you've typed so far, that won't work well if the item selected doesn't always
start with the letters you've typed.

Comment 17

17 years ago
I can't think of any widget in any app that behaves the way you describe.
(Assignee)

Comment 18

17 years ago
think there are 3 types of columns: first column, primary column, sorted 
column. (seems like there is not focused column). what's the priority of them 
when key-navigating? I prefer: sorted > primary > first.
(Assignee)

Comment 19

17 years ago
Created attachment 77388 [details] [diff] [review]
patch


suit my previous comment
Attachment #77198 - Attachment is obsolete: true

Comment 20

17 years ago
Kyle, your patch looks pretty good.
Some nits:
- the name GetPrimaryColumnIndex() is quite misleading,
  what about GetKeyColumnIndex() ?
- XUL developers should be able to enable/disable this feature,
  use enableKeyNavigation or disableKeyNavigation or something better (look at
  enableColumnDrag for example)
  not sure what should be default

Comment 21

17 years ago
I would think that key navigation should be true by default.  It's a pretty
standard feature that should be supplied unless the XUL author explicitly turns
it off.
(Assignee)

Comment 22

17 years ago
Thanks for your comments. I should add a new nsXULAtom -- disableKeyNavigation. 
If XUL author does not turn it on, we always do key navigation.

Comment 23

17 years ago
Just to be clear, you don't need a new nsXULAtom

add a new property
<property name="disableKeyNavigation"> (see enableColumnDrag for example)

and in keypress handler something like this:
else if (!this.disableKeyNavigation &&
         event.charCode >= 32 && event.charCode <= 122) {  // ' ' - 'z'
(Assignee)

Comment 24

17 years ago
Created attachment 77416 [details] [diff] [review]
patch (revised according to Jan's comments)


1. Change the function's name to GetKeyColumnIndex
2. Add a new property disableKeyNavigation

Need r=
(Assignee)

Comment 25

17 years ago
Created attachment 77417 [details] [diff] [review]
patch (revised according to Jan's comments)


1. Change the function's name to GetKeyColumnIndex
2. Add a new property disableKeyNavigation

Need r=
(Assignee)

Updated

17 years ago
Attachment #77388 - Attachment is obsolete: true
(Assignee)

Comment 26

17 years ago
Comment on attachment 77416 [details] [diff] [review]
patch (revised according to Jan's comments)


double clicked the "Submit" button :(
Attachment #77416 - Attachment is obsolete: true

Comment 27

17 years ago
- you forgot to change getPrimaryColumnIndex() in tree.xml
- add disableKeyNavigation="true" to trees in folderPane.xul and threadPane.xul

other than that, r=varga
(Assignee)

Comment 28

17 years ago
Created attachment 77568 [details] [diff] [review]
patch 2 (revised according to Jan's comments)


done!
Attachment #77417 - Attachment is obsolete: true

Comment 29

17 years ago
Comment on attachment 77568 [details] [diff] [review]
patch 2 (revised according to Jan's comments)

r=varga
Attachment #77568 - Flags: review+
(Reporter)

Comment 30

17 years ago
Rather than .8 seconds, for the keyboard repeat delay on Windows I think we're
supposed to use:

GetSystemParametersInfo()
SPI_GETKEYBOARDDELAY Get settings for delay time of key inputs.
(Reporter)

Comment 31

17 years ago
Spun off bug 135749, to complete XUL tree keyboard implementation.

Comment 32

17 years ago
Hrm, I don't know about that system parameter.  That's the amount of time before
keys start repeating, but I don't think it's tied to incremental searching.  I
have  a short delay but my incremental search delay seems unaffected.

It's 2500 ms in the patch not 800, isn't it?

Maybe if people complain we could look into moving this into nsLookAndFeel in
the future, to make it per-platform.

Comment 34

17 years ago
More descriptive information from the API Help:

SPI_GETKEYBOARDDELAY
Retrieves the keyboard repeat-delay setting. The pvParam parameter must point to
an integer variable that receives the setting.
SPI_GETKEYBOARDSPEED
Retrieves the keyboard repeat-speed setting. The pvParam parameter must point to
a DWORD variable that receives the setting.

This say to me that these settings are only for key repeat.  But try it for
yourself.  Change your key repeat settings and see if they affect the
incremental search window at all.
Whoo, this is looking nice.

+                 event.charCode >= 32 && event.charCode <= 122) {  // ' ' - 'z'

This won't work for characters like ä and Ú, when it should. I don't see any
need to limit the possible characters -- surely control characters will be
ignored already?

+           if (event.timeStamp - this._lastKeyTime > 2500)

On Mac OS this appears to be hard-coded to 1000, not varying with any key-repeat
settings.

-  <tree id="folderTree" class="plain" flex="1" seltype="single"
+  <tree id="folderTree" class="plain" flex="1" seltype="single"
disableKeyNavigation="true"

Please don't do that. The folder pane is perhaps where this feature would be
most useful, since the items in it are usually all visible and well-known, and
since scrolling through the list one item at a time can be very slow.
Component: Browser-General → XP Toolkit/Widgets: Trees

Updated

17 years ago
Severity: major → enhancement
QA Contact: doronr → shrir
(Assignee)

Comment 36

17 years ago
Hi, Matthew, thanks for your comment 
1. I think your are right. The charCode of control key is always 0, so we can 
modify this condition to (event.charCode > 0);
2. Do you think we should change the interval to 1000ms for all platform? This 
value is same as bug 92491 (XUL menu) and bug 133365 (XUL listbox);
3. Jan suggest to do this on comment 27.
(Assignee)

Comment 37

17 years ago
tested in mail module, when the focus is in folder pane, you press "n", the 
focus will jump to the first unread message in this folder. It still works even 
the keyboard navigation feature is on.
(Assignee)

Comment 38

17 years ago
Created attachment 78862 [details] [diff] [review]
patch (revised according to mpt's comments)


changes:
1. event.charCode >= 32 && event.charCode <= 122 =>
   event.charCode > 0
2. if (event.timeStamp - this._lastKeyTime > 2500) =>
   if (event.timeStamp - this._lastKeyTime > 1000)
3. remove disableKeyNavigation="true" from folderpane.xul

Jan, can you r= again?
Attachment #77568 - Attachment is obsolete: true

Comment 39

17 years ago
Comment on attachment 78862 [details] [diff] [review]
patch (revised according to mpt's comments)

looks good, the lower value seems to be much better
r=varga
Attachment #78862 - Flags: review+
(Assignee)

Comment 40

17 years ago
Created attachment 80365 [details] [diff] [review]
change nothing, just keep the patch up to date.
Attachment #78862 - Attachment is obsolete: true

Comment 41

17 years ago
Comment on attachment 80365 [details] [diff] [review]
change nothing, just keep the patch up to date.

r=varga
Attachment #80365 - Flags: review+

Comment 42

17 years ago
*** Bug 132764 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 43

17 years ago
Created attachment 83470 [details] [diff] [review]
something was changed in trunk, keep the patch up to date.


New interface method was checked in with bug109217. Only keep the changes of
tree.xml.
Attachment #80365 - Attachment is obsolete: true

Comment 44

17 years ago
Comment on attachment 83470 [details] [diff] [review]
something was changed in trunk, keep the patch up to date.

r=varga
Attachment #83470 - Flags: review+
Comment on attachment 83470 [details] [diff] [review]
something was changed in trunk, keep the patch up to date.

sr=jst
Attachment #83470 - Flags: superreview+
(Assignee)

Comment 46

17 years ago
checked into trunk!
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 47

17 years ago
This has (probably) caused problems in MailNews. See bug 145532.

Comment 48

17 years ago
hey, this broke keyboard navigation in the thread pane (not acceptable!) and was
not reviewed by a mail/news person!

Comment 49

17 years ago
if the focus is in the folder pane, and you press "n", it selects the first
folder who's name starts with "n".  It could be that this needs to be disabled
for the folder pane.
(Reporter)

Comment 50

17 years ago
Kyle needs to take care of this. We can back him out or wait until he gets to
work He's in Beijing, so that should be around 4 pm California time.

Comment 51

17 years ago
There is already a patch in 145532.

Comment 52

17 years ago
The thread seems to have died down here, but I would like to report that as of
moz 1.1 navigating by keys seems to only work for first letter and also (more
importantly) seems to uncheck my mail filter rules when I use it there - and yes
I am cycling through the many matches.

Comment 53

17 years ago
.
Status: RESOLVED → VERIFIED

Updated

14 years ago
Blocks: 132764

Updated

11 years ago
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: shrir → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.