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?

Confirming on a current linux nightly...

-> bryner

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.

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

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

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.

Could some one give me a r=?

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.

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

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

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

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.

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 on attachment 92358 [details] [diff] [review] patch following jkeiser's review Yep, r=jkeiser :)

Thanks to jkeiser.

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?

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

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?

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 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)

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 on attachment 92525 [details] [diff] [review] new patch after bryner's sr(patch_bz138191_20020724_a) carrying r=jkeiser and sr=bryner

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

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

check in trunk 1.369 <henry.jia@sun.com> 24 Jul 2002 20:14

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

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 on attachment 92849 [details] [diff] [review] patch for MOZILLA_1_0 Branch It's an identical patch, yeah.

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.

thanks for Kevin Hollenshead 's testing

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