279868 - [FIX] SelectedIndex does not move the focus to the element specified in listbox.

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[KIRAN SHIVARAMA SUNDAR]

User-Agent:       Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.8a1) Gecko/20040520
Build Identifier: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:1.8a1) Gecko/20040520

Whenever we make use of the selectedIndex property to move the focus between
elements in a listbox, even though the selection\highlight moves to the element
specified, it does not tend to move the focus and the focus ring to the
specified element.

Reproducible: Always

Steps to Reproduce:
1.Open the given testcase.
2.Select any element other than the first element in the Listbox.
3.Click the "Re-Scroll" button which triggers the selectedIndex to move the
selection to the firstelement.
4.Bring the focus back to the listbox to see the bug.
Actual Results:  
After clicking on the "Re-Scroll" button, the selection\highlight is moved back
to the first element, but on bringing the focus onto the listbox you see that
the focus as well as the focus ring is still on the element selected during the
step 1 of the testcase as is seen in the given screenshot.

Expected Results:  
The focus as well as the focus ring should have followed the
selection\highlight, i.e in this case the focus should have moved to the first
element 

This problem is seen to occur in the latest release of mozilla also i.e Mozilla
1.8 alpha 6 on Windows.

Comment 1

[KIRAN SHIVARAMA SUNDAR]

Attachment: Testcase depicting the Bug

Comment 2

[KIRAN SHIVARAMA SUNDAR]

Attachment: Screenshot showing the Bug

Comment 3

[Boris Zbarsky [:bzbarsky]]

Note that the focus and the selection are not the same thing -- I can move the
focus inside the listbox with ctrl+shift+up/down, whereas that does not move the
selection.  So I suspect this is invalid.

Comment 4

[arsukdeo]

Confirmed.  This should be fixed.

The programmatical scrolling of a listbox via the selectedIndex property should 
trigger a synchronized motion between the highlight bar and the focus, when the 
selectedIndex property has a value > -1, with both the highlight bar and the 
focus having a coincided position at the specified target (list element).

As a note, the -1 is the initialization value assigned to the selectedIndex 
property of listboxes when the page is loaded.  It reflects the visual clue 
that there is no highlight bar in the listbox.  This however, does not mean 
that a focus bar cannot exist in the listbox and be manipulated by the 
ctrl+shift+up/down keys.

If one is to hold the arrow key down, you will detect the nature of the 
conflict: the key down is a signal to scroll down and this is triggered before 
the javascript event (as specified by code).  The scroll occurs, causing the 
focus to move down, and when the javascript keyup event kick in, the highlight 
bar is repositioned to the top (as intended by code) leaving the focus behind.

Comment 5

[Mats Palmgren (inactive)]

I agree with Boris, this is invalid.

Moving focus would make selectedIndex hard to use in certain DOM event handlers.
For example changing the selected list item for every typed character
in a text field.

I tested IE6 and Opera 8.0 on Windows XP and Opera 8.0 on Linux -
none of them move the focus when setting selectedIndex.

Comment 6

[KIRAN SHIVARAMA SUNDAR]

Boris and Mats, I went through your comments. I do not understand as to why this
bug is an invalid one. Mats, I too have checked the result with Opera and IE6 on
Windows XP, but here I find that the focus is moved in co-ordination with the
highlight . I can validate this by the fact that when I do a key down after the
selectedindex has positioned the highlight at the first element, the highlight
tends to move from the first element instead of jumping down to the element
having focus as is happening wth mozilla. Kindly recheck your result and update.
I strongly feel that this is a valid bug.

Comment 7

[arsukdeo]

The following is just in case I misunderstood the intention of the reported 
point by Boris:

1.  Is there an agreement that there is something wrong with this behaviour, as 
pointed out by the "bug" report?  That is, is it inconsistent with the other 
browsers?  I can also confirm that this weird behaviour, reported by Kiran, 
exists for Win2000 professional.  This would seem to indicate that it is not a 
linux manifestation, but one from windows.

2.  If this behaviour is inconsistent, what is the theory behind why the focus 
is moving independently of the programmatic setting for selection upon keyup, 
and would someone fix it if is determined to be a code related behaviour bug 
(instead of some unlikely form of interference by the environment)?  I have 
tried but I cannot ssem to derive an "obvious" workaround.  If you guys can 
provide a theory, then maybe we can probably derive a workaround.  

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 1.  Is there an agreement that there is something wrong with this behaviour, as 
> pointed out by the "bug" report?

No.

Comment 9

[arsukdeo]

Does this mean that the behaviour on the linux platform is consistent for all 
browsers, while the behaviour on the windows platform is inconsistent?

