Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2482853002:
Suppress KeyUp events after PreHandleKeyboardEvent consumes RawKeyDown (Closed)
DescriptionSuppress KeyUp events after PreHandleKeyboardEvent consumes RawKeyDown
BUG=654140
Committed: https://crrev.com/35d322e24f91a372ecdc0b152891e0635187a07e
Cr-Commit-Position: refs/heads/master@{#430939}
Patch Set 1 #
Total comments: 3
Patch Set 2 : update name and documentation #Patch Set 3 : andt typo #
Total comments: 2
Messages
Total messages: 34 (15 generated)
The CQ bit was checked by foolip@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
foolip@chromium.org changed reviewers: + dtapuska@chromium.org, mgiuca@chromium.org
mgiuca@, I looked into issue 641711 and came up with this, PTAL. dtapuska@, there's very similar code in RenderWidgetInputHandler, can you tell if lone keyup events without the preceding keydown events can happen there too? suzhe@, you added this code in https://codereview.chromium.org/235039, will this change break stuff? The name suppress_next_char_events_ and some documentation is now wrong, I will update it if the logic itself makes sense.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2482853002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2482853002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_impl.cc:1147: // need to suppress all events generated by it until the next RawKeyDown or The member variable name is slightly confusing now. At least the comment here: https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... needs an update. Yes it could happen in the render_widget_input_handler as well. The OS is not necessarily guaranteed to send us a logical stream of events so having this state related code is somewhat dangerous. I'd prefer that we used a model in which we still fired the events at javascript but actually indicated that they weren't cancelable and then we'd just look up each time if the mapping is a shortcut for down, char and up instead of having state. But I have no idea how inter-operable that would be.
On 2016/11/07 16:00:44, dtapuska wrote: > https://codereview.chromium.org/2482853002/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > https://codereview.chromium.org/2482853002/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/render_widget_host_impl.cc:1147: // need to > suppress all events generated by it until the next RawKeyDown or > The member variable name is slightly confusing now. At least the comment here: > https://cs.chromium.org/chromium/src/content/browser/renderer_host/render_wid... > needs an update. > > Yes it could happen in the render_widget_input_handler as well. The OS is not > necessarily guaranteed to send us a logical stream of events so having this > state related code is somewhat dangerous. > > I'd prefer that we used a model in which we still fired the events at javascript > but actually indicated that they weren't cancelable and then we'd just look up > each time if the mapping is a shortcut for down, char and up instead of having > state. But I have no idea how inter-operable that would be. That being said it would also be interesting not to provide the user gesture indicator for "non-cancelable" events or something like that. Regardless of that I don't see any issues with this patch.
This is very nice. Tested and it works. I think this is the right place to put it --- if I can basically summarize your patch (to make sure I've understood it): whenever the browser suppresses or captures a key-down event, suppress the corresponding keyup. I've noted a few things which you may like to consider. But no changes required. LGTM. https://codereview.chromium.org/2482853002/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2482853002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_impl.cc:1147: // need to suppress all events generated by it until the next RawKeyDown or On 2016/11/07 16:00:44, dtapuska wrote: > The member variable name is slightly confusing now. At least the comment here: +1 > Yes it could happen in the render_widget_input_handler as well. The OS is not > necessarily guaranteed to send us a logical stream of events so having this > state related code is somewhat dangerous. Do you mean the OS could send a KeyUp before the corresponding RawKeyDown? I've never heard of that happening... do you have information? Maybe you're talking about interleaved key events (press A, press B, release A, release B). I'll mention that in a separate comment. > I'd prefer that we used a model in which we still fired the events at javascript > but actually indicated that they weren't cancelable and then we'd just look up > each time if the mapping is a shortcut for down, char and up instead of having > state. But I have no idea how inter-operable that would be. I would rather not fire the keyup at JS at all if we can help it. The keydown is being intercepted so we should suppress the corresponding keyup also. https://codereview.chromium.org/2482853002/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_impl.cc:1150: key_event.type == WebKeyboardEvent::Char) So I thought about the case where you interleave two key presses like this: 1. Press A [captured by browser] 2. Press B 3. Release A 4. Release B What happens is that neither Release A nor Release B are suppressed, because Press B sets suppress_next_char_events_ to false. That's a little bit dodgy; really Release A should still be suppressed. But it'd be hard to keep track of all the state required, so I am happy with this behaviour. It solves the main case of a single keypress. There's also another edge case: if you hold down the key long enough to trigger a key repeat: 1. Press A. 2. Hold A, triggering a second keydown event. 3. Release A. The release does not get suppressed, because the second keydown event sets suppress_next_char_events_ to false. I'm also happy with this behaviour, because you are legitimately delivering the key repeat to the web page and it isn't being suppressed the second time (in the case of Esc to exit fullscreen). So, no action required. Just noting a few things.
Note: You could remove 641711 (dupe) and 654145 (higher-level bug blocked on this one) and just keep "BUG=654140" for simplicity.
Description was changed from ========== Suppress KeyUp events after PreHandleKeyboardEvent consumes RawKeyDown BUG=641711,654140,654145 ========== to ========== Suppress KeyUp events after PreHandleKeyboardEvent consumes RawKeyDown BUG=654140 ==========
On 2016/11/08 03:09:45, Matt Giuca wrote: > Note: You could remove 641711 (dupe) and 654145 (higher-level bug blocked on > this one) and just keep "BUG=654140" for simplicity. Done.
The CQ bit was checked by foolip@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
foolip@chromium.org changed reviewers: + sadrul@chromium.org
Updated names and documentation, this is ready for final review. sadrul@, can you PTAL and loop others if necessary?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1121: // Some keypresses that are accepted by the listener may be followed by Char I guess the odd thing is that on platforms that generate KeyDown (aka Android and not RawKeyDown). They will get the KeyUp message.
On 2016/11/08 15:18:03, dtapuska wrote: > https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_impl.cc:1121: // Some > keypresses that are accepted by the listener may be followed by Char > I guess the odd thing is that on platforms that generate KeyDown (aka Android > and not RawKeyDown). They will get the KeyUp message. lgtm
https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_impl.cc (right): https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... content/browser/renderer_host/render_widget_host_impl.cc:1121: // Some keypresses that are accepted by the listener may be followed by Char On 2016/11/08 15:18:03, dtapuska wrote: > I guess the odd thing is that on platforms that generate KeyDown (aka Android > and not RawKeyDown). They will get the KeyUp message. I didn't try to understand the split between RawKeyDown and KeyDown. Does Android generate only KeyDown, or both RawKeyDown and KeyDown?
On 2016/11/08 15:23:15, foolip wrote: > https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_impl.cc:1121: // Some > keypresses that are accepted by the listener may be followed by Char > On 2016/11/08 15:18:03, dtapuska wrote: > > I guess the odd thing is that on platforms that generate KeyDown (aka Android > > and not RawKeyDown). They will get the KeyUp message. > > I didn't try to understand the split between RawKeyDown and KeyDown. Does > Android generate only KeyDown, or both RawKeyDown and KeyDown? It changed recently. It generates only KeyDown messages these days. https://codereview.chromium.org/2206053002
On 2016/11/08 15:28:57, dtapuska wrote: > On 2016/11/08 15:23:15, foolip wrote: > > > https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... > > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > > > > https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... > > content/browser/renderer_host/render_widget_host_impl.cc:1121: // Some > > keypresses that are accepted by the listener may be followed by Char > > On 2016/11/08 15:18:03, dtapuska wrote: > > > I guess the odd thing is that on platforms that generate KeyDown (aka > Android > > > and not RawKeyDown). They will get the KeyUp message. > > > > I didn't try to understand the split between RawKeyDown and KeyDown. Does > > Android generate only KeyDown, or both RawKeyDown and KeyDown? > > It changed recently. It generates only KeyDown messages these days. > https://codereview.chromium.org/2206053002 Hmm, so are there any platforms that can generate both? Because only a SuppressEventsUntilKeyDown() call or RawKeyDown with the special code paths can cause suppress_events_until_keydown_ to become true I suppose it doesn't matter much for this CL, but if merging into just one kind of KeyDown is at all possible, that seems worthwhile. Wondering about it slowed me down at least a little bit.
On 2016/11/08 15:35:23, foolip wrote: > On 2016/11/08 15:28:57, dtapuska wrote: > > On 2016/11/08 15:23:15, foolip wrote: > > > > > > https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... > > > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... > > > content/browser/renderer_host/render_widget_host_impl.cc:1121: // Some > > > keypresses that are accepted by the listener may be followed by Char > > > On 2016/11/08 15:18:03, dtapuska wrote: > > > > I guess the odd thing is that on platforms that generate KeyDown (aka > > Android > > > > and not RawKeyDown). They will get the KeyUp message. > > > > > > I didn't try to understand the split between RawKeyDown and KeyDown. Does > > > Android generate only KeyDown, or both RawKeyDown and KeyDown? > > > > It changed recently. It generates only KeyDown messages these days. > > https://codereview.chromium.org/2206053002 > > Hmm, so are there any platforms that can generate both? Because only a > SuppressEventsUntilKeyDown() call or RawKeyDown with the special code paths can > cause suppress_events_until_keydown_ to become true I suppose it doesn't matter > much for this CL, but if merging into just one kind of KeyDown is at all > possible, that seems worthwhile. Wondering about it slowed me down at least a > little bit. Windows generates RawKeyDown (and Char natively). X11, Ozone both generate RawKeyDown (and emulates generation of Char) Mac as well generates RawKeyDown (and emulates generation of Char via another code path)
On 2016/11/08 15:41:37, dtapuska wrote: > On 2016/11/08 15:35:23, foolip wrote: > > On 2016/11/08 15:28:57, dtapuska wrote: > > > On 2016/11/08 15:23:15, foolip wrote: > > > > > > > > > > https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... > > > > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > > > > > > > > > > > > > https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... > > > > content/browser/renderer_host/render_widget_host_impl.cc:1121: // Some > > > > keypresses that are accepted by the listener may be followed by Char > > > > On 2016/11/08 15:18:03, dtapuska wrote: > > > > > I guess the odd thing is that on platforms that generate KeyDown (aka > > > Android > > > > > and not RawKeyDown). They will get the KeyUp message. > > > > > > > > I didn't try to understand the split between RawKeyDown and KeyDown. Does > > > > Android generate only KeyDown, or both RawKeyDown and KeyDown? > > > > > > It changed recently. It generates only KeyDown messages these days. > > > https://codereview.chromium.org/2206053002 > > > > Hmm, so are there any platforms that can generate both? Because only a > > SuppressEventsUntilKeyDown() call or RawKeyDown with the special code paths > can > > cause suppress_events_until_keydown_ to become true I suppose it doesn't > matter > > much for this CL, but if merging into just one kind of KeyDown is at all > > possible, that seems worthwhile. Wondering about it slowed me down at least a > > little bit. > > Windows generates RawKeyDown (and Char natively). > X11, Ozone both generate RawKeyDown (and emulates generation of Char) > Mac as well generates RawKeyDown (and emulates generation of Char via another > code path) So, RawKeyDown and KeyDown are mutually exclusive as far as you know? Anyway, I won't go poking at any of that in this CL.
On 2016/11/08 15:54:40, foolip wrote: > On 2016/11/08 15:41:37, dtapuska wrote: > > On 2016/11/08 15:35:23, foolip wrote: > > > On 2016/11/08 15:28:57, dtapuska wrote: > > > > On 2016/11/08 15:23:15, foolip wrote: > > > > > > > > > > > > > > > https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... > > > > > File content/browser/renderer_host/render_widget_host_impl.cc (right): > > > > > > > > > > > > > > > > > > > > https://codereview.chromium.org/2482853002/diff/40001/content/browser/rendere... > > > > > content/browser/renderer_host/render_widget_host_impl.cc:1121: // Some > > > > > keypresses that are accepted by the listener may be followed by Char > > > > > On 2016/11/08 15:18:03, dtapuska wrote: > > > > > > I guess the odd thing is that on platforms that generate KeyDown (aka > > > > Android > > > > > > and not RawKeyDown). They will get the KeyUp message. > > > > > > > > > > I didn't try to understand the split between RawKeyDown and KeyDown. > Does > > > > > Android generate only KeyDown, or both RawKeyDown and KeyDown? > > > > > > > > It changed recently. It generates only KeyDown messages these days. > > > > https://codereview.chromium.org/2206053002 > > > > > > Hmm, so are there any platforms that can generate both? Because only a > > > SuppressEventsUntilKeyDown() call or RawKeyDown with the special code paths > > can > > > cause suppress_events_until_keydown_ to become true I suppose it doesn't > > matter > > > much for this CL, but if merging into just one kind of KeyDown is at all > > > possible, that seems worthwhile. Wondering about it slowed me down at least > a > > > little bit. > > > > Windows generates RawKeyDown (and Char natively). > > X11, Ozone both generate RawKeyDown (and emulates generation of Char) > > Mac as well generates RawKeyDown (and emulates generation of Char via another > > code path) > > So, RawKeyDown and KeyDown are mutually exclusive as far as you know? Anyway, I > won't go poking at any of that in this CL. If you gloss over using devtools yes :-)
lgtm
The CQ bit was checked by foolip@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mgiuca@chromium.org Link to the patchset: https://codereview.chromium.org/2482853002/#ps40001 (title: "andt typo")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Suppress KeyUp events after PreHandleKeyboardEvent consumes RawKeyDown BUG=654140 ========== to ========== Suppress KeyUp events after PreHandleKeyboardEvent consumes RawKeyDown BUG=654140 ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Suppress KeyUp events after PreHandleKeyboardEvent consumes RawKeyDown BUG=654140 ========== to ========== Suppress KeyUp events after PreHandleKeyboardEvent consumes RawKeyDown BUG=654140 Committed: https://crrev.com/35d322e24f91a372ecdc0b152891e0635187a07e Cr-Commit-Position: refs/heads/master@{#430939} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/35d322e24f91a372ecdc0b152891e0635187a07e Cr-Commit-Position: refs/heads/master@{#430939} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
