Closed Bug 1435717 Opened 2 years ago Closed Last year

[e10s] missing keyup event for AltGr [regression]

Categories

(Core :: DOM: Events, defect, P3)

58 Branch
All
Windows
defect

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox-esr52 --- wontfix
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- wontfix
firefox63 --- fixed

People

(Reporter: ossman, Assigned: masayuki)

References

Details

(Keywords: regression)

Attachments

(4 files)

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171130114058

Steps to reproduce:

1. Press AltGr
2. Release AltGr


Actual results:

1. keydown ControlLeft
2. keydown RightAlt
3. keyup ControlLeft


Expected results:

1. keydown ControlLeft
2. keydown RightAlt
3. keyup ControlLeft
4. keyup RightAlt
This works in Firefox 49, but breaks in Firefox 56 (I have not tested intermediate versions to see exactly where). Oddly enough a downgrade does not fix it without also nuking the profile. Might therefore be a duplicate of bug 1335347.

I'd like to upgrade the priority of this though as it makes noVNC (and similar applications that track key state) pretty much unusable with Firefox.

PS. User agent is wrong. I did the testing on Windows, but submitted this bug from Linux
(In reply to Pierre Ossman from comment #1)
> I have not tested intermediate versions to see exactly where
> I did the testing on Windows

Please use mozregression-gui to find the exact regression window. It's much faster than waiting for someone with an AltGr key to come along and do the testing.
https://github.com/mozilla/mozregression/releases

Link from the screenshots:
https://dvcs.w3.org/hg/d4e/raw-file/tip/key-event-test.html
Has Regression Range: --- → no
Has STR: --- → yes
Component: Untriaged → DOM: Events
Flags: needinfo?(ossman)
Product: Firefox → Core
Well, that tool didn't really help as it tests nightly builds, not release builds. But it does confirm that it is e10s that breaks things. And disabling e10s in the latest Firefox 58 also makes the issue go away.

So should I bisect when this was broken in e10s? Or should we assume it has always been there?
Flags: needinfo?(ossman)
(In reply to Pierre Ossman from comment #6)
> So should I bisect when this was broken in e10s? Or should we assume it has
> always been there?

You said it worked in Firefox 49. So if you test that version by force-enabling e10s in about:config and it doesn't work, there's no point in doing regression testing. The preference name is either browser.tabs.remote.autostart or something very similar.

> Well, that tool didn't really help as it tests nightly builds, not release
> builds.

I don't know what you mean by this. You can set the range to specific releases.

In  mozregression-gui,
1. File -> Run a new bisection
2. Next
3. Next
4. To the right of "Last known good build", click the "date" drop-down menu and pick "release". Then click the preceding drop-down menu and choose a release version. Do the same with "First known bad build".

or in mozregression's command line:
mozregression --good 49 --bad 56
Summary: missing keyup event for AltGr [regression] → [e10s] missing keyup event for AltGr [regression]
(In reply to Gingerbread Man from comment #7)
> 
> You said it worked in Firefox 49. So if you test that version by
> force-enabling e10s in about:config and it doesn't work, there's no point in
> doing regression testing. The preference name is either
> browser.tabs.remote.autostart or something very similar.
> 

I have now confirmed that in a release build of Firefox 49:

 a) Bug is present with e10s on
 b) Bug is *not* present with e10s off

> > Well, that tool didn't really help as it tests nightly builds, not release
> > builds.
> 
> I don't know what you mean by this. You can set the range to specific
> releases.
> 
> In  mozregression-gui,
> 1. File -> Run a new bisection
> 2. Next
> 3. Next
> 4. To the right of "Last known good build", click the "date" drop-down menu
> and pick "release". Then click the preceding drop-down menu and choose a
> release version. Do the same with "First known bad build".
> 
> or in mozregression's command line:
> mozregression --good 49 --bad 56

Right, I did the GUI version of that. And what it proceeded to download and run where builds with the nightly branding, and seemingly nightly default configuration (as e10s was on by default). I assume it picked the equivalent nightly versions, but I don't think it compensated for different defaults in release builds.
And in the absence of an outright fix, any pointers on how to work around this in Javascript would be extremely helpful.
Masayuki Nakano, would you be willing to take a look at this?
Flags: needinfo?(masayuki)
Okay, anyway, Chrome will change behavior of keydown/keypress/keyup events and we need to follow the new behavior in bug 900750. I'll check it at working on it.
Assignee: nobody → masayuki
Status: UNCONFIRMED → ASSIGNED
Depends on: 900750
Ever confirmed: true
Flags: needinfo?(masayuki)
OS: Unspecified → Windows
Hardware: Unspecified → All
Priority: -- → P3
This also affects us.
We provide hosted Windows VMs in Germany, so keyboard layout is set to German and users are required to change initial password via browser-based vnc (using novnc). Unfortunately there's no way around this since RDP doesn't work up until this initial change.
Since German layout needs AltGr for lots of special characters, this causes problems for entering the initial password.

OS: Windows 10 x64
Firefox 59.0 x64
Okay, I see what happens.

The direct regression point is enabling e10s.

When AltGr key is released, Alt keyup is notified with WM_KEYUP rather than WM_SYSKEYUP. In this case, we set defaultPrevented to true before dispatching keyup event for preventing menubar handles the keyup event. This is implemented by bug 708936.

So, even if Alt keyup is prevented its default before sent to the content process, we need to send it.
See Also: → 708936
Comment on attachment 8981087 [details]
Bug 1435717 - Make calling WidgetEvent::PreventDefault*() stop cross process forwarding too

https://reviewboard.mozilla.org/r/247200/#review253258

This is super scary change. This is changing the behavior we've had in e10s for 7 years or so.
I need to look at all XBL key event handling and other key event listeners in chrome... so this will take time.

::: commit-message-db78b:9
(Diff revision 1)
> +EventStateManager should send it to focused remote process since it was
> +dispatched into web content before we enable e10s.
> +
> +Note that If chrome does not want the keyboard event to go to web content,
> +chrome should stop it explicitly with calling
> +dgetEvent::StopCrossProcessForwarding().

dget?

How does chrome JS prevent events to be forwarded to child process?
Comment on attachment 8981087 [details]
Bug 1435717 - Make calling WidgetEvent::PreventDefault*() stop cross process forwarding too

https://reviewboard.mozilla.org/r/247200/#review253258

> dget?
> 
> How does chrome JS prevent events to be forwarded to child process?

Hmm, currently, no way.

Another possible thing is, we can check if PreventDefault() is called explicitly or PreventDefaultBeforeDispatch() is called since PreventDefaultBeforeDispatch() won't set mDefaultPreventedByChrome nor mDefaultPreventedByContent. Perhaps, it's better for risk management.  On the other hand, it's not clear rule that calling peventDefault() stops forwarding the event to content since it didn't prevent to send the event to web content without e10s.
Comment on attachment 8981087 [details]
Bug 1435717 - Make calling WidgetEvent::PreventDefault*() stop cross process forwarding too

https://reviewboard.mozilla.org/r/247200/#review253258

> Hmm, currently, no way.
> 
> Another possible thing is, we can check if PreventDefault() is called explicitly or PreventDefaultBeforeDispatch() is called since PreventDefaultBeforeDispatch() won't set mDefaultPreventedByChrome nor mDefaultPreventedByContent. Perhaps, it's better for risk management.  On the other hand, it's not clear rule that calling peventDefault() stops forwarding the event to content since it didn't prevent to send the event to web content without e10s.

FYI: there is Event::StopCrossProcessForwarding(), but looks like that this is not exposed to JS.
Comment on attachment 8981087 [details]
Bug 1435717 - Make calling WidgetEvent::PreventDefault*() stop cross process forwarding too

https://reviewboard.mozilla.org/r/247200/#review253258

> FYI: there is Event::StopCrossProcessForwarding(), but looks like that this is not exposed to JS.

Ah, and for compatibility with non-e10s's behavior, we should check PropagationStopped() rather than DefaultPrevented(). As far as I know, chrome script calls StopPropagaion() with PeventDefault().
Sorting out possible scenarios:

1. chrome calls preventDefault() in the capturing phase:
  In this case, currently, we don't send the event to remote process but in non-e10s mode, we dispatched the consumed events even though defaultPrevented is set to true.
2. chrome calls stopPropagation() in the capturing phase:
  In this case, currently, we don't send the event to remote process and also in non-e10s mode, we didn't dispatch the consumed events in the content.
3. chrome calls preventDefault() in the bubbling phase:
  In this case, currently, we don't send the event to remote process.  In non-e10s mode, this happens after dispatching the event into the content.
4. chrome calls stopPropagation() in the bubbling phase:
  Same as #3.
5. chrome calls preventDefault() and mark it dispatched chrome only (Fullscreen mode):
  In this case, currently, we don't send the event to remove process.  In non-e10s mode, this is not dispatched into the content since marked as chrome only.

My patch allows to send key events to remote process in any cases unless somebody calls StopCrossProcessForwarding().

Perhaps, safest approach is:

* Make stopPropagation() marks whether the event was stopped in the capturing phase or bubbling phase.
* Don't send keyboard events whose propagation were stopped in the capturing phase.
* Don't send keyboard events if marked as chrome only dispatch.

So, only allows #1, #3 and #4.  Then, the behavior becomes similar to non-e10s mode.

How do you think?
Flags: needinfo?(bugs)
(I'll try to find time to think this tomorrow.)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #21)
> Sorting out possible scenarios:
> 
> 1. chrome calls preventDefault() in the capturing phase:
>   In this case, currently, we don't send the event to remote process but in
> non-e10s mode, we dispatched the consumed events even though
> defaultPrevented is set to true.
> 2. chrome calls stopPropagation() in the capturing phase:
>   In this case, currently, we don't send the event to remote process
really? ESM cares about default handling (preventDefault()) only. Or are you talking about
the XBL implement key event handlers?

> and
> also in non-e10s mode, we didn't dispatch the consumed events in the content.
> 3. chrome calls preventDefault() in the bubbling phase:
>   In this case, currently, we don't send the event to remote process.  In
> non-e10s mode, this happens after dispatching the event into the content.
> 4. chrome calls stopPropagation() in the bubbling phase:
>   Same as #3.
> 5. chrome calls preventDefault() and mark it dispatched chrome only
> (Fullscreen mode):
>   In this case, currently, we don't send the event to remove process.  In
> non-e10s mode, this is not dispatched into the content since marked as
> chrome only.
> 
> My patch allows to send key events to remote process in any cases unless
> somebody calls StopCrossProcessForwarding().
> 
> Perhaps, safest approach is:
> 
> * Make stopPropagation() marks whether the event was stopped in the
> capturing phase or bubbling phase.
But then code needs to start to call preventDefault() and stopPropagation().


> * Don't send keyboard events whose propagation were stopped in the capturing
> phase.
I don't like this.

So what does prevent default on AltGr? Looks like comment 13 answers to that.
Isn't that the bug to fix. We shouldn't call preventDefault there, somehow.
Hmm, bug 708936 was Windows only, but this was filed on Linux.


(When I test this on a Finnish keyboard, I get keydown AltGraph, keyup AltGraph, which is what I'd expect.)
Flags: needinfo?(bugs)
(In reply to Olli Pettay [:smaug] from comment #23)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #21)
> > Sorting out possible scenarios:
> > 
> > 1. chrome calls preventDefault() in the capturing phase:
> >   In this case, currently, we don't send the event to remote process but in
> > non-e10s mode, we dispatched the consumed events even though
> > defaultPrevented is set to true.
> > 2. chrome calls stopPropagation() in the capturing phase:
> >   In this case, currently, we don't send the event to remote process
> really? ESM cares about default handling (preventDefault()) only.

Ah, yes, perhaps, when I inserted list items, I wrote wrong result.

> > and
> > also in non-e10s mode, we didn't dispatch the consumed events in the content.
> > 3. chrome calls preventDefault() in the bubbling phase:
> >   In this case, currently, we don't send the event to remote process.  In
> > non-e10s mode, this happens after dispatching the event into the content.
> > 4. chrome calls stopPropagation() in the bubbling phase:
> >   Same as #3.

So, in this case, we send the key event to remote process.

> > 5. chrome calls preventDefault() and mark it dispatched chrome only
> > (Fullscreen mode):
> >   In this case, currently, we don't send the event to remove process.  In
> > non-e10s mode, this is not dispatched into the content since marked as
> > chrome only.
> > 
> > My patch allows to send key events to remote process in any cases unless
> > somebody calls StopCrossProcessForwarding().
> > 
> > Perhaps, safest approach is:
> > 
> > * Make stopPropagation() marks whether the event was stopped in the
> > capturing phase or bubbling phase.
> But then code needs to start to call preventDefault() and stopPropagation().

Yes, it is. However, in strict speaking, StopCrossProcessForwarding() should be called explicitly.

> > * Don't send keyboard events whose propagation were stopped in the capturing
> > phase.
> I don't like this.
> 
> So what does prevent default on AltGr? Looks like comment 13 answers to that.

Yeah, the safest fix is we should send keyboard events only when the event's default is prevented before dispatch. However, it looks hacky. I wonder, why do we have both preventDefault() and StopCrossProcessForwarding()? For non-cencelable events? If so, perhaps, preventDefault() should call StopCrossProcessForwarding() too. I think that managing with those double state may cause other bugs.

> Isn't that the bug to fix. We shouldn't call preventDefault there, somehow.
> Hmm, bug 708936 was Windows only, but this was filed on Linux.

Yeah, reported from Linux environment, but the symptom explained in comment 0 is the behavior on Windows.

> (When I test this on a Finnish keyboard, I get keydown AltGraph, keyup
> AltGraph, which is what I'd expect.)

On Linux? Currently, "AltGraph" key value is used only on Linux. On macOS, option key is mapped to "Alt" and it activates both altKey and "AltGraph" state. On Windows, it's being changed in bug 900750 right now.


So, another possible fix is:

* Make PreventDefault() call StopCrossProcessForwarding() too.
* Keep PreventDefaultBeforeDispatch() not calling StopCrossProcessForwarding().
* ESM should check only IsCrossProcessForwardingStopped().
* (for Fullscreen,) Create new method to set mFlags.mOnlyChromeDispatch, and it should call StopCrossProcessForwarding().

I'm not sure the last is correct. If it's wrong approach, perhaps, Fullscreen key handler should just call StopCrossProcessForwarding().
# the explanation of mOnlyChromeDispatch is: "If mOnlyChromeDispatch is true, the event is dispatched to only chrome."
# I'm not sure if this works on remote process to: https://searchfox.org/mozilla-central/rev/292d295d6b084b43b70de26a42e68513bb7b36a3/dom/events/EventDispatcher.cpp#858-875
Flags: needinfo?(bugs)
We may want to handle event in parent process only, so StopCrossProcessForwarding is needed. preventDefault() would prevent handling in parent process too.
Flags: needinfo?(bugs)
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #24)

> 
> So, another possible fix is:
> 
> * Make PreventDefault() call StopCrossProcessForwarding() too.
this sounds reasonable

> * Keep PreventDefaultBeforeDispatch() not calling
> StopCrossProcessForwarding().
Ahaa. And then it should be called 
PreventDefaultBeforeDispatchAllowCrossProcessForwarding() 
"tiny" bit long name :)
...but I propose something different below


> * ESM should check only IsCrossProcessForwardingStopped().
yup

> * (for Fullscreen,) Create new method to set mFlags.mOnlyChromeDispatch, and
> it should call StopCrossProcessForwarding().
I guess PreventDefaultBeforeDispatch could actually just take an enum value as param
enum { eAllowCrossProcessForwarding, ePreventCrossProcessForwarning}


This could work.


AltGr of course shouldn't be preventDefaulted when web page gets the event, but that could be a separate bug.
Attachment #8981087 - Flags: review?(bugs)
Comment on attachment 8981087 [details]
Bug 1435717 - Make calling WidgetEvent::PreventDefault*() stop cross process forwarding too

https://reviewboard.mozilla.org/r/247200/#review259712

::: dom/ipc/TabChild.cpp:2135
(Diff revision 2)
>      // the event shouldn't be handled by nsMenuBarListener in the main process.
>      if (!localEvent.DefaultPrevented() &&
>          status == nsEventStatus_eConsumeNoDefault) {
>        localEvent.PreventDefault();
>      }
> +    // Although this is ugly hack.  mNoRemoteProcessDispatch is set to true

Perhaps
// This is an ugly hack, mNoRemoteProcessDispatch...

::: widget/BasicEvents.h:39
(Diff revision 2)
> +enum class CrossProcessForwarding
> +{
> +  // eStop prevents the event to be sent to remote process.
> +  eStop,
> +  // eHold keeps current state of the event whether it's sent to remote process.
> +  eHold,

I think I'd prefer something like
eAllow.

::: widget/BasicEvents.h:210
(Diff revision 2)
>      }
>    }
>    // This should be used only before dispatching events into the DOM tree.
> -  inline void PreventDefaultBeforeDispatch()
> +  inline void
> +  PreventDefaultBeforeDispatch(
> +    CrossProcessForwarding aIfStopCrossProcessForwarding)

Perhaps rename the argument to
aCrossProcessForwarding

::: widget/BasicEvents.h:670
(Diff revision 2)
>    void PreventDefault(bool aCalledByDefaultHandler = true,
>                        nsIPrincipal* aPrincipal = nullptr);
>  
> -  void PreventDefaultBeforeDispatch() { mFlags.PreventDefaultBeforeDispatch(); }
> +  void
> +  PreventDefaultBeforeDispatch(
> +    CrossProcessForwarding aIfStopCrossProcessForwarding)

Same here
Attachment #8981087 - Flags: review?(bugs) → review+
Comment on attachment 8981087 [details]
Bug 1435717 - Make calling WidgetEvent::PreventDefault*() stop cross process forwarding too

https://reviewboard.mozilla.org/r/247200/#review259712

> I think I'd prefer something like
> eAllow.

Thank you for the review.

I have a question here. Do you think that calling the method with eAllow forcibly reset the cross process forwarding state? I'm thinking that if somebody stops its cross process forwarding, any other calls of the method even with eAllow shouldn't reset the state.  Therefore, I used "eHold" to mean "keep current state".
smaug: ping for the confirmation.
Flags: needinfo?(bugs)
eAllow would have the same behavior as eHold had. I'm of course not native English speaker, but hold sounds somehow a bit weird in this context. The caller allows cross process forwarding, it doesn't mean that someone else couldn't disallow it.
Flags: needinfo?(bugs)
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/c0cd065ee5c8
Make calling WidgetEvent::PreventDefault*() stop cross process forwarding too r=smaug
https://hg.mozilla.org/mozilla-central/rev/c0cd065ee5c8
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Is this too intrusive for ESR? It does cause some issues so it would be nice not to have to wait until the next ESR release.
(In reply to Pierre Ossman from comment #42)
> Is this too intrusive for ESR? It does cause some issues so it would be nice
> not to have to wait until the next ESR release.

Unfortunately, it's really risky change. At least, we need to wait release of this feature on September.
(In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #43)
> Unfortunately, it's really risky change. At least, we need to wait release
> of this feature on September.

Fx63 doesn't ship until October - did you intend to request Beta approval on this?
Flags: needinfo?(masayuki)
(In reply to Ryan VanderMeulen [:RyanVM] from comment #44)
> (In reply to Masayuki Nakano [:masayuki] (JST, +0900) from comment #43)
> > Unfortunately, it's really risky change. At least, we need to wait release
> > of this feature on September.
> 
> Fx63 doesn't ship until October - did you intend to request Beta approval on
> this?

I intended to fix this bug at 62 cycle normally, but it was too late for landing the patch to 62. This bug depends on the patches for bug 900750, but the patches are too risky to uplift...
Flags: needinfo?(masayuki)
I've tested the current nightly build and can confirm this works (as well as bug 900750). Nice work. :)
You need to log in before you can comment on or make changes to this bug.