Left align bubble title and text, adjusted spacing between.

Left align bubble title and text, adjusted spacing between.

BUG=462407
Committed: https://crrev.com/493e5ff73986ecf679913854bbf7363981578029
Cr-Commit-Position: refs/heads/master@{#318571}

[xiaoling, 2015-02-26 23:33:42]

xiaolingbao@chromium.org changed reviewers:
+ msw@chromium.org

[xiaoling, 2015-02-26 23:35:51]

Will send screenshot in email.

[chromium-reviews, 2015-02-26 23:37:35]

Screenshot:
[image: Inline image 1]

Michael,
Is there a way to check that other dialog/bubble that uses BubbleFrameView
still look good?

On Thu, Feb 26, 2015 at 3:35 PM, <xiaolingbao@chromium.org> wrote:

> Reviewers: msw,
>
> Message:
> Will send screenshot in email.
>
> Description:
> Left align bubble title and text, adjusted spacing between.
>
> BUG=462407
>
> Please review this at https://codereview.chromium.org/962013002/
>
> Base URL: https://chromium.googlesource.com/chromium/src.git@master
>
> Affected files (+7, -4 lines):
>   M chrome/browser/ui/views/global_error_bubble_view.cc
>   M ui/views/bubble/bubble_frame_view.cc
>
>
> Index: chrome/browser/ui/views/global_error_bubble_view.cc
> diff --git a/chrome/browser/ui/views/global_error_bubble_view.cc
> b/chrome/browser/ui/views/global_error_bubble_view.cc
> index 1fae5f6ecac3155877a94062b443328ea69685e0..
> b316556e21d0b24d750716f9b0cd6a37d0d2f854 100644
> --- a/chrome/browser/ui/views/global_error_bubble_view.cc
> +++ b/chrome/browser/ui/views/global_error_bubble_view.cc
> @@ -35,6 +35,9 @@ const int kMaxBubbleViewWidth = 262;
>  // The vertical inset of the wrench bubble anchor from the wrench menu
> button.
>  const int kAnchorVerticalInset = 5;
>
> +// Align bubble text with the title.
> +const int kBubbleLeftPadding = 14;
> +
>  const int kBubblePadding = 6;
>
>  }  // namespace
> @@ -106,7 +109,7 @@ GlobalErrorBubbleView::GlobalErrorBubbleView(
>    SetLayoutManager(layout);
>
>    // BubbleFrameView adds enough padding below title, no top padding
> needed.
> -  layout->SetInsets(0, kBubblePadding, kBubblePadding, kBubblePadding);
> +  layout->SetInsets(0, kBubbleLeftPadding, kBubblePadding,
> kBubblePadding);
>
>    // First row, message labels.
>    views::ColumnSet* cs = layout->AddColumnSet(0);
> Index: ui/views/bubble/bubble_frame_view.cc
> diff --git a/ui/views/bubble/bubble_frame_view.cc
> b/ui/views/bubble/bubble_frame_view.cc
> index 599d5965f2880ab57ca9218144a0cf0877aa48cd..
> 7d34504e545239920013621e6f81ee0488682424 100644
> --- a/ui/views/bubble/bubble_frame_view.cc
> +++ b/ui/views/bubble/bubble_frame_view.cc
> @@ -23,9 +23,9 @@
>  namespace {
>
>  // Insets for the title bar views in pixels.
> -const int kTitleTopInset = 12;
> -const int kTitleLeftInset = 19;
> -const int kTitleBottomInset = 12;
> +const int kTitleTopInset = 18;
> +const int kTitleLeftInset = 20;
> +const int kTitleBottomInset = 7;
>  const int kTitleRightInset = 7;
>
>  // The horizontal padding between the title and the icon.
>
>
>


-- 
Xiaoling

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[msw, 2015-02-27 01:57:05]

https://codereview.chromium.org/962013002/diff/1/chrome/browser/ui/views/glob...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/962013002/diff/1/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:112: layout->SetInsets(0,
kBubbleLeftPadding, kBubblePadding, kBubblePadding);
You should probably leave the layout's insets empty and instead just call
BubbleDelegateView::set_margins, to replace the default 6px insets there.

https://codereview.chromium.org/962013002/diff/1/ui/views/bubble/bubble_frame...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/962013002/diff/1/ui/views/bubble/bubble_frame...
ui/views/bubble/bubble_frame_view.cc:26: const int kTitleTopInset = 18;
Don't change this value... the title wasn't too high. If anything, we may need
to move the icon down to match the cap-height centering of the title text, but
you can consider addressing that in a separate CL.

https://codereview.chromium.org/962013002/diff/1/ui/views/bubble/bubble_frame...
ui/views/bubble/bubble_frame_view.cc:27: const int kTitleLeftInset = 20;
Don't bother changing this, just add 1px in the icon case, if anything.

https://codereview.chromium.org/962013002/diff/1/ui/views/bubble/bubble_frame...
ui/views/bubble/bubble_frame_view.cc:28: const int kTitleBottomInset = 7;
This can't be right, it will affect all bubbles... The global error bubble
itself must be using incorrect content margins or something else that adds
unintended padding; see my comment with the bubble, it should replace it's
default 6px top margin and tweak other margins as needed.

[xiaoling, 2015-02-27 02:24:13]

PTAL.

https://codereview.chromium.org/962013002/diff/1/chrome/browser/ui/views/glob...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/962013002/diff/1/chrome/browser/ui/views/glob...
chrome/browser/ui/views/global_error_bubble_view.cc:112: layout->SetInsets(0,
kBubbleLeftPadding, kBubblePadding, kBubblePadding);
On 2015/02/27 01:57:05, msw wrote:
> You should probably leave the layout's insets empty and instead just call
> BubbleDelegateView::set_margins, to replace the default 6px insets there.

Done.

https://codereview.chromium.org/962013002/diff/1/ui/views/bubble/bubble_frame...
File ui/views/bubble/bubble_frame_view.cc (right):

https://codereview.chromium.org/962013002/diff/1/ui/views/bubble/bubble_frame...
ui/views/bubble/bubble_frame_view.cc:26: const int kTitleTopInset = 18;
On 2015/02/27 01:57:05, msw wrote:
> Don't change this value... the title wasn't too high. If anything, we may need
> to move the icon down to match the cap-height centering of the title text, but
> you can consider addressing that in a separate CL.

Was trying to make top margin to 20 px. Reverted.

https://codereview.chromium.org/962013002/diff/1/ui/views/bubble/bubble_frame...
ui/views/bubble/bubble_frame_view.cc:27: const int kTitleLeftInset = 20;
On 2015/02/27 01:57:05, msw wrote:
> Don't bother changing this, just add 1px in the icon case, if anything.

Done.

https://codereview.chromium.org/962013002/diff/1/ui/views/bubble/bubble_frame...
ui/views/bubble/bubble_frame_view.cc:28: const int kTitleBottomInset = 7;
On 2015/02/27 01:57:05, msw wrote:
> This can't be right, it will affect all bubbles... The global error bubble
> itself must be using incorrect content margins or something else that adds
> unintended padding; see my comment with the bubble, it should replace it's
> default 6px top margin and tweak other margins as needed.

Done.

[chromium-reviews, 2015-02-27 02:24:49]

Updated screenshot:
[image: Inline image 1]

On Thu, Feb 26, 2015 at 6:24 PM, <xiaolingbao@chromium.org> wrote:

> PTAL.
>
>
> https://codereview.chromium.org/962013002/diff/1/chrome/
> browser/ui/views/global_error_bubble_view.cc
> File chrome/browser/ui/views/global_error_bubble_view.cc (right):
>
> https://codereview.chromium.org/962013002/diff/1/chrome/
> browser/ui/views/global_error_bubble_view.cc#newcode112
> chrome/browser/ui/views/global_error_bubble_view.cc:112:
> layout->SetInsets(0, kBubbleLeftPadding, kBubblePadding,
> kBubblePadding);
> On 2015/02/27 01:57:05, msw wrote:
>
>> You should probably leave the layout's insets empty and instead just
>>
> call
>
>> BubbleDelegateView::set_margins, to replace the default 6px insets
>>
> there.
>
> Done.
>
> https://codereview.chromium.org/962013002/diff/1/ui/views/
> bubble/bubble_frame_view.cc
> File ui/views/bubble/bubble_frame_view.cc (right):
>
> https://codereview.chromium.org/962013002/diff/1/ui/views/
> bubble/bubble_frame_view.cc#newcode26
> ui/views/bubble/bubble_frame_view.cc:26: const int kTitleTopInset = 18;
> On 2015/02/27 01:57:05, msw wrote:
>
>> Don't change this value... the title wasn't too high. If anything, we
>>
> may need
>
>> to move the icon down to match the cap-height centering of the title
>>
> text, but
>
>> you can consider addressing that in a separate CL.
>>
>
> Was trying to make top margin to 20 px. Reverted.
>
> https://codereview.chromium.org/962013002/diff/1/ui/views/
> bubble/bubble_frame_view.cc#newcode27
> ui/views/bubble/bubble_frame_view.cc:27: const int kTitleLeftInset = 20;
> On 2015/02/27 01:57:05, msw wrote:
>
>> Don't bother changing this, just add 1px in the icon case, if
>>
> anything.
>
> Done.
>
> https://codereview.chromium.org/962013002/diff/1/ui/views/
> bubble/bubble_frame_view.cc#newcode28
> ui/views/bubble/bubble_frame_view.cc:28: const int kTitleBottomInset =
> 7;
> On 2015/02/27 01:57:05, msw wrote:
>
>> This can't be right, it will affect all bubbles... The global error
>>
> bubble
>
>> itself must be using incorrect content margins or something else that
>>
> adds
>
>> unintended padding; see my comment with the bubble, it should replace
>>
> it's
>
>> default 6px top margin and tweak other margins as needed.
>>
>
> Done.
>
> https://codereview.chromium.org/962013002/
>



-- 
Xiaoling

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[xiaoling, 2015-02-27 02:59:35]

https://codereview.chromium.org/962013002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/962013002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/global_error_bubble_view.cc:113: kBubblePadding,
kBubblePadding));
I thought I have moved set_margins to the top of the function, but apparently
not or not uploaded. I'll do that tomorrow.

