bettes@: The dialog isn't the right width, but otherwise how does this look? The mock has a close-X but since it's modal my understanding is that it shouldn't.

Deleted: add-to-desktop-harmony-4.PNG 12.3 KB

	add-to-desktop-harmony-4.PNG 12.3 KB View Download

Taking this back because I need to do some different things here based on the new spec.

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

commit bebfdd95c7fdc587686da076262cb9292696e909
Author: bsep <bsep@chromium.org>
Date: Fri Apr 14 20:17:16 2017

Harmony - Dialog-specific conversion for Add to Desktop.

Add to Desktop requires a layout rewrite for Harmony. This patch takes
care of that part of the conversion, though the dialog won't be ready
for review yet due to needing some general changes.

BUG= 652510 

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

[modify] https://crrev.com/bebfdd95c7fdc587686da076262cb9292696e909/chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.cc
[modify] https://crrev.com/bebfdd95c7fdc587686da076262cb9292696e909/chrome/browser/ui/views/extensions/bookmark_app_confirmation_view.h
[modify] https://crrev.com/bebfdd95c7fdc587686da076262cb9292696e909/chrome/browser/ui/views/harmony/chrome_layout_provider.cc
[modify] https://crrev.com/bebfdd95c7fdc587686da076262cb9292696e909/chrome/browser/ui/views/harmony/chrome_layout_provider.h
[modify] https://crrev.com/bebfdd95c7fdc587686da076262cb9292696e909/chrome/browser/ui/views/harmony/harmony_layout_provider.cc

Attaching screenshots for review. All are from Windows.

1. English 1x, with the checkbox control highlighted.
2. 1.5x HighDPI
3. German
4. Hebrew (RTL)
5. A website with a real favicon.

Deleted: a2d-control-highlighted.PNG 39.9 KB

	a2d-control-highlighted.PNG 39.9 KB View Download

Deleted: a2d-1.5x.PNG 59.0 KB

	a2d-1.5x.PNG 59.0 KB View Download

Deleted: a2d-german.PNG 40.4 KB

	a2d-german.PNG 40.4 KB View Download

Deleted: a2d-hebrew.PNG 39.3 KB

	a2d-hebrew.PNG 39.3 KB View Download

Deleted: a2d-full-avatar.PNG 1.0 MB

	a2d-full-avatar.PNG 1.0 MB View Download

Assigning to bettes@ for final review. If you'd like to play with the dialog it's available on any website: Hamburger>More tools>Add to Desktop...

Looks good - what does "control highlight" mean? Is that a Windows thing? I assume it's different than focusing a control (for accessibility)?

That's just what the control does when you hover it. It's easier to see its bounding box.

I would like Alan to look at this, so we can refine the review process.

When I said it looks good I was not reviewing the dialog. I wanted to say that overall what you've done looks good before I launched into asking about something that looks off.

I didn't understand your response about the gray highlight being something that the control does when you mouse over it. Are you saying this is a diagnostic mode you're using to show the bounding box? I'm making sure the control doesn't actually do this when it's being used.

Sorry, I was just confused as to why you assigned the bug back to me.

This is what the control actually does when it's being used... it's been like that for so long I assumed it was intentional, though now that you bring it up I don't see it mentioned in the spec.

It does seem kinda screwy.  Is that specific to this dialog?  If so you could probably blame-search to find out why it was added.  I might just remove it either way.

No, it's a normal checkbox. For example, the translate dialog does it too. Are there just few enough checkboxes floating around that it never got looked at by anyone else? At any rate, I can change it.

> Sorry, I was just confused as to why you assigned the bug back to me.

I didn't intentionally. I never touched the owner field. I think, maybe, possibly, my comment and yours just before (in which you changed the owner to bettes@) landed at just about the same time. I guess that would mean me clicking the Save changes button a split-second before your comment updated the bug. So to my frontend the bug hadn't changed but on the backend it had. Then I assume Save changes ships over field values from the frontend, and the backend diffs the changes with what's in the database. My frontend had you as owner at the time, but then the backend had bettes@, and so it thought I was changing it back to you.

Regardless, sorry about the confusion!

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

commit b6883a0122ecf806bc8ced8016bab4beca0c2d74
Author: bsep <bsep@chromium.org>
Date: Sat May 27 01:27:30 2017

