Handle dialog acclerators explicitly; nix default button switching.

Handle dialog acclerators explicitly; nix default button switching.

Stop switching default dialog buttons on focus changes.
Remove default button and focus tracking, and button accelerators.
( this is undesirable complexity and looks odd )
Add and handle DialogClientView return and escape accelerators.
( for escape: invoke the cancel button or CancelWindow )
( for return: invoke the focused view, ok button, or AcceptWindow )
Still call SetIsDefault to keep the ok/cancel default button appearance.
Remove the redundant set_focusable call, the button style is focusable.
Only return true from TreeView::AcceleratorPressed if it's handled.

BUG=166075
TEST=[Return] and [Escape] work on dialogs as expected regardless of the focused view in the dialog.
R=sky@chromium.org

[msw, 2013-04-16 23:12:10]

Hey Scott, please take a look; thanks!

[sky, 2013-04-16 23:41:54]

I agree switching the default button based on focus is weird, but it's
standard windows behavior.

On Tue, Apr 16, 2013 at 4:12 PM,  <msw@chromium.org> wrote:
> Reviewers: sky,
>
> Message:
> Hey Scott, please take a look; thanks!
>
> Description:
> Handle dialog acclerators explicitly; nix default button switching.
>
> Stop switching default dialog buttons on focus changes.
> Remove default button and focus tracking, and button accelerators.
> ( this is undesirable complexity and looks odd )
> Add and handle DialogClientView return and escape accelerators.
> ( for escape: invoke the cancel button or CancelWindow )
> ( for return: invoke the focused view, ok button, or AcceptWindow )
> Still call SetIsDefault to keep the ok/cancel default button appearance.
> Remove the redundant set_focusable call, the button style is focusable.
> Only return true from TreeView::AcceleratorPressed if it's handled.
>
> BUG=166075
> TEST=[Return] and [Escape] work on dialogs as expected regardless of the
> focused
> view in the dialog.
> R=sky@chromium.org
>
> Please review this at https://codereview.chromium.org/14230018/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files:
>   M ui/views/controls/tree/tree_view.cc
>   M ui/views/window/dialog_client_view.h
>   M ui/views/window/dialog_client_view.cc
>
>

[msw, 2013-04-16 23:49:31]

On 2013/04/16 23:41:54, sky wrote:
> I agree switching the default button based on focus is weird, but it's
standard windows behavior.

Hmm, so should we keep that behavior even if we can emulate it without changing
the actual default button?

Background motivation: The new button style uses bold text for default buttons,
which changes the button size. Doing that on focus changes looks *really* weird.
I guess I could instead enlarge all/dialog buttons to accommodate their bolded
text, WDYT?

[msw, 2013-04-17 21:16:08]

On 2013/04/16 23:49:31, msw wrote:
> I could instead enlarge all/dialog buttons to accommodate their bolded text,
WDYT?

http://crrev.com/194673 fits STYLE_BUTTON to its bold text.
I'm abandoning this CL since we want default button cycling.

[sky, 2013-04-22 14:24:09]

Does bold only effect the width and not the height?

On Tue, Apr 16, 2013 at 4:49 PM,  <msw@chromium.org> wrote:
> On 2013/04/16 23:41:54, sky wrote:
>>
>> I agree switching the default button based on focus is weird, but it's
>
> standard windows behavior.
>
> Hmm, so should we keep that behavior even if we can emulate it without
> changing
> the actual default button?
>
> Background motivation: The new button style uses bold text for default
> buttons,
> which changes the button size. Doing that on focus changes looks *really*
> weird.
> I guess I could instead enlarge all/dialog buttons to accommodate their
> bolded
> text, WDYT?
>
> https://codereview.chromium.org/14230018/

[msw, 2013-04-22 18:04:11]

