Closed Bug 1316330 Opened 8 years ago Closed 7 years ago

Preventing return key event no longer prevents form from submitting

Categories

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

49 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox49 --- wontfix
firefox50 - wontfix
firefox51 + wontfix
firefox52 + wontfix
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: jdm, Assigned: jessica)

References

(Regressed 1 open bug)

Details

(Keywords: regression, site-compat)

Attachments

(2 files, 10 obsolete files)

6.83 KB, patch
jessica
: review+
Details | Diff | Splinter Review
11.47 KB, patch
jessica
: review+
Details | Diff | Splinter Review
FF 49 changed the behaviour of the testcase at https://jsfiddle.net/vy8nuvz6/3/ to submit the form when enter is pressed.
Working on isolating the exact cause of the behaviour change.
Curiously, mozregression shows me different behaviour with a scratch profile than reusing a blank profile - in mozregression I only see the "onclick" alert before the form submits.
Ok, summary of weird, conflicting results:
* changing alert into console.log makes this problem disappear (theory: we end up running the queued onclick event while the first alert box is showing)
* running mozregression consistently shows only the "onclick" alert, no matter what regression range is chosen, no matter what profile is used
* running builds downloaded manually consistently shows the behaviour change in the regression range originally reported
I don't really know where to go from here. I'm open to suggestions!
Stone, do you have time to help Josh out a bit?
Flags: needinfo?(sshih)
Assignee: nobody → sshih
Flags: needinfo?(sshih)
This is related to e10s. We fire eKeyDown event to content, keep event status in mIgnoreKeyPressEvent to prevent the following eKeyPress event if content preventDefault on eKeyDown.

In the test case of this bug, js handler preventDefault on eKeyDown and alert. Alert (nsPrompter.js) calls nsThread::ProcessNextEvent to continue processing subsequent messages (including eKeyPress). At this moment, the call stack of handling eKeyDown is still pending for the alert and mIgnoreKeyPressEvent isn't updated. So the eKeyPress is fired to content and induce form submitting. That's why changing alert into console.log makes this problem disappear.

So far I have no idea how to handle this case correctly. Need some suggestions from experts.
May I learn from you if you have any comments or suggestions about this case? Thanks.
Flags: needinfo?(masayuki)
Flags: needinfo?(bugs)
Sounds like exactly same bug of bug 1309587.
Flags: needinfo?(masayuki)
indeed. both this and bug 1309587 is about spinning event loop while doing a "sync operation".
Flags: needinfo?(bugs)
Tracking 52+ so we can get to the bottom of what is going on here.
Jessica has been working on bug 1309587 for a while, assigning this to her. Thank you Stone and Jessica!
Assignee: sshih → jjong
It's late for 49 and 50 so mark wontfix.
Track 51+ as regression.
Although this sounds very similar to bug 1309587: "event not handled correctly in parent side due to a blocked event in content side", I am not sure whether the solution will be the same. Since in bug 1309587 we can know when a event is suppressed/delayed in nsPresShell cause 'mEventsSuppressed' is explicitly set, however in this case, event is blocked in nsPrompter.js and does not return to nsPresShell, we have to detect this case using other methods. Another question is, do we need to queue subsequent key events when a keydown event is blocked?
Flags: needinfo?(masayuki)
Flags: needinfo?(bugs)
Hmm, why does non-e10s behave differently here. nsPrompter.js should still get keyevents there.
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from comment #15)
> Hmm, why does non-e10s behave differently here. nsPrompter.js should still
> get keyevents there.

In non-e10s, keypress is not fired if keydown is default-prevented, this is done in widget code. So, when nsPrompter.js keep processing subsequent events, there is no keypress event in there.

