Status Panel Touch Feedback

Status Panel Touch Feedback

Add touch feedback to HoverHighlightView and TrayPopupItemContainer. These are responsible for rendering the backgrounds of items that appear in the expanded SystemTray. This covers both the default and the detailed views of system items. This also covers the highlight textof the back button to transition from detailed views to defaults views.

TrayPopupItemContainer was moved to its own file to allow for tests to be added in SystemTrayTest

TEST=SystemTrayTest.TrayPopupItemContainerTouchFeedback, SystemTrayTest.TrayPopupItemContainerTouchFeedbackCancellation, TrayDetailsViewTest.HoverHighlightViewTouchFeedback, TrayDetailsViewTest.HoverHighlightViewTouchFeedbackCancellation
BUG=398398
Committed: https://crrev.com/4ccdc6d40988948297afc0439037d93ef70f3773
Cr-Commit-Position: refs/heads/master@{#298748}

[jonross, 2014-09-10 19:36:46]

Patchset #1 (id:1) has been deleted

[jonross, 2014-09-10 19:37:13]

jonross@chromium.org changed reviewers:
+ flackr@chromium.org

[jonross, 2014-09-10 19:37:13]

Hi Rob,

Here's more touch feedback. This covers the items in the system tray popups.

[flackr, 2014-09-10 22:54:32]

https://codereview.chromium.org/556383002/diff/20001/ash/system/tray/hover_hi...
File ash/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/556383002/diff/20001/ash/system/tray/hover_hi...
ash/system/tray/hover_highlight_view.cc:193: } else if (event->type() ==
ui::ET_GESTURE_SCROLL_BEGIN ||
SCROLL_BEGIN shouldn't be necessary, should get a TAP_CANCEL. I think removing
on TAP and TAP_CANCEL is the nice and contained.

https://codereview.chromium.org/556383002/diff/20001/ash/system/tray/tray_pop...
File ash/system/tray/tray_popup_item_container.cc (right):

https://codereview.chromium.org/556383002/diff/20001/ash/system/tray/tray_pop...
ash/system/tray/tray_popup_item_container.cc:81: event->type() ==
ui::ET_GESTURE_END) {
END and SCROLL_BEGIN shouldn't be necessary as far as I can tell.

https://codereview.chromium.org/556383002/diff/20001/ash/system/tray/tray_pop...
File ash/system/tray/tray_popup_item_container.h (right):

https://codereview.chromium.org/556383002/diff/20001/ash/system/tray/tray_pop...
ash/system/tray/tray_popup_item_container.h:14: class TrayPopupItemContainer :
public views::View {
Can you get this and/or the .cc file to show up as a "moved" file by setting a
low similarity? i.e.
git cl upload --similarity=30

[jonross, 2014-09-11 15:53:23]

https://codereview.chromium.org/556383002/diff/20001/ash/system/tray/hover_hi...
File ash/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/556383002/diff/20001/ash/system/tray/hover_hi...
ash/system/tray/hover_highlight_view.cc:193: } else if (event->type() ==
ui::ET_GESTURE_SCROLL_BEGIN ||
On 2014/09/10 22:54:31, flackr wrote:
> SCROLL_BEGIN shouldn't be necessary, should get a TAP_CANCEL. I think removing
> on TAP and TAP_CANCEL is the nice and contained.

Done.

https://codereview.chromium.org/556383002/diff/20001/ash/system/tray/tray_pop...
File ash/system/tray/tray_popup_item_container.cc (right):

https://codereview.chromium.org/556383002/diff/20001/ash/system/tray/tray_pop...
ash/system/tray/tray_popup_item_container.cc:81: event->type() ==
ui::ET_GESTURE_END) {
On 2014/09/10 22:54:31, flackr wrote:
> END and SCROLL_BEGIN shouldn't be necessary as far as I can tell.

Done.

https://codereview.chromium.org/556383002/diff/20001/ash/system/tray/tray_pop...
File ash/system/tray/tray_popup_item_container.h (right):

https://codereview.chromium.org/556383002/diff/20001/ash/system/tray/tray_pop...
ash/system/tray/tray_popup_item_container.h:14: class TrayPopupItemContainer :
public views::View {
On 2014/09/10 22:54:32, flackr wrote:
> Can you get this and/or the .cc file to show up as a "moved" file by setting a
> low similarity? i.e.
> git cl upload --similarity=30

I ran this but it still shows as added

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/system_t...
File ash/system/tray/system_tray_unittest.cc (right):

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/system_t...
ash/system/tray/system_tray_unittest.cc:566: gfx::Point
move_point(view_bounds.x(), view_bounds.CenterPoint().y());
Moving directly out of bounds causes the view to not receive the TAP_CANCEL.
Moving within leads to proper message passing.

On device the moves happen in much smaller intervals leading to this behaviour
normally.

[flackr, 2014-09-11 20:49:38]

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/hover_hi...
File ash/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/hover_hi...
ash/system/tray/hover_highlight_view.cc:42: touch_feedback_enabled_(false) {
Initialize it here with
touch_feedback_enabled_(CommandLine::ForCurrentProcess...

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/hover_hi...
File ash/system/tray/hover_highlight_view.h (right):

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/hover_hi...
ash/system/tray/hover_highlight_view.h:67: // Sets the value for |hover_| and
updates the color of |text_label_|
nit: Don't describe the implementation of the function, just the high level
concept of what it does. I.e. If |hover| sets a highlighted background color on
the text label.

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/system_t...
File ash/system/tray/system_tray_unittest.cc (right):

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/system_t...
ash/system/tray/system_tray_unittest.cc:566: gfx::Point
move_point(view_bounds.x(), view_bounds.CenterPoint().y());
On 2014/09/11 15:53:23, jonross wrote:
> Moving directly out of bounds causes the view to not receive the TAP_CANCEL.
> Moving within leads to proper message passing.
> 
> On device the moves happen in much smaller intervals leading to this behaviour
> normally.

Sounds like a bug. Can you file and reference?

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/tray_pop...
File ash/system/tray/tray_popup_item_container.cc (right):

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/tray_pop...
ash/system/tray/tray_popup_item_container.cc:39: touch_feedback_enabled_ = true;
initializer list.

[jonross, 2014-09-12 18:57:48]

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/hover_hi...
File ash/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/hover_hi...
ash/system/tray/hover_highlight_view.cc:42: touch_feedback_enabled_(false) {
On 2014/09/11 20:49:37, flackr wrote:
> Initialize it here with
> touch_feedback_enabled_(CommandLine::ForCurrentProcess...

Done.

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/hover_hi...
File ash/system/tray/hover_highlight_view.h (right):

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/hover_hi...
ash/system/tray/hover_highlight_view.h:67: // Sets the value for |hover_| and
updates the color of |text_label_|
On 2014/09/11 20:49:37, flackr wrote:
> nit: Don't describe the implementation of the function, just the high level
> concept of what it does. I.e. If |hover| sets a highlighted background color
on
> the text label.
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...

Done.

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/system_t...
File ash/system/tray/system_tray_unittest.cc (right):

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/system_t...
ash/system/tray/system_tray_unittest.cc:566: gfx::Point
move_point(view_bounds.x(), view_bounds.CenterPoint().y());
On 2014/09/11 20:49:37, flackr wrote:
> On 2014/09/11 15:53:23, jonross wrote:
> > Moving directly out of bounds causes the view to not receive the TAP_CANCEL.
> > Moving within leads to proper message passing.
> > 
> > On device the moves happen in much smaller intervals leading to this
behaviour
> > normally.
> 
> Sounds like a bug. Can you file and reference?

crbug.com/413844 filed

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/tray_pop...
File ash/system/tray/tray_popup_item_container.cc (right):

https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/tray_pop...
ash/system/tray/tray_popup_item_container.cc:39: touch_feedback_enabled_ = true;
On 2014/09/11 20:49:37, flackr wrote:
> initializer list.

Done.

[flackr, 2014-09-17 18:33:44]

On 2014/09/12 18:57:48, jonross wrote:
>
https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/hover_hi...
> File ash/system/tray/hover_highlight_view.cc (right):
> 
>
https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/hover_hi...
> ash/system/tray/hover_highlight_view.cc:42: touch_feedback_enabled_(false) {
> On 2014/09/11 20:49:37, flackr wrote:
> > Initialize it here with
> > touch_feedback_enabled_(CommandLine::ForCurrentProcess...
> 
> Done.
> 
>
https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/hover_hi...
> File ash/system/tray/hover_highlight_view.h (right):
> 
>
https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/hover_hi...
> ash/system/tray/hover_highlight_view.h:67: // Sets the value for |hover_| and
> updates the color of |text_label_|
> On 2014/09/11 20:49:37, flackr wrote:
> > nit: Don't describe the implementation of the function, just the high level
> > concept of what it does. I.e. If |hover| sets a highlighted background color
> on
> > the text label.
> > 
> >
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Comm...
> 
> Done.
> 
>
https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/system_t...
> File ash/system/tray/system_tray_unittest.cc (right):
> 
>
https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/system_t...
> ash/system/tray/system_tray_unittest.cc:566: gfx::Point
> move_point(view_bounds.x(), view_bounds.CenterPoint().y());
> On 2014/09/11 20:49:37, flackr wrote:
> > On 2014/09/11 15:53:23, jonross wrote:
> > > Moving directly out of bounds causes the view to not receive the
TAP_CANCEL.
> > > Moving within leads to proper message passing.
> > > 
> > > On device the moves happen in much smaller intervals leading to this
> behaviour
> > > normally.
> > 
> > Sounds like a bug. Can you file and reference?
> 
> crbug.com/413844 filed
> 
>
https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/tray_pop...
> File ash/system/tray/tray_popup_item_container.cc (right):
> 
>
https://codereview.chromium.org/556383002/diff/40001/ash/system/tray/tray_pop...
> ash/system/tray/tray_popup_item_container.cc:39: touch_feedback_enabled_ =
true;
> On 2014/09/11 20:49:37, flackr wrote:
> > initializer list.
> 
> Done.

Can you try the suggestion in
https://codereview.chromium.org/560473003/diff/40001/ash/system/date/date_vie...
to consolidate where we track the flag to one bool in one helper function?
Thanks.

[jonross, 2014-09-30 18:57:36]

Updated to use the centralized switch and utility method in ui/base

[flackr, 2014-10-01 18:29:22]

https://codereview.chromium.org/556383002/diff/80001/ash/system/tray/hover_hi...
File ash/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/556383002/diff/80001/ash/system/tray/hover_hi...
ash/system/tray/hover_highlight_view.cc:10: #include "base/command_line.h"
Remove this include?

https://codereview.chromium.org/556383002/diff/80001/ash/system/tray/tray_pop...
File ash/system/tray/tray_popup_item_container.cc (right):

https://codereview.chromium.org/556383002/diff/80001/ash/system/tray/tray_pop...
ash/system/tray/tray_popup_item_container.cc:41: if (active_ == active)
Since it seems change_background_ is set once, this could probably be simplified
to only SchedulePaint and update active if change_background_. Right now we
schedule noop paints it looks like if !change_background_.

[jonross, 2014-10-01 18:51:16]

https://codereview.chromium.org/556383002/diff/80001/ash/system/tray/hover_hi...
File ash/system/tray/hover_highlight_view.cc (right):

https://codereview.chromium.org/556383002/diff/80001/ash/system/tray/hover_hi...
ash/system/tray/hover_highlight_view.cc:10: #include "base/command_line.h"
On 2014/10/01 18:29:21, flackr wrote:
> Remove this include?

Done.

https://codereview.chromium.org/556383002/diff/80001/ash/system/tray/tray_pop...
File ash/system/tray/tray_popup_item_container.cc (right):

https://codereview.chromium.org/556383002/diff/80001/ash/system/tray/tray_pop...
ash/system/tray/tray_popup_item_container.cc:41: if (active_ == active)
On 2014/10/01 18:29:21, flackr wrote:
> Since it seems change_background_ is set once, this could probably be
simplified
> to only SchedulePaint and update active if change_background_. Right now we
> schedule noop paints it looks like if !change_background_. 

Done.

[flackr, 2014-10-01 20:27:18]

lgtm

https://codereview.chromium.org/556383002/diff/100001/ash/system/tray/tray_po...
File ash/system/tray/tray_popup_item_container.cc (right):

https://codereview.chromium.org/556383002/diff/100001/ash/system/tray/tray_po...
ash/system/tray/tray_popup_item_container.cc:83:
canvas->FillRect(gfx::Rect(size()), (active_ && change_background_) ?
just check active_ here?

[jonross, 2014-10-01 20:50:39]

https://codereview.chromium.org/556383002/diff/100001/ash/system/tray/tray_po...
File ash/system/tray/tray_popup_item_container.cc (right):

https://codereview.chromium.org/556383002/diff/100001/ash/system/tray/tray_po...
ash/system/tray/tray_popup_item_container.cc:83:
canvas->FillRect(gfx::Rect(size()), (active_ && change_background_) ?
On 2014/10/01 20:27:17, flackr wrote:
> just check active_ here?

Done.

[jonross, 2014-10-01 20:51:50]

jonross@chromium.org changed reviewers:
+ skuhne@chromium.org

[jonross, 2014-10-01 20:51:51]

skuhne@chromium.org: Please review changes in ash/system/tray/

Another set of touch feedback implementation. This time affecting the status
panels.

[Mr4D (OOO till 08-26), 2014-10-07 22:51:45]

Only some nits. lgtm.

https://codereview.chromium.org/556383002/diff/120001/ash/system/tray/hover_h...
File ash/system/tray/hover_highlight_view.h (right):

https://codereview.chromium.org/556383002/diff/120001/ash/system/tray/hover_h...
ash/system/tray/hover_highlight_view.h:67: //If |hover| sets a highlighted
background color on the text label.
Maybe something like:

// Sets the highlighted background color on a test label if |hover| is set.

?

https://codereview.chromium.org/556383002/diff/120001/ash/system/tray/tray_de...
File ash/system/tray/tray_details_view_unittest.cc (right):

https://codereview.chromium.org/556383002/diff/120001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view_unittest.cc:201:
detailed_view()->footer()->content());
indent

https://codereview.chromium.org/556383002/diff/120001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view_unittest.cc:202: EXPECT_FALSE(view->hover());
Nit: The first part of these two tests appear to be the same. You might consider
putting that into a test setup function.

[jonross, 2014-10-08 14:17:54]

The CQ bit was checked by jonross@chromium.org

[jonross, 2014-10-08 14:18:00]

https://codereview.chromium.org/556383002/diff/120001/ash/system/tray/hover_h...
File ash/system/tray/hover_highlight_view.h (right):

https://codereview.chromium.org/556383002/diff/120001/ash/system/tray/hover_h...
ash/system/tray/hover_highlight_view.h:67: //If |hover| sets a highlighted
background color on the text label.
On 2014/10/07 22:51:45, Mr4D wrote:
> Maybe something like:
> 
> // Sets the highlighted background color on a test label if |hover| is set.
> 
> ?

Went with: Sets the highlighted color on a text label if |hover| is set.

As background isn't changed.

https://codereview.chromium.org/556383002/diff/120001/ash/system/tray/tray_de...
File ash/system/tray/tray_details_view_unittest.cc (right):

https://codereview.chromium.org/556383002/diff/120001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view_unittest.cc:201:
detailed_view()->footer()->content());
On 2014/10/07 22:51:45, Mr4D wrote:
> indent

Done.

https://codereview.chromium.org/556383002/diff/120001/ash/system/tray/tray_de...
ash/system/tray/tray_details_view_unittest.cc:202: EXPECT_FALSE(view->hover());
On 2014/10/07 22:51:45, Mr4D wrote:
> Nit: The first part of these two tests appear to be the same. You might
consider
> putting that into a test setup function.

Done.

[commit-bot: I haz the power, 2014-10-08 14:18:47]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/556383002/140001

[commit-bot: I haz the power, 2014-10-08 15:29:41]

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

[commit-bot: I haz the power, 2014-10-08 15:29:42]

Try jobs failed on following builders:
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[jonross, 2014-10-08 17:13:42]

The CQ bit was checked by jonross@chromium.org

[commit-bot: I haz the power, 2014-10-08 17:14:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/556383002/160001

[commit-bot: I haz the power, 2014-10-08 18:18:31]

Committed patchset #8 (id:160001) as 7b176b4ac6846030816bba736f232f7e529283d1

[commit-bot: I haz the power, 2014-10-08 18:19:45]

Patchset 8 (id:??) landed as
https://crrev.com/4ccdc6d40988948297afc0439037d93ef70f3773
Cr-Commit-Position: refs/heads/master@{#298748}