Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
250423004:
ChromeOS: Avoid overlapping app list with virtual keyboard. (Closed)
|
Created:
6 years, 8 months ago by Matt Giuca Modified:
6 years, 7 months ago CC:
chromium-reviews, chrome-apps-syd-reviews_chromium.org, tfarina, sadrul Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionChromeOS: Avoid overlapping app list with virtual keyboard.
When the app list is centered (--enable-experimental-app-list-position),
and the virtual keyboard is active, the app list is centered in the
screen rect that excludes the keyboard region.
BUG=363731
TEST=Run with --enable-experimental-app-list-position
--enable-virtual-keyboard. Open the app list, then the virtual keyboard.
The app list should move upwards so it is centered in the part of the
screen without the virtual keyboard.
TEST=Same as above, but with the additional flag
--enable-virtual-keyboard-overscroll.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268131
Patch Set 1 #Patch Set 2 : Added ui/keyboard to app_list DEPS. #Patch Set 3 : Avoid DEPS changes; use AppListViewDelegate instead. #
Total comments: 6
Patch Set 4 : Fix tapted's comments. #Patch Set 5 : Fix tests. #Patch Set 6 : Fixed ExampleAppList. #Patch Set 7 : Rebase off CL 260663002. #Patch Set 8 : Remove cruft. #Patch Set 9 : Fix when overscroll is enabled (added KeyboardControllerObserver). #
Total comments: 4
Patch Set 10 : Rebase. #Patch Set 11 : Remove the KeyboardControllerObserver on shutdown. #
Total comments: 4
Patch Set 12 : Null check KeyboardController. #Messages
Total messages: 42 (0 generated)
tapted: OWNERS. sadrul: ui/keyboard OWNERS. I need to access ui/keyboard from app_list to move the app list based on the virtual keyboard's position. Sadrul, let me know if this is an inappropriate DEPS addition and I'll try to find another way around it.
+kevers@ for ui/keyboard/
Hey, sorry to do this. I've had a discussion with calamity and tapted here, and I think a better solution is to go through AppListViewDelegate instead of piping through some new dependencies within UI. Please hold off on reviews until I finish updating it (and I can probably remove both sadrul and kevers). Sorry!
On 2014/04/28 04:09:05, Matt Giuca wrote: > Hey, sorry to do this. I've had a discussion with calamity and tapted here, and > I think a better solution is to go through AppListViewDelegate instead of piping > through some new dependencies within UI. Please hold off on reviews until I > finish updating it (and I can probably remove both sadrul and kevers). Sorry! From this CL, you mean ... I hope.
Yes, that wasn't some kind of evil supervillain euphemism, if you're wondering :)
Oops forgot to send this out again. tapted: PTAL. sadrul, kevers: No longer required. Thank you!
https://codereview.chromium.org/250423004/diff/40001/ui/app_list/views/app_li... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/250423004/diff/40001/ui/app_list/views/app_li... ui/app_list/views/app_list_view.cc:362: gfx::Rect bounds = screen_to_keep_centered_on_->GetPrimaryDisplay().bounds(); Can we just center in the WorkArea rather than bounds()? shelf_layout_manager.cc seems to subtract the keyboard from the work area around line 842. i.e. just a 1-line change here: gfx::Rect bounds = screen_to_keep_centered_on_->GetPrimaryDisplay().work_area();
Oh... :| Well if that's true then that's certainly better. I'll try it out this afternoon.
https://codereview.chromium.org/250423004/diff/40001/ui/app_list/views/app_li... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/250423004/diff/40001/ui/app_list/views/app_li... ui/app_list/views/app_list_view.cc:362: gfx::Rect bounds = screen_to_keep_centered_on_->GetPrimaryDisplay().bounds(); Hmm... unfortunately it looks like the order is wrong. If I make this change, the app list doesn't move when opening the keyboard. But it does move upwards on subsequent updates (like closing the keyboard). Basically, AppListView::UpdateBounds is being called before ShelfLayoutManager::CalculateTargetBounds. I've had a look at the stack traces, and they don't have a lot in common (it won't be easy to change this order around). I think we should just stick with the code I had. (Pity, it was a good suggestion.)
https://codereview.chromium.org/250423004/diff/40001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_view_delegate.cc (right): https://codereview.chromium.org/250423004/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_view_delegate.cc:419: keyboard::KeyboardController::GetInstance(); I think you'll need to guard this in a USE_ASH or similar. keyboard.gyp:keyboard depends on AURA, so I know it's not gonna be there on Mac ;) (and this file definitely is compiled on mac..) https://codereview.chromium.org/250423004/diff/40001/ui/app_list/views/app_li... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/250423004/diff/40001/ui/app_list/views/app_li... ui/app_list/views/app_list_view.cc:362: gfx::Rect bounds = screen_to_keep_centered_on_->GetPrimaryDisplay().bounds(); On 2014/04/29 05:51:35, Matt Giuca wrote: > Hmm... unfortunately it looks like the order is wrong. If I make this change, > the app list doesn't move when opening the keyboard. But it does move upwards on > subsequent updates (like closing the keyboard). Basically, > AppListView::UpdateBounds is being called before > ShelfLayoutManager::CalculateTargetBounds. > > I've had a look at the stack traces, and they don't have a lot in common (it > won't be easy to change this order around). I think we should just stick with > the code I had. (Pity, it was a good suggestion.) Drat. Maybe a short note here like // Can't use Display::work_area() because it is updated after UpdateBounds().
https://codereview.chromium.org/250423004/diff/40001/chrome/browser/ui/app_li... File chrome/browser/ui/app_list/app_list_view_delegate.cc (right): https://codereview.chromium.org/250423004/diff/40001/chrome/browser/ui/app_li... chrome/browser/ui/app_list/app_list_view_delegate.cc:419: keyboard::KeyboardController::GetInstance(); I thought you originally told me not to have USE_ASH if it wasn't strictly necessary? Looks like USE_AURA will be necessary (but not ASH), so adding it under USE_AURA. https://codereview.chromium.org/250423004/diff/40001/ui/app_list/views/app_li... File ui/app_list/views/app_list_view.cc (right): https://codereview.chromium.org/250423004/diff/40001/ui/app_list/views/app_li... ui/app_list/views/app_list_view.cc:362: gfx::Rect bounds = screen_to_keep_centered_on_->GetPrimaryDisplay().bounds(); On 2014/04/29 06:09:53, tapted wrote: > On 2014/04/29 05:51:35, Matt Giuca wrote: > > Hmm... unfortunately it looks like the order is wrong. If I make this change, > > the app list doesn't move when opening the keyboard. But it does move upwards > on > > subsequent updates (like closing the keyboard). Basically, > > AppListView::UpdateBounds is being called before > > ShelfLayoutManager::CalculateTargetBounds. > > > > I've had a look at the stack traces, and they don't have a lot in common (it > > won't be easy to change this order around). I think we should just stick with > > the code I had. (Pity, it was a good suggestion.) > > Drat. Maybe a short note here like > > // Can't use Display::work_area() because it is updated after UpdateBounds(). > Done.
lgtm
Sorry, tests failed. Can you quickly look at Patch Set 5?
oshima: Hi, can you please look at the small change to ash/shell/app_list.cc (ExampleAppList). Thanks.
slgtm
ash lgtm
Hey Matt, Just a heads up, support for virtual keyboard overscroll landed yesterday (https://codereview.chromium.org/195793004/). Just tested your patch, and it fixes the case when overscroll is disabled, but does not handle when the feature is enabled. Overscroll avoids resizing the work area / windows and instead extends the scrollable range of the underlying web content.
On 2014/04/29 15:30:48, kevers wrote: > Hey Matt, > > Just a heads up, support for virtual keyboard overscroll landed yesterday > (https://codereview.chromium.org/195793004/). Just tested your patch, and it > fixes the case when overscroll is disabled, but does not handle when the feature > is enabled. > > Overscroll avoids resizing the work area / windows and instead extends the > scrollable range of the underlying web content. Suggest making the dialog a KeyboardControllerObserver in order to receive notifications of when the keyboard is shown or dismissed. I believe UpdateBounds is being called for the non-overscroll case as a result of the work-area changing for the display. With overscroll, the bounds of the work-area remain unchanged.
Thanks for the heads up; I will investigate this.
OK I've investigated the overscroll feature. I can fix it by adding a KeyboardObserver, but plumbing the notification down into AppListView is a Rube-Goldberg-esque exercise in itself. (I've successfully done it, and it works, but it's so messy I'll need to think harder about how to structure it before committing.) Since overscroll is behind a flag, I think I'll deal with this in a follow-up CL.
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/250423004/90001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel on tryserver.chromium
Oshima: Sorry about this, I had a chat with Trent about this CL and he convinced me to refactor my existing logic for centering the app list. Now it is in AppListController, and this code is MUCH simpler (I was able to roll the fix for overscroll mode into this CL instead of delaying it to a follow-up). So the code is completely new, can you please take another look. Thanks :)
Note that this is now based on the uncommitted CL: https://codereview.chromium.org/260663002/
On 2014/04/30 08:01:16, Matt Giuca wrote: > Note that this is now based on the uncommitted CL: > https://codereview.chromium.org/260663002/ Would you mind creating a new CL as this is completely different and old CL has long review history?
On 2014/04/30 14:51:18, oshima wrote: > Would you mind creating a new CL as this is completely different and old CL has > long review history? Is this really important? (Can't you just ignore the old history?) I'm not at work and I won't be for several hours, so I can't upload a patch until then. I'd rather just stay on this one.
On 2014/04/30 22:20:31, Matt Giuca wrote: > On 2014/04/30 14:51:18, oshima wrote: > > Would you mind creating a new CL as this is completely different and old CL > has > > long review history? > > Is this really important? (Can't you just ignore the old history?) I'm not at > work and I won't be for several hours, so I can't upload a patch until then. I'd > rather just stay on this one. It's not necessary. It's just a good practice as chrome team is big and you never know who may have to look at the code later. (And you may appreciate it if you were in such a position) If you can't do it soon, this is fine.
https://codereview.chromium.org/250423004/diff/170001/ash/wm/app_list_control... File ash/wm/app_list_controller.cc (right): https://codereview.chromium.org/250423004/diff/170001/ash/wm/app_list_control... ash/wm/app_list_controller.cc:119: gfx::Rect bounds = Shell::GetScreen()->GetPrimaryDisplay().bounds(); This always returns the center of primary screen, which may not be the same as one the app list is on. bounds = Shell::GetScreen()->GetDisplayNearestWindow(view->GetWidget()->GetNativeView()).bounds(); should give you the correct one. https://codereview.chromium.org/250423004/diff/170001/ash/wm/app_list_control... ash/wm/app_list_controller.cc:248: keyboard::KeyboardController::GetInstance()->AddObserver(this); don't you have to remove the observer later?
> It's not necessary. It's just a good practice as chrome team is big and you > never know who may have to look at > the code later. (And you may appreciate it if you were in such a position) > If you can't do it soon, this is fine. I don't really understand what it would have accomplished (nobody looking at the code is going to see the Reitveld history; anyone who does bother to see the Reitveld history will typically only look at the most recent patch set; if they do look at the other patch sets it will be easier for them to understand the history if it is all in one place instead of split up in another CL). https://codereview.chromium.org/250423004/diff/170001/ash/wm/app_list_control... File ash/wm/app_list_controller.cc (right): https://codereview.chromium.org/250423004/diff/170001/ash/wm/app_list_control... ash/wm/app_list_controller.cc:119: gfx::Rect bounds = Shell::GetScreen()->GetPrimaryDisplay().bounds(); Ah, thanks for bringing this to my attention. This is actually an existing bug (it predates all of my refactoring); I have filed it here: https://code.google.com/p/chromium/issues/detail?id=368974 I will treat it as a high priority, but the fix does not belong in this CL. https://codereview.chromium.org/250423004/diff/170001/ash/wm/app_list_control... ash/wm/app_list_controller.cc:248: keyboard::KeyboardController::GetInstance()->AddObserver(this); On 2014/04/30 23:44:28, oshima wrote: > don't you have to remove the observer later? Done.
https://codereview.chromium.org/250423004/diff/210001/ash/wm/app_list_control... File ash/wm/app_list_controller.cc (right): https://codereview.chromium.org/250423004/diff/210001/ash/wm/app_list_control... ash/wm/app_list_controller.cc:247: keyboard::KeyboardController::GetInstance()->AddObserver(this); GetInstance may return NULL. https://codereview.chromium.org/250423004/diff/210001/ash/wm/app_list_control... ash/wm/app_list_controller.cc:263: keyboard::KeyboardController::GetInstance()->RemoveObserver(this); GetInstance may return NULL.
https://codereview.chromium.org/250423004/diff/210001/ash/wm/app_list_control... File ash/wm/app_list_controller.cc (right): https://codereview.chromium.org/250423004/diff/210001/ash/wm/app_list_control... ash/wm/app_list_controller.cc:247: keyboard::KeyboardController::GetInstance()->AddObserver(this); Interesting, Shell::GetInstance() doesn't have a null check. I thought these things on ash were always present. Still, doesn't hurt to check for null. Done. https://codereview.chromium.org/250423004/diff/210001/ash/wm/app_list_control... ash/wm/app_list_controller.cc:263: keyboard::KeyboardController::GetInstance()->RemoveObserver(this); On 2014/05/01 11:55:20, kevers wrote: > GetInstance may return NULL. Done.
lgtm
lgtm
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/250423004/230001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: ios_rel_device on tryserver.chromium
The CQ bit was checked by mgiuca@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/mgiuca@chromium.org/250423004/230001
Message was sent while issue was closed.
Change committed as 268131 |
