Right clicking the back and forward buttons is buggy. A single right click op...

Right clicking the back and forward buttons is buggy. A single right click opens the drop down menu, however the button does not enter the PUSHED state. If you double right click then click on the tab content area, the button stays in a PUSHED state until you mouseover it.

The fix should replicate the intuitive behavior. On a single right click, the button enters the PUSHED state and the drop down appears immediately. As soon as the user clicks on another View, the drop down closes and the button returns to the NORMAL state. Double right click behaves similarly. 

BUG=9717
TEST=none

[Peter Kasting, 2009-08-04 16:43:40]

Setting reviewers more correctly

[sky, 2009-08-04 22:49:35]

http://codereview.chromium.org/159838/diff/1002/1003
File views/controls/button/button_dropdown.cc (right):

http://codereview.chromium.org/159838/diff/1002/1003#newcode58
Line 58: // intuitive behavior that we see with a left click.
Your comment isn't quite right here. The reason we don't enter the PUSHED state
is that normally the right mouse button isn't a triggerable event. You want to
say something like:

// A right mouse click shows the menu. Change the state to pressed
// just as left mouse button does. We have to explicitly do this as 
// ImageButton doesn't normally change the state to pushed for a
// right mouse button.

http://codereview.chromium.org/159838/diff/1002/1003#newcode78
Line 78: // Set the state back to normal when the drop down menu is closed.
This comment should be after ShowDropDownMenu.

http://codereview.chromium.org/159838/diff/1002/1003#newcode85
Line 85: // N.B. When we are in the BS_PUSHED state, the state should not change
until
N.B.? You should move this comment into OnMouseDragged.

http://codereview.chromium.org/159838/diff/1002/1003#newcode107
Line 107: if (state_ != BS_DISABLED && state_ != BS_PUSHED)
Do you really need this?

http://codereview.chromium.org/159838/diff/1002/1003#newcode112
Line 112: if (state_ != BS_DISABLED && state_ != BS_PUSHED) {
Do you really need this?

http://codereview.chromium.org/159838/diff/1002/1003#newcode122
Line 122: // Starting a drag results in a MouseExited, we need to ignore it.
Doesn't this also happen when showing on right click?

[pierre.lafayette, 2009-08-05 18:48:38]

Thanks for the review. I believe I've addressed all the issues.

[sky, 2009-08-06 22:33:05]

http://codereview.chromium.org/159838/diff/1007/1008
File views/controls/button/button_dropdown.cc (right):

http://codereview.chromium.org/159838/diff/1007/1008#newcode82
Line 82: //        context menu message loop has terminated.
Isn't the message loop done by the time you get here? As such, seems like this
NOTE isn't particularly useful.

http://codereview.chromium.org/159838/diff/1007/1008#newcode94
Line 94: result = ImageButton::OnMouseDragged(e);
This isn't quite right. In particular if you drag the mouse outside the button
it should change back to normal.

After looking at this again I think the right thing to do is to change
CustomButton to have a method that subclasses can override to determine if the
button should become pushed during mouse operations. Perhaps something like:

// Returns true if the button should become pressed when the user
// holds the mouse down over the button. This implementation returns
// the value from IsTriggerableEvent.
virtual bool ShouldEnterPushedState(const MouseEvent& e);

[pierre.lafayette, 2009-08-07 03:17:42]

Updated.

[sky, 2009-08-07 15:41:33]

http://codereview.chromium.org/159838/diff/5015/4008
File views/controls/button/button_dropdown.cc (right):

http://codereview.chromium.org/159838/diff/5015/4008#newcode60
Line 60: SetState(BS_PUSHED);
Do you need this anymore? Isn't this covered by way of ShouldEnterPushedState?

http://codereview.chromium.org/159838/diff/5015/4008#newcode131
Line 131: return true;
There are other modifiers that may be pressed. You should only return true if
left/right is down.

http://codereview.chromium.org/159838/diff/5015/4009
File views/controls/button/button_dropdown.h (right):

http://codereview.chromium.org/159838/diff/5015/4009#newcode46
Line 46: virtual bool ShouldEnterPushedState(const MouseEvent& e);
Please add a description here.

http://codereview.chromium.org/159838/diff/5015/4011
File views/controls/button/custom_button.h (right):

http://codereview.chromium.org/159838/diff/5015/4011#newcode80
Line 80: // holds the mouse down over the button.
Please mention CustomButton's implementation invokes IsTriggerableEvent.

[pierre.lafayette, 2009-08-07 20:52:30]

http://codereview.chromium.org/159838/diff/5015/4008
File views/controls/button/button_dropdown.cc (right):

http://codereview.chromium.org/159838/diff/5015/4008#newcode60
Line 60: SetState(BS_PUSHED);
Correct. Removing this block.

http://codereview.chromium.org/159838/diff/5015/4008#newcode131
Line 131: return true;
Didn't mean to do that. Good eye.

http://codereview.chromium.org/159838/diff/5015/4009
File views/controls/button/button_dropdown.h (right):

http://codereview.chromium.org/159838/diff/5015/4009#newcode46
Line 46: virtual bool ShouldEnterPushedState(const MouseEvent& e);
On 2009/08/07 15:41:33, sky wrote:
> Please add a description here.

Done.

http://codereview.chromium.org/159838/diff/5015/4011
File views/controls/button/custom_button.h (right):

http://codereview.chromium.org/159838/diff/5015/4011#newcode80
Line 80: // holds the mouse down over the button.
On 2009/08/07 15:41:33, sky wrote:
> Please mention CustomButton's implementation invokes IsTriggerableEvent.

Done.

[sky, 2009-08-11 16:20:10]

http://codereview.chromium.org/159838/diff/4043/5028
File views/controls/button/button_dropdown.cc (right):

http://codereview.chromium.org/159838/diff/4043/5028#newcode51
Line 51: show_menu_factory_.NewRunnableMethod(&ButtonDropDown::ShowDropDownMenu,
The old indentation is correct. The style guide says all wrapped lines should
either align with the previous ( or be 4 space intended.

http://codereview.chromium.org/159838/diff/4043/5028#newcode59
Line 59: if (e.IsLeftMouseButton())
You need to handle the case of right mouse button release and !hittest.

[pierre.lafayette, 2009-08-11 17:47:21]

Updated.

[Peter Kasting, 2009-08-21 00:10:58]

Pinging so sky will take another look at this, I think it fell off his radar.

[pierre.lafayette, 2009-08-21 00:45:32]

On 2009/08/21 00:10:58, Peter Kasting wrote:
> Pinging so sky will take another look at this, I think it fell off his radar.

This was landed in 23101. Thanks.

[Peter Kasting, 2009-08-21 00:48:30]

On 2009/08/21 00:45:32, pierre.lafayette wrote:
> This was landed in 23101. Thanks.

OK, closed.