Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
476763007:
[Win] Add tab and keyboard navigation to the new avatar bubble (Closed)
|
Created:
6 years, 4 months ago by noms (inactive) Modified:
6 years, 4 months ago Reviewers:
msw CC:
chromium-reviews, tfarina Base URL:
svn://svn.chromium.org/chrome/trunk/src Project:
chromium Visibility:
Public. |
Description[Win] Add tab and keyboard navigation to the new avatar bubble
Both tabbing and up/down arrows should navigate through all the
visible controls.
I've changed the EditablePhotoButton to actually be a button and
not just an image, so that tabbing and pressing enter actually
works.
BUG=405057
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=291079
Patch Set 1 : #
Total comments: 18
Patch Set 2 : msw comments #
Total comments: 8
Patch Set 3 : less sketchy event handling for all #
Total comments: 10
Patch Set 4 : nits #
Messages
Total messages: 17 (0 generated)
Hi, Finally bringing tabs back to the new avatar bubble. Please take a look. Thanks! https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:692: users_button_->RequestFocus(); This doesn't do what I think it should do. Ideally, I'd like this button to have the tab_index=0, and always start tabbing from it. This doesn't happen, though. Pressing tab will start from the "top" of the bubble, i.e. the EditablePhotoButton. :/ I've also tried this->GetFocusManager()->SetFocusedView(users_button_) with no results. I think I am doing this wrong?
https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:280: photo_overlay_->SetVisible(false); Hmm, does this still work as intended with hovering? (even in edge cases like hovering the button while cycling through the views?) https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:378: RightAlignedIconLabelButton::OnBlur(); nit: I imagine you mean to call OnFocus? https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:379: SetState(views::CustomButton::STATE_HOVERED); Hmm, does this still work as intended with clicking? (even in edge cases, like mouse down and drag off the button?) https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:384: SetState(views::CustomButton::STATE_NORMAL); Hmm, does this still work as intended with hovering? (even in edge cases like hovering the button while cycling through the views?) https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:692: users_button_->RequestFocus(); On 2014/08/19 20:15:00, Monica Dinculescu wrote: > This doesn't do what I think it should do. Ideally, I'd like this button to have > the tab_index=0, and always start tabbing from it. This doesn't happen, though. > Pressing tab will start from the "top" of the bubble, i.e. the > EditablePhotoButton. :/ > > I've also tried this->GetFocusManager()->SetFocusedView(users_button_) with no > results. I think I am doing this wrong? Try overloading GetInitiallyFocusedView. https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:761: } else if (current_profile_photo_ && sender == current_profile_photo_) { nit: Is the |current_profile_photo_| NULL check still needed? https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:888: GetFocusManager()->AdvanceFocus(accelerator.key_code() != ui::VKEY_DOWN); The old avatar just used up/down to focus the various profiles (not the add new user button). It seems odd for up/down to cycle focus among all the bubble content... was that discussed with PM/UX/Accessibility?
https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:280: photo_overlay_->SetVisible(false); No, not excellently. I have removed the focus in mouse exited -- i think this should cover all weird cases. On 2014/08/19 22:39:06, msw wrote: > Hmm, does this still work as intended with hovering? (even in edge cases like > hovering the button while cycling through the views?) https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:378: RightAlignedIconLabelButton::OnBlur(); On 2014/08/19 22:39:06, msw wrote: > nit: I imagine you mean to call OnFocus? Done. https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:379: SetState(views::CustomButton::STATE_HOVERED); The clicking works fine. The only thing that happens is that if the button is focused (and shows the pencil icon), if you mouse on it, and mouse away, the pencil icon will go away. The button is still focused, so pressing enter on it will still activate it. Do you think it's worth adding a MouseExisted ClearFocus() like in EditableProfilePhoto? On 2014/08/19 22:39:06, msw wrote: > Hmm, does this still work as intended with clicking? (even in edge cases, like > mouse down and drag off the button?) https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:384: SetState(views::CustomButton::STATE_NORMAL); See above. On 2014/08/19 22:39:05, msw wrote: > Hmm, does this still work as intended with hovering? (even in edge cases like > hovering the button while cycling through the views?) https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:692: users_button_->RequestFocus(); We've decided that's not needed right now (and it might lead to a weird tabbing paradigm), so taking it out for now. On 2014/08/19 22:39:06, msw wrote: > On 2014/08/19 20:15:00, Monica Dinculescu wrote: > > This doesn't do what I think it should do. Ideally, I'd like this button to > have > > the tab_index=0, and always start tabbing from it. This doesn't happen, > though. > > Pressing tab will start from the "top" of the bubble, i.e. the > > EditablePhotoButton. :/ > > > > I've also tried this->GetFocusManager()->SetFocusedView(users_button_) with no > > results. I think I am doing this wrong? > > Try overloading GetInitiallyFocusedView. https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:761: } else if (current_profile_photo_ && sender == current_profile_photo_) { On 2014/08/19 22:39:05, msw wrote: > nit: Is the |current_profile_photo_| NULL check still needed? Done. https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:888: GetFocusManager()->AdvanceFocus(accelerator.key_code() != ui::VKEY_DOWN); On 2014/08/19 22:39:06, msw wrote: > The old avatar just used up/down to focus the various profiles (not the add new > user button). It seems odd for up/down to cycle focus among all the bubble > content... was that discussed with PM/UX/Accessibility? Done. I think I was just being too keen on keeping the old functionality, but you're right that it's weird.
https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:280: photo_overlay_->SetVisible(false); On 2014/08/20 16:43:52, Monica Dinculescu wrote: > No, not excellently. I have removed the focus in mouse exited -- i think this > should cover all weird cases. > > On 2014/08/19 22:39:06, msw wrote: > > Hmm, does this still work as intended with hovering? (even in edge cases like > > hovering the button while cycling through the views?) > What? That seems quite bad... if the user focuses some view (not necessarily this one), then hovers and exits *this* view, the focus will be cleared? I imagine the right fix is something like: if (photo_overlay_ && state() != STATE_HOVERED) photo_overlay_->SetVisible(false); Otherwise, I would rather this was left with some odd edge case behavior when the user hovers/tabs simultaneously than risk clobbering focus on mouse exit (ie. just remove the ClearFocus call). https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:379: SetState(views::CustomButton::STATE_HOVERED); On 2014/08/20 16:43:52, Monica Dinculescu wrote: > The clicking works fine. The only thing that happens is that if the button is > focused (and shows the pencil icon), if you mouse on it, and mouse away, the > pencil icon will go away. The button is still focused, so pressing enter on it > will still activate it. That sounds fine. > Do you think it's worth adding a MouseExisted ClearFocus() like in > EditableProfilePhoto? No, I don't think that pattern is good at all. > On 2014/08/19 22:39:06, msw wrote: > > Hmm, does this still work as intended with clicking? (even in edge cases, like > > mouse down and drag off the button?) > https://codereview.chromium.org/476763007/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/476763007/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:261: virtual void OnMouseEntered(const ui::MouseEvent& event) OVERRIDE { nit: maybe overload StateChanged instead of OnMouseEntered/Exited? (then OnFocus could SetState(views::CustomButton::STATE_HOVERED)). You might not even need the OnBlur (and maybe OnFocus?) overrides. https://codereview.chromium.org/476763007/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:268: GetFocusManager()->ClearFocus(); This seems very wrong (see my other comment). https://codereview.chromium.org/476763007/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:381: SetState(views::CustomButton::STATE_HOVERED); nit: This might be done for you, but I'm not sure. https://codereview.chromium.org/476763007/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:386: SetState(views::CustomButton::STATE_NORMAL); nit: This may already be accomplished by CustomButton::OnBlur.
https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/476763007/diff/40001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:280: photo_overlay_->SetVisible(false); Oh, no, it would have just cleared the focus on the photo control, not all of them (if i understood the focus manager correctly). In any case, StateChanged() is better. On 2014/08/20 19:29:25, msw wrote: > On 2014/08/20 16:43:52, Monica Dinculescu wrote: > > No, not excellently. I have removed the focus in mouse exited -- i think this > > should cover all weird cases. > > > > On 2014/08/19 22:39:06, msw wrote: > > > Hmm, does this still work as intended with hovering? (even in edge cases > like > > > hovering the button while cycling through the views?) > > > > What? That seems quite bad... if the user focuses some view (not necessarily > this one), then hovers and exits *this* view, the focus will be cleared? I > imagine the right fix is something like: > if (photo_overlay_ && state() != STATE_HOVERED) > photo_overlay_->SetVisible(false); > Otherwise, I would rather this was left with some odd edge case behavior when > the user hovers/tabs simultaneously than risk clobbering focus on mouse exit > (ie. just remove the ClearFocus call). https://codereview.chromium.org/476763007/diff/60001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/476763007/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:261: virtual void OnMouseEntered(const ui::MouseEvent& event) OVERRIDE { Still need OnFocus/OnBlur because focusing on/away doesn't trigger a state change. But nuked the mice and went to StateChanged() for less hackiness. On 2014/08/20 19:29:25, msw wrote: > nit: maybe overload StateChanged instead of OnMouseEntered/Exited? > (then OnFocus could SetState(views::CustomButton::STATE_HOVERED)). > You might not even need the OnBlur (and maybe OnFocus?) overrides. https://codereview.chromium.org/476763007/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:268: GetFocusManager()->ClearFocus(); On 2014/08/20 19:29:25, msw wrote: > This seems very wrong (see my other comment). Done. https://codereview.chromium.org/476763007/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:381: SetState(views::CustomButton::STATE_HOVERED); Sadly, no. OnFocus/Blur doesn't trigger a state change. It probably should, but I'd rather not do it in this CL as it will get merged to M38 and break everything. :) On 2014/08/20 19:29:25, msw wrote: > nit: This might be done for you, but I'm not sure. https://codereview.chromium.org/476763007/diff/60001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:386: SetState(views::CustomButton::STATE_NORMAL); Same as above. On 2014/08/20 19:29:25, msw wrote: > nit: This may already be accomplished by CustomButton::OnBlur.
LGTM with nits. https://codereview.chromium.org/476763007/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/476763007/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:271: if (photo_overlay_) nit: maybe SetState(STATE_HOVERED)? https://codereview.chromium.org/476763007/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:277: if (photo_overlay_) nit: maybe add |&& state() != STATE_HOVERED| https://codereview.chromium.org/476763007/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:377: SetState(views::CustomButton::STATE_HOVERED); nit: you may be able to remove "views::CustomButton::" here and below.
(no code, just questions) https://codereview.chromium.org/476763007/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/476763007/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:271: if (photo_overlay_) Hmm, setting the state here and in OnBlur leads to this weird thing where if you tab on it, then mouseover, the tab away, you'll be in a STATE_NORMAL, but the overlay will still display (because of how the if statement in StateChanged() works), and then mousing away won't clear the overlay. On 2014/08/20 20:49:56, msw wrote: > nit: maybe SetState(STATE_HOVERED)? https://codereview.chromium.org/476763007/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:277: if (photo_overlay_) Hmm, but it's definitely going to to be in a hovered state, if i do make the above state change, no? On 2014/08/20 20:49:56, msw wrote: > nit: maybe add |&& state() != STATE_HOVERED|
https://codereview.chromium.org/476763007/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/476763007/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:271: if (photo_overlay_) On 2014/08/20 21:11:48, Monica Dinculescu wrote: > Hmm, setting the state here and in OnBlur leads to this weird thing where if you > tab on it, then mouseover, the tab away, you'll be in a STATE_NORMAL, but the > overlay will still display (because of how the if statement in StateChanged() > works), and then mousing away won't clear the overlay. > > > On 2014/08/20 20:49:56, msw wrote: > > nit: maybe SetState(STATE_HOVERED)? > Ignore my suggestion if it works better as-is. https://codereview.chromium.org/476763007/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:277: if (photo_overlay_) On 2014/08/20 21:11:48, Monica Dinculescu wrote: > Hmm, but it's definitely going to to be in a hovered state, if i do make the > above state change, no? > > On 2014/08/20 20:49:56, msw wrote: > > nit: maybe add |&& state() != STATE_HOVERED| > You can ignore my comment above. Only address this comment if it helps the case where you (1) tab focus into the view (OnFocus shows |photo_overlay_|), (2) mouse over the view (StateChanged sees STATE_HOVERED and sets |photo_overlay_| visible again), (3) tab focus out of the view (ie. this shouldn't hide |photo_overlay_| because the mouse is still hovering the view).
Thanks for the help! Catapulting it to the CQ. https://codereview.chromium.org/476763007/diff/80001/chrome/browser/ui/views/... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/476763007/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:271: if (photo_overlay_) On 2014/08/20 21:20:08, msw wrote: > On 2014/08/20 21:11:48, Monica Dinculescu wrote: > > Hmm, setting the state here and in OnBlur leads to this weird thing where if > you > > tab on it, then mouseover, the tab away, you'll be in a STATE_NORMAL, but the > > overlay will still display (because of how the if statement in StateChanged() > > works), and then mousing away won't clear the overlay. > > > > > > On 2014/08/20 20:49:56, msw wrote: > > > nit: maybe SetState(STATE_HOVERED)? > > > > Ignore my suggestion if it works better as-is. Done. https://codereview.chromium.org/476763007/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:277: if (photo_overlay_) Did this now that i didn't do the other one :) Yay for mutual exclusivity. On 2014/08/20 21:20:08, msw wrote: > On 2014/08/20 21:11:48, Monica Dinculescu wrote: > > Hmm, but it's definitely going to to be in a hovered state, if i do make the > > above state change, no? > > > > On 2014/08/20 20:49:56, msw wrote: > > > nit: maybe add |&& state() != STATE_HOVERED| > > > > You can ignore my comment above. Only address this comment if it helps the case > where you (1) tab focus into the view (OnFocus shows |photo_overlay_|), (2) > mouse over the view (StateChanged sees STATE_HOVERED and sets |photo_overlay_| > visible again), (3) tab focus out of the view (ie. this shouldn't hide > |photo_overlay_| because the mouse is still hovering the view). https://codereview.chromium.org/476763007/diff/80001/chrome/browser/ui/views/... chrome/browser/ui/views/profiles/profile_chooser_view.cc:377: SetState(views::CustomButton::STATE_HOVERED); On 2014/08/20 20:49:56, msw wrote: > nit: you may be able to remove "views::CustomButton::" here and below. Done.
The CQ bit was checked by noms@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/476763007/100001
FYI, CQ is re-trying this CL (attempt #1). The failing builders are: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...) android_clang_dbg on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...) linux_chromium_rel_swarming on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_gpu on tryserver.chromium.gpu (http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
The CQ bit was checked by noms@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/noms@chromium.org/476763007/100001
Message was sent while issue was closed.
Committed patchset #4 (100001) as 291079 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
