Closed Bug 262236 Opened 16 years ago Closed 16 years ago

Can't select listitems which are not next to.

Categories

(Core :: Layout: Form Controls, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: nian.liu, Assigned: nian.liu)

References

Details

(Keywords: access, Whiteboard: sunport17)

Attachments

(4 files, 4 obsolete files)

1. open test case
2. highlight one option in select
3. press ctrl+up/down anytimes
4. press space to toggle select current option

expect result: two options are selected
actural result: only current option is selected
Attached file test case (obsolete) —
Attached patch patch (obsolete) — Splinter Review
Assignee: general → nian.liu
Attachment #160607 - Flags: review?(aaronleventhal)
Attachment #160606 - Attachment mime type: text/plain → text/html
The code looks okay but I'm not 100% sure this is what we want, because
Control+Space already works.

On Windows so far we're doing everything right except for Shift+Space
(http://msdn.microsoft.com/library/default.asp?url=/library/en-us/dnacc/html/ATG_KeyboardShortcuts.asp)
	SPACE 	Locates new selection and anchor for the item.
  	SHIFT+SPACE 	Extends the selection from anchor to the item.
  	CTRL+SPACE 	Invoke additional selection or deselection and move the anchor to
the selected item.

For Gnome I still can't find developer docs for keyboard support. All I have is
this:
http://docs.sun.com/db/doc/817-1972/6mhlsbe8h?a=view
Is there a Gnome spec or example program I can look at that shows how this works
there?
The testcase doesn't work for IE6 on my box.

Ctrl+Space will not work, if you have IM(Input Method) installed. Ctrl+Space is
always used for switching IM.
Hi Ginn.

You're right, they don't do that in IE. They do it everywhere else and document
it that way, so they are inconsistent.

You're right, the IME problem does seem important.

How does a user turn off this mode? By pressing a regular arrow key?
From Nian's patch,
I think any other key except Space or Ctrl+Arrow will escape this mode.
Comment on attachment 160607 [details] [diff] [review]
patch

> +  PRBool mControlSelectMode;
Make this a PRPackedBool and but all of the PRPackedBool members in that class
next to each other. When there are 4 in a row it compacts into 32 bits.
Otherwise it uses 32 bits for each.

Also, please put a comment next to the member variable and/or code saying that
it is for multiple discontiguous selection.

+  } else if (mControlSelectMode && keycode == 0) {
+  } else {
+    mControlSelectMode = PR_FALSE;
+  }
It seems like you have an extra else here.
Couldn't it just say
} else if (keyCode) {
  mControlSelectMode = PR_FALSE;
}

Have you tested to make sure this patch works okay with incremtal find using
the first few characters of a list item? Does that work as you would expect?
Attached patch patchv2 (obsolete) — Splinter Review
rapid search with first character seems work same with previous version
Attachment #160607 - Attachment is obsolete: true
Attachment #161644 - Flags: review?(aaronleventhal)
Attachment #160607 - Flags: review?(aaronleventhal)
Comment on attachment 161644 [details] [diff] [review]
patchv2

Thanks, if you want to, you can change all of the PRBool's to PRPackedBool and
put them next to each other.

Either way, r=aaronlev
Attachment #161644 - Flags: review?(aaronleventhal) → review+
Attached patch patch v3 (obsolete) — Splinter Review
change PRBool to PRPackedBool and restruct the sequence of PRPackedPool
variable
Aaron, could you recommend one for sr?
Attachment #161647 - Flags: superreview?(neil.parkwaycc.co.uk)
Attachment #161647 - Flags: review+
Comment on attachment 161647 [details] [diff] [review]
patch v3

Hmm... the following steps don't do what I expect:
1. Load testcase attachment 160606 [details]
2. Tab to the "menu" list ;-)
3. Press Ctrl+Down
4. Press I
5. Wait 1 second
6. Press space
Note that the listbox is still in toggle mode.
Also, what about page up/down and home and end? Plus that if statement looks
ugly; maybe you could set mControlSelectMode FALSE (except for space) before
the switch, then set it to PR_TRUE after calling AdjustIndexForDisabledOpt in
each of the applicable cases. Then again, perhaps my way is even uglier!
neil, i'm a little lost. the scenerio you mentioned seems works for me since the
keycode won't be equal to zero when you press "I" which will change the
mControlSelectionMode to false.

i'm using Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8a5) Gecko/20041010 with
my patch v3.
Comment on attachment 161647 [details] [diff] [review]
patch v3

neil?
No, the control select mode gets set false when the keycode is not zero.
For typing characters the keycode is zero and the charcode is not zero.
neil, i can't reproduce what you described with 09/11/2004 nightly+my patch.

could you make a test case to break it?
Blocks: caretnav
Keywords: access, sec508
Whiteboard: sunport17
Product: Browser → Seamonkey
Attached patch patch v4Splinter Review
modified according neil's comments
Attachment #160606 - Attachment is obsolete: true
Attachment #161644 - Attachment is obsolete: true
Attachment #161647 - Attachment is obsolete: true
Attachment #166922 - Flags: superreview?(neil.parkwaycc.co.uk)
Neil, is this good now? No comment in two weeks.
Comment on attachment 166922 [details] [diff] [review]
patch v4

>+  } else if (keycode || (keycode == 0 && charcode != 0x20)) {
You could have used some Boolean algebra here:
A || (!A && !B) === (A || !A) && (A || !B)
But A || !A is a tautology, so this just equals (A || !B)
Now in Mozilla keycode != 0 => charcode == 0 => charcode != 0x20
So (keycode) => (keycode && charcode != 0x20)
This makes our original condition
((keycode && charcode != 0x20) || (keycode == 0 && charcode != 0x20))
This time we apply that Boolean algebra in reverse and get
((keycode || keycode == 0) && charcode != 0x20)
And since (keycode || keycode == 0) is always true we get
(charcode != 0x20)
Although as it's a char code I'd like it compared to a ' ' char please.

Also the .h file is missing from this diff. Apart from these, sr=me.

(In reply to comment #18)
>Neil, is this good now? No comment in two weeks.
My review queue is practically in triple figures, I can't keep up.
Attachment #166922 - Flags: superreview?(neil.parkwaycc.co.uk) → superreview+
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Does this affect all listboxes or only when IME is enabled?  For me, a non-IME
user, this looks like it changes the behavior.  I read it that if I press Space
to select the first item, press Ctrl+Down, release Ctrl, and press Space, both
items will be selected.  That's not what I intended, otherwise I would not have
released Ctrl.
Nian or Aaron, can you clarify if I'm right in how I read the patch please?
yes, Dean, your understanding is right. one question for you, what's your
intended behavior for incontinuous selection in both ime/non-ime environment?
Nian, in my non-IME Windows environment, I have to have Ctrl held down while I
press space.  So the sequence to select the first and third items would be:

1. Press Space to select #1
2. Hold down Ctrl
3. Press Down Arrow twice
4. Press Space to select #3
5. Release Ctrl

That's expected behavior on Windows.  If I release Ctrl before step 4, only #3
is selected.  We should maintain this behavior, because it's what any keyboard
user will expect.

Someone slap my hand, but I'm going to re-open this because it wasn't the right
implementation.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
i'll go one further. please back this out.
Component: General → Layout: Form Controls
Product: Mozilla Application Suite → Core
Dean,
I've removed IMEs and shutdown Input Service on Windows XP.
But I still can't use Ctrl+Space to make a selection with neither IE or Firefox.
And after ctrl+arrow, press space make a selection without clear prev selection.
Press arrow without ctrl, prev selection will be cleared, only the focus item
will be selected.

Which version of Windows are you using?

Can someone confirm the behaviour?
I'm using Windows XP and it works like this for me all over the OS.  Explorer
trees, file pickers, list boxes, and anything else that allows multiple
selection.  For an example, run MSConfig.exe and play around with the check list
box on the Services or Startup tab.

You are correct, though, that releasing Ctrl before my step 3 above will clear
the selection as soon as the down arrow is pressed.

I just tried the exact steps I described in comment 26 in the CC: box of this
bug report and it selected the first and third email addresses in the list.  I'm
using Firefox 1.0.

Lovely.  Wouldn't you know it, they broke this in IE 6.  I can't use the
keyboard to select multiple CC: entries.  That's OK, we should be consistent
with the OS and not with IE in cases where IE has strayed.
Seems that I should try a machine without IME installed.

As comments 5,
"they don't do that in IE. They do it everywhere else and document
it that way, so they are inconsistent."

But what should we do for IME users?
Ctrl+Space is conflict.
I changed my IME keybinding and tested with explorer.exe on Windows,
both Ctrl+Space and Space works as additional selection or deselection.

And I tested on Solaris 10 without IME, both Ctrl+Space and Space works as
additional selection or deselection with Mozilla.

Dean, the only thing broke is,
"If I release Ctrl before step 4, only #3 is selected.  We should maintain this
behavior, because it's what any keyboard user will expect."
But it can't work with explorer.exe or IE either.

Do you think we should back it out?
Yes, I think it should be backed out, as does timeless.  I expect things to work
consistently across all applications on an OS.  Since this is trying to get
around an IME problem, can whatever solution is found be limited to be in effect
only when IME is enabled?
patch has been backed out
anyone have an idea how to judge whether there is an ime open in mozilla?
Comment on attachment 161647 [details] [diff] [review]
patch v3

Removing review request from obsolete patch.
Attachment #161647 - Flags: superreview?(neil.parkwaycc.co.uk)
(In reply to comment #34)
> anyone have an idea how to judge whether there is an ime open in mozilla?

I found this for Windows but couldn't find it used anywhere in the code.

http://support.microsoft.com/default.aspx?scid=kb;en-us;250280
actually, playing around with this in windows it looks like space will always
*add* the focused item to the selection, in other words, ctrl+space and space
behaves exactly the same.

This was quite a surprice for me since I've never noticed it before. I've always
assumed that I need to hold the ctrl key. However since this appears to be the
standard on windows I think we should do it in mozilla too.
Oh crap.  This is where I have to eat the famous humble pie and agree with
Jonas.  It's the arrow key movement that clears the selection without Ctrl
pressed, not pressing space.
(In reply to comment #38)
> Oh crap.  This is where I have to eat the famous humble pie and agree with
> Jonas.  It's the arrow key movement that clears the selection without Ctrl
> pressed, not pressing space.

So where do we stand on this bug now? Does the most recently attached patch do
what Dean wants, and is it ready for review? We shouldn't let a valid fix rot.
Agreed.  I take back my objections.  Timeless, are you OK with this as well? 
Again, my apologies.
nope :).

appname: help (winhlp32.exe)
task: find

steps (approximate, make sure you help file is non trivial)
open help file
trigger 'search'
ctrl-tab to the 'find' tab
[if you're given a chance to build an index, select the most comprehensive one]
type the word(s) you want to find:
"a"
click the first item in "select some matching words to narrow your search" (should be 'a')
<space> (nothing should happen)
<ctrl>-<space> a should deselect
<space> a should select
<space> (nothing should happen)
<ctrl>-<space> a should deselect
<ctrl>-<space> a should select
<ctrl>-<down> dotted focus should be around the next item (hopefully 'A')
<ctrl>-<down> dotted focus should be around the following item (no guesses)
press <space> selection should only include the newly selected item
<ctrl>-<home> dotted focus should be around the first item ('a')
<ctrl>-<space> selection should include first and third items.
I wouldn't use WinHlp32 as an example for any UI aspect.  Looking at the 
MSConfig and the commonly-used Explorer, multi-select behaves like in the 
patch.
I agree. Nian, please check the patch back in.

(In reply to comment #42)
> I wouldn't use WinHlp32 as an example for any UI aspect.  Looking at the 
> MSConfig and the commonly-used Explorer, multi-select behaves like in the 
> patch.

Checking in forms/nsListControlFrame.h;
/cvsroot/mozilla/layout/forms/nsListControlFrame.h,v  <--  nsListControlFrame.h
new revision: 1.137; previous revision: 1.136
done
Checking in forms/nsListControlFrame.cpp;
/cvsroot/mozilla/layout/forms/nsListControlFrame.cpp,v  <--  nsListControlFrame.cpp
new revision: 1.359; previous revision: 1.358
done
Status: REOPENED → RESOLVED
Closed: 16 years ago16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.