Resets hover state when ToolbarActionView is dragged

Resets hover state when ToolbarActionView is dragged

HOVERED state is now reset when drag is started. The fix is similar to
https://codereview.chromium.org/1517003002

BUG=590941
Committed: https://crrev.com/fb50932d80d11040f5637b17173c072a61d15f5a
Cr-Commit-Position: refs/heads/master@{#378810}

[varkha, 2016-03-01 02:52:42]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 02:53:04]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748903003/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748903003/20001

[varkha, 2016-03-01 02:54:54]

varkha@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org, sky@chromium.org

[varkha, 2016-03-01 02:54:55]

rdevlin.cronin@chromium.org: Please review changes in
chrome/browser/ui/views/toolbar/.

sky@chromium.org: Please review a small change in ui/views/widget/widget.h to
enable a test.

Thanks!

[commit-bot: I haz the power, 2016-03-01 03:42:51]

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

[commit-bot: I haz the power, 2016-03-01 03:42:52]

Dry run: This issue passed the CQ dry run.

[sky, 2016-03-01 03:58:48]

https://codereview.chromium.org/1748903003/diff/20001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/1748903003/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.h:600: void set_dragged_view(View* dragged_view) {
dragged_view_ = dragged_view; }
Exposing api like this, especially when it doesn't indicate it is for test only
is easy to misuse. Instead friend the test.

[varkha, 2016-03-01 04:03:34]

https://codereview.chromium.org/1748903003/diff/20001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/1748903003/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.h:600: void set_dragged_view(View* dragged_view) {
dragged_view_ = dragged_view; }
On 2016/03/01 03:58:48, sky wrote:
> Exposing api like this, especially when it doesn't indicate it is for test
only
> is easy to misuse. Instead friend the test.

I've tried that (making it private and making ToolbarActionViewUnitTest a friend
of Widget) before sending it for review and the compiler didn't like that. Is
there an easy reason that you can see why it breaks in this case? Is there some
visibility constraint in component build that gets broken?

[varkha, 2016-03-01 04:58:37]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 04:58:45]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748903003/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748903003/40001

[varkha, 2016-03-01 04:58:53]

https://codereview.chromium.org/1748903003/diff/20001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/1748903003/diff/20001/ui/views/widget/widget....
ui/views/widget/widget.h:600: void set_dragged_view(View* dragged_view) {
dragged_view_ = dragged_view; }
On 2016/03/01 04:03:34, varkha wrote:
> On 2016/03/01 03:58:48, sky wrote:
> > Exposing api like this, especially when it doesn't indicate it is for test
> only
> > is easy to misuse. Instead friend the test.
> 
> I've tried that (making it private and making ToolbarActionViewUnitTest a
friend
> of Widget) before sending it for review and the compiler didn't like that. Is
> there an easy reason that you can see why it breaks in this case? Is there
some
> visibility constraint in component build that gets broken?

Done. Just needed to mention that the friend test class was in global namespace.

[commit-bot: I haz the power, 2016-03-01 05:48:19]

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

[commit-bot: I haz the power, 2016-03-01 05:48:20]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-03-01 15:00:59]

https://codereview.chromium.org/1748903003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right):