[msw, 2015-02-27 03:25:50]

Hopefully the bubble will look decent with my new suggestions.

https://codereview.chromium.org/962013002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/962013002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/global_error_bubble_view.cc:41: const int kBubblePadding
= 11;
nit: this should just be 19px or 20px and combined with the left side constant.
This will make the text spaced evenly on both sides, and the insets will match
those of a standard dialog. (in a followup, we can try to use DialogClientView)

https://codereview.chromium.org/962013002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/global_error_bubble_view.cc:113: kBubblePadding,
kBubblePadding));
On 2015/02/27 02:59:34, xiaoling wrote:
> I thought I have moved set_margins to the top of the function, but apparently
> not or not uploaded. I'll do that tomorrow.

Acknowledged.

https://codereview.chromium.org/962013002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/global_error_bubble_view.cc:137:
layout->AddPaddingRow(0, views::kLabelToControlVerticalSpacing);
I think this 8px padding between the message and the buttons (visually ~13px)
should be increased to match the padding between the title and the message
(visually ~19px). So, I guess replace this with a local constant of ~14px.

[chromium-reviews, 2015-02-27 17:56:47]

This is the new look. How does the bottom margin look to you?

[image: Inline image 1]

On Thu, Feb 26, 2015 at 7:25 PM, <msw@chromium.org> wrote:

