138191 - Changing field focus with javascript and backspacing breaks tabbing

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Kevin Hollenshead]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.0rc1)
Gecko/20020417
BuildID:    2002041711

I have some javascripts that I use to perform auto-tabbing (i.e. when you've
entered the maximum allowable characters for a field, focus is shifted to the
next field).  These scripts use the onkeyup handler to  determine whether or not
to shift focus to a field.  Under NS 4.79 and IE, these work fine.  Under
Mozilla, in certain cases, the tab order gets screwed up so that focus is given
to some other (seemingly) random element in the page.  They all seem to resolve
around use of the backspace key and that, somehow, it is not being handled
properly.  I set the tabindex properties on the fields in question to see if
that would change anything, but it doesn't.

I will attach a testcase that demonstrates the problem.  I have one repeatable
scenario.

Reproducible: Always
Steps to Reproduce:
1. Tab from the first field to the second
2. Backspace, which will cause focus to shift back to the first field
3. Tab again -- focus will incorrectly end up on the 3rd field

Actual Results:  Focus incorrectly ends up on the 3rd field.

Expected Results:  Focus should be on the second field.

This seems to be a problem no matter which field you start from.

I was *NOT* able to reproduce this on a Mozilla 0.9.9 linux build (20020203 from
a Mandrake Linux RPM) or a Netscape 6.22 (20020314) linux build.  I **was** able
to reproduce it on the same linux nightly as the windows build.  Regression?

Comment 1

[Kevin Hollenshead]

Attachment: HTML 4.01 - form with 3 fields

Comment 2

[Boris Zbarsky [:bzbarsky]]

Confirming on a current linux nightly...

Comment 3

[Aaron Leventhal]

-> bryner

Comment 4

[Eric Dimick Eastman]

The "other (seemingly) random element" is the next element after the last one to
get focus by clicking or tabbing.  In other words focus() doesn't have any
affect on the tabbing.  It just keeps going to the next field as if the focus
hadn't changed.

I have a much larger form in which I use the enter key to move down a column
with JavaScript.  (I am also waiting on #81739 so I can keep the enter key from
trying to submit the form, but that is another issue :)  I would be more than
happy  to share my form if that would help, but think (and hope) this
description is clear enough.

Comment 5

[Gilbert Fang]

This is in my interests. Could I take this bug? 

Comment 6

[Gilbert Fang]

It is a regression from m099 to m100.
mozilla 099 on windows does not have this bug, but mozilla 1.0 has. 

Comment 7

[Gilbert Fang]

It actually the incomplete focus by "aPrevField.focus();".
The "focus()"--"HTML input field dom interface" does something less then the tab
key event handler.It seems involved  with the Caret. 

Comment 8

[Gilbert Fang]

Attachment: Adding MoveCareToFocus for dom interface focus to do the complete focus.

Could some one give me a r=?

Comment 9

[Gilbert Fang]

Comment on attachment 92323 [details] [diff] [review]
Adding  MoveCareToFocus for dom interface focus to do the complete focus. 

oh, To say accurately, the patch is add MoveCaretToFocus  in the  HTML input
element focus method which implements the dom interface focus. I have not
modified any interface at all.

Comment 10

[John Keiser (jkeiser)]

CC'ing joki, the events guru, for input on what I'm about to say :)

Comment 11

[John Keiser (jkeiser)]

Comment on attachment 92323 [details] [diff] [review]
Adding  MoveCareToFocus for dom interface focus to do the complete focus. 

This needs to be in nsGenericHTMLElement::SetElementFocus() so that the other
elements (anchor, button, select, textarea) that do focus() can benefit from
it.

Further, I think it's a good idea to make EventStateManager::ChangeFocus()  a
public function so that anybody who changes the focus can have this sort of
thing done automatically, but you don't have to do that in this patch ... it
can be handled later unless joki says otherwise. 

http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.
cpp#2790

Comment 12

[Gilbert Fang]

Attachment: new patch after jkeiser ' comments

Hi, John Keiser , could u give the patch a r=? thank u . 
This patch moves the "MoveCaretToFocus" into
nsGenericHTMLElements::SetElementFocus.

Comment 13

[John Keiser (jkeiser)]

Comment on attachment 92351 [details] [diff] [review]
new patch after jkeiser ' comments

Good work!

One nitpick: instead of NS_SUCCEEDED(rv) just do NS_ENSURE_SUCCESS(rv,rv); that
way there's less nesting.

Fix that and you have r=jkeiser.  I think the sr needs to be joki though.

Comment 14

[Gilbert Fang]

Attachment: patch following jkeiser's review

Thanks Jkeiser. If u think this patch is good, please set the patch's review
status.
Could I  send a sr= request to joki now?

Comment 15

[John Keiser (jkeiser)]

Comment on attachment 92358 [details] [diff] [review]
patch  following jkeiser's review 

Yep, r=jkeiser :)

