Make the account chooser and CVC dialog use the same icon with toolip for Views.

Make the account chooser and CVC dialog use the same icon with toolip for Views.

The CL changes the icon to (i). The account chooser reuses the implementation of the icon and the tooltip for the CVC dialog.

BUG=690920
Review-Url: https://codereview.chromium.org/2684343006
Cr-Commit-Position: refs/heads/master@{#451273}
Committed: https://chromium.googlesource.com/chromium/src/+/864f2986aa206bae957944685e1bbb0d7b4fdcc4

[vasilii, 2017-02-10 14:05:07]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-10 14:05:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2017-02-10 14:15:15]

vasilii@chromium.org changed reviewers:
+ estade@chromium.org

[vasilii, 2017-02-10 14:15:16]

Hi Evan,

I have few questions
- What's the magic constant '233' in info_bubble.cc? Due to this hardcoded width
the tooltip doesn't look nice for the account chooser (the screenshots are in
the bug). Do you think we should change it so that the autofill sets the
hardcoded value but by default it just stretches accordingly?
- The Enamel team wants the icon to consume the click. It's not user-visible for
the CVC dialog. Do you want me to move the click consumption to
autofill::TooltipIcon?

[commit-bot: I haz the power, 2017-02-10 14:52:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-10 14:52:21]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2017-02-10 15:43:38]

On 2017/02/10 14:15:16, vasilii wrote:
> Hi Evan,
> 
> I have few questions
> - What's the magic constant '233' in info_bubble.cc? Due to this hardcoded
width
> the tooltip doesn't look nice for the account chooser (the screenshots are in
> the bug). Do you think we should change it so that the autofill sets the
> hardcoded value but by default it just stretches accordingly?

yea that member's a bit braindead. The reality doesn't match the docs in the
header, there's no way to set it, etc. I suspect it used to be set via a setter,
but was changed to hardcoded because it was always being set to 233. (This
bubble was previously used for showing input errors as well but I think that's
no more.) You could expose a setter for clients. Perhaps if the preference is 0,
you use the content's (i.e. label's) preferred dimensions.

> - The Enamel team wants the icon to consume the click. It's not user-visible
for
> the CVC dialog. Do you want me to move the click consumption to
> autofill::TooltipIcon?

Not user visible? Wouldn't it mean that clicking no longer focuses the cvc
entry? One other option is to make the credential item check the mouse event's
target and ignore as appropriate.

https://codereview.chromium.org/2684343006/diff/1/chrome/browser/ui/views/pas...
File chrome/browser/ui/views/passwords/credentials_item_view.cc (right):

https://codereview.chromium.org/2684343006/diff/1/chrome/browser/ui/views/pas...
chrome/browser/ui/views/passwords/credentials_item_view.cc:66: };
disallow copy and assign

https://codereview.chromium.org/2684343006/diff/1/chrome/browser/ui/views/pas...
chrome/browser/ui/views/passwords/credentials_item_view.cc:77: :
LabelButton(button_listener, base::string16()),
why is this even a label button? It seems like you're overriding basically all
the behaviors specific to label buttons. Should it inherit from CustomButton?

[vasilii, 2017-02-10 16:54:22]

> > - The Enamel team wants the icon to consume the click. It's not user-visible
> for
> > the CVC dialog. Do you want me to move the click consumption to
> > autofill::TooltipIcon?
> 
> Not user visible? Wouldn't it mean that clicking no longer focuses the cvc
> entry? One other option is to make the credential item check the mouse event's
> target and ignore as appropriate.


What do you mean? Clicking on (?) currently doesn't do anything with the focus.

[vasilii, 2017-02-10 18:33:20]

> Not user visible? Wouldn't it mean that clicking no longer focuses the cvc
> entry? One other option is to make the credential item check the mouse event's
> target and ignore as appropriate.

Checking the target doesn't work because RootView::OnMousePressed dispatches the
event with the current target (=CredentialsItemView). The previous one that
didn't handle the click is forgotten.

[Evan Stade, 2017-02-10 19:06:15]

I may be misremembering but I thought the icon was inside the text field.
I'm OK with swallowing clicks in the tooltip icon. But also I'd request you
move the class out of autofill/ if it's going to be reused.

On Feb 10, 2017 11:33 AM, <vasilii@chromium.org> wrote:

