Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
565373002:
Move virtual keyboard behind context menus. (Closed)
|
Created:
6 years, 3 months ago by kevers Modified:
6 years, 3 months ago Reviewers:
James Cook CC:
chromium-reviews, kalyank, sadrul, ben+ash_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionAdjust the z-order of the virtual keyboard to ensure that it does not overlap context menus. Previously, the virtual keyboard was placed just below the cursor container. Adding a test case to ensure that the virtual keyboard overlaps normal windows but not menus.
BUG=377180
Committed: https://crrev.com/23f3987d5ecd4be30938bd9673148830482ac777
Cr-Commit-Position: refs/heads/master@{#295268}
Patch Set 1 #
Total comments: 20
Patch Set 2 : Code cleanup. #
Total comments: 6
Messages
Total messages: 13 (2 generated)
kevers@chromium.org changed reviewers: + jamescook@chromium.org
Hi James, Can you please take a look at this CL.
https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller.cc File ash/root_window_controller.cc (right): https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller.c... ash/root_window_controller.cc:1024: lock_screen_related_containers); Just to double-check -- the virtual keyboard still has expected behavior at the lock screen? I no longer remember what "lock_screen_related_containers" means. https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... File ash/root_window_controller_unittest.cc (right): https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:652: TargetHitTestEventHandler() : ui::test::TestEventHandler() {} nit: "ui::test::TestEventHandler" not needed https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:654: virtual void OnMouseEvent(ui::MouseEvent* event) OVERRIDE { nit: "// ui::test::TestEventHandler overrides:" or similar https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:655: if (event->type() == ui::ET_MOUSE_PRESSED) nit: one space after == https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:858: TEST_F(VirtualKeyboardRootWindowControllerTest, ZOrderTest) { Add a brief comment about what this test is testing. https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:871: aura::Window* keyboard_window =proxy->GetKeyboardWindow(); nit: space after = Or just run "git cl format" https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:884: // Normal window is should be partially occluded by the virtual keyboard. nit: "is should be"? https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:904: int left = window_width / 2; I'm having a hard time visualizing the coordinates here. Since you control the display size above could you use actual numbers instead of computing all these locations? Alternately, if you think that will make the test fragile, could you include some kind of ASCII-art drawing showing how all this stuff lines up? https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:909: // Test that only the click on the top portion of the window is pickd up. This block might be clearer if it was moved up to the "normal window" section. Also "pickd" https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:925: delete normal; Can you use scoped_ptr<> for these? https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:927: } Thanks for writing a nice test for this! It's always nice to see good test coverage.
Also, this could use a more detailed commit description -- maybe say something about layers or containers or visual stacking, and why this change needs to happen.
https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller.cc File ash/root_window_controller.cc (right): https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller.c... ash/root_window_controller.cc:1024: lock_screen_related_containers); On 2014/09/12 22:01:52, James Cook wrote: > Just to double-check -- the virtual keyboard still has expected behavior at the > lock screen? > > I no longer remember what "lock_screen_related_containers" means. Confirmed that the lock screen is well behaved. https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... File ash/root_window_controller_unittest.cc (right): https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:652: TargetHitTestEventHandler() : ui::test::TestEventHandler() {} On 2014/09/12 22:01:52, James Cook wrote: > nit: "ui::test::TestEventHandler" not needed Done. https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:654: virtual void OnMouseEvent(ui::MouseEvent* event) OVERRIDE { On 2014/09/12 22:01:52, James Cook wrote: > nit: "// ui::test::TestEventHandler overrides:" or similar Done. https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:655: if (event->type() == ui::ET_MOUSE_PRESSED) On 2014/09/12 22:01:52, James Cook wrote: > nit: one space after == Done. https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:858: TEST_F(VirtualKeyboardRootWindowControllerTest, ZOrderTest) { On 2014/09/12 22:01:52, James Cook wrote: > Add a brief comment about what this test is testing. Done. https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:871: aura::Window* keyboard_window =proxy->GetKeyboardWindow(); On 2014/09/12 22:01:52, James Cook wrote: > nit: space after = > > Or just run "git cl format" Whoops...done. https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:884: // Normal window is should be partially occluded by the virtual keyboard. On 2014/09/12 22:01:52, James Cook wrote: > nit: "is should be"? Fixed wording. https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:904: int left = window_width / 2; On 2014/09/12 22:01:52, James Cook wrote: > I'm having a hard time visualizing the coordinates here. Since you control the > display size above could you use actual numbers instead of computing all these > locations? > > Alternately, if you think that will make the test fragile, could you include > some kind of ASCII-art drawing showing how all this stuff lines up? Added comment block. I do feel that numbers are more fragile. https://codereview.chromium.org/565373002/diff/1/ash/root_window_controller_u... ash/root_window_controller_unittest.cc:909: // Test that only the click on the top portion of the window is pickd up. On 2014/09/12 22:01:52, James Cook wrote: > This block might be clearer if it was moved up to the "normal window" section. > > Also "pickd" Done.
LGTM with nits. Also, needs a more detailed commit description. https://codereview.chromium.org/565373002/diff/20001/ash/root_window_controll... File ash/root_window_controller.cc (right): https://codereview.chromium.org/565373002/diff/20001/ash/root_window_controll... ash/root_window_controller.cc:287: CHECK(Shell::HasInstance()); Do you need these for this CL? Do they need to be CHECK instead of DCHECK? https://codereview.chromium.org/565373002/diff/20001/ash/root_window_controll... File ash/root_window_controller_unittest.cc (right): https://codereview.chromium.org/565373002/diff/20001/ash/root_window_controll... ash/root_window_controller_unittest.cc:892: // 'right' is centered in the context menu. Very helpful, thanks! https://codereview.chromium.org/565373002/diff/20001/ash/root_window_controll... ash/root_window_controller_unittest.cc:944: menu.reset(); You don't need these, they clean up themselves and it's more common in the rest of Chrome to not explicitly do the cleanup. I would delete the last 4 lines.
https://codereview.chromium.org/565373002/diff/20001/ash/root_window_controll... File ash/root_window_controller_unittest.cc (right): https://codereview.chromium.org/565373002/diff/20001/ash/root_window_controll... ash/root_window_controller_unittest.cc:944: menu.reset(); On 2014/09/15 17:45:20, James Cook wrote: > You don't need these, they clean up themselves and it's more common in the rest > of Chrome to not explicitly do the cleanup. I would delete the last 4 lines. Without the explicit resets, the test crashes. My suspicion is that on delete, the delegate is being used to sent notifications that the window is being destroyed. Without the explicit resets, the delegates are already out of scope when the window destructors are called.
https://codereview.chromium.org/565373002/diff/20001/ash/root_window_controll... File ash/root_window_controller_unittest.cc (right): https://codereview.chromium.org/565373002/diff/20001/ash/root_window_controll... ash/root_window_controller_unittest.cc:944: menu.reset(); On 2014/09/15 18:13:04, kevers wrote: > On 2014/09/15 17:45:20, James Cook wrote: > > You don't need these, they clean up themselves and it's more common in the > rest > > of Chrome to not explicitly do the cleanup. I would delete the last 4 lines. > > Without the explicit resets, the test crashes. My suspicion is that on delete, > the delegate is being used to sent notifications that the window is being > destroyed. Without the explicit resets, the delegates are already out of scope > when the window destructors are called. Ah. I should have read the comments in more detail. I see other tests doing that too.
https://codereview.chromium.org/565373002/diff/20001/ash/root_window_controll... File ash/root_window_controller.cc (right): https://codereview.chromium.org/565373002/diff/20001/ash/root_window_controll... ash/root_window_controller.cc:287: CHECK(Shell::HasInstance()); On 2014/09/15 17:45:20, James Cook wrote: > Do you need these for this CL? Do they need to be CHECK instead of DCHECK? Sorry, missed this comment earlier. ForShelf is not required by the CL. The CHECK was added as part of https://codereview.chromium.org/549453005 to "identify the Chrome code accessing Ash".
The CQ bit was checked by kevers@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patchset/565373002/20001
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as d0dfa6577145cc7bc03f5236600c9d9fcfeff016
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/23f3987d5ecd4be30938bd9673148830482ac777 Cr-Commit-Position: refs/heads/master@{#295268} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
