Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2710543002:
Linux/Windows: Adding tooltip in profile switcher menu. (Closed)
DescriptionLinux/Windows: Adding tooltip in profile switcher menu.
The current profile item menu displays the photo, the profile name and
the email (only if the user is signed in). To display the profile name
and the email one top of the other, the class NonInteractiveContainer is
used. This view blocks the tooltips (see
View::CanProcessEventsWithinSubtree()).
To solve this problem, this NonInteractiveContainer view is removed and
replaced by a GridLayout to be able to show the profile icon on the left,
and the profile name and the email on the right.
Impact for the current profile in the following cases:
- Guest profile
- Supervised profile
- Not logged in
- Logged in
- Not logged in with account consistency flag
- Logged in with account consistency flag
Screenshot on Linux: https://drive.google.com/drive/folders/0ByXziH_JVCGJbExCUkJSeWdmbVk?usp=sharing
Tested on windows by msarda
BUG=674462
Review-Url: https://codereview.chromium.org/2710543002
Cr-Commit-Position: refs/heads/master@{#453904}
Committed: https://chromium.googlesource.com/chromium/src/+/eded7142ea9f7ff109e9b3a5606cac4161aafbdb
Patch Set 1 #
Total comments: 6
Patch Set 2 : Update #
Total comments: 2
Patch Set 3 : Renaming #Patch Set 4 : Fix for account consistency #
Total comments: 2
Patch Set 5 : Adding tmp variable #
Total comments: 2
Patch Set 6 : Fixing when logged in with account consistency flag #
Total comments: 2
Patch Set 7 : Nit #Messages
Total messages: 30 (16 generated)
The CQ bit was checked by jlebel@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Linux/Windows: Adding tooltip in profile switcher menu. The current profile item menu displays the photo, the profile name and the email (only if the user is signed in). To display the profile name and the email one top of the other, the class NonInteractiveContainer is used. This view blocks the tooltips (see View::CanProcessEventsWithinSubtree()). To solve this problem, this NonInteractiveContainer view is removed and a GridLayout is used to be able to profile icon on the left, and have the profile name and the email on the right. BUG=674462 ========== to ========== Linux/Windows: Adding tooltip in profile switcher menu. The current profile item menu displays the photo, the profile name and the email (only if the user is signed in). To display the profile name and the email one top of the other, the class NonInteractiveContainer is used. This view blocks the tooltips (see View::CanProcessEventsWithinSubtree()). To solve this problem, this NonInteractiveContainer view is removed and replaced by a GridLayout to be able to show the profile icon on the left, and the profile name and the email on the right. BUG=674462 ==========
jlebel@chromium.org changed reviewers: + msarda@chromium.org
Hello Mihai, Can you review this patch? Thanks,
https://codereview.chromium.org/2710543002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2710543002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1749: ? (kCurrentProfileMenuItemHeight / 2) Should you divide by 2 if there is a single line? https://codereview.chromium.org/2710543002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1749: ? (kCurrentProfileMenuItemHeight / 2) Do you need this new "magic" constant kCurrentProfileMenuItemHeight (why is it 58 and not another value)? Maybe this value can be computed dynamically based on the height of the photo. https://codereview.chromium.org/2710543002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1798: view->AddChildView(manage_accounts_button_); I think the view will not be properly displayed in this case as it looks like a second label is not displayed in this case. To testing it, go to chrome://flags and enable "account consistency".
Patchset #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
https://codereview.chromium.org/2710543002/diff/1/chrome/browser/ui/views/pro... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2710543002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1749: ? (kCurrentProfileMenuItemHeight / 2) On 2017/02/20 17:01:46, msarda wrote: > Should you divide by 2 if there is a single line? If I don't set the height as half of the total height, the first line takes its preferred height (so about 20 pixels), and the second line takes the rest. https://codereview.chromium.org/2710543002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1749: ? (kCurrentProfileMenuItemHeight / 2) On 2017/02/20 17:01:46, msarda wrote: > Do you need this new "magic" constant kCurrentProfileMenuItemHeight (why is it > 58 and not another value)? Maybe this value can be computed dynamically based on > the height of the photo. Done. https://codereview.chromium.org/2710543002/diff/1/chrome/browser/ui/views/pro... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1798: view->AddChildView(manage_accounts_button_); On 2017/02/20 17:01:46, msarda wrote: > I think the view will not be properly displayed in this case as it looks like a > second label is not displayed in this case. To testing it, go to chrome://flags > and enable "account consistency". Done.
LGTM with a nit. https://codereview.chromium.org/2710543002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2710543002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1747: int row_height = EditableProfilePhoto::icon_image_side() + I think this is actually the height of the entire current_profile_card. Please rename it to profile_card_height.
jlebel@chromium.org changed reviewers: + pkasting@chromium.org
Hello Peter, I've redone my previous patch by following your advice. I removed the NonInteractiveContainer view. Can you review my patch when you will have time? Thanks, https://codereview.chromium.org/2710543002/diff/60001/chrome/browser/ui/views... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2710543002/diff/60001/chrome/browser/ui/views... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1747: int row_height = EditableProfilePhoto::icon_image_side() + On 2017/02/21 08:51:48, msarda wrote: > I think this is actually the height of the entire current_profile_card. Please > rename it to profile_card_height. Done.
Patchset #4 (id:100001) has been deleted
I'm sort of confused as to how this fixes the bug. Wasn't the original issue that BackgroundColorHoverButton wouldn't get events when they hittested into its child views? It seems like this will still be true even if you use GridLayout rather than BoxLayout. What is different about the GridLayout approach that makes this work? https://codereview.chromium.org/2710543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2710543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1773: (avatar_item.signed_in && !switches::IsEnableAccountConsistency()) ? 2 Nit: I would pull this last expression out to a well-named temp, which will make it easier to read the whole expression (and the wrapping will be less torturous).
Hello Peter, Sorry for taking so much time from you. The goal is to be able to display the profile with 2 columns inside the BackgroundColorHoverButton view. This view has to be highlighed when the mouse is anywhere inside that view. On the first column, there is the profile photo that is 2 lines height. On the second column, the name is on the first line, and the email is on the second line. The previous code was doing that with a first horizontal layout containing the profile photo and the NonInteractiveContainer view. And the NonInteractiveContainer view was using vertical layout with 2 labels (the name and the email). The NonInteractiveContainer view was refusing mouse events so they would only be received by BackgroundColorHoverButton view. This was done by NonInteractiveContainer::CanProcessEventsWithinSubtree(). The Label is not an issue related to the mouse events. Only NonInteractiveContainer view was an issue. I removed NonInteractiveContainer view, and I now use a grid layout to create the 2 columns. All mouse events are now received by BackgroundColorHoverButton view, so the button is always highlighted as long as the mouse is inside it. And the tooltips over the labels works. Jérôme, https://codereview.chromium.org/2710543002/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2710543002/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1773: (avatar_item.signed_in && !switches::IsEnableAccountConsistency()) ? 2 On 2017/02/23 02:02:22, Peter Kasting wrote: > Nit: I would pull this last expression out to a well-named temp, which will make > it easier to read the whole expression (and the wrapping will be less > torturous). Done.
https://codereview.chromium.org/2710543002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2710543002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1753: avatar_item.signed_in ? (profile_card_height / 2) : profile_card_height); I don't know why there is a format change here. git cl format really wants to be that way. Do you want me to keep it that way, or should I revert this part and go against the default format?
On 2017/02/23 10:53:41, jlebel wrote: > The previous code was doing that with a first horizontal layout > containing the profile photo and the NonInteractiveContainer view. And > the NonInteractiveContainer view was using vertical layout with 2 > labels (the name and the email). Right, and the NonInteractiveContainer was needed because without it, when the mouse was over the views inside, it was considered to be "not over" the button itself for the purposes of highlighting, right? Which seems like it would still be an issue here, though you say below: > All mouse events are now received by > BackgroundColorHoverButton view, so the button is always highlighted as > long as the mouse is inside it. That's the part that confuses me. It's not clear to me why with this GridLayout setup mouse events are received by the BackgroundColorHoverButton, but with the previous setup, minus the NonInteractiveContainer, they weren't. In both cases, the BackgroundColorHoverButton is hosting multiple child views (which is already weird, considering it's a LabelButton) and using a layout manager. It's just a different layout manager. So what's the key difference that makes this work? ...And does is really make sense to use BackgroundColorHoverButton as a base here at all? We don't actually want the "label" part of it being a LabelButton, we really just want it to be a clickable, hoverable thing (so, just a Button). I wonder if BackgroundColorHoverButton is really composed of some pieces that all instantiations want, and other pieces only some instantiations want, and we should split this into multiple different classes for the different uses. This is what I meant about assuming this code might all be potentially wrong and rewriting it. https://codereview.chromium.org/2710543002/diff/140001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2710543002/diff/140001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1753: avatar_item.signed_in ? (profile_card_height / 2) : profile_card_height); On 2017/02/23 13:38:23, jlebel wrote: > I don't know why there is a format change here. > git cl format really wants to be that way. > > Do you want me to keep it that way, or should I revert this part and go against > the default format? git cl format's formatting here is fine, IMO slightly better.
Description was changed from ========== Linux/Windows: Adding tooltip in profile switcher menu. The current profile item menu displays the photo, the profile name and the email (only if the user is signed in). To display the profile name and the email one top of the other, the class NonInteractiveContainer is used. This view blocks the tooltips (see View::CanProcessEventsWithinSubtree()). To solve this problem, this NonInteractiveContainer view is removed and replaced by a GridLayout to be able to show the profile icon on the left, and the profile name and the email on the right. BUG=674462 ========== to ========== Linux/Windows: Adding tooltip in profile switcher menu. The current profile item menu displays the photo, the profile name and the email (only if the user is signed in). To display the profile name and the email one top of the other, the class NonInteractiveContainer is used. This view blocks the tooltips (see View::CanProcessEventsWithinSubtree()). To solve this problem, this NonInteractiveContainer view is removed and replaced by a GridLayout to be able to show the profile icon on the left, and the profile name and the email on the right. Impact for the current profile in the following cases: - Guest profile - Supervised profile - Not logged in - Logged in - Not logged in with account consistency flag - Logged in with account consistency flag Screenshot on Linux: https://drive.google.com/drive/folders/0ByXziH_JVCGJbExCUkJSeWdmbVk?usp=sharing BUG=674462 ==========
Description was changed from ========== Linux/Windows: Adding tooltip in profile switcher menu. The current profile item menu displays the photo, the profile name and the email (only if the user is signed in). To display the profile name and the email one top of the other, the class NonInteractiveContainer is used. This view blocks the tooltips (see View::CanProcessEventsWithinSubtree()). To solve this problem, this NonInteractiveContainer view is removed and replaced by a GridLayout to be able to show the profile icon on the left, and the profile name and the email on the right. Impact for the current profile in the following cases: - Guest profile - Supervised profile - Not logged in - Logged in - Not logged in with account consistency flag - Logged in with account consistency flag Screenshot on Linux: https://drive.google.com/drive/folders/0ByXziH_JVCGJbExCUkJSeWdmbVk?usp=sharing BUG=674462 ========== to ========== Linux/Windows: Adding tooltip in profile switcher menu. The current profile item menu displays the photo, the profile name and the email (only if the user is signed in). To display the profile name and the email one top of the other, the class NonInteractiveContainer is used. This view blocks the tooltips (see View::CanProcessEventsWithinSubtree()). To solve this problem, this NonInteractiveContainer view is removed and replaced by a GridLayout to be able to show the profile icon on the left, and the profile name and the email on the right. Impact for the current profile in the following cases: - Guest profile - Supervised profile - Not logged in - Logged in - Not logged in with account consistency flag - Logged in with account consistency flag Screenshot on Linux: https://drive.google.com/drive/folders/0ByXziH_JVCGJbExCUkJSeWdmbVk?usp=sharing Tested on windows by msarda BUG=674462 ==========
Hello Peter, At the beginning, I didn’t know understand why the author of that code created NonInteractiveContainer. The goal was only for the layout: to be able to have 2 columns. The first column contains the profile photo across 2 lines, and in the second column contains 2 labels vertically. To implements those 2 lines next to the photo, NonInteractiveContainer has been introduced. The drawback of this design was catching mouse events. This view was preventing the button (the parent view, BackgroundColorHoverButton) to be highlighted. So to solve this problem, the author override CanProcessEventsWithinSubtree() [1]. But this prevented the NonInteractiveContainer to display the labels tooltips [2]. Now without NonInteractiveContainer, the layout is done with GridLayout. The mouse events are still received by BackgroundColorHoverButton, because Label::CanProcessEventsWithinSubtree() returns false since the label is not selectable, see [1]: https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?type=cs&l=396 https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?type=cs&l=783 Tooltips works on label because the default implementation of View::GetTooltipHandlerForPoint() is overridden in Label [2]: https://cs.chromium.org/chromium/src/ui/views/controls/label.cc?type=cs&l=387 In this implementation, Label doesn’t care about CanProcessEventsWithinSubtree(). About BackgroundColorHoverButton, this class is used for 2 cases: the current profile, the other profiles, the guest and other buttons in this menu [3]. For the current profile, the label and the image are not used. But any other buttons in this menu, the image and the label of the LabelButton is used. The goal is to share the same behavior of the selection (BackgroundColorHoverButton::OnNativeThemeChaged() and BackgroundColorHoverButton::StateChanged()). I think BackgroundColorHoverButton can probably a subclass of CustomButton. For that I would have to create constructor with a profile photo, a title (non null) and a subtitle (nullable). The layout would be done automatically in that constructor based on the subtitle or not. This way, I would used only one way to get the instance of BackgroundColorHoverButton for the current profile (signed in or not), and for the other profiles. If it is ok with you, I would like to do that part in a next patch. I already need to remove pre-material design code. I should have started by that, but unfortunately I didn’t. Also I’m changing again this patch because I missed a case (when the current profile is signed in with account consistency). I updated the description of this patch to list all cases that exist. [1] Background about mouse events: Mouse events cannot be sent to a view when CanProcessEventsWithinSubtree() returns false because of: https://cs.chromium.org/chromium/src/ui/views/view_targeter_delegate.cc?type=... [2] Background about tooltips: To get the tooltip, GetTooltipHandlerForPoint() is called. The implementation in View is to find a subview that has a tooltip. The first thing in this method is to return NULL if !CanProcessEventsWithinSubtree(). https://cs.chromium.org/chromium/src/ui/views/view.cc?l=997 [3] https://cs.chromium.org/chromium/src/chrome/browser/ui/views/profiles/profile... https://cs.chromium.org/chromium/src/chrome/browser/ui/views/profiles/profile... https://cs.chromium.org/chromium/src/chrome/browser/ui/views/profiles/profile... https://cs.chromium.org/chromium/src/chrome/browser/ui/views/profiles/profile...
LGTM! On 2017/02/28 15:21:38, jlebel wrote: > I think BackgroundColorHoverButton can probably a subclass of CustomButton. For > that I would have to create constructor with a profile photo, a title (non null) > and a subtitle (nullable). The layout would be done automatically in that > constructor based on the subtitle or not. This way, I would used only one way to > get the instance of BackgroundColorHoverButton for the current profile (signed > in or not), and for the other profiles. > If it is ok with you, I would like to do that part in a next patch. I already > need to remove pre-material design code. I should have started by that, but > unfortunately I didn’t. SGTM; I'm almost always OK with refactoring separately from fixing bugs, once we understand what the plan is. I just wanted to make sure I understood the plan and why this code was doing what it's doing. Thanks to your excellent explanation, now I feel like I do :) https://codereview.chromium.org/2710543002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2710543002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1761: (avatar_item.signed_in && !switches::IsEnableAccountConsistency()); Nit: If you do: const int num_labels = avatar_item.signed_in && !switches::IsEnableAccountConsistency() ? 2 : 1; Then below you can do: int line_height = profile_card_height / num_labels; And replace |row_span| with a direct inline use of |num_labels|. (The final use of |has_two_labels| would have to become num_labels == 2.)
Thanks for your time. https://codereview.chromium.org/2710543002/diff/160001/chrome/browser/ui/view... File chrome/browser/ui/views/profiles/profile_chooser_view.cc (right): https://codereview.chromium.org/2710543002/diff/160001/chrome/browser/ui/view... chrome/browser/ui/views/profiles/profile_chooser_view.cc:1761: (avatar_item.signed_in && !switches::IsEnableAccountConsistency()); On 2017/03/01 06:15:59, Peter Kasting wrote: > Nit: If you do: > > const int num_labels = > avatar_item.signed_in && !switches::IsEnableAccountConsistency() ? 2 : 1; > > Then below you can do: > > int line_height = profile_card_height / num_labels; > > And replace |row_span| with a direct inline use of |num_labels|. > > (The final use of |has_two_labels| would have to become num_labels == 2.) Done.
The CQ bit was checked by jlebel@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from msarda@chromium.org, pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2710543002/#ps180001 (title: "Nit")
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": 180001, "attempt_start_ts": 1488364802337420,
"parent_rev": "4c9507d55e235b96d792dd0244f0ef41e470a5f1", "commit_rev":
"eded7142ea9f7ff109e9b3a5606cac4161aafbdb"}
Message was sent while issue was closed.
Description was changed from ========== Linux/Windows: Adding tooltip in profile switcher menu. The current profile item menu displays the photo, the profile name and the email (only if the user is signed in). To display the profile name and the email one top of the other, the class NonInteractiveContainer is used. This view blocks the tooltips (see View::CanProcessEventsWithinSubtree()). To solve this problem, this NonInteractiveContainer view is removed and replaced by a GridLayout to be able to show the profile icon on the left, and the profile name and the email on the right. Impact for the current profile in the following cases: - Guest profile - Supervised profile - Not logged in - Logged in - Not logged in with account consistency flag - Logged in with account consistency flag Screenshot on Linux: https://drive.google.com/drive/folders/0ByXziH_JVCGJbExCUkJSeWdmbVk?usp=sharing Tested on windows by msarda BUG=674462 ========== to ========== Linux/Windows: Adding tooltip in profile switcher menu. The current profile item menu displays the photo, the profile name and the email (only if the user is signed in). To display the profile name and the email one top of the other, the class NonInteractiveContainer is used. This view blocks the tooltips (see View::CanProcessEventsWithinSubtree()). To solve this problem, this NonInteractiveContainer view is removed and replaced by a GridLayout to be able to show the profile icon on the left, and the profile name and the email on the right. Impact for the current profile in the following cases: - Guest profile - Supervised profile - Not logged in - Logged in - Not logged in with account consistency flag - Logged in with account consistency flag Screenshot on Linux: https://drive.google.com/drive/folders/0ByXziH_JVCGJbExCUkJSeWdmbVk?usp=sharing Tested on windows by msarda BUG=674462 Review-Url: https://codereview.chromium.org/2710543002 Cr-Commit-Position: refs/heads/master@{#453904} Committed: https://chromium.googlesource.com/chromium/src/+/eded7142ea9f7ff109e9b3a5606c... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/eded7142ea9f7ff109e9b3a5606c... |
