Closed
Bug 48064
Opened 24 years ago
Closed 24 years ago
Pressing Enter in text input causes 2 onchange events to fire
Categories
(Core :: DOM: UI Events & Focus Handling, defect, P2)
Core
DOM: UI Events & Focus Handling
Tracking
()
VERIFIED
FIXED
People
(Reporter: yshkolni, Assigned: mjudge)
References
()
Details
(Whiteboard: [rtm++]fixed in branch)
Attachments
(5 files)
(deleted),
text/plain
|
Details | |
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review |
From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; WinNT4.0; en-US; m17) Gecko/20000807
BuildID: 2000080712
Browser doesn't wait for the security warning about submitting insecure data
before proceeding.
Reproducible: Always
Steps to Reproduce:
1. Open Mozilla (fresh)
2. Go to http://www.mozilla.org/quality/help/bug-form.html
3. under search bugzilla type anything (for example, security warning)
4. Press enter (clicking with the mouse doesn't give you security warning, not
sure if it is a bug though)
5. A warning box will pop up, saying are you sure you want to submit?
Actual Results: The browser windows will pop up with the result even if you
didn't click on security dialog yet.
Expected Results: Wait for my input first, before proceeding with search.
1) There also should be a way to select whether you want that warning to pop up
or not from settings. (I couldn't find it though).
2) There should be no difference between submitting with pressing enter or with
the mouse, it should give the same warning.
Comment 1•24 years ago
|
||
Changing component to Crypto.
Assignee: asa → javi
Component: Browser-General → Security: Crypto
QA Contact: doronr → junruh
Comment 3•24 years ago
|
||
The Bugzilla Helper isn't expecting the form to be submitted.
Comment 4•24 years ago
|
||
Last time I saw this behavior it was a problem with calling the nsIDOMWindow
methods not blocking until the user had pushed a button.
Keywords: nsbeta3
Whiteboard: {nsbeta3+]
Updated•24 years ago
|
Whiteboard: {nsbeta3+] → [nsbeta3+]
Comment 6•24 years ago
|
||
PDT agrees P2 (lchiang temporarily using trudelle's system).
Whiteboard: [nsbeta3+] → [nsbeta3+][PDTP2]
Comment 7•24 years ago
|
||
I'm gonna need help fixing this one. Here's a summary of what's going on:
Going to the form described above properly calls the CheckPost method which
prompts the user. The method nsSecureBrowserUIImpl::CheckPost properly calls
dialog->ConfirmCheck but before the call to ConfirmCheck returns Mozilla has
already opened up a new window with the search results.
Seems the event to open a new window is queued up and processed. This doesn't a
ppear to be a bug in psm-glue. Who else can look at this? Perhaps DOM or forms
are the problem here.
Comment 8•24 years ago
|
||
Sounds like we need blocking dialogs. danm?
Comment 10•24 years ago
|
||
Stack trace looks to me as if the newly created window(s) have JS event
handlers that are explicitly opening new windows. This doesn't make much sense
to me, and of course JS is a huge black hole in a C++ stack trace.
But in the attached trace, toward the bottom, you'll see
nsSecureBrowserUIImpl::CheckPost. Half a dozen lines above that, the modal
loop. Above that, a window (presumably the alert) has been loaded, fires a focus
event, some other window catches the corresponding blur, some text field's JS
event handler opens a new window(!?), and the onload handler of that window kind
of does the same thing, opening a third window.
I don't really get it, but it might be useful to try to figure out what JS is
executing in that stack trace.
Comment 11•24 years ago
|
||
Something else that should be added to this bug is that 2 windows with the same
content come up after hitting enter. There is something else going on here that
is beyond my area of expertise.
The longer I look into this bug, the more I'm convinced it should belong to some
one else. Probably dom or some other component.
Comment 12•24 years ago
|
||
Adding comments sent to me by jst:
Javier Delgadillo wrote:
> Hello jst,
Hello Javier!
> I don't know if you're the right person for this bug, but since you
> helped me last time I needed help with dom issues, I'm forwarding this
> to you hoping you can at least refer me to the right engineer.
>
I might not be the right person for this problem, but read on...
> I'm working on bug 48064
> (http://bugzilla.mozilla.org/show_bug.cgi?id=48064). >From my
> debugging I've found the following:
>
> in forms, if you hit enter and have the preference for warning if
> submitting data to a site insecurely mozilla doesn't wait for you to
> hit OK or cancel before bringing up the new window (matter of fact, it
> brings up two windows with the same content).
>
> My current theory is that in nsGfxTextControlFram2::SubmitAttempt a
> call to HandleDOMEvent posts an event that doesn't wait on the user to
> click OK or Cancel before it gets processed. Is this even close?
> Should I be looking somewhere else?
>
I know there are various similar problems in mozilla right now,
http://bugzilla.mozilla.org/show_bug.cgi?id=52667 is one of those, and I
talked to Rick Potts about this and the problem seems to be that mozilla
processess PLEvents in the wrong order when a modal dialog is open, IOW,
this is a very low-level problem that needs to be fixed before you can
continue working on what could possibly be the real problem in bug
48064. Also, there's a bug on pressing enter in an input field doesn't
fire the onchange event handler, you'll notice that on the bugzilla page
mentioned in the bug.
Comment 13•24 years ago
|
||
After extensive debugging, I've come to the conclusion that the code in psm-glue
is doing the right thing, ie it prompts the user to see if he/she wants to
continue to when submiting data to an insecure site.
The bug lies somwhere in the dom code or the form handling code.
Re-assigning to jst so he can re-assign to the engineer who is more qualified
than me to handle this bug. (Sorry jst, I lost the e-mail where you told me who
the main guy was for form submission.)
Assignee: javi → jst
Comment 14•24 years ago
|
||
Marking nsbeta3-, putting on rtm radar, re-assigning to Eric Pollmann, the owner
of the form submission code and the person that Javier originally wanted to
re-assign this bug to. Eric, I haven't done an in depth analysis of the bug, so
please give it to the right person if it isn't you. Thanks!
Comment 15•24 years ago
|
||
As Johnny mentioned above, this sounds like it could be a dup of bug 52667. I
tried the fix attached there with mixed results:
- The form no longer submits until Okay is pressed, but...
- The form no longer submits at all.
This looks like a nasty one... :S I'll take a closer look.
Comment 16•24 years ago
|
||
Eric, try out my patch that I posted on the xpcom newsgroup. It sounds like a
dup of the nested event queue problem.
Updated•24 years ago
|
Status: NEW → ASSIGNED
Comment 17•24 years ago
|
||
Okay, the more I look at this the less sure I am that these behaviours are
bugs. First, here's the HTML for the text input on the page:
<INPUT type=text size=60 name=keywords
onChange="SearchForBugs(this.form.keywords.value,this.form.searchLong)">
When Enter is pressed in the text input, two things happen. First, the onclick
event handler is fired. This calls SearchForBugs. In SearchForBugs, we never
submit the form, we instead open up a new window named "other" with a URL that
is manually generated in the function to look like a form post. You'll notice
in Nav 4.x, the new window opens with the search results before you acknoledge
that the for post is insecure too.
Second, since the click event is never cancelled with a "return false" in the
event handler, the form goes ahead and submits. This causes a warning dialog to
pop up, but should not prevent the above page load (not the result of the form
submit) from happening. However, once you click "ok" on the warning dialog, it
should should cause the original page to reload with some extra cruft after the
URL, like this:
http://www.mozilla.org/quality/help/bugzilla-helper.html?keywords=security+warning&searchLong=min
Okay, now let's look at the HTML for the button and see what that does:
<INPUT type=button value=Search
onClick="SearchForBugs(this.form.keywords.value,this.form.searchLong)">
Clicking on the button will have the same effect as pressing Enter in the text
field. That is, a new window is opened up, and a manually generated URL is
loaded into that window. Since this is not the result of a form submit, no
warning dialog should appear. The difference here is that <input type=button>
does not cause a form submit to appear when it is clicked on (<input
type=submit> does that) That is why no warning dialog ever appears.
The only behaviour that is wrong in this test case is that two windows are
opened up. I'm looking into that...
Comment 18•24 years ago
|
||
When entering a search term, then clicking the "Search" button, we cause two
things to happen:
1) The text field gets blurred - this causes an onchange to fire (per the HTML
spec). This causes the onchange event handler to fire (assuming the text in the
input has changed since it last received focus)
2) The button is clicked - this causes an onclick event to fire (again, per the
spec)
That explains the two windows in the button click case
However, it looks like there is one bug - there should only be one window when
you press Enter in the text input. However, the onchange event is firing
twice. The first time it is firing, there is a nested event loop - and a
OnEndLoad is firing on a crome channel (presumably the security warning dialog)
in the first event loop, which causes the onblur to fire in the second event
loop. I'll try Doug's fix to see if that fixes this problem...
Comment 19•24 years ago
|
||
I tried the patch again (fresh tip build), and had the same results as before.
Instead of doing things mostly right like we are without the patch, now the
security dialog pops up and just halts the browser in it's tracks. Nothing
loads in the new windows.
In fact, I see nothing load in the window created when clicking the "Search"
button either.
Comment 20•24 years ago
|
||
Updating summary to reflect the only bug I found in our behaviour on this page.
Component: Security: Crypto → Event Handling
OS: Windows NT → All
Hardware: PC → All
Summary: Browser not waiting for security warning → Pressing Enter in text input causes 2 onchange events to fire
Comment 22•24 years ago
|
||
Marking [rtm need info] Waiting for patch to be attached, review + super-review.
Whiteboard: [rtm+][nsbeta3-][PDTP2] → [rtm need info][nsbeta3-][PDTP2]
Comment 23•24 years ago
|
||
I'll take this one and see what I can find
Assignee: pollmann → rods
Status: ASSIGNED → NEW
Comment 24•24 years ago
|
||
Trying the newest patch that Doug posted, fixed the loading problem, but we
still post two onchange events. The double onChange event may be unrelated to
the nested event loop changes Doug is working on.
Comment 25•24 years ago
|
||
Comment 26•24 years ago
|
||
This is an editor problem.
The problem is when you hit the return key you get on onchange event and a
submit. When the page is going to be disposed of because of the submit the focus
is removed from the text field and the "blur" processing causes a second
onchange event to be fired.
So you only see the double onchange event when you have an onchange event
handler on the test field instead of a form where a submit will take place.
note: in the attached reduced testcase, if you remove the form so the submit
doesn't happen you only get on onchange fired.
reassigning to the editor group.
Assignee: rods → beppe
Comment 27•24 years ago
|
||
After speaking with kevin, he was wondering if the text field should be getting
a blur when the document was going away. Something to think about anyway.
Comment 29•24 years ago
|
||
Simon, shouldn't this rally go to joki? it seems like it is more in his area
than it is an editor issue
Assignee | ||
Comment 31•24 years ago
|
||
i have been messing with this and i cant seem to see 2 blurs. i will look into
this more.
Status: NEW → ASSIGNED
Assignee | ||
Comment 32•24 years ago
|
||
ok i belive i have a fix i will post bug. this is LOW RISK I REPEAT LOW RISK. in
fact i think it was an oversight on my part that i did not update mFocusedValue
unless a focus was called and did not think that multiple blur/entrekey could be
called until a focus was called.
post rtm we should remove the need for a string such as focusstring and use the
editors internal state to decide if something has changed.
Assignee | ||
Comment 33•24 years ago
|
||
Comment 34•24 years ago
|
||
Yup, looks similar to Windows-specific that code chouck wrote five years ago to
implement onchange in the pre-beta-Nav2.0 WinFE. a=brendan@mozilla.org. Who
will r=?
/be
/be
This looks like a dup of bug 51268. It seems to happen on Linux only, though.
joki tried fixing that (he has no access to Linux at the moment) but the fix did
not work. However, the fix seemed to be in the same direction as mjudge's.
I am adding joki to CC, can you review? And if someone could try the attached
testcase in bug 51268 and see if this patch fixes that bug as well...
Comment 36•24 years ago
|
||
Looking at the patch...
Do we need to be defensive and check mFrame after each call to CallOnChange(), in
case the onChange handler did something that blew away frmaes (see bug 51772)?
Assignee | ||
Comment 37•24 years ago
|
||
this is NOT a combination of the two patches. yes this will be taken into
account when i merge the two changes.
Assignee | ||
Comment 38•24 years ago
|
||
setting to rtm+ to get attention i guess.
Whiteboard: [rtm need info][nsbeta3-][PDTP2] → [rtm+][nsbeta3-][PDTP2]
Updated•24 years ago
|
Whiteboard: [rtm+][nsbeta3-][PDTP2] → [rtm need info][nsbeta3-][PDTP2]
Comment 39•24 years ago
|
||
You have an "a" from brendan, but you still need a second reviewer r=
Please get the reviews lined up, and then nominate to rtm-plus.
Marking rtm need info.
Also... subtle question.. is it really the correct order to first call the
onchange() method, and then set the member to the new state? I'm not familiar
with the code, but I would have guessed that the state would first be changed,
and then the onchange() event would be queued (maybe all events get serviced on
this solo thread... and it is no big deal... but the reverse order makes more
sense to me... but I don't know how the code is structured). It would also be a
shame if the onchange() synchronously (or rapidly) processed the event, based on
the *old* state (gulp).
Oh well.. just trying to help with the reviews.
Jim
Comment 40•24 years ago
|
||
jar: it turns out not to matter. Everything is synchronous on the main thread,
you can't nest KeyPress events, and mFocusedValue is used only (check lxr) to
decide if the value on blur differs. mjudge is right that this code needs to be
improved later, on the trunk, but I think he has a minimal fix for the branch.
I agree that setting mFocusedValue before calling onchange is the more general
order, if there were any way for the event handler to depend on mFocusedValue.
To get an r= from sfraser and an a= from me on the combined patch, though, maybe
we should see a patch that merges this one with the fix for bug 51772. mjudge,
whaddya say?
/be
Assignee | ||
Comment 41•24 years ago
|
||
ok here is the unified diff of 51772 and 48064. ready to go and works well. only
1 onchange fired when hit return and no crashing.
Assignee | ||
Comment 42•24 years ago
|
||
Comment 44•24 years ago
|
||
adding rtm + and sending off to pdt for approval, this also fixes 51772
+
Whiteboard: [rtm need info][nsbeta3-][PDTP2] → [rtm+][nsbeta3-][PDTP2]
Comment 45•24 years ago
|
||
marking 51772 as a dup of this since the fix for this one includes the fix for
that one
Comment 46•24 years ago
|
||
*** Bug 51772 has been marked as a duplicate of this bug. ***
Comment 47•24 years ago
|
||
PDT says need a= and r= on the actual attached combined patch.
Whiteboard: [rtm+][nsbeta3-][PDTP2] → [rtm need info][nsbeta3-][PDTP2]
Comment 48•24 years ago
|
||
PDT would also like to know why the patches were combined. If bug 51772 is a
dup of this, why was a separate fix for it merged into the fix for this?
Assignee | ||
Comment 49•24 years ago
|
||
patches were combined because I was TOLD to combine them. I dont care, I can
uncombine them. please read 51772 it describes why i combined them. I dont want
to bother people to get r= or a= if the bugs wont be taken anyway. I hate to
waste peoples time. If all you need is r= and a= by someone you trust please
let me know and i can get that in 10 min.
thanks
Assignee | ||
Comment 50•24 years ago
|
||
crap gotta mark this rtm+ or it wont show up.
Whiteboard: [rtm need info][nsbeta3-][PDTP2] → [rtm+]
Comment 51•24 years ago
|
||
Please come discuss this in person at the PDT meeting Friday. We'd like to
resolve this in one shot rather than drag it out over email and bugzilla
comments.
Whiteboard: [rtm+] → [rtm need info]
Comment 53•24 years ago
|
||
See bug 57514 - there are two onchange events firing when pressing Enter in a
text input on that test case as well.
Comment 54•24 years ago
|
||
As far as I can tell, mjudge's latest patch has been checked in. I see only one
onChange event being fired after hitting return in a text field. Is this bug
fixed then? If so, I have another complication to add, garnered from bug 57514.
The test case in that bug issues an Alert in a text field's onChange handler.
So after hitting <return>, in the middle of processing the onChange handler, the
alert window comes up, the text field is blurred, and its onChange handler called
a second time, resulting in a second alert.
Note that Navigator 4 does nearly the same thing (two alerts, but not
simultaneously) while IE brings up only one alert. Gosh, they're good. I notice
that if I change mjudge's latest patch to set mFocusedValue = blurValue before
calling CallOnChange in nsTextInputListener::KeyPress, only one alert is issued.
Just a thought.
Comment 55•24 years ago
|
||
jar was right, I was wrong: our modal dialogs are synchronous, so update
"changed" state before dispatching the event handler. How about doing a quick
patch for that bug, mjudge?
/be
Assignee | ||
Comment 56•24 years ago
|
||
jeesh i did it in one location of callonchange and not the other. moved the
setting of mFocused value to pair up with callonchange not submitattempt. I was
trying to be too clever in not wasting time when frame was going away.
ok i feel rather embarassed but here is another simple patch here with comment
explaining what i tweaked.
Assignee | ||
Comment 57•24 years ago
|
||
Comment 58•24 years ago
|
||
r=brendan@mozilla.org -- get an sr= and get this in the trunk, ok? Can we go
for limbo-rtm?
/be
Comment 59•24 years ago
|
||
sr=buster. you should probably carry over the "crash" keyword from bug 57514
since if I understand it right the way things are today we crash in that case,
and your latest patch here fixes that. That will make it more likely PDT will
still accept this fix.
Comment 60•24 years ago
|
||
Patch looks good, r=sfraser
Comment 61•24 years ago
|
||
This fix has missed the first N6 candidate build, so we can not take it unless
we respin. This bug is in candidate limbo. We will reconsider this fix once we
have a candidate in hand, but we can't take this fix before then. PDT marking [rtm+]
Whiteboard: [rtm++] → [rtm+]
Comment 62•24 years ago
|
||
PDT marking [rtm++]. This bug is now out of limbo and approved for checkin to
the branch. Please check in ASAP.
Whiteboard: [rtm+] → [rtm++]
Updated•24 years ago
|
Whiteboard: [rtm++] → [rtm++]fixed in branch
Comment 63•24 years ago
|
||
The fix has been checked in on the branch. The trunk has been in flames all
afternoon, so we'll have to wait a little while to get the fix there. Hence,
not yet marking this bug fixed.
Comment 65•24 years ago
|
||
Akkana: Can we check this in to the trunk now? It's been almost a week.
Also, can we rtm- this now to get it off the rtm radar?
Comment 66•24 years ago
|
||
Mike? Were you going to check this in on the trunk, or did you want me to?
Comment 67•24 years ago
|
||
Marking fixed, since this now works on the 11/13 trunk builds.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•