> Hopefully the bubble will look decent with my new suggestions.
>
>
> https://codereview.chromium.org/962013002/diff/20001/
> chrome/browser/ui/views/global_error_bubble_view.cc
> File chrome/browser/ui/views/global_error_bubble_view.cc (right):
>
> https://codereview.chromium.org/962013002/diff/20001/
> chrome/browser/ui/views/global_error_bubble_view.cc#newcode41
> chrome/browser/ui/views/global_error_bubble_view.cc:41: const int
> kBubblePadding = 11;
> nit: this should just be 19px or 20px and combined with the left side
> constant. This will make the text spaced evenly on both sides, and the
> insets will match those of a standard dialog. (in a followup, we can try
> to use DialogClientView)
>
> https://codereview.chromium.org/962013002/diff/20001/
> chrome/browser/ui/views/global_error_bubble_view.cc#newcode113
> chrome/browser/ui/views/global_error_bubble_view.cc:113: kBubblePadding,
> kBubblePadding));
> On 2015/02/27 02:59:34, xiaoling wrote:
>
>> I thought I have moved set_margins to the top of the function, but
>>
> apparently
>
>> not or not uploaded. I'll do that tomorrow.
>>
>
> Acknowledged.
>
> https://codereview.chromium.org/962013002/diff/20001/
> chrome/browser/ui/views/global_error_bubble_view.cc#newcode137
> chrome/browser/ui/views/global_error_bubble_view.cc:137:
> layout->AddPaddingRow(0, views::kLabelToControlVerticalSpacing);
> I think this 8px padding between the message and the buttons (visually
> ~13px) should be increased to match the padding between the title and
> the message (visually ~19px). So, I guess replace this with a local
> constant of ~14px.
>
> https://codereview.chromium.org/962013002/
>



-- 
Xiaoling

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[xiaoling, 2015-02-27 18:00:38]

Nice suggestions.

https://codereview.chromium.org/962013002/diff/20001/chrome/browser/ui/views/...
File chrome/browser/ui/views/global_error_bubble_view.cc (right):

https://codereview.chromium.org/962013002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/global_error_bubble_view.cc:41: const int kBubblePadding
= 11;
On 2015/02/27 03:25:50, msw wrote:
> nit: this should just be 19px or 20px and combined with the left side
constant.
> This will make the text spaced evenly on both sides, and the insets will match
> those of a standard dialog. (in a followup, we can try to use
DialogClientView)

Done.

https://codereview.chromium.org/962013002/diff/20001/chrome/browser/ui/views/...
chrome/browser/ui/views/global_error_bubble_view.cc:137:
layout->AddPaddingRow(0, views::kLabelToControlVerticalSpacing);
On 2015/02/27 03:25:50, msw wrote:
> I think this 8px padding between the message and the buttons (visually ~13px)
> should be increased to match the padding between the title and the message
> (visually ~19px). So, I guess replace this with a local constant of ~14px.

Done.

[msw, 2015-02-27 18:26:41]

The code and screenshot lgtm, but please post the screenshot to the bug and
reply to the e-mail thread, so UX folks can weigh in.

[xiaoling, 2015-02-28 01:46:57]

The CQ bit was checked by xiaolingbao@chromium.org

[xiaoling, 2015-02-28 01:46:57]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/962013002/#ps60001 (title:
" ")

[commit-bot: I haz the power, 2015-02-28 01:47:56]

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

[commit-bot: I haz the power, 2015-02-28 02:05:00]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-02-28 02:05:27]

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