New issue
Advanced search Search tips

Issue 758402 link

Starred by 0 users

Issue metadata

Status: Archived
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

Ink drop for app list launcher is incorrect.

Project Member Reported by sammiequon@chromium.org, Aug 23 2017

Issue description

(1) In clamshell mode, open the app launcher.
(2) Switch to tablet mode.

Actual:
Ink drop is on the back button

Expected:
Ink drop is not on the back button
 
shibasheikh@ - What should the expected result be?

1) Hide launcher before switching (and then the ink drop will disappear).
2) Hide ink drop during animating the back button in and then show the ink drop on the app list launcher button after animation is finished.
3) Animate the ink drop with the button (implementation tough I think).

Comment 2 Deleted

Comment 3 Deleted

Oops, added the wrong Sebastien.

@Sebastien, option 2 sounds reasonable to me. Do we do option 3 like animations in the shelf?
Option 3 should be the one we aim for. As we animate the icon, its state should follow.
If it too hard for 61 #2 is valid but not really for long term. Sammie it'd be your call on this.
Project Member

Comment 6 by bugdroid1@chromium.org, Aug 30 2017

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

commit b7d433795baae7855f9f9fc54ee85fde95eae4fe
Author: Sammie Quon <sammiequon@google.com>
Date: Wed Aug 30 00:05:07 2017

shelf: Deactivates app list button ink drop during animations.

This fixes a bug where the ink drop is incorrectly placed, since the ink drop does not move with the app list button. Moving the ink drop with the button is the long term goal, this is a short term fix.

Test: ShelfViewInkDropTest.AppListButtonInkDropDisabledOnAnimations
Bug:  758402 
Change-Id: I962f143d359304d84e53e84218560a1f73b5fc6f
Reviewed-on: https://chromium-review.googlesource.com/639570
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Commit-Position: refs/heads/master@{#498293}
[modify] https://crrev.com/b7d433795baae7855f9f9fc54ee85fde95eae4fe/ash/shelf/app_list_button.cc
[modify] https://crrev.com/b7d433795baae7855f9f9fc54ee85fde95eae4fe/ash/shelf/app_list_button.h
[modify] https://crrev.com/b7d433795baae7855f9f9fc54ee85fde95eae4fe/ash/shelf/shelf_view.cc
[modify] https://crrev.com/b7d433795baae7855f9f9fc54ee85fde95eae4fe/ash/shelf/shelf_view_unittest.cc

Labels: Merge-Request-61
Project Member

Comment 8 by sheriffbot@chromium.org, Aug 30 2017

Labels: -Merge-Request-61 Merge-Review-61 Hotlist-Merge-Review
This bug requires manual review: We are only 5 days from stable.
Please contact the milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), ketakid@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-61 Merge-Approved-61
Approving merge for M61 and M62.
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 6 2017

Labels: -merge-approved-61 merge-merged-3163
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/214a8e6d908511d99e2b0b30351c45cde0e4783d

commit 214a8e6d908511d99e2b0b30351c45cde0e4783d
Author: Sammie Quon <sammiequon@google.com>
Date: Wed Sep 06 15:41:48 2017

[merge to 61] shelf: Deactivates app list button ink drop during animations.

This fixes a bug where the ink drop is incorrectly placed, since the ink drop does not move with the app list button. Moving the ink drop with the button is the long term goal, this is a short term fix.

(cherry picked from commit b7d433795baae7855f9f9fc54ee85fde95eae4fe)

TBR: msw@chromium.org
Test: ShelfViewInkDropTest.AppListButtonInkDropDisabledOnAnimations
Bug:  758402 
Change-Id: I962f143d359304d84e53e84218560a1f73b5fc6f
Reviewed-on: https://chromium-review.googlesource.com/639570
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#498293}
Reviewed-on: https://chromium-review.googlesource.com/652943
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#1115}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/214a8e6d908511d99e2b0b30351c45cde0e4783d/ash/shelf/app_list_button.cc
[modify] https://crrev.com/214a8e6d908511d99e2b0b30351c45cde0e4783d/ash/shelf/app_list_button.h
[modify] https://crrev.com/214a8e6d908511d99e2b0b30351c45cde0e4783d/ash/shelf/shelf_view.cc
[modify] https://crrev.com/214a8e6d908511d99e2b0b30351c45cde0e4783d/ash/shelf/shelf_view_unittest.cc

Project Member

Comment 11 by bugdroid1@chromium.org, Sep 6 2017

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

commit f2acbd32dcfd08952febfe12627614ab668a71da
Author: Sammie Quon <sammiequon@chromium.org>
Date: Wed Sep 06 16:56:19 2017

Revert "[merge to 61] shelf: Deactivates app list button ink drop during animations."

This reverts commit 214a8e6d908511d99e2b0b30351c45cde0e4783d.

Reason for revert: Breaks ash_unittests compile.

Original change's description:
> [merge to 61] shelf: Deactivates app list button ink drop during animations.
> 
> This fixes a bug where the ink drop is incorrectly placed, since the ink drop does not move with the app list button. Moving the ink drop with the button is the long term goal, this is a short term fix.
> 
> (cherry picked from commit b7d433795baae7855f9f9fc54ee85fde95eae4fe)
> 
> TBR: msw@chromium.org
> Test: ShelfViewInkDropTest.AppListButtonInkDropDisabledOnAnimations
> Bug:  758402 
> Change-Id: I962f143d359304d84e53e84218560a1f73b5fc6f
> Reviewed-on: https://chromium-review.googlesource.com/639570
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Reviewed-by: Michael Wasserman <msw@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#498293}
> Reviewed-on: https://chromium-review.googlesource.com/652943
> Reviewed-by: Sammie Quon <sammiequon@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3163@{#1115}
> Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}

TBR=sammiequon@chromium.org

Change-Id: I840eb4d375603f38b29befed9a2830902010dfdd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug:  758402 
Reviewed-on: https://chromium-review.googlesource.com/653417
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#1120}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/f2acbd32dcfd08952febfe12627614ab668a71da/ash/shelf/app_list_button.cc
[modify] https://crrev.com/f2acbd32dcfd08952febfe12627614ab668a71da/ash/shelf/app_list_button.h
[modify] https://crrev.com/f2acbd32dcfd08952febfe12627614ab668a71da/ash/shelf/shelf_view.cc
[modify] https://crrev.com/f2acbd32dcfd08952febfe12627614ab668a71da/ash/shelf/shelf_view_unittest.cc

Project Member

Comment 12 by bugdroid1@chromium.org, Sep 7 2017

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

commit 57dff83695097d4876d7ee0add54b6279ff581f0
Author: Sammie Quon <sammiequon@google.com>
Date: Thu Sep 07 18:21:29 2017

Reland "[merge to 61] shelf: Deactivates app list button ink drop during animations."

