1316330 - Preventing return key event no longer prevents form from submitting

Status: RESOLVED FIXED

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

Description

[Josh Matthews [:jdm]]

FF 49 changed the behaviour of the testcase at https://jsfiddle.net/vy8nuvz6/3/ to submit the form when enter is pressed.

Comment 1

[Josh Matthews [:jdm]]

Working on isolating the exact cause of the behaviour change.

Comment 2

[Josh Matthews [:jdm]]

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.

Comment 3

[Josh Matthews [:jdm]]

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

Comment 4

[Josh Matthews [:jdm]]

I don't really know where to go from here. I'm open to suggestions!

Comment 5

[Andrew Overholt [:overholt]]

Stone, do you have time to help Josh out a bit?

Comment 6

[Ming-Chou Shih [:stone]]

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.

Comment 7

[Ming-Chou Shih [:stone]]

May I learn from you if you have any comments or suggestions about this case? Thanks.

Comment 8

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sounds like exactly same bug of bug 1309587.

Comment 9

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

indeed. both this and bug 1309587 is about spinning event loop while doing a "sync operation".

Comment 10

[Marcia Knous [:marcia]]

Tracking 52+ so we can get to the bottom of what is going on here.

Comment 11

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Jessica has been working on bug 1309587 for a while, assigning this to her. Thank you Stone and Jessica!

Comment 12

[Hsin-Yi Tsai (she/her) [:hsinyi]]

It's late for 49 and 50 so mark wontfix.

Comment 13

[Gerry Chang [:gchang]]

Track 51+ as regression.

Comment 14

[Jessica Jong [:jessica] (inactive)]

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?

Comment 15

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

Hmm, why does non-e10s behave differently here. nsPrompter.js should still get keyevents there.

Comment 16

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 17

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

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.

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 19

[Jessica Jong [:jessica] (inactive)]