https://codereview.chromium.org/1748903003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_action_view.cc:202: void
ToolbarActionView::OnMouseCaptureLost() {
Why is this in ToolbarActionView instead of a button class?  It looks like
CustomButton has an implementation of OnMouseCaptureLost that does this for
everything *except* dragged views (because starting a drag results in capture
lost)... and I thought that was intentional.  Don't we want the view to remain
"pressed" when we are dragging it around?

[varkha, 2016-03-01 16:32:30]

varkha@chromium.org changed reviewers:
+ estade@chromium.org

[varkha, 2016-03-01 16:32:31]

estade@, can you comment here? I think we can simplify it by resetting state
even during a drag.

https://codereview.chromium.org/1748903003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right):

https://codereview.chromium.org/1748903003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_action_view.cc:202: void
ToolbarActionView::OnMouseCaptureLost() {
On 2016/03/01 15:00:59, Devlin wrote:
> Why is this in ToolbarActionView instead of a button class?  It looks like
> CustomButton has an implementation of OnMouseCaptureLost that does this for
> everything *except* dragged views (because starting a drag results in capture
> lost)... and I thought that was intentional.  Don't we want the view to remain
> "pressed" when we are dragging it around?

Yes, I thought so too but wasn't sure if there is more to it. I think with MD
the hovered or pressed state looks bad during a drag, particularly in the app
menu. estade@ pointed this out and we thought it would be OK to remove this
everywhere. I am open to removing InDrag() check from
CustomButton::OnMouseCaptureLost. I think during a drag we give enough feedback
with the drag widget and insertion point to make the pressed / hovered state of
the drag target redundant.

[sky, 2016-03-01 17:24:48]

LGTM

https://codereview.chromium.org/1748903003/diff/40001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/1748903003/diff/40001/ui/views/widget/widget....
ui/views/widget/widget.h:849: void set_dragged_view(View* dragged_view) {
dragged_view_ = dragged_view; }
Now that you friended the class you no longer need the setter. You can set
dragged_view_ directly.

[varkha, 2016-03-01 17:58:46]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 17:59:19]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748903003/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748903003/60001

[varkha, 2016-03-01 18:00:05]

I commented on the bug, seeking some feedback. My preference would be to remove
the visual state (PRESSED | HOVERED) from the source button when drag starts. I
think that visual feedback is redundant.

https://codereview.chromium.org/1748903003/diff/40001/ui/views/widget/widget.h
File ui/views/widget/widget.h (right):

https://codereview.chromium.org/1748903003/diff/40001/ui/views/widget/widget....
ui/views/widget/widget.h:849: void set_dragged_view(View* dragged_view) {
dragged_view_ = dragged_view; }
On 2016/03/01 17:24:48, sky wrote:
> Now that you friended the class you no longer need the setter. You can set
> dragged_view_ directly.

Done.

[commit-bot: I haz the power, 2016-03-01 18:44:02]

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

[commit-bot: I haz the power, 2016-03-01 18:44:03]

Dry run: This issue passed the CQ dry run.

[Evan Stade, 2016-03-01 21:01:17]

https://codereview.chromium.org/1748903003/diff/40001/chrome/browser/ui/views...
File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right):