Boris & Mats both see a consistent behaviour on the linux platform, which means 
that users on this platform will be unhappy with changes to the behavour of the 
listbox there.  The "reset" requirement/implementation by Kiran is unusual, and 
it is possible that the listbox is not used in this way.

However, what do we do about the inconsistency in the windows platform?  Please 
note that the FireFox/Mozilla browsers are better than IE, and the fact that 
they are a tiny bit slower than IE is irrelevant.  It is now only a matter of 
time before the much larger IE base (far>>linux) starts realizing that the 
alternative to IE is much better and start converting to FireFox/Mozilla.  It 
is important to keep this crowd happy to move the migration along.  The open 
source community is the only hope to complete the dream to make the browser the 
powerful VM it is destined to be, where plugins are not needed, and text-based 
instructions (javascript, html, css, etc.) can produce complex UIs and the 
streaming of media from a dynamic datastore via the HTTP server can be done 
with an open protocol for the necessary handshaking between the browser and the 
server. All of this and much more is possible if you can move the mass to 
FireFox/Mozilla.  So, this means that someone higher up needs to make a 
decision on what to do about any potential inconsistencies.  This one has to be 
resolved.  So Boris, you'll have to move up the ladder and get a decision.  :~) 

Comment 10

[Boris Zbarsky [:bzbarsky]]

Er... did you read comment 5?  Per Mats' testing IE and Mozilla behave the same,
is the way I understood that comment.

I'm curious as to why Mats and Kiran are getting different results, though.

Comment 11

[arsukdeo]

I did, but I wasn't sure how to interpret Mat's report.  But I have a feeling 
that it is bad news.

Unfortunately for you, you are the expert on this subject, because I know that 
you had worked with the code to fix an important bug I reported.

I know the following will make sense, and both Mat and Kiran can do some 
investigation to help you make a decision on a recommendation to send up the 
ladder.

Here is the important premise we have to work on:  From a user perspective, the 
behaviour of a form element, has to be consistent across all platforms (linux, 
macs, windows, and all other computing devices).  This helps integrators to 
create products and solutions that can be consistent across these platforms.  
Even though IE has the larger user base, and we need the migration, I will have 
to agree that it makes no sense to implement "quirks".  So, for this particular 
problem/bug, we need to understand the behaviour of the listbox before 
attempting to fix/change anything.  

If a change is necessary, we need to determine if it will raise "hell" or not, 
and plan for a roll back on any deployment with the changes if necessary - 
resulting in the disgusting support of two behaviour. But lets put this step 
off for last.

Here is how I think the listbox should behave, and you guys can help to fix the 
logic:

What does it mean to have both a highlighted bar and a focus bar that can have 
independent movement; that is, what would the impression be to an observer 
(with short term memory) who is presented with a listbox having one item 
highlighed and another with a focus?

The answer has to be consistent, and I propose that the observer will say that 
the operator is showing "intent" to select the element with the focus, and that 
the element that was last selected is currently highlighted.

Now, if the operator has made a decision on "intent" and select the element 
that has focus, then I propose that the observer will see this as the highlight 
bar on the element.  The highlight bar coincides with the focus at this point.

If the operator makes on decision and leaves the listbox, then I propose that 
the observer has to see the focus somewhere else to confirm that the operator 
did leave the listbox and that focus is lost from the element.

If the operator then makes a decision to return to the listbox, (investigation 
required across the platforms) the observer should see the highlighed element 
(even if it was previously scrolled) as focus is returned to the listbox, and 
the focus should be on the highlighted element.

The summary of all these propositions is that where intent coincides with a 
decision, the focus bar and the highlighted bar points to the same element.  
Intent does not have to be preserved, and has a lifetime that is as long as the 
duration of the focus on the listbox.  All intentions related to the listbox is 
lost once focus is lost on the listbox.

We need to determine if this behaviour model makes sense and to determine if it 
holds up under the various platforms for all browsers.  Then we can address 
Kiran's issue (coloured as intent and decision) and make the listbox behaviour 
in the browser (Mozilla/FireFox) consistent across all platforms (necessary for 
web-based systems to support users on all sort of computing devices).

Comment 12

[timeless]

i'm not having a good day, but i think i'm just going to say that i'm 99%
certain that i believe you're wrong. thankfully for you, i only have input on
seamonkey (the product against which this wayward bug is filed)'s ui (as a peer
to the gui module) and not gecko core, i'm also a peer for the Windows port and
the BeOS port. I also use just about every os that mozilla runs on. The os's
behave differently to things, including whether it makes sense to have selection
and focus differ. being consistent across platforms at the expense of alienating
users of individual platforms runs counter to the current plans for mozilla.org
hosted projects (esp firefox, a product over which i have absolutely 0 influence).

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 172423 [details]
Testcase depicting the Bug

This testcase does not work in IE6/Windows XP.
(selection does not change when clicking the button)

Comment 14

[Mats Palmgren (inactive)]

Attachment: Testcase #2

Comment 15

[Mats Palmgren (inactive)]

I have to admit that I misunderstood the reported problem, I thought the
reporter wanted us to move focus from the button to the list when the
button was clicked (that is, as a result of setting 'selectedIndex').
Sorry for the confusion, my bad.

After re-testing using "Testcase #2" the results are:
IE6 and Opera8 on Windows XP:             "A"
Opera 8 on Linux:                         "A"
Mozilla/FF trunk on Windows XP and Linux: "C"

Given the results, I think it could be worth fixing this (for compat reasons).
(Unless some W3C spec explicitly says otherwise of course)

Comment 16

[arsukdeo]

Thanks Mat.  I think Kiran can do us a favour and ask her coworkers at IBM to 
do a test for Mozilla on the other platforms and she can let us know the 
results.  This can then help guide us towards an agreement that Kiran's 
problem/bug should be fixed or not and that any decision that is made should be 
submitted for address by the other platforms on which Mozilla/FireFox runs 
where there are exceptions to the decision.  I have a feeling that the proposed 
logic in the behaviour of the listbox with respect to the focus and highlight 
bar would make sense to everyone if the logic itself makes sense and does not 
violate any official w3c specs.  The commercial (and  non-commercial) world of 
server based products will be happy to know that a Mizilla/FireFox user on any 
platform will experience identical behaviour for this particular exercise.

For throwing all this energy into Kiran's problem and trying to reach a 
consensus on a solution, maybe we can retain the assistance of the good friends 
of Kiran at IBM to help us with future problems by doing testing across the 
platforms in the future.

Comment 17

[KIRAN SHIVARAMA SUNDAR]

Hi everybody, I have tried the testcase on the platforms available over here i.e
Windows XP, OS/2, Linux. I can confirm to see the same behaviour on all the
platforms. The focus is on "C" only on each of these platforms.

Comment 18

[KIRAN SHIVARAMA SUNDAR]

I have tried to look into the code. I have found that the code for implementing
the selection was at nsHTMLSelectElement::SetOptionsSelectedByIndex(
nsHTMLSelectElement.cpp). Stepping into the code I do not find any implentation
which speaks of getting the focus to the element selected. So, I think this
could be of any help to further our analysis on this problem.

Comment 19

[Mats Palmgren (inactive)]

Attachment: Testcase #3 (scrolling)

Comment 20

[Mats Palmgren (inactive)]

After considering scrolling I think it makes sense to fix this.
Especially the combobox is confusing with the current behaviour - after 3.
it has a focus outline around "A" but the kbd navigation point is really at "C".
Taking bug...

Comment 21

[arsukdeo]