Remove hover highlight on Harmony checkboxes.

As per the Harmony spec, checkboxes now do nothing on hover but still have
the activation ripple on click. Pre-Harmony checkboxes have disabled ink
drops, so this won't affect them.

BUG= 652510 

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

[modify] https://crrev.com/b6883a0122ecf806bc8ced8016bab4beca0c2d74/ui/views/controls/button/checkbox.cc
[modify] https://crrev.com/b6883a0122ecf806bc8ced8016bab4beca0c2d74/ui/views/controls/button/checkbox.h

The checkbox doesn't highlight any more, so you can disregard the first screenshot. It just does nothing on hover now.

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

commit 5651a37f2caccaaaae395438195c3eced402e3a0
Author: bsep <bsep@chromium.org>
Date: Wed Jun 21 01:44:37 2017

Revert of Fix Harmony checkbox having an ink drop highlight on click. (patchset #1 id:1 of https://codereview.chromium.org/2939043005/ )

Reason for revert:
Breaks Chrome OS x86.

Original issue's description:
> Fix Harmony checkbox having an ink drop highlight on click.
>
> Follow-up to crrev.com/2911573002. There was a regression with a later
> refactor of the ink drop code, and this fixes that.
>
> BUG= 652510 
>
> Review-Url: https://codereview.chromium.org/2939043005
> Cr-Commit-Position: refs/heads/master@{#480958}
> Committed: https://chromium.googlesource.com/chromium/src/+/6be568e4976c957d72d685a18b395bb60842273f

TBR=sky@chromium.org,bruthig@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 652510 

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

[modify] https://crrev.com/5651a37f2caccaaaae395438195c3eced402e3a0/ui/views/controls/button/checkbox.cc

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

commit 037435e4c3f4ed899e609a948feeb3aa6957ffd1
Author: bsep <bsep@chromium.org>
Date: Thu Jun 22 20:02:15 2017

Fix Harmony checkbox having an ink drop highlight on click.

Follow-up to crrev.com/2911573002. There was a regression with a later
refactor of the ink drop code, and this fixes that.

BUG= 652510 

Review-Url: https://codereview.chromium.org/2939043005
Cr-Original-Commit-Position: refs/heads/master@{#480958}
Committed: https://chromium.googlesource.com/chromium/src/+/6be568e4976c957d72d685a18b395bb60842273f
Review-Url: https://codereview.chromium.org/2939043005
Cr-Commit-Position: refs/heads/master@{#481639}

[modify] https://crrev.com/037435e4c3f4ed899e609a948feeb3aa6957ffd1/ui/views/controls/button/checkbox.cc

Review for Add to Desktop:

_All

- Incorrect blue color on checkbox
- Add back 1px hairline for focused input fields
- Focused states for checkbox control should only be around the checkbox, not the entire row
- Focused checkbox control is incorrect
- Size of checkbox is surprising. Double check
- Favicons and monograms are different sizes?
- Spacing b/t label and control should be 12px, not 16px

Nothing Mac or Windows specific


------------------------

All notes can be tracked in the implementation review deck: 
https://docs.google.com/presentation/d/1efIBdWozcOXv1hEaIxdcdf4A1-8P8HQu7bRpEokLisI/edit?userstoinvite=crivero@google.com&ts=5991f0cf#slide=id.g24e39cc577_0_163

Spoke with Bret this morning, notes below: 

Most of the feedback in #23 is general feedback for all of Harmony and not specific to 'Add to Desktop' specific. I've added each as 'Blocked on'

'Favicons and monograms are different sizes' is the only one pertaining to 'Add to Desktop' specifically. The cause of this issue is out-of-scope for Harmony v1 because it has to deal with the entire Chrome Extensions ecosystem and how we display self-generated assets like the monogram tile. 

Assigning back to bsep@ for now. 


Mocks:
https://folio.googleplex.com/chrome-ux-specs-and-sources/Chrome%20browser%20(MD)/Secondary%20UI%20Previews%20and%20specs%20(exports)/Preview/15-Add%20to%20desktop#%2F01-Add-to-desktop.png%3Fz=width

The NextAction date has arrived: 2017-11-10

This is as much work as we're going to do for Harmony.