https://codereview.chromium.org/1748903003/diff/40001/chrome/browser/ui/views...
chrome/browser/ui/views/toolbar/toolbar_action_view.cc:202: void
ToolbarActionView::OnMouseCaptureLost() {
On 2016/03/01 16:32:31, varkha wrote:
> On 2016/03/01 15:00:59, Devlin wrote:
> > Why is this in ToolbarActionView instead of a button class?  It looks like
> > CustomButton has an implementation of OnMouseCaptureLost that does this for
> > everything *except* dragged views (because starting a drag results in
capture
> > lost)... and I thought that was intentional.  Don't we want the view to
remain
> > "pressed" when we are dragging it around?
> 
> Yes, I thought so too but wasn't sure if there is more to it. I think with MD
> the hovered or pressed state looks bad during a drag, particularly in the app
> menu. estade@ pointed this out and we thought it would be OK to remove this
> everywhere. I am open to removing InDrag() check from
> CustomButton::OnMouseCaptureLost. I think during a drag we give enough
feedback
> with the drag widget and insertion point to make the pressed / hovered state
of
> the drag target redundant.
 
The problem is that MD ink drops (which provide hover and pressed effects) look
substantially different from the pre-MD pressed state. So mixing and matching
these (i.e. using ink drops for hover and press, but reverting to pre-md press
effect for drag) looks bad. That's why I suggested just removing it. I think the
change should be MD-only to be conservative.

Does this mean we don't want any effect at all on the dragged button in the
world of MD? That's a question for Sebastien and something to fix down the line
based on his answer. But I'm pretty sure no effect is better than what we have
now.

As far as moving this to CustomButton, that would make sense and it would make
the bookmark bar buttons consistent with the toolbar action buttons... but not
all custom buttons are updated to used ink drops yet so I'd be careful about
determining if some draggable button somewhere were losing this pressed state
despite not having an inkdrop applied to it yet.

[Devlin, 2016-03-01 21:03:38]

On 2016/03/01 21:01:17, Evan Stade wrote:
>
https://codereview.chromium.org/1748903003/diff/40001/chrome/browser/ui/views...
> File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right):
> 
>
https://codereview.chromium.org/1748903003/diff/40001/chrome/browser/ui/views...
> chrome/browser/ui/views/toolbar/toolbar_action_view.cc:202: void
> ToolbarActionView::OnMouseCaptureLost() {
> On 2016/03/01 16:32:31, varkha wrote:
> > On 2016/03/01 15:00:59, Devlin wrote:
> > > Why is this in ToolbarActionView instead of a button class?  It looks like
> > > CustomButton has an implementation of OnMouseCaptureLost that does this
for
> > > everything *except* dragged views (because starting a drag results in
> capture
> > > lost)... and I thought that was intentional.  Don't we want the view to
> remain
> > > "pressed" when we are dragging it around?
> > 
> > Yes, I thought so too but wasn't sure if there is more to it. I think with
MD
> > the hovered or pressed state looks bad during a drag, particularly in the
app
> > menu. estade@ pointed this out and we thought it would be OK to remove this
> > everywhere. I am open to removing InDrag() check from
> > CustomButton::OnMouseCaptureLost. I think during a drag we give enough
> feedback
> > with the drag widget and insertion point to make the pressed / hovered state
> of
> > the drag target redundant.
>  
> The problem is that MD ink drops (which provide hover and pressed effects)
look
> substantially different from the pre-MD pressed state. So mixing and matching
> these (i.e. using ink drops for hover and press, but reverting to pre-md press
> effect for drag) looks bad. That's why I suggested just removing it. I think
the
> change should be MD-only to be conservative.
> 
> Does this mean we don't want any effect at all on the dragged button in the
> world of MD? That's a question for Sebastien and something to fix down the
line
> based on his answer. But I'm pretty sure no effect is better than what we have
> now.
> 
> As far as moving this to CustomButton, that would make sense and it would make
> the bookmark bar buttons consistent with the toolbar action buttons... but not
> all custom buttons are updated to used ink drops yet so I'd be careful about
> determining if some draggable button somewhere were losing this pressed state
> despite not having an inkdrop applied to it yet.

It's easy to check by looking at CustomButton::ink_drop_delegate_, right?

[Evan Stade, 2016-03-01 21:12:14]

On 2016/03/01 21:03:38, Devlin wrote:
> On 2016/03/01 21:01:17, Evan Stade wrote:
> >
>
https://codereview.chromium.org/1748903003/diff/40001/chrome/browser/ui/views...
> > File chrome/browser/ui/views/toolbar/toolbar_action_view.cc (right):
> > 
> >
>
https://codereview.chromium.org/1748903003/diff/40001/chrome/browser/ui/views...
> > chrome/browser/ui/views/toolbar/toolbar_action_view.cc:202: void
> > ToolbarActionView::OnMouseCaptureLost() {
> > On 2016/03/01 16:32:31, varkha wrote:
> > > On 2016/03/01 15:00:59, Devlin wrote:
> > > > Why is this in ToolbarActionView instead of a button class?  It looks
like
> > > > CustomButton has an implementation of OnMouseCaptureLost that does this
> for
> > > > everything *except* dragged views (because starting a drag results in
> > capture
> > > > lost)... and I thought that was intentional.  Don't we want the view to
> > remain
> > > > "pressed" when we are dragging it around?
> > > 
> > > Yes, I thought so too but wasn't sure if there is more to it. I think with
> MD
> > > the hovered or pressed state looks bad during a drag, particularly in the
> app
> > > menu. estade@ pointed this out and we thought it would be OK to remove
this
> > > everywhere. I am open to removing InDrag() check from
> > > CustomButton::OnMouseCaptureLost. I think during a drag we give enough
> > feedback
> > > with the drag widget and insertion point to make the pressed / hovered
state
> > of
> > > the drag target redundant.
> >  
> > The problem is that MD ink drops (which provide hover and pressed effects)
> look
> > substantially different from the pre-MD pressed state. So mixing and
matching
> > these (i.e. using ink drops for hover and press, but reverting to pre-md
press
> > effect for drag) looks bad. That's why I suggested just removing it. I think
> the
> > change should be MD-only to be conservative.
> > 
> > Does this mean we don't want any effect at all on the dragged button in the
> > world of MD? That's a question for Sebastien and something to fix down the
> line
> > based on his answer. But I'm pretty sure no effect is better than what we
have
> > now.
> > 
> > As far as moving this to CustomButton, that would make sense and it would
make
> > the bookmark bar buttons consistent with the toolbar action buttons... but
not
> > all custom buttons are updated to used ink drops yet so I'd be careful about
> > determining if some draggable button somewhere were losing this pressed
state
> > despite not having an inkdrop applied to it yet.
> 
> It's easy to check by looking at CustomButton::ink_drop_delegate_, right?

Yea, that looks like it should do it.

[varkha, 2016-03-01 21:40:25]

We are not really dropping the pressed or hovered state, just resetting to
NORMAL on drag starts. Since the draggable buttons are few I don't think this is
risky. Based on what I saw in the bug discussion I will go ahead with just
patching the CustomButton::OnCaptureLost for both MD and non-MD.

[varkha, 2016-03-01 22:34:39]

Patchset #5 (id:80001) has been deleted

[varkha, 2016-03-01 22:35:25]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 22:36:43]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748903003/100001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748903003/100001

[varkha, 2016-03-01 22:36:55]

sky@, I feel like you should have another look at this. The change is
effectively same, just done in the base CustomButton so that it applies
generally rather than just for extension buttons. The test was also moved.
Thanks!

[varkha, 2016-03-01 22:48:51]

Patchset #5 (id:100001) has been deleted

[varkha, 2016-03-01 22:49:07]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-01 22:49:54]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748903003/120001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748903003/120001

