Fixes of settings view layout bugs.

Fixes of settings view layout bugs.

- better to focus on entries rather than the specific settings item
- scroll to the settings item when focus has moved out of the scroll view
- EntryView keeps the same margin both left and right
- resize the scroll contents when the window is resized

BUG=196804, 226678
R=dharcourt@chromium.org, dewittj@chromium.org


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=192991

[dharcourt, 2013-04-05 16:07:03]

LGTM.... and please pardon the nits, overall this is a very nice fix :-).

https://codereview.chromium.org/13544009/diff/1/ui/message_center/views/notif...
File ui/message_center/views/notifier_settings_view.cc (right):

https://codereview.chromium.org/13544009/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:69: int contents_height =
contents->GetHeightForWidth(contents_width);
Nit: contents_width/contents_height should be content_width/content_height
(singular, like "horse race" instead of "horses race")... and I know we have
many other places in the code where this is wrong :-).

https://codereview.chromium.org/13544009/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:72: y = (height() -
contents_height) / 2;
This _might_ be simpler as y = std::max((height() - content_height) / 2, 0),
your call.

https://codereview.chromium.org/13544009/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:312: int contents_height =
contents_view->GetHeightForWidth(contents_width);
Nit: Same contents_width/contents_height -> content_width/content_height.

[dewittj, 2013-04-05 20:48:41]

lgtm to me too, my comment about scroll views is more of a question.

https://codereview.chromium.org/13544009/diff/1/ui/message_center/views/notif...
File ui/message_center/views/notifier_settings_view.cc (right):

https://codereview.chromium.org/13544009/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:150:
checkbox_->set_focusable(false);
I believe you could just remove this line, since focusable is default false.

https://codereview.chromium.org/13544009/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:310: views::View*
contents_view = scroller_->contents();
You are more familiar with scroll views than me but it strikes me as strange
that we are manually resizing the contents view instead of telling the
scrollview that it has a different amount of room now.

[Jun Mukai, 2013-04-06 03:14:04]

https://codereview.chromium.org/13544009/diff/1/ui/message_center/views/notif...
File ui/message_center/views/notifier_settings_view.cc (right):

https://codereview.chromium.org/13544009/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:69: int contents_height =
contents->GetHeightForWidth(contents_width);
On 2013/04/05 16:07:03, dharcourt wrote:
> Nit: contents_width/contents_height should be content_width/content_height
> (singular, like "horse race" instead of "horses race")... and I know we have
> many other places in the code where this is wrong :-).

Done.

https://codereview.chromium.org/13544009/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:72: y = (height() -
contents_height) / 2;
On 2013/04/05 16:07:03, dharcourt wrote:
> This _might_ be simpler as y = std::max((height() - content_height) / 2, 0),
> your call.

Done.

https://codereview.chromium.org/13544009/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:150:
checkbox_->set_focusable(false);
On 2013/04/05 20:48:41, dewittj wrote:
> I believe you could just remove this line, since focusable is default false.

Done.

https://codereview.chromium.org/13544009/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:310: views::View*
contents_view = scroller_->contents();
On 2013/04/05 20:48:41, dewittj wrote:
> You are more familiar with scroll views than me but it strikes me as strange
> that we are manually resizing the contents view instead of telling the
> scrollview that it has a different amount of room now. 

That is a tricky part of the scroll view.
The size change of the ScrollView changes the visible rectangle size of its
contents, and that has no relationship with the layout of the contents view.
So essentially, if the contents view is wider than the width of scroll view, it
will show the horizontal scroll bar rather than changing the width of contents
view.

This is good to ideal scrollers, but we want to allow the vertical scroll bar
only here and wrap the text properly. This actually needs to change the width of
the contents view properly.

https://codereview.chromium.org/13544009/diff/1/ui/message_center/views/notif...
ui/message_center/views/notifier_settings_view.cc:312: int contents_height =
contents_view->GetHeightForWidth(contents_width);
On 2013/04/05 16:07:03, dharcourt wrote:
> Nit: Same contents_width/contents_height -> content_width/content_height.

Done.

[dewittj, 2013-04-08 21:23:42]

On 2013/04/06 03:14:04, Jun Mukai wrote:
> This is good to ideal scrollers, but we want to allow the vertical scroll bar
> only here and wrap the text properly. This actually needs to change the width
of
> the contents view properly.
Okay.  If we end up needing the vertical-only scroll view elsewhere perhaps that
should be its own class.

[dharcourt, 2013-04-08 21:43:26]

On 2013/04/08 21:23:42, dewittj wrote:
> On 2013/04/06 03:14:04, Jun Mukai wrote:
> > This is good to ideal scrollers, but we want to allow the vertical scroll
bar
> > only here and wrap the text properly. This actually needs to change the
width
> of
> > the contents view properly.
> Okay.  If we end up needing the vertical-only scroll view elsewhere perhaps
that
> should be its own class.

Or we could add that to views' ScrollView :-). We do a similar workaround in the
notification center, and there too the child view ends up being laid out twice.
I'm currently implementing a layout "cache" to avoid that, but with a vertical
ScrollBar that would go away and simplify the code there as well as here.

[Jun Mukai, 2013-04-08 21:54:49]

On 2013/04/08 21:43:26, dharcourt wrote:
> On 2013/04/08 21:23:42, dewittj wrote:
> > On 2013/04/06 03:14:04, Jun Mukai wrote:
> > > This is good to ideal scrollers, but we want to allow the vertical scroll
> bar
> > > only here and wrap the text properly. This actually needs to change the
> width
> > of
> > > the contents view properly.
> > Okay.  If we end up needing the vertical-only scroll view elsewhere perhaps
> that
> > should be its own class.
> 
> Or we could add that to views' ScrollView :-). We do a similar workaround in
the
> notification center, and there too the child view ends up being laid out
twice.
> I'm currently implementing a layout "cache" to avoid that, but with a vertical
> ScrollBar that would go away and simplify the code there as well as here.

Yeah, probably better to do it in views side. Also the system tray code
(ash/system/tray) is also doing similar thing. filed crbug.com/229074 for this.

[commit-bot: I haz the power, 2013-04-08 21:55:15]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mukai@chromium.org/13544009/6001

[commit-bot: I haz the power, 2013-04-09 03:04:28]

Change committed as 192991