I see what Mats has done to illustrate the confusion (quirky behaviour) with 
his latest test case (#3).  

I hereby pool my vote with Mats that the current behaviour of the listbox with 
respect to the bug/problem Kiran posted, is inconsistent and that it should be 
fixed.

The following is necessary for documentation (and future retrospective 
justification) purposes, as to what it is we are trying to achieve before 
anyone changes the code.  I had made a proposal on how the listbox should 
logically behave.  Someone (other than myself) needs to clearly restate it 
(removing all my personal pleadings on what I think is best for the 
Mozilla/FireFox browsers) and make any necessary ammendments for us to agree as 
to what it is (i.e. behaviour) we are trying to implement.  It is probably best 
that the person who would actually fix the code be the one to do so.  What does 
Boris think?

Comment 22

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This makes our behaviour compatible with that of IE6/Opera8.
Tested on Mozilla/Firefox on Linux and Firefox on Windows XP.
I also verified a few other cases like <select multiple> and 'display:none'.

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 172764 [details] [diff] [review]
Patch rev. 1

>Index: layout/forms/nsListControlFrame.cpp
>+  if (mComboboxFrame) {
>+    mComboboxFrame->UpdateRecentIndex(aOldIndex);
>+  }

Is it me, or is the recent index thing write-only?  If I'm not mistaken and it
is, mind just removing it and simplifying this patch accordingly?

>Index: layout/forms/nsComboboxControlFrame.cpp
>+  NS_ASSERTION(listFrame, "No list frame!");
>+
>+  if (listFrame) {

No need for that if, given that assert, I would think.

Also, does this happen to fix bug 273681 by any chance?

Comment 24

[arsukdeo]

Boris and Mats, thank you for the great work that benefits so many people, 
including myself!!!  This particular bug (279868) did not affect my project, 
but I am certain the fix means that Kiran's group at IBM will be able to 
deliver a product/service to hopefully hundreds of users on Mozilla/Firefox.  I 
am glad that you guys decided to fix her problem.  Thank you.  As a result, I 
hope her group will now actively support the Mozilla/Firefox project.

Comment 25

[Mats Palmgren (inactive)]

Comment on attachment 172764 [details] [diff] [review]
Patch rev. 1

(In reply to comment #23)
>
> mind just removing it and simplifying this patch accordingly?

It is used to avoid firing an event in case the value is the same:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/forms/nsListControlF
rame.cpp&rev=1.352&root=/cvsroot&mark=2146#2140

There seems to be three separate values kept:
1. nsHTMLSelectElement::mSelectedIndex
2. nsComboboxControlFrame::mRecentSelectedIndex
3. nsComboboxControlFrame::mDisplayedIndex

My gut feeling is that 2 could be eliminated but the code the updates
these values at different events is rather intricate, so I'll leave it
as is for now.

> 
> Also, does this happen to fix bug 273681 by any chance?

No such luck.

Comment 26

[Mats Palmgren (inactive)]

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/forms/nsListControlFrame.cpp&rev=1.352&root=/cvsroot&mark=2146#2140

Comment 27

[Mats Palmgren (inactive)]

(Hmmm, why is it that submitting comments from the textarea in
"Edit attachments" breaks long URLs but not the "Additional Comments"
textarea here?)

Comment 28

[timeless]

one uses wrap=hard, one uses wrap=soft. it's a bug, i don't believe it's filed.

Comment 29

[Mats Palmgren (inactive)]

(Re: comment 27/28 - found bug 11901 and their test installation works fine:
http://landfill.bugzilla.org/bz11901v2/show_bug.cgi?id=411)

Comment 30

[arsukdeo]

Re comment 25:

The behaviour model would seem to indicate the need for a variable to track the 
motion of the focus bar vs the highlight bar.  The <prefix>SelectedIndex 
variable is primarily associated with the highlight bar.  It would make sense 
to create a <prefix>FocusIndex to remove this ambiguity and to illustrate the 
fact that the listbox has a complex scrolling behaviour.

In an elaborate collaborative environment, it is not a sign of bad programming 
if a developer is explicit. From my personal experience in the past, I have 
been too clever with C-based languages and the resulting code created can be 
obscure to the people who follow to do maintenance.  Once everything works as 
expected, someone can later come along and consolidate/optimize the code, if 
required.

By the way, where can I find the external documented object model for the 
framework in which the listbox exists?

Comment 31

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

1. Don't null-check after ASSERTION.
2. remove a couple of bogus NS_RELEASEs.

Comment 32

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 173303 [details] [diff] [review]
Patch rev. 2

>Index: layout/forms/nsComboboxControlFrame.cpp
> GetOption(nsIDOMHTMLCollection& aCollection, PRInt32 aIndex)
>-      NS_RELEASE(node);

No, this leaks....  Leave that release in.

Mind doing a separate patch on making this method return an already_AddRefed
and removing dont_AddRef/already_AddRefed from callers?

r+sr=bzbarsky on the patch with that change removed.

It may also be worth creating a GetListFrame() method and refactoring all these
callers to use it....

Comment 33

[Stephen Donner [:stephend] Not actively reading bugmail]

Setting STATUS to confirmed, since this bug has a patch that has already landed.

Comment 34

[Mats Palmgren (inactive)]

Filed bug 281001 for the code cleanup.

Checked in at 2005-02-04 14:56 PDT (leaving "NS_RELEASE(node)" intact).

-> FIXED

Comment 35

[[e-mail]]

(In reply to comment #25)
>There seem to be three separate values kept:
Ah yes, that brings back memories of bug 244761 and bug 251833 :-/
>1. nsHTMLSelectElement::mSelectedIndex
The index highlighted (hovered) in the dropdown.
>2. nsComboboxControlFrame::mRecentSelectedIndex
Used to determine if an onchange should be fired.
>3. nsComboboxControlFrame::mDisplayedIndex
The index displayed in the static area.

Comment 36

[Boris Zbarsky [:bzbarsky]]

This caused a regression -- bug 281635.

Comment 37

[arsukdeo]

Re: comment #36

I am new to this process, with a minimal grasp of the derivation of the code.  
I hope I am wrong, but a "regression" in any computing profession does not 
sound good.

Who or what entity manages the successful evolution of this operation?  And how 
does it get the intermediate executables out to testers to achieve success?  
What is the scale and timeframe for such an operation in terms of meeting basic 
deliverables (bugs do have negative effects on offically released versions, 
with few people willing to update to a nightly build), etc.?  

For my part, I can help with the retesting the "listbox" issues I have reported 
in the past (that severely affects my project), even though I am very busy and 
hate revisiting old issues.