In e10s, keydown is not default-prevented in parent side, so keydown and keypress are both sent to content side, and we only drop keypress in TabChild when mIgnoreKeyPressEvent is set.
Flags: needinfo?(bugs)
oh, and we don't set mIgnoreKeyPressEvent because we're spinning event loop and keypress gets through, right? Interesting case.
Do we still suppress events on the content document? I sure hope so, and we should.
But assuming that is done, sounds like we need tweaking to mDelayedEvents handling. 
We should somehow know in the mDelayedEvents whether keydown causing the suppression was cancelled, and if so, not dispatch keypress. That shouldn't be too hard.
Flags: needinfo?(bugs)
I think that like composition events, we have some mechanism which guarantees keydown event and keypress event are fired in same document. For example, if we dispatch keypress events to different domain's document, it may cause something bad effect. So, ideally, we should fix bug 1181501 (although, it needs a lot of time).
Flags: needinfo?(masayuki)
(In reply to Olli Pettay [:smaug] (vacation Nov 19-26) from comment #17)
> oh, and we don't set mIgnoreKeyPressEvent because we're spinning event loop
> and keypress gets through, right? Interesting case.

Exactly.

> Do we still suppress events on the content document? I sure hope so, and we
> should.

Now we only suppress animations when entering modal state [1].

> But assuming that is done, sounds like we need tweaking to mDelayedEvents
> handling. 
> We should somehow know in the mDelayedEvents whether keydown causing the
> suppression was cancelled, and if so, not dispatch keypress. That shouldn't
> be too hard.

If we also suppress input events when entering modal state, keypress will be added into mDelayedEvents (but not keydown), so when keydown returns, we can set a flag for whether it was canceled, and when mDelayedEvents is going to dispatch the keypress event, we can check that flag before dispatching. Is that what you mean? How can we guarantee it's same the keydown/keypress pair?


[1] https://hg.mozilla.org/mozilla-central/file/b7f895c1dc2e/dom/base/nsGlobalWindow.cpp#l8977
Flags: needinfo?(bugs)
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #18)
> I think that like composition events, we have some mechanism which
> guarantees keydown event and keypress event are fired in same document. For
> example, if we dispatch keypress events to different domain's document, it
> may cause something bad effect. So, ideally, we should fix bug 1181501
> (although, it needs a lot of time).

Thanks. I agree that it'd be more consistent if we send keydown and keypress events in the same document. While bug 1181501 is being fixed, I think we'll need a solution for the current design.
(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #18)
> I think that like composition events, we have some mechanism which
> guarantees keydown event and keypress event are fired in same document.
Sounds rather regression prone. Web pages might move focus to some iframe.
At least better to test other browsers.
Flags: needinfo?(bugs)
(In reply to Jessica Jong [:jessica] from comment #19) 
> If we also suppress input events when entering modal state, keypress will be
> added into mDelayedEvents (but not keydown), so when keydown returns, we can
> set a flag for whether it was canceled, and when mDelayedEvents is going to
> dispatch the keypress event, we can check that flag before dispatching. Is
> that what you mean? How can we guarantee it's same the keydown/keypress pair?
Yeah, like that. And do we need to know it is the same keydown/keypress pair, since every key event after
preventDefaulted keydown would be prevent defaulted too.
(In reply to Olli Pettay [:smaug] from comment #22)
> (In reply to Jessica Jong [:jessica] from comment #19) 
> > If we also suppress input events when entering modal state, keypress will be
> > added into mDelayedEvents (but not keydown), so when keydown returns, we can
> > set a flag for whether it was canceled, and when mDelayedEvents is going to
> > dispatch the keypress event, we can check that flag before dispatching. Is
> > that what you mean? How can we guarantee it's same the keydown/keypress pair?
> Yeah, like that. And do we need to know it is the same keydown/keypress
> pair, since every key event after
> preventDefaulted keydown would be prevent defaulted too.

Hmm, so all the *delayed* keypress events will be canceled if a previous keydown event is canceled, right?
Now, I am getting this crash when the delayed event is dispatched:

	xul.dll!mozilla::widget::PuppetWidget::ExecuteNativeKeyBinding(nsIWidget::NativeKeyBindingsType aType, const mozilla::WidgetKeyboardEvent & aEvent, void(*)(mozilla::Command, void *) aCallback, void * aCallbackData) Line 549
 	xul.dll!nsTextInputListener::HandleEvent(nsIDOMEvent * aEvent) Line 927
 	xul.dll!mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener * aListener, nsIDOMEvent * aDOMEvent, mozilla::dom::EventTarget * aCurrentTarget) Line 1133
 	xul.dll!mozilla::EventListenerManager::HandleEventInternal(nsPresContext * aPresContext, mozilla::WidgetEvent * aEvent, nsIDOMEvent * * aDOMEvent, mozilla::dom::EventTarget * aCurrentTarget, nsEventStatus * aEventStatus) Line 1286
 	xul.dll!mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor & aVisitor, mozilla::ELMCreationDetector & aCd) Line 318
 	xul.dll!mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem> & aChain, mozilla::EventChainPostVisitor & aVisitor, mozilla::EventDispatchingCallback * aCallback, mozilla::ELMCreationDetector & aCd) Line 465
 	xul.dll!mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem> & aChain, mozilla::EventChainPostVisitor & aVisitor, mozilla::EventDispatchingCallback * aCallback, mozilla::ELMCreationDetector & aCd) Line 520
 	xul.dll!mozilla::EventDispatcher::Dispatch(nsISupports * aTarget, nsPresContext * aPresContext, mozilla::WidgetEvent * aEvent, nsIDOMEvent * aDOMEvent, nsEventStatus * aEventStatus, mozilla::EventDispatchingCallback * aCallback, nsTArray<mozilla::dom::EventTarget *> * aTargets) Line 825
 	xul.dll!PresShell::ForwardKeyToInputMethodAppOrDispatch(bool aIsTargetRemote, nsINode * aTarget, mozilla::WidgetKeyboardEvent & aEvent, nsEventStatus * aStatus, mozilla::EventDispatchingCallback * aEventCB) Line 7303
 	xul.dll!PresShell::HandleKeyboardEvent(nsINode * aTarget, mozilla::WidgetKeyboardEvent & aEvent, bool aEmbeddedCancelled, nsEventStatus * aStatus, mozilla::EventDispatchingCallback * aEventCB) Line 7258	C++
 	xul.dll!PresShell::DispatchEventToDOM(mozilla::WidgetEvent * aEvent, nsEventStatus * aStatus, nsPresShellEventCB * aEventCB) Line 8440
 	xul.dll!PresShell::HandleEventInternal(mozilla::WidgetEvent * aEvent, nsEventStatus * aStatus, bool aIsHandlingNativeEvent) Line 8316
 	xul.dll!PresShell::HandleEvent(nsIFrame * aFrame, mozilla::WidgetGUIEvent * aEvent, bool aDontRetargetEvents, nsEventStatus * aEventStatus, nsIContent * * aTargetContent) Line 8023
 	xul.dll!PresShell::HandleEvent(nsIFrame * aFrame, mozilla::WidgetGUIEvent * aEvent, bool aDontRetargetEvents, nsEventStatus * aEventStatus, nsIContent * * aTargetContent) Line 7490
 	xul.dll!nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent * aEvent, nsView * aView, nsEventStatus * aStatus) Line 816
 	xul.dll!nsView::HandleEvent(mozilla::WidgetGUIEvent * aEvent, bool aUseAttachedEvents) Line 1118
 	xul.dll!mozilla::widget::PuppetWidget::DispatchEvent(mozilla::WidgetGUIEvent * event, nsEventStatus & aStatus) Line 357
 	xul.dll!PresShell::DelayedInputEvent::Dispatch() Line 9923
 	xul.dll!PresShell::FireOrClearDelayedEvents(bool aFireEvents) Line 9147	C++
 	xul.dll!FireOrClearDelayedEvents(nsTArray<nsCOMPtr<nsIDocument> > & aDocuments, bool aFireEvents) Line 9186
 	xul.dll!nsDelayedEventDispatcher::Run() Line 9457
 	xul.dll!nsThread::ProcessNextEvent(bool aMayWait, bool * aResult) Line 1213
 	xul.dll!NS_ProcessNextEvent(nsIThread * aThread, bool aMayWait) Line 361
 	xul.dll!mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate * aDelegate) Line 96
 	xul.dll!mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate * aDelegate) Line 301	C
 	xul.dll!MessageLoop::RunHandler() Line 226
 	xul.dll!MessageLoop::Run() Line 206
 	xul.dll!nsBaseAppShell::Run() Line 158
 	xul.dll!nsAppShell::Run() Line 264
 	xul.dll!XRE_RunAppShell() Line 924
 	xul.dll!mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate * aDelegate) Line 269
 	xul.dll!MessageLoop::RunHandler() Line 226
 	xul.dll!MessageLoop::Run() Line 206
 	xul.dll!XRE_InitChildProcess(int aArgc, char * * aArgv, const XREChildData * aChildData) Line 760

