262236 - Can't select listitems which are not next to.

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Nian Liu(n/a in a long time)]

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

Comment 1

[Nian Liu(n/a in a long time)]

Attachment: test case

Comment 2

[Nian Liu(n/a in a long time)]

Attachment: patch

Comment 3

[Aaron Leventhal]

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?

Comment 4

[Ginn Chen]

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.

Comment 5

[Aaron Leventhal]

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?

Comment 6

[Ginn Chen]

From Nian's patch,
I think any other key except Space or Ctrl+Arrow will escape this mode.

Comment 7

[Aaron Leventhal]

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?

Comment 8

[Nian Liu(n/a in a long time)]

Attachment: patchv2

rapid search with first character seems work same with previous version

Comment 9

[Aaron Leventhal]

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

Comment 10

[Nian Liu(n/a in a long time)]

Attachment: patch v3

change PRBool to PRPackedBool and restruct the sequence of PRPackedPool
variable

Comment 11

[Nian Liu(n/a in a long time)]

Aaron, could you recommend one for sr?

Comment 12

[[e-mail]]

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!

Comment 13

[Nian Liu(n/a in a long time)]

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 14

[Nian Liu(n/a in a long time)]

Comment on attachment 161647 [details] [diff] [review]
patch v3

neil?

Comment 15

[[e-mail]]

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.

Comment 16

[Nian Liu(n/a in a long time)]

neil, i can't reproduce what you described with 09/11/2004 nightly+my patch.

could you make a test case to break it?

Comment 17

[Nian Liu(n/a in a long time)]

Attachment: patch v4

modified according neil's comments

Comment 18

[Aaron Leventhal]

Neil, is this good now? No comment in two weeks.

Comment 19

[[e-mail]]

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.

Comment 20

[Nian Liu(n/a in a long time)]

Attachment: final patch accroding to neil's comment

Comment 21

[Nian Liu(n/a in a long time)]

Attachment: patch after community reorg source tree

Comment 22

[Nian Liu(n/a in a long time)]

Attachment: patch v2 after reorg

Comment 23

[:deanis74]

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.

Comment 24

[:deanis74]

Nian or Aaron, can you clarify if I'm right in how I read the patch please?

Comment 25

[Nian Liu(n/a in a long time)]

yes, Dean, your understanding is right. one question for you, what's your
intended behavior for incontinuous selection in both ime/non-ime environment?

Comment 26

[:deanis74]

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.

Comment 27

[timeless]

i'll go one further. please back this out.

Comment 28

[Ginn Chen]

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?

Comment 29

[:deanis74]

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.

Comment 30

[Ginn Chen]

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.

Comment 31

[Ginn Chen]

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?

Comment 32

[:deanis74]

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?

Comment 33

[Robin Lu]

patch has been backed out

Comment 34

[Nian Liu(n/a in a long time)]

anyone have an idea how to judge whether there is an ime open in mozilla?

Comment 35

[Simon Paquet [:sipaq]]

Comment on attachment 161647 [details] [diff] [review]
patch v3

Removing review request from obsolete patch.

Comment 36

[:deanis74]

(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

Comment 37

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 38

[:deanis74]

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.

Comment 39

[Aaron Leventhal]

(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.

Comment 40

[:deanis74]

Agreed.  I take back my objections.  Timeless, are you OK with this as well? 
Again, my apologies.

Comment 41

[timeless]

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.

Comment 42

[:deanis74]

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.

Comment 43

[Aaron Leventhal]

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.

Comment 44

[Ginn Chen]

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

Comment 45

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381