This is a reland of 214a8e6d908511d99e2b0b30351c45cde0e4783d
Original change's description:
> [merge to 61] shelf: Deactivates app list button ink drop during animations.
> 
> This fixes a bug where the ink drop is incorrectly placed, since the ink drop does not move with the app list button. Moving the ink drop with the button is the long term goal, this is a short term fix.
> 
> (cherry picked from commit b7d433795baae7855f9f9fc54ee85fde95eae4fe)
> 
> TBR: msw@chromium.org
> Test: ShelfViewInkDropTest.AppListButtonInkDropDisabledOnAnimations
> Bug:  758402 
> Change-Id: I962f143d359304d84e53e84218560a1f73b5fc6f
> Reviewed-on: https://chromium-review.googlesource.com/639570
> Commit-Queue: Sammie Quon <sammiequon@chromium.org>
> Reviewed-by: Michael Wasserman <msw@chromium.org>
> Cr-Original-Commit-Position: refs/heads/master@{#498293}
> Reviewed-on: https://chromium-review.googlesource.com/652943
> Reviewed-by: Sammie Quon <sammiequon@chromium.org>
> Cr-Commit-Position: refs/branch-heads/3163@{#1115}
> Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}

TBR: msw@chromium.org
Bug:  758402 
Change-Id: Ibb66d83f9095298625c2ed5c6aaf3c887505d35d
Reviewed-on: https://chromium-review.googlesource.com/655700
Reviewed-by: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/branch-heads/3163@{#1131}
Cr-Branched-From: ff259bab28b35d242e10186cd63af7ed404fae0d-refs/heads/master@{#488528}
[modify] https://crrev.com/57dff83695097d4876d7ee0add54b6279ff581f0/ash/shelf/app_list_button.cc
[modify] https://crrev.com/57dff83695097d4876d7ee0add54b6279ff581f0/ash/shelf/app_list_button.h
[modify] https://crrev.com/57dff83695097d4876d7ee0add54b6279ff581f0/ash/shelf/shelf_view.cc
[modify] https://crrev.com/57dff83695097d4876d7ee0add54b6279ff581f0/ash/shelf/shelf_view_unittest.cc

Labels: Not-Touch-Friendly-Launcher
Project Member

Comment 14 by bugdroid1@chromium.org, Dec 15 2017

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

commit 9b911f2f81d8bde89b659e8a40d7db9748c9e072
Author: Sammie Quon <sammiequon@google.com>
Date: Fri Dec 15 02:53:15 2017

shelf: Separate back button and app list button.

Seperate the back button and app list button into separate elements.
Draws the background in ShelfView::OnPaint. This fixes issues with ink
drop and accessibility focusing not working correctly. Made changes to
ShelfView focus search to accommodate this.

Add back_button_unittest.
Removed app_list_button/back button ink drop unittest which tested
if different parts of the old app list button's ink drop acted
different if different parts were pressed. Not needed as the two
separate buttons' ink drops should work normally.
Removed app_list_button rtl unittest as the buttons should be flipped
normally by the views rtl handling.

Test: ash_unittests *Shelf* *ApplistButton* *BackButton*
Bug:  791638 ,  758402 
Change-Id: I07bdd0b490ffb2619a13cebacf388e2e08190114
Reviewed-on: https://chromium-review.googlesource.com/761856
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Reviewed-by: Mitsuru Oshima <oshima@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Commit-Queue: Sammie Quon <sammiequon@chromium.org>
Cr-Commit-Position: refs/heads/master@{#524301}
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/BUILD.gn
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/ash_strings.grd
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/metrics/user_metrics_recorder.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/metrics/user_metrics_recorder_unittest.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/public/cpp/shelf_model.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/public/cpp/shelf_model.h
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/public/cpp/shelf_model_unittest.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/public/cpp/shelf_struct_traits.h
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/public/cpp/shelf_types.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/public/cpp/shelf_types.h
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/public/interfaces/shelf.mojom
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/resources/vector_icons/shelf_back.1x.icon
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/resources/vector_icons/shelf_back.icon
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/app_list_button.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/app_list_button.h
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/app_list_button_unittest.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/assistant_overlay.cc
[add] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/back_button.cc
[add] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/back_button.h
[add] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/back_button_unittest.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/shelf.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/shelf_controller.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/shelf_controller_unittest.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/shelf_unittest.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/shelf_view.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/shelf_view.h
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/shelf_view_test_api.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/shelf_view_unittest.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/ash/shelf/shelf_window_watcher_unittest.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/chrome/browser/ui/ash/launcher/chrome_launcher_controller.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/chrome/browser/ui/ash/launcher/chrome_launcher_controller_browsertest.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/chrome/browser/ui/ash/launcher/chrome_launcher_controller_unittest.cc
[modify] https://crrev.com/9b911f2f81d8bde89b659e8a40d7db9748c9e072/chrome/browser/ui/window_sizer/window_sizer_ash_uitest.cc

Status: Fixed (was: Assigned)
Status: Archived (was: Fixed)

Sign in to add a comment