(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

Comment 20

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 21

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

(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.

Comment 22

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

(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.

Comment 23

[Jessica Jong [:jessica] (inactive)]

(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?

Comment 24

[Jessica Jong [:jessica] (inactive)]

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.

Comment 25

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

(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.

Comment 26

[Jessica Jong [:jessica] (inactive)]

Attachment: 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

Comment 27

[Jessica Jong [:jessica] (inactive)]

(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

Comment 28

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

Oh, right we do http://searchfox.org/mozilla-central/rev/ac40ca3ec39efe85bfb111274c10ee4ceea5bb7a/dom/base/nsDocument.cpp#9225
Is that perhaps causing the issue here?

Comment 29

[Jessica Jong [:jessica] (inactive)]

(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..

Comment 30

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v1.

Olli, I changed to use inline events to avoid breaking the test. Could you take a look? Thanks.

Comment 31

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v2.

Added test case.

Comment 32

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

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?

Comment 33

[Jessica Jong [:jessica] (inactive)]

(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.. :/

Comment 34

[Jessica Jong [:jessica] (inactive)]

(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...

Comment 35

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

Does http://searchfox.org/mozilla-central/source/dom/tests/mochitest/bugs/test_bug61098.html perhaps help?

Comment 36

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v3.

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

Comment 37

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

So the testcase makes sense only if it fails without the fix and doesn't fail with the fix.

Comment 38

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

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 39

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

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

Comment 40

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 41

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Please do not use nsIDOMWindowUtils.sendKeyEvent(). It's already deprecated. Use or improve nsITextInputProcessor instead.

Comment 42

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v4.

It's me again! :)

Changed to use browser chrome test, could you take a look at the new test? Thanks.

Comment 43

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

Comment on attachment 8825701 [details] [diff] [review]
patch, v4.

I assume the test fails without the code changes and pass with, so r+

Comment 44

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

Does the test work in non-e10s too or should it be marked skip-if = !e10s or so?

Comment 45

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 46

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v5.

Addressed nit: { goes to its own line. Carrying r+.

Comment 47

[Jessica Jong [:jessica] (inactive)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c0336e9ef558145bf7d463cc4b1b1570dff4b045&group_state=expanded&selectedJob=68672054

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a60d62c55e27
Cancel delayed keypress events if last keydown was canceled. r=smaug

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a60d62c55e27

Comment 50

[Ryan VanderMeulen [:RyanVM]]

Probably a wontfix for 51 at this point (we're creating the RC build today), but please request Aurora approval on this at least.

Comment 51

[Jessica Jong [:jessica] (inactive)]

Attachment: patch for aurora.

Comment 52

[Jessica Jong [:jessica] (inactive)]

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

Comment 53

[Julien Cristau [:jcristau]]

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?

Comment 54

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fd642e0cca77

Comment 55

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 56

[Gerry Chang [:gchang]]

Too late for 51. Mark 51 won't fix.

Comment 57

[Jorg K (CEST = GMT+2)]

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.

Comment 58

[Jorg K (CEST = GMT+2)]

As discussed with Ryan on IRC: He'll back that out from Aurora for now.

Comment 59

[Iris Hsiao [:ihsiao]]

After discussed with jessica, I have backed this out from Aurora.
https://hg.mozilla.org/releases/mozilla-aurora/rev/4a274293090d

Comment 60

[Jorg K (CEST = GMT+2)]

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.

Comment 61

[Jorg K (CEST = GMT+2)]

Olli suggested in bug 1332433 comment #12 to have this backed out.

Dear Sheriff, can you please back this out from M-C.

Comment 62

[Carsten Book [:Tomcat]]

backed out in https://hg.mozilla.org/mozilla-central/rev/e674ae0954df

Comment 63

[Julien Cristau [:jcristau]]

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.

Comment 64

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, v6.

rebased after backout, backout from mozilla-central did not revert added files.

Comment 65

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, followup to fix bug 1332433.

As discussed with Olli on IRC, focus events from WindowRaised are called from window activation, which should be suppressed/delayed.

Comment 66

[Jorg K (CEST = GMT+2)]

(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.

Comment 67

[Carsten Book [:Tomcat]]

(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 ?

Comment 68

[Jorg K (CEST = GMT+2)]

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.

Comment 69

[Jorg K (CEST = GMT+2)]

(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.

Comment 70

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, followup to fix bug 1332433, v2.

Comment 71

[Jorg K (CEST = GMT+2)]

Attachment 8830635 [details] [diff] still works fine in TB (in case anyone cares ;-)).

Comment 72

[Jessica Jong [:jessica] (inactive)]

(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 73

[Jessica Jong [:jessica] (inactive)]

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.

Comment 74

[Jorg K (CEST = GMT+2)]

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

Comment 75

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

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 76

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

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)

Comment 77

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, followup to fix bug 1332433, v3.

Addressed review comment 76, carrying r+.

Comment 78

[Jessica Jong [:jessica] (inactive)]

checkin-needed for attachment 8830168 [details] [diff] [review] and attachment 8833196 [details] [diff] [review] to trunk.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02177dfe72d53daa7ca7d2796383c5a9486a9271&group_state=expanded

Comment 79

[Carsten Book [:Tomcat]]

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

Comment 80

[Jessica Jong [:jessica] (inactive)]

Attachment: patch, followup to fix bug 1332433, v3 (rebased).

rebased.

Comment 81

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1dcbda94eac
Cancel delayed keypress events if last keydown was canceled. r=smaug

Comment 82

[Jorg K (CEST = GMT+2)]

Sorry, I've been watching this bug. So the changeset in comment #81 is the combined patch. Thanks, Ryan.

Comment 83

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a1dcbda94eac

Comment 84

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 85

[Jorg K (CEST = GMT+2)]

As per comment #66, the patch could go "as is" to M-A, but M-B needs a different patch.

Comment 86

[Jessica Jong [:jessica] (inactive)]

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

Comment 87

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8830168 [details] [diff] [review]
patch, v6.

Nice to see this fixed! And with some tests. Let's bring it to aurora.

Comment 88

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/13ef12e80f26

Comment 89

[Boris Zbarsky [:bzbarsky]]

Doesn't this basically back out bug 956704 (but leave all the "suspend animations" machinery hanging around and unused)?  Why is that ok?

Comment 90

[Jessica Jong [:jessica] (inactive)]

(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.

Comment 91

[Ryan VanderMeulen [:RyanVM]]

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

Comment 92

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(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.