Sebastien, can you comment on what this focus style should look like in top chrome MD?

Let's use the hover states for the focus states.

For the omnibox, let's set the focus state to a 2pt outer stroke rather than the inner dotted text-filed bound thing we currently have. Spec attached.

Deleted: P - Chrome Cros - 200 - normal + incognito - Core ui focus .png 98.8 KB

	P - Chrome Cros - 200 - normal + incognito - Core ui focus .png 98.8 KB View Download

 Issue 596756  has been merged into this issue.

 Issue 599642  has been merged into this issue.

Sebastien: Peter wants me to make sure you want the focus border to be thicker than the unfocused border (at 2x). Is this intentional?

Yes it's intended to pop-out. The regular border being a constant 1px, a bit too thin for a focus state. 

Cool, thanks.

> Let's use the hover states for the focus states.

Currently the find bar buttons have focus states, but they're blue round rect outlines. Should those all change to the hover state?

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/333ac64c2e01fce66aa363da999dbcffa9724343

commit 333ac64c2e01fce66aa363da999dbcffa9724343
Author: estade <estade@chromium.org>
Date: Sat May 21 00:13:24 2016

Update appearance of omnibox focus for MD.

Also remove some dead focus painter code in TextField.
Also remove OmniboxEditController::OnSetFocus(), which was totally dead on Views and not necessary on Cocoa.

BUG= 591140 

Review-Url: https://codereview.chromium.org/2001433002
Cr-Commit-Position: refs/heads/master@{#395213}

[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.h
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/chrome/browser/ui/cocoa/location_bar/location_bar_view_mac.mm
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac.mm
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/chrome/browser/ui/cocoa/omnibox/omnibox_view_mac_unittest.mm
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/chrome/browser/ui/views/location_bar/background_with_1_px_border.cc
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/chrome/browser/ui/views/location_bar/background_with_1_px_border.h
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/chrome/browser/ui/views/location_bar/location_bar_view.h
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/chrome/browser/ui/views/omnibox/omnibox_view_views.cc
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/chrome/browser/ui/views/omnibox/omnibox_view_views_unittest.cc
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/components/omnibox/browser/omnibox_edit_controller.h
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/components/omnibox/browser/omnibox_edit_unittest.cc
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/ios/chrome/browser/ui/omnibox/web_omnibox_edit_controller.h
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/ui/app_list/views/folder_header_view.cc
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/ui/native_theme/common_theme.cc
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/ui/native_theme/native_theme_dark_aura.cc
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/ui/views/controls/textfield/textfield.cc
[modify] https://crrev.com/333ac64c2e01fce66aa363da999dbcffa9724343/ui/views/controls/textfield/textfield.h

sgabriel, dmazzoni: What do you think should happen and how much do you care what happens in the following scenario:

a) user tabs to some button
b) user mouses over same button, then mouses out of the button

In this case nothing should happen, tabbing takes priority over hover. imo

Is this related to https://bugs.chromium.org/p/chromium/issues/detail?id=614994 ?

uhh not really. Will add comment on other bug.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/43b34ce93a59b52dfb0f5c125a12d66b299c36d3

commit 43b34ce93a59b52dfb0f5c125a12d66b299c36d3
Author: estade <estade@chromium.org>
Date: Fri May 27 00:30:39 2016

Use ink drop hover for focus states on buttons. Remove the dashed border.

Only button-style buttons (i.e. MdTextButton) retain the blue ring for focus, as per sgabriel's orders.

BUG= 591140 