> > Not user visible? Wouldn't it mean that clicking no longer focuses the
> cvc
> > entry? One other option is to make the credential item check the mouse
> event's
> > target and ignore as appropriate.
>
> Checking the target doesn't work because RootView::OnMousePressed
> dispatches the
> event with the current target (=CredentialsItemView). The previous one that
> didn't handle the click is forgotten.
>
> https://codereview.chromium.org/2684343006/
>

-- 
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.

[vasilii, 2017-02-13 15:26:51]

On 2017/02/10 19:06:15, Evan Stade wrote:
> I may be misremembering but I thought the icon was inside the text field.
> I'm OK with swallowing clicks in the tooltip icon. But also I'd request you
> move the class out of autofill/ if it's going to be reused.
> 
> On Feb 10, 2017 11:33 AM, <mailto:vasilii@chromium.org> wrote:
> 
> > > Not user visible? Wouldn't it mean that clicking no longer focuses the
> > cvc
> > > entry? One other option is to make the credential item check the mouse
> > event's
> > > target and ignore as appropriate.
> >
> > Checking the target doesn't work because RootView::OnMousePressed
> > dispatches the
> > event with the current target (=CredentialsItemView). The previous one that
> > didn't handle the click is forgotten.
> >
> > https://codereview.chromium.org/2684343006/
> >
> 
> -- 
> 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.

The icon is a sibling of the checkbox
(https://cs.chromium.org/chromium/src/chrome/browser/ui/views/autofill/card_un...)

[vasilii, 2017-02-15 16:28:34]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-15 16:29:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[vasilii, 2017-02-15 16:29:21]

https://codereview.chromium.org/2684343006/diff/1/chrome/browser/ui/views/pas...
File chrome/browser/ui/views/passwords/credentials_item_view.cc (right):

https://codereview.chromium.org/2684343006/diff/1/chrome/browser/ui/views/pas...
chrome/browser/ui/views/passwords/credentials_item_view.cc:66: };
On 2017/02/10 15:43:38, Evan Stade wrote:
> disallow copy and assign

Done.

https://codereview.chromium.org/2684343006/diff/1/chrome/browser/ui/views/pas...
chrome/browser/ui/views/passwords/credentials_item_view.cc:77: :
LabelButton(button_listener, base::string16()),
On 2017/02/10 15:43:38, Evan Stade wrote:
> why is this even a label button? It seems like you're overriding basically all
> the behaviors specific to label buttons. Should it inherit from CustomButton?

Done.

[vasilii, 2017-02-15 16:34:04]

vasilii@chromium.org changed reviewers:
+ msw@chromium.org

[vasilii, 2017-02-15 16:34:04]

Hi Mike,

please review the files added to ui/views/bubble. I think it's the right place.

[commit-bot: I haz the power, 2017-02-15 18:04:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-15 18:04:54]

Dry run: This issue passed the CQ dry run.

[msw, 2017-02-15 21:06:39]

Have we considered just using standard views tooltips?

https://codereview.chromium.org/2684343006/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc (right):

https://codereview.chromium.org/2684343006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:239:
icon->set_bubble_width(233);
nit: avoid inlining magic numbers, instead use |const int kTooltipWidth = 233;|
directly above this.

https://codereview.chromium.org/2684343006/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/credentials_item_view.cc (right):

https://codereview.chromium.org/2684343006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_item_view.cc:199: if (info_icon_
== event.target())
nit: add a comment, eg. "Do not handle clicks on the tooltip info icon."

https://codereview.chromium.org/2684343006/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/credentials_item_view.h (right):