Comment 16

[Gilbert Fang]

Thanks to jkeiser. 

Comment 17

[Gilbert Fang]

Oh, I have sent a sr request to joki, but  I did not find his name on mozilla sr
list "http://www.mozilla.org/hacking/reviewers.html" . I think the list may  be
 out of date again. Is there any one who update the list regularly? 

Comment 18

[Boris Zbarsky [:bzbarsky]]

The list was last updated less than two weeks ago....

Comment 19

[kinmoz]

What is the impact, if any, of this change on pages loaded in Composer and HTML
Mail Compose? Specifically, when clicking on links, input elements, and any
other elements that are normally focused in the browser.

I ask only because we've had trouble with changes like this in the past which
have caused the caret to disappear in Composer.

Also, what happens if this element being focused doesn't use a caret (radio
button, checkbox)? Is this code called for disabled and readonly elements?

Comment 20

[John Keiser (jkeiser)]

AFAICT this only affects the focus() JS method on anchor, select, input,
textarea and button--if Composer calls these methods then it will affect
composer.  Otherwise it won't.

As for how it affects other elements: This MoveCaretToCursor code is already
called from ESM on all sorts of elements, not just input type=text and textarea.
 Essentially what MoveCaretToCursor does is move the selection to the focused
element.  (If it can't be selected, there will be no selection.)

The underlying reason for this patch is that when the current selection (caret)
is not the focused, we tab from the selection, not the focus.  Thus if you don't
move the selection when you focus(), we will tab from the previously focused
(and selected) element.  I think it is reasonable to select an element when you
focus(), since that's what happens when you tab in.

See
http://lxr.mozilla.org/seamonkey/source/content/events/src/nsEventStateManager.cpp#2929

Comment 21

[Brian Ryner (not reading)]

Comment on attachment 92358 [details] [diff] [review]
patch  following jkeiser's review 

>Index: nsGenericHTMLElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/html/content/src/nsGenericHTMLElement.cpp,v
>retrieving revision 1.368
>diff -u -r1.368 nsGenericHTMLElement.cpp
>--- nsGenericHTMLElement.cpp	20 Jul 2002 23:09:24 -0000	1.368
>+++ nsGenericHTMLElement.cpp	23 Jul 2002 07:45:42 -0000
>@@ -4659,7 +4659,15 @@
>   }
> 
>   if (aDoFocus) {
>-    return SetFocus(presContext);
>+    nsresult rv;
>+    rv = SetFocus(presContext);
>+    NS_ENSURE_SUCCESS(rv,rv);

Nit: move the declaration of 'rv' onto the same line as SetFocus, i.e.

nsresult rv = SetFocus(presContext);

Make that change and sr=bryner (no need to attach a new patch with the change)

Comment 22

[Gilbert Fang]

Attachment: new patch after bryner's sr(patch_bz138191_20020724_a)

Thanks to bryner. I think I'd better make a new patch even there is only so
little difference. That is because I have no cvs check-in permission now and I
have to ask some one else to check in it for me. I do not want	to waste
others' time to do the changes that I can do it very easily. 
Anyway, maybe It is the time for me to request for check-in permission. 
Aha, The TIME is coming. :-)

Comment 23

[Gilbert Fang]

Comment on attachment 92525 [details] [diff] [review]
new patch after bryner's sr(patch_bz138191_20020724_a)

carrying  r=jkeiser and sr=bryner

Comment 24

[Asa Dotzler [:asa]]

Comment on attachment 92525 [details] [diff] [review]
new patch after bryner's sr(patch_bz138191_20020724_a)

a=asa (on behalf of drivers) for checkin to 1.1

Comment 25

[Daniel Wang]

*** Bug 159289 has been marked as a duplicate of this bug. ***

Comment 26

[Gilbert Fang]

check in trunk 
1.369 <[e-mail]> 24 Jul 2002 20:14

Comment 27

[Gilbert Fang]

Attachment: patch for MOZILLA_1_0 Branch

Hi, jkeiser and bryner, Could this patch carry your r= and sr= ?

Comment 28

[Gilbert Fang]

Comment on attachment 92849 [details] [diff] [review]
patch for MOZILLA_1_0 Branch

This patch also works well in my solaris sandbox for mozilla 1.0 branch

Comment 29

[John Keiser (jkeiser)]

Comment on attachment 92849 [details] [diff] [review]
patch for MOZILLA_1_0 Branch

It's an identical patch, yeah.

Comment 30

[Kevin Hollenshead]

This works using the current nightly builds on the pages where I first observed
the error as well as the test cases I came up with to demonstrate the problem. 
Thanks.

Comment 31

[Gilbert Fang]

thanks for  Kevin Hollenshead 's testing

Comment 32

[sairuh (rarely reading bugmail)]

using the attached test case, this is fixed using 2002.09.19.08 comm trunk
builds on all platforms.