On 2013/04/22 14:24:09, sky wrote:
> Does bold only effect the width and not the height?
It probably depends on the font; any reason you ask?
( this CL was abandoned in favor of http://crrev.com/194673 )

[sky, 2013-04-22 22:30:17]

We don't want buttons (or any other static type view) to change in
size. It leads to layouts dancing around and other weird things.
Additionally if the height changes it could effect existing layouts,
say a label with a button right next to it could look different.

I see badness no matter what we attempt here.

On Mon, Apr 22, 2013 at 11:04 AM,  <msw@chromium.org> wrote:
> On 2013/04/22 14:24:09, sky wrote:
>>
>> Does bold only effect the width and not the height?
>
> It probably depends on the font; any reason you ask?
> ( this CL was abandoned in favor of http://crrev.com/194673 )
>
> https://codereview.chromium.org/14230018/

[msw, 2013-04-22 22:40:41]

On 2013/04/22 22:30:17, sky wrote:
> We don't want buttons (or any other static type view) to change in
> size. It leads to layouts dancing around and other weird things.
> Additionally if the height changes it could effect existing layouts,
> say a label with a button right next to it could look different.
> 
> I see badness no matter what we attempt here.
> 
> On Mon, Apr 22, 2013 at 11:04 AM,  <mailto:msw@chromium.org> wrote:
> > On 2013/04/22 14:24:09, sky wrote:
> >>
> >> Does bold only effect the width and not the height?
> >
> > It probably depends on the font; any reason you ask?
> > ( this CL was abandoned in favor of http://crrev.com/194673 )
> >
> > https://codereview.chromium.org/14230018/

The way the current ToT code works ( from http://crrev.com/194673 ) is by
pre-sizing the buttons to accommodate bold text. Buttons also monotonically
increase in size (don't shrink), until their minimum size is explicitly reset.
That way, the size and position doesn't change as we make any random button
default. It actually looks pretty good, you can try it yourself by tabbing
between buttons in several dialogs: Edit Bookmark, Collected Cookies, etc. Give
it a shot and let me know what you think.

[sky, 2013-04-22 23:23:14]

How does it look for a font where the bold size is a different height?

On Mon, Apr 22, 2013 at 3:40 PM,  <msw@chromium.org> wrote:
> On 2013/04/22 22:30:17, sky wrote:
>>
>> We don't want buttons (or any other static type view) to change in
>> size. It leads to layouts dancing around and other weird things.
>> Additionally if the height changes it could effect existing layouts,
>> say a label with a button right next to it could look different.
>
>
>> I see badness no matter what we attempt here.
>
>
>> On Mon, Apr 22, 2013 at 11:04 AM,  <mailto:msw@chromium.org> wrote:
>> > On 2013/04/22 14:24:09, sky wrote:
>> >>
>> >> Does bold only effect the width and not the height?
>> >
>> > It probably depends on the font; any reason you ask?
>> > ( this CL was abandoned in favor of http://crrev.com/194673 )
>> >
>> > https://codereview.chromium.org/14230018/
>
>
> The way the current ToT code works ( from http://crrev.com/194673 ) is by
> pre-sizing the buttons to accommodate bold text. Buttons also monotonically
> increase in size (don't shrink), until their minimum size is explicitly
> reset.
> That way, the size and position doesn't change as we make any random button
> default. It actually looks pretty good, you can try it yourself by tabbing
> between buttons in several dialogs: Edit Bookmark, Collected Cookies, etc.
> Give
> it a shot and let me know what you think.
>
> https://codereview.chromium.org/14230018/

[msw, 2013-04-23 00:35:24]

On 2013/04/22 23:23:14, sky wrote:
> How does it look for a font where the bold size is a different height?

The default Win and CrOS fonts do not change height when made bold, but by
forcing the buttons to have gfx::Font("Arial", 16); the bold styled text is one
pixel taller than it's non-bold counterpart. The button layout code gives all
available height to its Label, so Label's  vertical centering takes over and
keeps the text vertically centered. It doesn't look bad, even when you change
the default dynamically with focus. I'll send you an image via e-mail; let me
know if you have specific concerns; perhaps file an issue rather than discussing
it here?