Need to fix this first... maybe file a separate bug for this.
(In reply to Jessica Jong [:jessica] from comment #23)
> 
> Hmm, so all the *delayed* keypress events will be canceled if a previous
> keydown event is canceled, right?
Yeah, I was thinking something as simple as that would work normally. Otherwise deciding which event to cancel becomes rather tricky.
Attached patch WIP (obsolete) — Splinter Review
This patch solves the issue, however, it's breaking toolkit/components/passwordmgr/test/test_master_password.html, which is run on non-e10s only. Need to dig deeper..

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a056aec035ffb452dcf0241e4ee96ba2fa97d16d
(In reply to Jessica Jong (OOO Dec.12-16) [:jessica] from comment #26)
> Created attachment 8819768 [details] [diff] [review]
> WIP
> 
> This patch solves the issue, however, it's breaking
> toolkit/components/passwordmgr/test/test_master_password.html, which is run
> on non-e10s only. Need to dig deeper..
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=a056aec035ffb452dcf0241e4ee96ba2fa97d16d

I tested locally on mac and ubuntu, and could only reproduce this on mac. It seems that iframe2 does not get the load event [1] after we suppress eEvents (instead of eAnimationsOnly) when entering modal state, so checkTest4B_delay() did not get called.
Olli, do you have any hint about this? Would it be something with FocusManager? Any help is appreciated. Thanks!

[1] http://searchfox.org/mozilla-central/rev/cc2a84852bd4e6f6d8d4d5b17b8382bb5d005749/toolkit/components/passwordmgr/test/test_master_password.html#230
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #28)
> Oh, right we do
> http://searchfox.org/mozilla-central/rev/
> ac40ca3ec39efe85bfb111274c10ee4ceea5bb7a/dom/base/nsDocument.cpp#9225
> Is that perhaps causing the issue here?

right! I'll see what can be done..
Attached patch patch, v1. (obsolete) — Splinter Review
Olli, I changed to use inline events to avoid breaking the test. Could you take a look? Thanks.
Attachment #8819768 - Attachment is obsolete: true
Attachment #8821415 - Flags: feedback?(bugs)
Attachment #8821415 - Flags: feedback?(bugs) → feedback+
Attached patch patch, v2. (obsolete) — Splinter Review
Added test case.
Attachment #8821415 - Attachment is obsolete: true
Attachment #8822332 - Flags: review?(bugs)
Version: unspecified → 49 Branch
Comment on attachment 8822332 [details] [diff] [review]
patch, v2.

This is fine, but the testcase isn't actually testing the interesting case, since that requires alert() or similar.
Would it be possible to test also the alert() case?
Attachment #8822332 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #32)
> Comment on attachment 8822332 [details] [diff] [review]
> patch, v2.
> 
> This is fine, but the testcase isn't actually testing the interesting case,
> since that requires alert() or similar.
> Would it be possible to test also the alert() case?

Oh, right, how did I miss that.. :/
(In reply to Jessica Jong [:jessica] from comment #33)
> (In reply to Olli Pettay [:smaug] from comment #32)
> > Comment on attachment 8822332 [details] [diff] [review]
> > patch, v2.
> > 
> > This is fine, but the testcase isn't actually testing the interesting case,
> > since that requires alert() or similar.
> > Would it be possible to test also the alert() case?
> 
> Oh, right, how did I miss that.. :/

Sorry for bothering you again about this :(

I can not find a good way to test the alert() case since I can not dismiss the dialog from the test file. Same with other modal dialogs, it requires chrome privileges to manipulate them, but what we want to test is in e10s. Do you have any ideas? If there is no other way, I prefer to remove the test case, since it's not testing what is being fixed here...
Flags: needinfo?(bugs)
Attached patch patch, v3. (obsolete) — Splinter Review
Hi Olli, so I modified my test case according to comment 35 and it worked well. But I found that test cases use sendKey/synthesizeKey, which is different from real key events. In our case, keypress is not fired, even before the fix due to [1]. So, I am not sure if the test case makes sense... let me know if I should keep or not. Thanks.

[1] http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/widget/TextEventDispatcher.cpp#545
Attachment #8822332 - Attachment is obsolete: true
Attachment #8823955 - Flags: review?(bugs)
So the testcase makes sense only if it fails without the fix and doesn't fail with the fix.
Could you possibly use http://searchfox.org/mozilla-central/rev/82ebc0c5ea45477ef4a4bfb3bbff35069bd18cee/dom/interfaces/base/nsIDOMWindowUtils.idl#589 
and not sendKey("RETURN"); ?
EventUtils.js seem to have _getDOMWindowUtils to get DOMWindowUtils. I know, sort internal method of EventUtils, but might be useful here.
Comment on attachment 8823955 [details] [diff] [review]
patch, v3.

>+  didTryToSubmit = false;
>+  sendString("abcd");
>+  sendKey("RETURN");
So, could you try of using DOMWindowUtils.sendKeyEvent would work here?


>+      if (aEvent->mMessage == eKeyDown) {
>+        mIsLastKeyDownCanceled = aEvent->mFlags.mDefaultPrevented;
>+      }
mIsLastKeyDownCanceled isn't actually reset to false anywere (unless keydown is dispached again)

>+PresShell::DelayedKeyEvent::IsKeyPressEvent() {
{ goes to its own line
Attachment #8823955 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #39)
> Comment on attachment 8823955 [details] [diff] [review]
> patch, v3.
> 
> >+  didTryToSubmit = false;
> >+  sendString("abcd");
> >+  sendKey("RETURN");
> So, could you try of using DOMWindowUtils.sendKeyEvent would work here?

I could use DOMWindowUtils.sendKeyEvent, but this method is synchronous, so if I do:

  winUtils.sendKeyEvent("keydown", DOM_VK_RETURN, 0, modifier);
  winUtils.sendKeyEvent("keypress", DOM_VK_RETURN, 0, modifier);
  winUtils.sendKeyEvent("keyup", DOM_VK_RETURN, 0, modifier);

keypress will never be delayed, since we will have already left modal state when keydown returns. We can't simulate the case of e10s, where key events are received continuously in content side.

> 
> 
> >+      if (aEvent->mMessage == eKeyDown) {
> >+        mIsLastKeyDownCanceled = aEvent->mFlags.mDefaultPrevented;
> >+      }
> mIsLastKeyDownCanceled isn't actually reset to false anywere (unless keydown
> is dispached again)

Hmm, do we really need to reset it? mIsLastKeyDownCanceled is only used by keypress, which should be preceded by a keydown.

> 
> >+PresShell::DelayedKeyEvent::IsKeyPressEvent() {
> { goes to its own line

Will do.
Please do not use nsIDOMWindowUtils.sendKeyEvent(). It's already deprecated. Use or improve nsITextInputProcessor instead.
Attached patch patch, v4. (obsolete) — Splinter Review
It's me again! :)

Changed to use browser chrome test, could you take a look at the new test? Thanks.
Attachment #8823955 - Attachment is obsolete: true
Attachment #8825701 - Flags: review?(bugs)
Comment on attachment 8825701 [details] [diff] [review]
patch, v4.

I assume the test fails without the code changes and pass with, so r+
Attachment #8825701 - Flags: review?(bugs) → review+
Does the test work in non-e10s too or should it be marked skip-if = !e10s or so?
(In reply to Olli Pettay [:smaug] from comment #44)
> Does the test work in non-e10s too or should it be marked skip-if = !e10s or
> so?

The fix has effect only on e10s (the behavior on non-e10s was already right), but the test works in both e10s and non-e10s, so I think we can keep it.
Attached patch patch, v5. (obsolete) — Splinter Review
Addressed nit: { goes to its own line. Carrying r+.
Attachment #8825701 - Attachment is obsolete: true
Attachment #8826506 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a60d62c55e27
Cancel delayed keypress events if last keydown was canceled. r=smaug
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/a60d62c55e27
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Probably a wontfix for 51 at this point (we're creating the RC build today), but please request Aurora approval on this at least.
Flags: needinfo?(jjong)
Attached patch patch for aurora. (obsolete) — Splinter Review
Comment on attachment 8827357 [details] [diff] [review]
patch for aurora.

Approval Request Comment
[Feature/Bug causing the regression]: e10s
[User impact if declined]: Some sites (that cancel keydown and shows a modal dialog on the event handler) may not behave properly
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes, manually
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Slightly
[Why is the change risky/not risky?]: Now we suppress/delay input events when entering modal state (we used to suppress animations only)
[String changes made/needed]: None
Flags: needinfo?(jjong)
Attachment #8827357 - Flags: approval-mozilla-aurora?
Comment on attachment 8827357 [details] [diff] [review]
patch for aurora.

event handling fix with e10s, aurora52+

I saw bug 1309587 being mentioned as similar in this bug's comments, should we also try to get that fix in 52?
Flags: needinfo?(jjong)
Attachment #8827357 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Julien Cristau [:jcristau] from comment #53)
> Comment on attachment 8827357 [details] [diff] [review]
> patch for aurora.
> 
> event handling fix with e10s, aurora52+
> 
> I saw bug 1309587 being mentioned as similar in this bug's comments, should
> we also try to get that fix in 52?

Sure, I think we can get that fix in 52, and that fix is even less risky.
Flags: needinfo?(jjong)
Too late for 51. Mark 51 won't fix.
Depends on: 1332433
Can you please have this backed out from Aurora as it causes a visible loss of the editing cursor as described in bug 1332433 in Thunderbird and also in an editing situation if Firefox.

Aurora is going to beta next week, so this should not be in beta.

Please fix this in a follow-up bug or back it out completely from trunk.
Flags: needinfo?(jjong)
Flags: needinfo?(jcristau)
As discussed with Ryan on IRC: He'll back that out from Aurora for now.
Flags: needinfo?(ryanvm)
After discussed with jessica, I have backed this out from Aurora.
https://hg.mozilla.org/releases/mozilla-aurora/rev/4a274293090d
Thanks, please raise a follow-up bug to address the issue or also back it out from trunk. Merge day is on Monday, and you'd have to push a fix to Aurora to fix FF 53 if you fix the follow-up in FF 54.
Flags: needinfo?(ryanvm)
Flags: needinfo?(jjong)
Flags: needinfo?(jcristau)
Olli suggested in bug 1332433 comment #12 to have this backed out.

Dear Sheriff, can you please back this out from M-C.
Status: RESOLVED → REOPENED
Flags: needinfo?(ryanvm)
Flags: needinfo?(cbook)
Resolution: FIXED → ---
backed out in https://hg.mozilla.org/mozilla-central/rev/e674ae0954df
Flags: needinfo?(ryanvm)
Flags: needinfo?(cbook)
Target Milestone: mozilla53 → ---
Comment on attachment 8827357 [details] [diff] [review]
patch for aurora.

Clearing aurora approval flag since this had to be backed out.  We can consider an updated patch for beta52 if a new uplift request comes in.
Attachment #8827357 - Flags: approval-mozilla-aurora+
Attached patch patch, v6.Splinter Review
rebased after backout, backout from mozilla-central did not revert added files.
Attachment #8826506 - Attachment is obsolete: true
Attachment #8830168 - Flags: review+
As discussed with Olli on IRC, focus events from WindowRaised are called from window activation, which should be suppressed/delayed.
(In reply to Jessica Jong [:jessica] from comment #64)
> rebased after backout, backout from mozilla-central did not revert added
> files.
Something strange has happened here: The M-A backout removed the added files (tests), the M-C backout didn't:
https://hg.mozilla.org/releases/mozilla-aurora/rev/4a274293090d
in https://hg.mozilla.org/mozilla-central/rev/e674ae0954df
How is that possible?

Since merges happened two days ago the situation is now this:
Beta 52: No traces.
Aurora 53 and Central 54: Have those tests since they weren't backed out.
Hmm.

That said, I'll apply the two patches here and try it in Thunderbird.
Flags: needinfo?(cbook)
(In reply to Jorg K (GMT+1) from comment #66)
> (In reply to Jessica Jong [:jessica] from comment #64)
> > rebased after backout, backout from mozilla-central did not revert added
> > files.
> Something strange has happened here: The M-A backout removed the added files
> (tests), the M-C backout didn't:
> https://hg.mozilla.org/releases/mozilla-aurora/rev/4a274293090d
> in https://hg.mozilla.org/mozilla-central/rev/e674ae0954df
> How is that possible?
> 
> Since merges happened two days ago the situation is now this:
> Beta 52: No traces.
> Aurora 53 and Central 54: Have those tests since they weren't backed out.
> Hmm.
> 
> That said, I'll apply the two patches here and try it in Thunderbird.

so we would need to delete browser_cancel_keydown_keypress_event.js ?
Flags: needinfo?(cbook)
Well, browser_cancel_keydown_keypress_event.js and prevent_return_key.html got added in
https://hg.mozilla.org/mozilla-central/rev/a60d62c55e27 (comment #49).

To re-establish my faith in Mercurial, I'd like to know how a backout didn't remove those files, well, the corresponding M-A backout did remove them.

I don't know whether we need to remove them now, since they will be needed. Since tests which aren't run cause no harm, there's no need to remove them. However, to uplift, the beta patch needs to be different.
(In reply to Jorg K (GMT+1) from comment #66)
> That said, I'll apply the two patches here and try it in Thunderbird.
With attachment 8830168 [details] [diff] [review] only: cursor disappears.
With attachment 8830171 [details] [diff] [review] applied: cursor doesn't disappear any more. So thanks.
However, Alice still mentions a problem in bug 1332433 comment #19.
Attachment #8830171 - Attachment is obsolete: true
Attachment 8830635 [details] [diff] still works fine in TB (in case anyone cares ;-)).
(In reply to Jorg K (GMT+1) from comment #71)
> Attachment 8830635 [details] [diff] still works fine in TB (in case anyone
> cares ;-)).

I do. Appreciate your help! :)
Comment on attachment 8830635 [details] [diff] [review]
patch, followup to fix bug 1332433, v2.

Review of attachment 8830635 [details] [diff] [review]:
-----------------------------------------------------------------

Olli, as discussed, delay all focus/blur events when event handling is suppressed. Also, move "unsuppress event handling" in sync xhr to ChangeStateToDone(). 
While running try, I found that we also need to unsuppress event handling in CloseRequest() in case request was aborted. Do you know any other case that ChangeStateToDone() does not get called? Thanks.
Attachment #8830635 - Flags: review?(bugs)
(In reply to Jessica Jong [:jessica] (CNY Jan.27 - Feb.2 ) from comment #72)
> I do. Appreciate your help! :)
I can try FF, too, if you have another try build. But I'm sure the case reported in bug 1332433 comment #8 is fixed now ;-)
We could (In reply to Jessica Jong [:jessica] (CNY Jan.27 - Feb.2 ) from comment #73)
> Olli, as discussed, delay all focus/blur events when event handling is
> suppressed. Also, move "unsuppress event handling" in sync xhr to
> ChangeStateToDone(). 
> While running try, I found that we also need to unsuppress event handling in
> CloseRequest() in case request was aborted. Do you know any other case that
> ChangeStateToDone() does not get called? Thanks.

on option is to keep also the current unsuppress code, but only call it if we still have suppressed doc stored.
Comment on attachment 8830635 [details] [diff] [review]
patch, followup to fix bug 1332433, v2.

> XMLHttpRequestMainThread::CloseRequest()
> {
>   if (mChannel) {
>     mChannel->Cancel(NS_BINDING_ABORTED);
>   }
>   if (mTimeoutTimer) {
>     mTimeoutTimer->Cancel();
>   }
>+
>+  if (mFlagSynchronous) {
>+    if (mSuspendedDoc) {
>+      mSuspendedDoc->UnsuppressEventHandlingAndFireEvents(nsIDocument::eEvents,
>+                                                         true);
>+      mSuspendedDoc = nullptr;
>+    }
>+
>+    if (mResumeTimeoutRunnable) {
>+      NS_DispatchToCurrentThread(mResumeTimeoutRunnable);
>+      mResumeTimeoutRunnable = nullptr;
>+    }
>+  }
You don't need to add this if you keep the exiting Unsuppress code where we spin event loop.
So, move this code to XMLHttpRequestMainThread::SendInternal and unsuppress there if we still
have mSuspendedDoc set. Same with mResumeTimeoutRunnable.

Might be even nice to have some helper method to unsuppress and call NS_DispatchToCurrentThread(mResumeTimeoutRunnable);
and then call that method in ::SendInternal and in XMLHttpRequestMainThread::ChangeStateToDone

> }
> 
> void
> XMLHttpRequestMainThread::CloseRequestWithError(const ProgressEventType aType)
> {
>   CloseRequest();
> 
>   ResetResponse();
>@@ -2264,16 +2277,29 @@ XMLHttpRequestMainThread::ChangeStateToD
>   ChangeState(State::done, true);
> 
>   // Per spec, if we failed in the upload phase, fire a final error
>   // and loadend events for the upload after readystatechange=4/done.
>   if (!mFlagSynchronous && mUpload && !mUploadComplete) {
>     DispatchProgressEvent(mUpload, ProgressEventType::error, 0, -1);
>   }
> 
>+  if (mFlagSynchronous) {
>+    if (mSuspendedDoc) {
>+      mSuspendedDoc->UnsuppressEventHandlingAndFireEvents(nsIDocument::eEvents,
>+                                                         true);
>+      mSuspendedDoc = nullptr;
>+    }
>+
>+    if (mResumeTimeoutRunnable) {
>+      NS_DispatchToCurrentThread(mResumeTimeoutRunnable);
>+      mResumeTimeoutRunnable = nullptr;
>+    }
>+  }
>+
I think you should have this new code before ChangeState(State::done, true); since ChangeState may fire events

>-    if (suspendedDoc) {
>-      suspendedDoc->UnsuppressEventHandlingAndFireEvents(nsIDocument::eEvents,
>-                                                         true);
>-    }
>-
>-    if (resumeTimeoutRunnable) {
>-      NS_DispatchToCurrentThread(resumeTimeoutRunnable);
>-    }
So you would still have this code but just use the stored member variables and null them after use
(or have the helper method to do it)
Attachment #8830635 - Flags: review?(bugs) → review+
Addressed review comment 76, carrying r+.
Attachment #8830635 - Attachment is obsolete: true
Attachment #8833196 - Flags: review+
seems this has problems to apply like patching file dom/tests/browser/browser.ini
Hunk #1 FAILED at 39
1 out of 1 hunks FAILED -- saving rejects to file dom/tests/browser/browser.ini.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh bug-1316330-prevent-return.patch
Flags: needinfo?(jjong)
rebased.
Attachment #8833196 - Attachment is obsolete: true
Flags: needinfo?(jjong)
Attachment #8833279 - Flags: review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1dcbda94eac
Cancel delayed keypress events if last keydown was canceled. r=smaug
Keywords: checkin-needed
Sorry, I've been watching this bug. So the changeset in comment #81 is the combined patch. Thanks, Ryan.
https://hg.mozilla.org/mozilla-central/rev/a1dcbda94eac
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Please nominate this for Aurora/Beta approval when you feel it's sufficiently baked. As noted by Jorg, I folded the two patches together when I pushed it to trunk. Presumably we'll do the same for the branch uplifts.
Flags: needinfo?(jjong)
Attachment #8827357 - Attachment is obsolete: true
As per comment #66, the patch could go "as is" to M-A, but M-B needs a different patch.
Comment on attachment 8830168 [details] [diff] [review]
patch, v6.

Approval Request Comment
[Feature/Bug causing the regression]: e10s
[User impact if declined]:  Some sites (that shows a modal dialog on keydown and cancels the event) may not behave properly
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: attachment 8833279 [details] [diff] [review]
[Is the change risky?]: Slightly
[Why is the change risky/not risky?]: Now we suppress/delay input events when entering modal state (we used to suppress animations only)
[String changes made/needed]: None
Flags: needinfo?(jjong)
Attachment #8830168 - Flags: approval-mozilla-aurora?
Comment on attachment 8830168 [details] [diff] [review]
patch, v6.

Nice to see this fixed! And with some tests. Let's bring it to aurora.
Attachment #8830168 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 1338911
Doesn't this basically back out bug 956704 (but leave all the "suspend animations" machinery hanging around and unused)?  Why is that ok?
Flags: needinfo?(jjong)
Flags: needinfo?(bugs)
(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #89)
> Doesn't this basically back out bug 956704 (but leave all the "suspend
> animations" machinery hanging around and unused)?  Why is that ok?

Oh right, but I tested the STR in bug 956704 comment 0 and it works fine now. I'll file another bug for cleaning up 'suspend animations' stuff.
Flags: needinfo?(jjong)
Flags: needinfo?(bugs)
Is this something we're still considering for possible uplift to 52 or should we wontfix it? I know that any uplift would be blocked on fixing bug 1338911 first, though :)
Flags: needinfo?(jjong)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #91)
> Is this something we're still considering for possible uplift to 52 or
> should we wontfix it? I know that any uplift would be blocked on fixing bug
> 1338911 first, though :)
Per more discussion with Jessica, I think we should wontfix it based on the timeframe we are now, according to the facts that a) it's not a new issue, b) there are more uplifts to cover this thoroughly, and c) it sounds regression prone.
Flags: needinfo?(jjong)
Blocks: 1340086
Depends on: 1358926
Component: Event Handling → User events and focus handling
Regressions: 1565505
You need to log in before you can comment on or make changes to this bug.