[commit-bot: I haz the power, 2016-03-01 23:45:20]

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

[commit-bot: I haz the power, 2016-03-01 23:45:21]

Dry run: This issue passed the CQ dry run.

[Devlin, 2016-03-02 03:08:42]

Looks like you need fewer reviewers now (i.e., just sky@). :)  That said, it
looks like estade@ recommended a conservative MD-only change, and this will
affect both versions.  I don't have a problem either way, as long as UI/UX folks
are happy.

[sky, 2016-03-02 03:48:28]

I tend to think removing the IsDrag conditional is right. That said, I'm not
sure why it was there in the first place. Did you investigate when it was added?

[varkha, 2016-03-02 16:00:36]

varkha@chromium.org changed reviewers:
+ pkasting@chromium.org

[varkha, 2016-03-02 16:00:36]

On 2016/03/02 03:48:28, sky wrote:
> I tend to think removing the IsDrag conditional is right.

I asked that on the bug and below is a summary of quotes:
[pkasting] "Your first three options (confusingly, numbered 1, 2, and 2') don't
make sense, but both 3 and 4 seem about equally OK."
Option 1 was to reset the button state but only for MD. Option 3 was to do it
for all draggable buttons.
[estade] "I think the change should be MD-only to be conservative."
[sgabriel] "3 seems fine to me." [all draggable buttons.]
[devlin] "I don't have a problem either way, as long as UI/UX folks are happy."

So I got a "don't make sense" on MD-only option and "MD-only" as a suggestion
while a UI designer was OK with reset everywhere. I'm a bit confused, but I have
posted a patch with MD-only for now and copied pkasting@ to disambiguate here. I
am all for caution when changing behavior and I don't mind this particular
change to be associated with MD whenever that comes to each platform.


> That said, I'm not
> sure why it was there in the first place. Did you investigate when it was
added?

Looking at
https://codereview.chromium.org/6685069/diff/162/views/controls/button/custom...
the InDrag() check was there when capture loss was triggering OnMouseReleased
and I think we wanted to prevent a click from being fired upon capture loss.
With a separate OnMouseCaptureLost() handler I think the only thing we can worry
about here is the visual feedback on a source during drag.

[varkha, 2016-03-02 16:00:48]

The CQ bit was checked by varkha@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-03-02 16:00:58]

Dry run: CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1748903003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1748903003/140001

[commit-bot: I haz the power, 2016-03-02 16:46:11]

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

[commit-bot: I haz the power, 2016-03-02 16:46:13]

Dry run: This issue passed the CQ dry run.

[sky, 2016-03-02 18:52:20]

Ok, LGTM

[varkha, 2016-03-02 20:04:38]

The CQ bit was checked by varkha@chromium.org

[commit-bot: I haz the power, 2016-03-02 20:04:57]

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

[commit-bot: I haz the power, 2016-03-02 20:10:24]

Committed patchset #6 (id:140001)

[commit-bot: I haz the power, 2016-03-02 20:12:01]

Description was changed from

==========
Resets hover state when ToolbarActionView is dragged

HOVERED state is now reset when drag is started. The fix is similar to
https://codereview.chromium.org/1517003002

BUG=590941
==========

to

==========
Resets hover state when ToolbarActionView is dragged

HOVERED state is now reset when drag is started. The fix is similar to
https://codereview.chromium.org/1517003002

BUG=590941

Committed: https://crrev.com/fb50932d80d11040f5637b17173c072a61d15f5a
Cr-Commit-Position: refs/heads/master@{#378810}
==========

[commit-bot: I haz the power, 2016-03-02 20:12:02]

Patchset 6 (id:??) landed as
https://crrev.com/fb50932d80d11040f5637b17173c072a61d15f5a
Cr-Commit-Position: refs/heads/master@{#378810}