Review-Url: https://codereview.chromium.org/2001843002
Cr-Commit-Position: refs/heads/master@{#396328}

[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ash/system/toast/toast_overlay.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/chrome/browser/ui/views/bar_control_button.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/chrome/browser/ui/views/bar_control_button.h
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/chrome/browser/ui/views/bookmarks/bookmark_bar_view.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/chrome/browser/ui/views/download/download_item_view_md.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/chrome/browser/ui/views/location_bar/bubble_icon_view.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/chrome/browser/ui/views/location_bar/bubble_icon_view.h
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/chrome/browser/ui/views/toolbar/app_menu_button.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/chrome/browser/ui/views/toolbar/app_menu_button.h
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/chrome/browser/ui/views/toolbar/toolbar_action_view.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/chrome/browser/ui/views/toolbar/toolbar_action_view.h
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/chrome/browser/ui/views/toolbar/toolbar_action_view_unittest.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/chrome/browser/ui/views/toolbar/toolbar_button.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/chrome/browser/ui/views/toolbar/toolbar_button.h
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/animation/button_ink_drop_delegate.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/animation/button_ink_drop_delegate.h
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/animation/ink_drop.h
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/animation/ink_drop_delegate.h
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/animation/ink_drop_factory.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/animation/ink_drop_host_view.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/animation/ink_drop_host_view.h
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/animation/ink_drop_impl.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/animation/ink_drop_impl.h
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/animation/ink_drop_impl_unittest.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/animation/test/test_ink_drop_delegate.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/animation/test/test_ink_drop_delegate.h
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/controls/button/blue_button_unittest.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/controls/button/custom_button.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/controls/button/custom_button.h
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/controls/button/custom_button_unittest.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/controls/button/label_button.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/controls/button/label_button_unittest.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/controls/button/md_text_button.cc
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/controls/button/md_text_button.h
[modify] https://crrev.com/43b34ce93a59b52dfb0f5c125a12d66b299c36d3/ui/views/controls/button/menu_button_unittest.cc

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/de354e47c0ee02b889343db624b1996df7322c2f

commit de354e47c0ee02b889343db624b1996df7322c2f
Author: estade <estade@chromium.org>
Date: Fri May 27 21:52:59 2016

MD links - underline to indicate focus.

As discussed with sgabriel@; this is somewhat exploratory. We can't
reuse the bold treatment that polymer uses as many fonts change width
when emboldened.

BUG= 571500 , 591140 

Review-Url: https://codereview.chromium.org/2023453002
Cr-Commit-Position: refs/heads/master@{#396574}

[modify] https://crrev.com/de354e47c0ee02b889343db624b1996df7322c2f/chrome/browser/ui/views/website_settings/website_settings_popup_view.cc
[modify] https://crrev.com/de354e47c0ee02b889343db624b1996df7322c2f/ui/views/controls/label.cc
[modify] https://crrev.com/de354e47c0ee02b889343db624b1996df7322c2f/ui/views/controls/link.cc

 Issue 622038  has been merged into this issue.

Sebastien, the content settings icons and bookmark star don't have a hover effect and they still use dashed rect for focus. Should they get a hover state? Should focus be updated?

Also  bug 608976  is closely related (it covers hover, press, focus, etc. for location icon)

Bookmark star uses the "ink hover" state for focus and press, it just doesn't use it for hover.  Seems inconsistent.

It looks nice in the focused/pressed state, so I can't think why we'd want to stick with the current no hover/dashed focus rect state for any of the location bar views...

From today's meeting, the current direction is to use the same sort of hover/focus/pressed style on items within the location bar as on items in the rest of the toolbar (the grey roundrect).

This bug is nominally about focus right now, should this be morphed to "update hover/focus/pressed state for location bar child views"?  Or should we file a new bug for that?

Leaving  bug 608976  about the location icon since that's a trickier case.

#22. I think this is still in line with the designer's comment #2 so reusing this bug should be fine (what remains of it).

Morphing then.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fc47fab1198649e7e3c7e447db6453ae7f2ba612

commit fc47fab1198649e7e3c7e447db6453ae7f2ba612
Author: estade <estade@chromium.org>
Date: Thu Jun 30 14:23:07 2016

Add ink drop highlight for hover and focus on location icons (e.g. star,
zoom, translate, passwords) and content settings icon.

This doesn't do anything for other icon label bubble views (like the
location icon). Some of the code in ContentSettingImageView should
probably be moved into IconLabelBubbleView for that. Also, there's no
hover or focus effect when the label is showing (e.g. when Chrome first
blocks a popup and there's text next to the icon). Since it remains
unclear to me how the hover and press effects should look in this
scenario, limit the ink drop to when only the icon is visible.

BUG= 591140 

Review-Url: https://codereview.chromium.org/2098643002
Cr-Commit-Position: refs/heads/master@{#403170}

[modify] https://crrev.com/fc47fab1198649e7e3c7e447db6453ae7f2ba612/chrome/browser/ui/views/location_bar/bubble_icon_view.cc
[modify] https://crrev.com/fc47fab1198649e7e3c7e447db6453ae7f2ba612/chrome/browser/ui/views/location_bar/bubble_icon_view.h
[modify] https://crrev.com/fc47fab1198649e7e3c7e447db6453ae7f2ba612/chrome/browser/ui/views/location_bar/content_setting_image_view.cc
[modify] https://crrev.com/fc47fab1198649e7e3c7e447db6453ae7f2ba612/chrome/browser/ui/views/location_bar/content_setting_image_view.h
[modify] https://crrev.com/fc47fab1198649e7e3c7e447db6453ae7f2ba612/chrome/browser/ui/views/location_bar/icon_label_bubble_view.cc

remaining work retargeted to m54

Is bug 590285 comment 27 problem 2 covered by this bug or another bug that you own?  I'm not sure if we have a catchall bug for moving away from the dashed rect focus state in all places it's currently used.

That dashed rect is in secondary UI which has not yet been MD-ified, so it is still correct. Getting rid of all dashed rects is covered by Harmony, but in this case we probably just change that x view to a BarControlButton (yes, bad name) and get the improved focus state as a corollary.

Does this bug also cover the lock icon? It currently has no visible focus state.

lock icon is more complicated because it has the EV/verbose state. That's tracked by  crbug.com/608976 

After discussing options via mail, please find the final spec in:
Chrome UX specs and sources > Chrome Omnibox & Security > Non-Touch (https://drive.google.com/file/d/0B6Wxmj9LZL6XOEhOMjlybGd6QkE/view).

Summary: 
- Button state: covers icon/chip including text label and divider to avoid mismatch between click/hover- and highlighted area
- Ripple: all icons/chips inside omnibox have bounded ripples, toolbar buttons outside omnibox use unbounded
- Mac specifics: no ripple, blue focus ring, darker active state (for differentiation to hover state), add hover/active/focus states to all omnibox icons

over to shrike for triage