617304 - Keyboard events get sent twice when doorhanger is active

Status: VERIFIED FIXED

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

Description

[Stefan]

User-Agent:       
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20100101 Firefox/4.0b8pre

If a doorhanger is active and a form is submitted by pressing the space bar, the form's obsubmit code gets invoked twice (or more times).



Reproducible: Always

Steps to Reproduce:
1. open doorhang.html (no mouse movement from here!)
2. go to first input field (press tab single time)
3. enter 'u' - <TAB> - 'u' - <TAB> - <SPACE> (doorhanger appears, focus goes to "execute"-button)
4. press <SPACE>
Actual Results:  
123 123 

Expected Results:  
123

Comment 1

[Stefan]

Attachment: testcase: doorhang.html

Comment 2

[Ed Morley [:emorley]]

WFM using Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101206 Firefox/4.0b8pre ID:20101206030318.

Can you post your full useragent as appears on about:support please and if you have not already, try with the latest nightly - thanks.

Comment 3

[Stefan]

The tests were performed with a version

Built from http://hg.mozilla.org/mozilla-central/rev/6c2e0020c898

which I have built from the mercurial repository. This:

»Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20100101 Firefox/4.0b8pre«

is the full user agent of that version which I have built today.

Comment 4

[Ed Morley [:emorley]]

Does this still occur, if you try a newer nightly?
How about when using the x86 version or say 3.6.13?

Can you also try with a new, empty profile.

Thanks!

Comment 5

[Stefan]

Yes it does still occur.

1. In 
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b9pre) Gecko/20100101 Firefox/4.0b9pre Built from http://hg.mozilla.org/mozilla-central/rev/ad8da8012e3a

2. In the latest nightly 2010-12-17-03-mozilla-central with empty profile
Mozilla/5.0 (X11; Linux x86_64; rv:2.0b9pre) Gecko/20101217 Firefox/4.0b9pre
Built from http://hg.mozilla.org/mozilla-central/rev/d1da1005b6d6

3. FF 3.5.16 and FF 3.6.13 do NOT show the issue.

Comment 6

[Stefan]

Attachment: testcase some space added top of the fields

Comment 7

[Ed Morley [:emorley]]

Would you be able to find the regression range using http://harthur.github.com/mozregression/ ?

I'd do it myself but I can't reproduce (presume it must either be a Linux only or else x64 issue).

Thanks!

Comment 8

[Stefan]

I wouldn't call it a regression. AFAIR the issue was already present in the first doorhanger implementation that came in 6a8048ca8b63 (Bug 567814). I'll check this.

Comment 9

[Stefan]

OK. Win does not seem to be affected.

2010-08-31-03-mozilla-central/ has the old style notification bar and is OK.
2010-09-30-03-mozilla-central/ has the doorhanger and is affected.

Comment 10

[Stefan]

Here is the result of the mercurial bisection:

first bad is 6a8048ca8b63 (first with doorhanger)
last good is 95289cf4beac (last with notification bar)

Comment 11

[Mounir Lamouri (:mounir)]

I'm using a 64 bits GNU/Linux build and I can't reproduce that.
Can you describe how to use your test case with the steps and the expected result and the results that happen when it's failing. I tried to use your test case and I was not able to see any issue. I wrote my own test case and it was fine too.

Comment 12

[Stefan]

step by step, "->" denotes result.

1. Open the testcase' url in a FF with *new* *profile*.
   -> Testcase appears.
2. Now use the mouse for the last time: Click into the first field (this
   is the upper- and leftmost field.
   -> textcursor appears in this field and blinks.
3. press the key lables 'U' on your keyboard
   -> 'u' is displayed in the first field, cursor advances
4. press the TAB key on your keyboard
   -> textcursor goes to second field and blinks
5. press the key lables 'U' on your keyboard
   -> 'u' is displayed in the second field, cursor advances
6. press the TAB key on your keyboard
   -> textcursor vanished, button "submit" is focused
7. press SPACE bar on keyboard
   -> a) doorhanger appeard and asks 'Remember password for "u" on mozilla.org?"
      b) focus goes from "submit" to "execute"
8. press SPACE bar on keyboard
   -> a) focus vanished from "execute" button
      b) "123 123" appears

expeced: "123" appears

Comment 13

[Mounir Lamouri (:mounir)]

Thanks for these highly described steps. I can reproduce it now.

Comment 14

[Mounir Lamouri (:mounir)]

It looks like it is not really related because form submission but all keyboard events are send twice when the doorhanger is visible. The form is submitted twice when you press ENTER or SPACE on the submit button because it gots two events.

Comment 15

[Mounir Lamouri (:mounir)]

(In reply to comment #14)
> It looks like it is not really related because form submission but all keyboard

It looks like it is not really related *to* form submission *because* all keyboard

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

Neil should know how doorhangers work.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

I can't reproduce on Linux.  I followed step-to-reproduce from comment 12 and
got 123 in the bottom of the page.

Comment 18

[Stefan]

@Olli: Well, you can *no* *longer* reproduce it. The first nightly which behaves correctly is 2010-12-28-03-mozilla-central.

Comment 19

[Stefan]

The issue vanished with

changeset:   59702:b7acaa08a9c5
user:        Boris Zbarsky <[e-mail]>
date:        Mon Dec 27 14:42:10 2010 -0600
summary:     Bug 331959 and bug 127903.  Make situations in which an anchor or submit control is nested inside another anchor and the inner thing is clicked trigger the inner thing, not both.  r=smaug,gavin a=blocker

Comment 20

[:Gavin Sharp [email: [e-mail]]]

We should make sure we know *why* that fixed this... On first glance it doesn't look particularly related.

Comment 21

[Boris Zbarsky [:bzbarsky]]

And we should definitely add a test to make sure this doesn't regress.

Comment 22

[Mounir Lamouri (:mounir)]

(In reply to comment #20)
> We should make sure we know *why* that fixed this... On first glance it doesn't
> look particularly related.

Boris' patch is adding |aVisitor.mEventStatus = nsEventStatus_eConsumeNoDefault;| at two places in nsHTMLInputElement.cpp and nsHTMLButtonElement.cpp. Those lines are fixing this bug.

Comment 23

[Mounir Lamouri (:mounir)]

Attachment: Tests

I've set the review to Gavin given that he wrote the initial test suite where my test has been added but I guess Olli and Boris, you could review it if you feel like it :)

Comment 24

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 500344 [details] [diff] [review]
Tests

do you know that this test failed prior to the fix (i.e. with changed mentioned in comment 22 reverted)? We should make sure of that, otherwise there isn't much point in adding this. Also it looks like you'll need to do a trivial merging (this test is now #21, not #18)

sorry for the ridiculous delay