Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2692093005:
Suppress context menu on virtual keyboard to fix crash (Closed)
DescriptionSuppress context menu on virtual keyboard to fix crash
KeyboardLayoutManager assumes WebContentsViewAura is the only child
of the KeyboardContainer window and crash happens if this assumption
is broken by context menus created on long tap.
This CL fixes the issue by suppressing gesture events including the
long press from being passed to renderer, who would create a context
menu; gesture events which are not consumed by IME are first handled by
KeyboardContentsDelegate, a WebContentsDelegate, and if the event is
not a scroll event, it's consumed by the delegate. If it's a scroll
event we let it go so that the renderer can scroll the IME menu element.
BUG=685140
TEST=
- manually tested using Link that crash doesn't happen on long press,
but scroll is doable.
- out/Release/browser_tests --gtest_filter="VirtualKeyboard*"
Review-Url: https://codereview.chromium.org/2692093005
Cr-Commit-Position: refs/heads/master@{#450833}
Committed: https://chromium.googlesource.com/chromium/src/+/626fecb851db4b9d0547752645ab0dcccc780eea
Patch Set 1 #Patch Set 2 : . #Patch Set 3 : . #Patch Set 4 : Rebased. #Messages
Total messages: 32 (22 generated)
The CQ bit was checked by oka@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...
Description was changed from ========== Suppress context menu on virtual keyboard to fix crash This CL fixes a crash bug, which happens when another window (=context menu) than the WebContentsViewAura is added to KeyboardContainer window, by suppressing gesture events including the long press, which would let renderer create a context menu. With this CL, gesture events which are not consumed by IME are first handled by KeyboardContentsDelegate, a WebContentsDelegate. If the event is not a scroll event, it's consumed by the delegate. If it's a scroll event we let it go so that the renderer can scroll the IME menu element. BUG=685140 TEST=TBD ========== to ========== Suppress context menu on virtual keyboard to fix crash This CL fixes a crash bug, which happens when another window (=context menu) than the WebContentsViewAura is added to KeyboardContainer window breaking KeyboardLayoutManager's assumption, by suppressing gesture events including the long press from being passed to renderer, who would create a context menu. With this CL, gesture events which are not consumed by IME are first handled by KeyboardContentsDelegate, a WebContentsDelegate, and if the event is not a scroll event, it's consumed by the delegate. If it's a scroll event we let it go so that the renderer can scroll the IME menu element. BUG=685140 TEST=TBD ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by oka@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...
Description was changed from ========== Suppress context menu on virtual keyboard to fix crash This CL fixes a crash bug, which happens when another window (=context menu) than the WebContentsViewAura is added to KeyboardContainer window breaking KeyboardLayoutManager's assumption, by suppressing gesture events including the long press from being passed to renderer, who would create a context menu. With this CL, gesture events which are not consumed by IME are first handled by KeyboardContentsDelegate, a WebContentsDelegate, and if the event is not a scroll event, it's consumed by the delegate. If it's a scroll event we let it go so that the renderer can scroll the IME menu element. BUG=685140 TEST=TBD ========== to ========== Suppress context menu on virtual keyboard to fix crash This CL fixes a crash bug, which happens when another window (=context menu) than the WebContentsViewAura is added to KeyboardContainer window breaking KeyboardLayoutManager's assumption, by suppressing gesture events including the long press from being passed to renderer, who would create a context menu. With this CL, gesture events which are not consumed by IME are first handled by KeyboardContentsDelegate, a WebContentsDelegate, and if the event is not a scroll event, it's consumed by the delegate. If it's a scroll event we let it go so that the renderer can scroll the IME menu element. BUG=685140 TEST=manually tested the crash doesn't happen on long press, but scroll can happen using Link. ==========
Description was changed from ========== Suppress context menu on virtual keyboard to fix crash This CL fixes a crash bug, which happens when another window (=context menu) than the WebContentsViewAura is added to KeyboardContainer window breaking KeyboardLayoutManager's assumption, by suppressing gesture events including the long press from being passed to renderer, who would create a context menu. With this CL, gesture events which are not consumed by IME are first handled by KeyboardContentsDelegate, a WebContentsDelegate, and if the event is not a scroll event, it's consumed by the delegate. If it's a scroll event we let it go so that the renderer can scroll the IME menu element. BUG=685140 TEST=manually tested the crash doesn't happen on long press, but scroll can happen using Link. ========== to ========== Suppress context menu on virtual keyboard to fix crash KeyboardLayoutManager assumes WebContentsViewAura is the only child of the KeyboardContainer window and crash happens if this assumption is broken, by context menu is created on long press. This CL fixes the crash bug, by suppressing gesture events including the long press from being passed to renderer, who would create a context menu. With this CL, gesture events which are not consumed by IME are first handled by KeyboardContentsDelegate, a WebContentsDelegate, and if the event is not a scroll event, it's consumed by the delegate. If it's a scroll event we let it go so that the renderer can scroll the IME menu element. BUG=685140 TEST=manually tested the crash doesn't happen on long press, but scroll can happen using Link. ==========
oka@chromium.org changed reviewers: + bshe@chromium.org
Description was changed from ========== Suppress context menu on virtual keyboard to fix crash KeyboardLayoutManager assumes WebContentsViewAura is the only child of the KeyboardContainer window and crash happens if this assumption is broken, by context menu is created on long press. This CL fixes the crash bug, by suppressing gesture events including the long press from being passed to renderer, who would create a context menu. With this CL, gesture events which are not consumed by IME are first handled by KeyboardContentsDelegate, a WebContentsDelegate, and if the event is not a scroll event, it's consumed by the delegate. If it's a scroll event we let it go so that the renderer can scroll the IME menu element. BUG=685140 TEST=manually tested the crash doesn't happen on long press, but scroll can happen using Link. ========== to ========== Suppress context menu on virtual keyboard to fix crash KeyboardLayoutManager assumes WebContentsViewAura is the only child of the KeyboardContainer window and crash happens if this assumption is broken by context menus created on long tap. This CL fixes the issue by suppressing gesture events including the long press from being passed to renderer, who would create a context menu; gesture events which are not consumed by IME are first handled by KeyboardContentsDelegate, a WebContentsDelegate, and if the event is not a scroll event, it's consumed by the delegate. If it's a scroll event we let it go so that the renderer can scroll the IME menu element. BUG=685140 TEST=manually tested that crash doesn't happen on long press, but scroll is doable, using Link. ==========
.
The CQ bit was checked by oka@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...
PTAL.
Description was changed from ========== Suppress context menu on virtual keyboard to fix crash KeyboardLayoutManager assumes WebContentsViewAura is the only child of the KeyboardContainer window and crash happens if this assumption is broken by context menus created on long tap. This CL fixes the issue by suppressing gesture events including the long press from being passed to renderer, who would create a context menu; gesture events which are not consumed by IME are first handled by KeyboardContentsDelegate, a WebContentsDelegate, and if the event is not a scroll event, it's consumed by the delegate. If it's a scroll event we let it go so that the renderer can scroll the IME menu element. BUG=685140 TEST=manually tested that crash doesn't happen on long press, but scroll is doable, using Link. ========== to ========== Suppress context menu on virtual keyboard to fix crash KeyboardLayoutManager assumes WebContentsViewAura is the only child of the KeyboardContainer window and crash happens if this assumption is broken by context menus created on long tap. This CL fixes the issue by suppressing gesture events including the long press from being passed to renderer, who would create a context menu; gesture events which are not consumed by IME are first handled by KeyboardContentsDelegate, a WebContentsDelegate, and if the event is not a scroll event, it's consumed by the delegate. If it's a scroll event we let it go so that the renderer can scroll the IME menu element. BUG=685140 TEST= - manually tested using Link that crash doesn't happen on long press, but scroll is doable. - out/Release/browser_tests --gtest_filter="VirtualKeyboard*" ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/02/15 03:33:24, oka wrote: > PTAL. Should we suppress context menu from mouse too?
It is somehow already surpressed. Right click triggers a button press. On Wed, Feb 15, 2017, 7:30 AM <bshe@chromium.org> wrote: > On 2017/02/15 03:33:24, oka wrote: > > PTAL. > > Should we suppress context menu from mouse too? > > https://codereview.chromium.org/2692093005/ > -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2017/02/15 15:40:53, oka wrote: > It is somehow already surpressed. Right click triggers a button press. > > On Wed, Feb 15, 2017, 7:30 AM <mailto:bshe@chromium.org> wrote: > > > On 2017/02/15 03:33:24, oka wrote: > > > PTAL. > > > > Should we suppress context menu from mouse too? > > > > https://codereview.chromium.org/2692093005/ > > > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org. lgtm
The CQ bit was checked by oka@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Failed to apply patch for ui/keyboard/content/keyboard_ui_content.cc:
While running git apply --index -p1;
error: patch failed: ui/keyboard/content/keyboard_ui_content.cc:12
error: ui/keyboard/content/keyboard_ui_content.cc: patch does not apply
Patch: ui/keyboard/content/keyboard_ui_content.cc
Index: ui/keyboard/content/keyboard_ui_content.cc
diff --git a/ui/keyboard/content/keyboard_ui_content.cc
b/ui/keyboard/content/keyboard_ui_content.cc
index
1dba63ec09b4d1ac76ca8950f4f284d99fc45492..189b9c106786765dc69b4cb7e3cb8af02a1e83cf
100644
--- a/ui/keyboard/content/keyboard_ui_content.cc
+++ b/ui/keyboard/content/keyboard_ui_content.cc
@@ -12,7 +12,6 @@
#include "content/public/browser/render_widget_host_view.h"
#include "content/public/browser/site_instance.h"
#include "content/public/browser/web_contents.h"
-#include "content/public/browser/web_contents.h"
#include "content/public/browser/web_contents_delegate.h"
#include "content/public/browser/web_contents_observer.h"
#include "content/public/browser/web_ui.h"
@@ -96,6 +95,25 @@ class KeyboardContentsDelegate : public
content::WebContentsDelegate,
ui_->RequestAudioInput(web_contents, request, callback);
}
+ // Overridden from content::WebContentsDelegate:
+ bool PreHandleGestureEvent(content::WebContents* source,
+ const blink::WebGestureEvent& event) override {
+ switch (event.type()) {
+ // Scroll events are not suppressed because the menu to select IME should
+ // be scrollable.
+ case blink::WebInputEvent::GestureScrollBegin:
+ case blink::WebInputEvent::GestureScrollEnd:
+ case blink::WebInputEvent::GestureScrollUpdate:
+ case blink::WebInputEvent::GestureFlingStart:
+ case blink::WebInputEvent::GestureFlingCancel:
+ return false;
+ default:
+ // Stop gesture events from being passed to renderer to suppress the
+ // context menu. crbug.com/685140
+ return true;
+ }
+ }
+
// Overridden from content::WebContentsObserver:
void WebContentsDestroyed() override { delete this; }
Rebased.
The CQ bit was checked by oka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bshe@chromium.org Link to the patchset: https://codereview.chromium.org/2692093005/#ps60001 (title: "Rebased.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1487194351495620,
"parent_rev": "f3e75ecdc65ff5ee192fba45952848e5b3b6ddac", "commit_rev":
"626fecb851db4b9d0547752645ab0dcccc780eea"}
Message was sent while issue was closed.
Description was changed from ========== Suppress context menu on virtual keyboard to fix crash KeyboardLayoutManager assumes WebContentsViewAura is the only child of the KeyboardContainer window and crash happens if this assumption is broken by context menus created on long tap. This CL fixes the issue by suppressing gesture events including the long press from being passed to renderer, who would create a context menu; gesture events which are not consumed by IME are first handled by KeyboardContentsDelegate, a WebContentsDelegate, and if the event is not a scroll event, it's consumed by the delegate. If it's a scroll event we let it go so that the renderer can scroll the IME menu element. BUG=685140 TEST= - manually tested using Link that crash doesn't happen on long press, but scroll is doable. - out/Release/browser_tests --gtest_filter="VirtualKeyboard*" ========== to ========== Suppress context menu on virtual keyboard to fix crash KeyboardLayoutManager assumes WebContentsViewAura is the only child of the KeyboardContainer window and crash happens if this assumption is broken by context menus created on long tap. This CL fixes the issue by suppressing gesture events including the long press from being passed to renderer, who would create a context menu; gesture events which are not consumed by IME are first handled by KeyboardContentsDelegate, a WebContentsDelegate, and if the event is not a scroll event, it's consumed by the delegate. If it's a scroll event we let it go so that the renderer can scroll the IME menu element. BUG=685140 TEST= - manually tested using Link that crash doesn't happen on long press, but scroll is doable. - out/Release/browser_tests --gtest_filter="VirtualKeyboard*" Review-Url: https://codereview.chromium.org/2692093005 Cr-Commit-Position: refs/heads/master@{#450833} Committed: https://chromium.googlesource.com/chromium/src/+/626fecb851db4b9d0547752645ab... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/626fecb851db4b9d0547752645ab... |