https://codereview.chromium.org/2684343006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_item_view.h:60: void
NotifyClick(const ui::Event& event) override;
nit: add |// views::CustomButton:| above this

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
File ui/views/bubble/info_bubble.cc (right):

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:7: #include "base/i18n/rtl.h"
nit: remove

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:8: #include "base/macros.h"
nit: remove

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:12: #include "ui/gfx/text_constants.h"
nit: remove?

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:15: #include
"ui/views/controls/combobox/combobox.h"
nit: remove

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:18: #include "ui/views/layout/layout_constants.h"
nit: remove?

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:32: gfx::Insets GetPreferredInsets(const View*
view) {
nit: add a comment or inline in the one caller

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:35: gfx::Point origin =
local_bounds.CenterPoint();
I'm confused by this function; what is it trying to do? Should this be
|gfx::Point origin = local_bounds.origin();|? (that seems to be the intent of
taking the center point and then moving it up and left by half the preferred
width...), and iirc, the local bounds' origin is always (0, 0), so this may be
entirely unnecessary? (or perhaps this is trying to get the origin of the image
within a view that might be sized larger than necessary? comments would help)

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:75: gfx::Insets(kInfoBubbleVerticalMargin,
kInfoBubbleHorizontalMargin,
nit: use two-arg ctor

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:89: // TODO(dbeam): currently we assume that
combobox menus always show downward
nit: update if appropriate (not specific to combobox anymore)

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:139: if (!preferred_width_)
nit: if |preferred_width_ <= 0| (or ==)

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
File ui/views/bubble/info_bubble.h (right):

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.h:11: #include "ui/gfx/geometry/insets.h"
nit: remove

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.h:31: void UpdatePosition();
nit: private if possible (and reorder the definition to match)

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.h:45: void set_align_to_anchor_edge(bool
align_to_anchor_edge) {
nit: remove setter and member if unused

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.h:49: void set_preferred_width(int preferred_width)
{
nit: remove setter and member if unused

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.h:53: void set_show_above_anchor(bool
show_above_anchor) {
nit: remove setter and member if unused

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
File ui/views/bubble/tooltip_icon.cc (right):

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.cc:7: #include "base/macros.h"
nit: remove

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.cc:15: #include "ui/views/painter.h"
nit: remove?

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.cc:34: const char TooltipIcon::kViewClassName[] =
"autofill/TooltipIcon";
nit: update 'autofill'

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.cc:97: new MouseWatcherViewHost(frame,
gfx::Insets()));
nit: base::MakeUnique here and below

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.cc:113: bubble_ = NULL;
nit: nullptr here and elsewhere

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
File ui/views/bubble/tooltip_icon.h (right):

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.h:10: #include "base/compiler_specific.h"
nit: remove

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.h:24: // A tooltip icon that shows a bubble on
hover. Looks like (?).
nit: "Looks like (i)."

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.h:29: static const char kViewClassName[];
nit: inline in TooltipIcon::GetClassName()

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.h:47: // View:
nit: order after ImageView, reorder definition to match

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.h:50: void set_bubble_arrow(BubbleBorder::Arrow
arrow) { bubble_arrow_ = arrow; }
nit: remove setter and member (and bubble_border.h include) if unused

[vasilii, 2017-02-16 14:28:07]

The standard tooltip is used now in the account chooser. There was a request
from a designer to change it
(https://groups.google.com/a/google.com/forum/#!msg/chrome-yolo-core/VIQGEwQD2...).

https://codereview.chromium.org/2684343006/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc (right):

https://codereview.chromium.org/2684343006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/autofill/card_unmask_prompt_views.cc:239:
icon->set_bubble_width(233);
On 2017/02/15 21:06:37, msw wrote:
> nit: avoid inlining magic numbers, instead use |const int kTooltipWidth =
233;|
> directly above this.

Done.

https://codereview.chromium.org/2684343006/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/credentials_item_view.cc (right):

https://codereview.chromium.org/2684343006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_item_view.cc:199: if (info_icon_
== event.target())
On 2017/02/15 21:06:37, msw wrote:
> nit: add a comment, eg. "Do not handle clicks on the tooltip info icon."

Removed because it doesn't work this way.

https://codereview.chromium.org/2684343006/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/passwords/credentials_item_view.h (right):

https://codereview.chromium.org/2684343006/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/passwords/credentials_item_view.h:60: void
NotifyClick(const ui::Event& event) override;
On 2017/02/15 21:06:37, msw wrote:
> nit: add |// views::CustomButton:| above this

It's obsolete code.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
File ui/views/bubble/info_bubble.cc (right):

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:7: #include "base/i18n/rtl.h"
On 2017/02/15 21:06:38, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:8: #include "base/macros.h"
On 2017/02/15 21:06:38, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:12: #include "ui/gfx/text_constants.h"
On 2017/02/15 21:06:38, msw wrote:
> nit: remove?

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:15: #include
"ui/views/controls/combobox/combobox.h"
On 2017/02/15 21:06:38, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:18: #include "ui/views/layout/layout_constants.h"
On 2017/02/15 21:06:38, msw wrote:
> nit: remove?

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:32: gfx::Insets GetPreferredInsets(const View*
view) {
On 2017/02/15 21:06:38, msw wrote:
> nit: add a comment or inline in the one caller

Acknowledged.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:35: gfx::Point origin =
local_bounds.CenterPoint();
On 2017/02/15 21:06:38, msw wrote:
> I'm confused by this function; what is it trying to do? Should this be
> |gfx::Point origin = local_bounds.origin();|? (that seems to be the intent of
> taking the center point and then moving it up and left by half the preferred
> width...), and iirc, the local bounds' origin is always (0, 0), so this may be
> entirely unnecessary? (or perhaps this is trying to get the origin of the
image
> within a view that might be sized larger than necessary? comments would help)

Looks like the function does nothing. The insets are empty. I deleted the
function.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:75: gfx::Insets(kInfoBubbleVerticalMargin,
kInfoBubbleHorizontalMargin,
On 2017/02/15 21:06:38, msw wrote:
> nit: use two-arg ctor

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:89: // TODO(dbeam): currently we assume that
combobox menus always show downward
On 2017/02/15 21:06:38, msw wrote:
> nit: update if appropriate (not specific to combobox anymore)

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.cc:139: if (!preferred_width_)
On 2017/02/15 21:06:38, msw wrote:
> nit: if |preferred_width_ <= 0| (or ==)

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
File ui/views/bubble/info_bubble.h (right):

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.h:11: #include "ui/gfx/geometry/insets.h"
On 2017/02/15 21:06:38, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.h:31: void UpdatePosition();
On 2017/02/15 21:06:38, msw wrote:
> nit: private if possible (and reorder the definition to match)

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.h:45: void set_align_to_anchor_edge(bool
align_to_anchor_edge) {
On 2017/02/15 21:06:38, msw wrote:
> nit: remove setter and member if unused

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.h:49: void set_preferred_width(int preferred_width)
{
On 2017/02/15 21:06:38, msw wrote:
> nit: remove setter and member if unused

I am now using it.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/info_bu...
ui/views/bubble/info_bubble.h:53: void set_show_above_anchor(bool
show_above_anchor) {
On 2017/02/15 21:06:38, msw wrote:
> nit: remove setter and member if unused

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
File ui/views/bubble/tooltip_icon.cc (right):

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.cc:7: #include "base/macros.h"
On 2017/02/15 21:06:39, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.cc:15: #include "ui/views/painter.h"
On 2017/02/15 21:06:39, msw wrote:
> nit: remove?

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.cc:34: const char TooltipIcon::kViewClassName[] =
"autofill/TooltipIcon";
On 2017/02/15 21:06:39, msw wrote:
> nit: update 'autofill'

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.cc:97: new MouseWatcherViewHost(frame,
gfx::Insets()));
On 2017/02/15 21:06:39, msw wrote:
> nit: base::MakeUnique here and below

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.cc:113: bubble_ = NULL;
On 2017/02/15 21:06:39, msw wrote:
> nit: nullptr here and elsewhere

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
File ui/views/bubble/tooltip_icon.h (right):

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.h:10: #include "base/compiler_specific.h"
On 2017/02/15 21:06:39, msw wrote:
> nit: remove

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.h:24: // A tooltip icon that shows a bubble on
hover. Looks like (?).
On 2017/02/15 21:06:39, msw wrote:
> nit: "Looks like (i)."

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.h:29: static const char kViewClassName[];
On 2017/02/15 21:06:39, msw wrote:
> nit: inline in TooltipIcon::GetClassName()

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.h:47: // View:
On 2017/02/15 21:06:39, msw wrote:
> nit: order after ImageView, reorder definition to match

Done.

https://codereview.chromium.org/2684343006/diff/20001/ui/views/bubble/tooltip...
ui/views/bubble/tooltip_icon.h:50: void set_bubble_arrow(BubbleBorder::Arrow
arrow) { bubble_arrow_ = arrow; }
On 2017/02/15 21:06:39, msw wrote:
> nit: remove setter and member (and bubble_border.h include) if unused

Done.

[vasilii, 2017-02-16 14:30:21]

The CQ bit was checked by vasilii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-02-16 14:30:46]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-16 16:24:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-02-16 16:24:09]

Dry run: This issue passed the CQ dry run.

[msw, 2017-02-16 19:04:29]

I would *highly* encourage use of the standard Views tooltip, as one-off
solutions like this are hard to keep in-line with the UI style/behavior of the
year, and require additional maintenance. Please push back to use and refine
standard mechanisms wherever possible. That said, this change lgtm.

[Evan Stade, 2017-02-16 19:16:23]

On 2017/02/16 19:04:29, msw wrote:
> I would *highly* encourage use of the standard Views tooltip, as one-off
> solutions like this are hard to keep in-line with the UI style/behavior of the
> year, and require additional maintenance. Please push back to use and refine
> standard mechanisms wherever possible. That said, this change lgtm.

to chime in here, there's a bit of a difference between this and a normal
tooltip. Perhaps it should not be called tooltip_icon to avoid confusion. This
"tooltip" shows up when you click and its appearance is the primary purpose of
the icon. The icon alone doesn't tell you much of anything; it serves the same
purpose as a "learn more" link. Compare to standard tooltips, which only show up
on long hover and contain auxiliary information such as an a11y description, a
non-elided version of the text, etc.

There are some places in the cros system menu that are also going to want
something similar or identical and explicitly do not want normal tooltips
because the contents must be interactive (e.g. copying text, clicking links,
etc.).

[hwi1, 2017-02-17 04:58:49]

On 2017/02/16 19:04:29, msw wrote:
> I would *highly* encourage use of the standard Views tooltip, as one-off
> solutions like this are hard to keep in-line with the UI style/behavior of the
> year, and require additional maintenance. Please push back to use and refine
> standard mechanisms wherever possible. That said, this change lgtm.

Hi Mike, I agree that we should have a goal to have a single standard tooltip on
Views. 

The current one-off state should be interim until we get there. Out of the two
flavors of tooltip of the existing implementations (#1. standard Views tooltip,
#2. custom tooltip used on CVC dialog), #2 is preferred since the tooltip bubble
shows up faster and supports better visibility and more events (hover, click,
and potentially long press on touch) than #1. 
 
In a long run, the UX team has an intent/goal to propose Harmony Eng to unify #1
and #2 to MD style custom tooltip
(https://material.io/guidelines/components/tooltips.html#tooltips-usage) on
Views. I think it's likely be with Harmony V2. MD settings(webUI) and CrOS
already have the same style implemented and used.

Until the unification happens, the preferred choice on tooltip style is likely
be #2. From my understanding, it's often tricky for a feature team to revamp an
existing pattern within a timeframe the team sets.

I hope this makes sense. If not, please let me know! Thanks!

Best,
Hwi

[hwi1, 2017-02-17 05:09:07]

On 2017/02/16 19:16:23, Evan Stade wrote:
> On 2017/02/16 19:04:29, msw wrote:
> > I would *highly* encourage use of the standard Views tooltip, as one-off
> > solutions like this are hard to keep in-line with the UI style/behavior of
the
> > year, and require additional maintenance. Please push back to use and refine
> > standard mechanisms wherever possible. That said, this change lgtm.
> 
> to chime in here, there's a bit of a difference between this and a normal
> tooltip. Perhaps it should not be called tooltip_icon to avoid confusion. This
> "tooltip" shows up when you click and its appearance is the primary purpose of
> the icon. The icon alone doesn't tell you much of anything; it serves the same
> purpose as a "learn more" link. Compare to standard tooltips, which only show
up
> on long hover and contain auxiliary information such as an a11y description, a
> non-elided version of the text, etc.
> 
> There are some places in the cros system menu that are also going to want
> something similar or identical and explicitly do not want normal tooltips
> because the contents must be interactive (e.g. copying text, clicking links,
> etc.).

re: tooltip_icon - Tooltip should *not* be tied with an icon. Tooltip is the
bubble shape text container only. It can be triggered by hovering or clicking on
an element. Although the element often happens to be an icon, icons can vary and
it doesn't need to be always an icon.

[vasilii, 2017-02-17 09:40:43]

The CQ bit was checked by vasilii@chromium.org

[commit-bot: I haz the power, 2017-02-17 09:41:02]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-17 09:45:09]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1487324443511990,
"parent_rev": "c125f77ab44ad504b5dd8171958aeece8410541e", "commit_rev":
"864f2986aa206bae957944685e1bbb0d7b4fdcc4"}

[commit-bot: I haz the power, 2017-02-17 09:45:50]

Description was changed from

==========
Make the account chooser and CVC dialog use the same icon with toolip for Views.

The CL changes the icon to (i). The account chooser reuses the implementation of
the icon and the tooltip for the CVC dialog.

BUG=690920
==========

to

==========
Make the account chooser and CVC dialog use the same icon with toolip for Views.

The CL changes the icon to (i). The account chooser reuses the implementation of
the icon and the tooltip for the CVC dialog.

BUG=690920

Review-Url: https://codereview.chromium.org/2684343006
Cr-Commit-Position: refs/heads/master@{#451273}
Committed:
https://chromium.googlesource.com/chromium/src/+/864f2986aa206bae957944685e1b...
==========

[commit-bot: I haz the power, 2017-02-17 09:45:51]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/864f2986aa206bae957944685e1b...