Adding Manu for shelf behavior, Xiaohui for Assistant burst ripple implementation details.

We need to clarify nomenclature here.
Following the spec for which I attached the corresponding image:

- On controls, including left-hand control such as Back and launcher button have a "base fill" also named "highlight" in a bunch of thread that is always visible, not matter where the shelf is displayed in in what mode. This is to indicate better tappable targets. It's the equivalent of what you see today in the status tray when the shelf is transparent, an always visible box

- When tapping one of the icon in the left-hand controls, we show a flood ripple bounded within the 40*40 circular mask/tap target of said icon. The behavior of this flood ripple is always the same for the back button (flooding then fades-out) but is different from the launcher button as followed:

In laptop mode: 

    -  Tapping/clicking the button shows the peeking launcher and stays "active" until the launcher is dismissed.
    -  Long Pressing should show and independent big burst ripple for the assistant motion which should have no impact on the state of activeness of the launcher button.

In tablet mode:

    -  Tapping/clicking launcher button acts like the back button in term of visuals, the flood ripple animation happens but fades-out. It never sticks as "Active" because the home-cher is the default state, in any case (app to launcher, launcher page #2 to page #1, etc...). 
    -  Long pressing should trigger the same motion for the assistant, again completely independent of the launcher button state.

Deleted: left-control-spec.png 212 KB

	left-control-spec.png 212 KB View Download

I think this is supposed to go back to manu, so I'm assigning it to him. 

Xiaohuic, I guess you may need a separate bug for assistant button ripple?

Issue 878182 has been merged into this issue.

Assistant button ripple should already be there.

I believe the latest changes around the launcher and back buttons should cover this. Sebastien if you still see things that are not according to the spec, please file specific bugs against me. Thanks!

Just marking this as un-fixed because we saw this in M-71 ToT.

When an app has been shown, homecher button is still highlighted.

Ok for the record this wasn't on my list until just now because the component was launcher instead of shelf. @Alex I'm not sure what you mean by "when an app has been show"? Could you give me repro steps for when the button has incorrect highlighting? Thanks!

Chatted with Alex offline about this. I wasn't seeing the bug because I needed an extra flag to make homecher work with the emulator (--use-first-display-as-internal) -- should probably be default, but anyway.

Let me summarize what Alex said offline.

I think because.. OnAppListVisibility is not being called :)

Lets look at my cl that updates the shelf bg, maybe we can fix it there too.
https://chromium-review.googlesource.com/c/chromium/src/+/1200408

yep, looks like we are relying on the visibility of the widget changing.
https://cs.chromium.org/chromium/src/ash/shelf/app_list_button.cc?sq=package:chromium&g=0&l=308

we should call OnAppListDismissed() here[1]
[1] https://cs.chromium.org/chromium/src/ash/app_list/app_list_controller_impl.cc?q=app_list_controller_im&sq=package:chromium&g=0&l=544

Basically the shelf thinks "the app list is being shown, let's highlight the launcher button", but since it can be behind a window, the highlight looks buggy.

@Weidong, what do you think is the best path here?

+sgabriel@ for ux input:
I think a simpler solution is to always avoid the highlight for app list button for tablet mode.

So implementation is making the button an observer of tablet mode and avoid showing highlight when tablet mode is on.

manucornet@
Talked with sgabriel@ offline, the solution in #13 also makes sense to him. WDYT?

Sounds good to me!

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

commit cd6238e9ed4b06319964de8675289934afd7cceb
Author: Manu Cornet <manucornet@chromium.org>
Date: Fri Sep 14 18:40:23 2018

CrOS Shelf: No highlight for app list button in tablet mode

Because the "homecher" view is now always enabled in tablet mode, it
doesn't make any sense to show a highlight behind the app list button
(which would always be there). Never showing the highlight in tablet
mode is the recommended solution from UX (see linked bug).

Bug:  878957 
Change-Id: I0c5de178b6c9a3ad24d3390b645f5f91d034ed0e
Reviewed-on: https://chromium-review.googlesource.com/1226491
Commit-Queue: Manu Cornet <manucornet@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#591411}
[modify] https://crrev.com/cd6238e9ed4b06319964de8675289934afd7cceb/ash/shelf/app_list_button.cc
[modify] https://crrev.com/cd6238e9ed4b06319964de8675289934afd7cceb/ash/shelf/shelf_view.cc
[modify] https://crrev.com/cd6238e9ed4b06319964de8675289934afd7cceb/ash/shelf/shelf_view.h

This bug requires manual review: M70 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

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

commit 7c5002ef12ccae3d3b142b1f7d4f97a35efa6c80
Author: Manu Cornet <manucornet@chromium.org>
Date: Fri Sep 21 22:03:31 2018

CrOS Shelf: No highlight for app list button in tablet mode

Because the "homecher" view is now always enabled in tablet mode, it
doesn't make any sense to show a highlight behind the app list button
(which would always be there). Never showing the highlight in tablet
mode is the recommended solution from UX (see linked bug).

Bug:  878957 
Change-Id: I0c5de178b6c9a3ad24d3390b645f5f91d034ed0e
Reviewed-on: https://chromium-review.googlesource.com/1226491
Commit-Queue: Manu Cornet <manucornet@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#591411}(cherry picked from commit cd6238e9ed4b06319964de8675289934afd7cceb)
Reviewed-on: https://chromium-review.googlesource.com/1239207
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#568}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/7c5002ef12ccae3d3b142b1f7d4f97a35efa6c80/ash/shelf/app_list_button.cc
[modify] https://crrev.com/7c5002ef12ccae3d3b142b1f7d4f97a35efa6c80/ash/shelf/shelf_view.cc
[modify] https://crrev.com/7c5002ef12ccae3d3b142b1f7d4f97a35efa6c80/ash/shelf/shelf_view.h

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

Commit: 7c5002ef12ccae3d3b142b1f7d4f97a35efa6c80
Author: manucornet@chromium.org
Commiter: newcomer@chromium.org
Date: 2018-09-21 22:03:31 +0000 UTC

CrOS Shelf: No highlight for app list button in tablet mode

Because the "homecher" view is now always enabled in tablet mode, it
doesn't make any sense to show a highlight behind the app list button
(which would always be there). Never showing the highlight in tablet
mode is the recommended solution from UX (see linked bug).

Bug:  878957 
Change-Id: I0c5de178b6c9a3ad24d3390b645f5f91d034ed0e
Reviewed-on: https://chromium-review.googlesource.com/1226491
Commit-Queue: Manu Cornet <manucornet@chromium.org>
Reviewed-by: Xiyuan Xia <xiyuan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#591411}(cherry picked from commit cd6238e9ed4b06319964de8675289934afd7cceb)
Reviewed-on: https://chromium-review.googlesource.com/1239207
Reviewed-by: Alex Newcomer <newcomer@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#568}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}

Here's a summary of the rules that were executed: 
 - OnlyMergeApprovedChange: Rule Failed -- Revision 7c5002ef12ccae3d3b142b1f7d4f97a35efa6c80 was merged to refs/branch-heads/3538 branch with no merge approval from a TPM! 
Please explain why this change was merged to the branch!
 - AcknowledgeMerge: Notification Required -- 

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot