Make some updates to BM bar for MD.

Make some updates to BM bar for MD.

- Adjust some layout constants. The height of the bar is already spot-on
so no adjustments in the vertical dimension.
- Empty bookmark bar:
 * import link text uses link color, as long as it's readable enough.
   Otherwise, fall back to current behavior (bookmark bar text color).
 * spacing between label and link is one space, rather than 6px.
- Detached bookmark bar bg color (bottom separator color is not updated but is already pretty close to spec, and is calculated programatically, so I'm leaving it alone for now)

BUG=505807, 538172, 538173
Committed: https://crrev.com/e61501e21cce896426e724c047e2d7998e89b05e
Cr-Commit-Position: refs/heads/master@{#356496}

[Evan Stade, 2015-10-16 22:54:13]

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

[Evan Stade, 2015-10-16 22:54:13]

+sky for review
+tdanderson FYI because of the link color logic. This was the best I could come
up with to match Sebastien's mocks for the default theme while still supporting
custom themes. Has there already been discussion on this topic that I missed?

[tdanderson, 2015-10-19 14:36:58]

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

[tdanderson, 2015-10-19 14:38:39]

On 2015/10/16 22:54:13, Evan Stade wrote:
> +sky for review
> +tdanderson FYI because of the link color logic. This was the best I could
come
> up with to match Sebastien's mocks for the default theme while still
supporting
> custom themes. Has there already been discussion on this topic that I missed?

+varkha@ since this is something he has been thinking about / discussing
recently.

[varkha, 2015-10-19 15:01:29]

Thanks for the pointer. I'll build this locally to experiment with the looks. A
couple of nits in the meanwhile.

https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc
(right):

https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:139: if
(import_link_) {
nit: could return early on !import_link to simplify the rest.

https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:158: }
nit: maybe shorten this similar to:

bool enough_contrast = color_utils::GetContrastRatio(link_color, bg) >=
color_utils::kMinimumReadableContrastRatio;
import_link_->SetUnderline(!enough_contrast);
import_link_->SetEnabledColor(enough_contrast ? link_color : text_color);

[sky, 2015-10-19 15:38:40]

https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc
(right):

https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:39:
instructions_text += base::ASCIIToUTF16(" ");
Why do you add a space for material? Might the space have undesirable effects in
other languages?

[Evan Stade, 2015-10-19 16:49:50]

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

[Evan Stade, 2015-10-19 16:49:52]

+jshin in case he has insight into Scott's question

https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc
(right):

https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:39:
instructions_text += base::ASCIIToUTF16(" ");
On 2015/10/19 15:38:40, sky wrote:
> Why do you add a space for material? Might the space have undesirable effects
in
> other languages?

Previously this space was implemented with kViewPadding, but that didn't create
the right amount of space (especially if you change the font). I tried to think
of how this could fail in other languages. It works in RTL and LTR. In languages
without space characters it creates too much space (I just tested with zh_CN),
but that was already a problem. Also it doesn't look that bad because the zh_CN
period character [。] already has some trailing empty space attached, so we get a
slightly too-wide space rather than a space where there should be none.

https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:139: if
(import_link_) {
On 2015/10/19 15:01:29, varkha wrote:
> nit: could return early on !import_link to simplify the rest.

Done.

https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:158: }
On 2015/10/19 15:01:29, varkha wrote:
> nit: maybe shorten this similar to:
> 
> bool enough_contrast = color_utils::GetContrastRatio(link_color, bg) >=
> color_utils::kMinimumReadableContrastRatio;
> import_link_->SetUnderline(!enough_contrast);
> import_link_->SetEnabledColor(enough_contrast ? link_color : text_color);

Done.

[Evan Stade, 2015-10-19 16:50:55]

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

[Evan Stade, 2015-10-19 16:52:13]

Description was changed from

==========
Make some updates to BM bar for MD.

- Adjust some layout constants. The height of the bar is already spot-on
so no adjustments in the vertical dimension.
- Empty bookmark bar:
 * import link text uses link color, as long as it's readable enough.
   Otherwise, fall back to current behavior (bookmark bar text color).
 * spacing between label and link is one space, rather than 6px.
- Detached bookmark bar bg color (bottom separator color is not updated but is
already pretty close to spec, and is calculated programatically, so I'm leaving
it alone for now)

BUG=505807, 538172
==========

to

==========
Make some updates to BM bar for MD.

- Adjust some layout constants. The height of the bar is already spot-on
so no adjustments in the vertical dimension.
- Empty bookmark bar:
 * import link text uses link color, as long as it's readable enough.
   Otherwise, fall back to current behavior (bookmark bar text color).
 * spacing between label and link is one space, rather than 6px.
- Detached bookmark bar bg color (bottom separator color is not updated but is
already pretty close to spec, and is calculated programatically, so I'm leaving
it alone for now)

BUG=505807, 538172, 538173
==========

[commit-bot: I haz the power, 2015-10-19 16:52:26]

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

[commit-bot: I haz the power, 2015-10-19 17:33:27]

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

[commit-bot: I haz the power, 2015-10-19 17:33:27]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sky, 2015-10-20 23:10:28]

On 2015/10/19 16:49:52, Evan Stade wrote:
> +jshin in case he has insight into Scott's question
> 
>
https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc
> (right):
> 
>
https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:39:
> instructions_text += base::ASCIIToUTF16(" ");
> On 2015/10/19 15:38:40, sky wrote:
> > Why do you add a space for material? Might the space have undesirable
effects
> in
> > other languages?
> 
> Previously this space was implemented with kViewPadding, but that didn't
create
> the right amount of space (especially if you change the font). I tried to
think
> of how this could fail in other languages. It works in RTL and LTR. In
languages
> without space characters it creates too much space (I just tested with zh_CN),
> but that was already a problem. Also it doesn't look that bad because the
zh_CN
> period character [。] already has some trailing empty space attached, so we get
a
> slightly too-wide space rather than a space where there should be none.

Could you derive the padding from the font then? I don't like having to worry
about the side effects of adding a space (alignment, events ...).

> 
>
https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:139: if
> (import_link_) {
> On 2015/10/19 15:01:29, varkha wrote:
> > nit: could return early on !import_link to simplify the rest.
> 
> Done.
> 
>
https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:158: }
> On 2015/10/19 15:01:29, varkha wrote:
> > nit: maybe shorten this similar to:
> > 
> > bool enough_contrast = color_utils::GetContrastRatio(link_color, bg) >=
> > color_utils::kMinimumReadableContrastRatio;
> > import_link_->SetUnderline(!enough_contrast);
> > import_link_->SetEnabledColor(enough_contrast ? link_color : text_color);
> 
> Done.

[Evan Stade, 2015-10-21 04:18:15]

On 2015/10/20 23:10:28, sky wrote:
> 
> Could you derive the padding from the font then? I don't like having to worry
> about the side effects of adding a space (alignment, events ...).
> 

if I were going to derive the padding from the font, I'd probably do something
like views::Label(" ").GetPreferredSize().width() or use gfx::RenderText
directly but both of those seem like added complexity compared to appending a
space. Can you elaborate on the alignment/events/etc concerns? As far as I know
there shouldn't be events for this label, unless perhaps you're using a screen
reader and then the difference is (guessing here) it might read the text aloud
when you hover the blank space directly to its right, which doesn't seem bad.

[sky, 2015-10-21 20:43:26]

My disdain for this approach is that you're adding unnecessary space
with the hope that it gives you the padding you want. This has the
undesirable side effects I mentioned and is subtle. I would prefer you
calculate the padding separately.

  -Scott

On Tue, Oct 20, 2015 at 9:18 PM,  <estade@chromium.org> wrote:
> On 2015/10/20 23:10:28, sky wrote:
>
>> Could you derive the padding from the font then? I don't like having to
>> worry
>> about the side effects of adding a space (alignment, events ...).
>
>
>
> if I were going to derive the padding from the font, I'd probably do
> something
> like views::Label(" ").GetPreferredSize().width() or use gfx::RenderText
> directly but both of those seem like added complexity compared to appending
> a
> space. Can you elaborate on the alignment/events/etc concerns? As far as I
> know
> there shouldn't be events for this label, unless perhaps you're using a
> screen
> reader and then the difference is (guessing here) it might read the text
> aloud
> when you hover the blank space directly to its right, which doesn't seem
> bad.
>
> https://codereview.chromium.org/1408273002/

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

[Evan Stade, 2015-10-21 22:10:35]

On 2015/10/21 20:43:26, sky wrote:
> My disdain for this approach is that you're adding unnecessary space
> with the hope that it gives you the padding you want.

I'm pretty sure the spec is "put one space character worth of space here".
Perhaps I should switch this to a single StyledLabel? (But then it's harder to
hide in the cros case.)

> This has the
> undesirable side effects I mentioned and is subtle.

I want to better understand those side effects. I didn't actually follow what
you meant. In concrete terms, what might go wrong?

[sky, 2015-10-21 23:04:52]

On Wed, Oct 21, 2015 at 3:10 PM,  <estade@chromium.org> wrote:
> On 2015/10/21 20:43:26, sky wrote:
>>
>> My disdain for this approach is that you're adding unnecessary space
>> with the hope that it gives you the padding you want.
>
>
> I'm pretty sure the spec is "put one space character worth of space here".
> Perhaps I should switch this to a single StyledLabel? (But then it's harder
> to
> hide in the cros case.)
>
>> This has the
>> undesirable side effects I mentioned and is subtle.
>
>
> I want to better understand those side effects. I didn't actually follow
> what
> you meant. In concrete terms, what might go wrong?

I outlined them:
. You have to make sure the alignment is correct (which is easy to miss).
. You're extending the bounds of the view when what you want is to
effect padding.

  -Scott

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

[Evan Stade, 2015-10-22 21:52:12]

On 2015/10/21 23:04:52, sky wrote:
> I outlined them:
> . You have to make sure the alignment is correct (which is easy to miss).
> . You're extending the bounds of the view when what you want is to
> effect padding.
> 
>   -Scott

Alright. The reason I keep persisting is not to challenge the validity of these
concerns, but because without understanding them I can't evaluate whether a
solution addresses the issues. I guess by alignment you mean vertical alignment
(all the baseline arithmetic in GetPreferredSize() and Layout()). StyledLabel
doesn't seem to deal with that and so it's probably broken. Plus, it doesn't
allow for single-line text. Otherwise, I think it would be the right solution
(even fixing those languages for which there should be no space between the text
and link).

PTAL the latest patchset and let me know if it's what you had in mind.

https://codereview.chromium.org/1408273002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc
(right):

https://codereview.chromium.org/1408273002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:45:
instructions_->SetHorizontalAlignment(gfx::ALIGN_LEFT);
the left align here and below fixes some jiggling that occurs when you resize
the browser window (the horizontal alignment of the text changes slightly as the
point of elision changes and the label is on the default center alignment).

[jungshik at Google, 2015-10-23 06:30:19]

https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc
(right):

https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:39:
instructions_text += base::ASCIIToUTF16(" ");
On 2015/10/19 16:49:52, Evan Stade wrote:
> On 2015/10/19 15:38:40, sky wrote:
> > Why do you add a space for material? Might the space have undesirable
effects
> in
> > other languages?
> 
> Previously this space was implemented with kViewPadding, but that didn't
create
> the right amount of space (especially if you change the font). I tried to
think
> of how this could fail in other languages. It works in RTL and LTR. In
languages
> without space characters it creates too much space (I just tested with zh_CN),
> but that was already a problem. Also it doesn't look that bad because the
zh_CN
> period character [。] already has some trailing empty space attached, so we get
a
> slightly too-wide space rather than a space where there should be none.

Sorry for the late reply and thanks for testing in various locales. 
Even though there's no side effect, I really wish we could do without adding an
extra space, but apparently not. 

BTW, "。" (Ideographic full stop) is so-called 'full-width' and its advance width
is the same as other CJK Ideographs.

[Evan Stade, 2015-10-23 17:01:19]

On 2015/10/23 06:30:19, jshin (jungshik at g) wrote:
>
https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
> File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc
> (right):
> 
>
https://codereview.chromium.org/1408273002/diff/20001/chrome/browser/ui/views...
> chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:39:
> instructions_text += base::ASCIIToUTF16(" ");
> On 2015/10/19 16:49:52, Evan Stade wrote:
> > On 2015/10/19 15:38:40, sky wrote:
> > > Why do you add a space for material? Might the space have undesirable
> effects
> > in
> > > other languages?
> > 
> > Previously this space was implemented with kViewPadding, but that didn't
> create
> > the right amount of space (especially if you change the font). I tried to
> think
> > of how this could fail in other languages. It works in RTL and LTR. In
> languages
> > without space characters it creates too much space (I just tested with
zh_CN),
> > but that was already a problem. Also it doesn't look that bad because the
> zh_CN
> > period character [。] already has some trailing empty space attached, so we
get
> a
> > slightly too-wide space rather than a space where there should be none.
> 
> Sorry for the late reply and thanks for testing in various locales. 
> Even though there's no side effect, I really wish we could do without adding
an
> extra space, but apparently not. 

You mean in Chinese et al, right? We could address this issue by making the grd
entry:

"Foo bar baz. <ph>link text</ph>"

then splicing that text up at run time, but in English and other languages with
spaces, that would leave us with "Foo bar baz. " as the label text, which is
effectively the same as the append-a-space approach that sky had misgivings
about.

[Evan Stade, 2015-10-26 22:29:07]

ping sky for patchset 6.

[sky, 2015-10-26 23:56:38]

Sorry, I must have forgot to hit send the other day.

https://codereview.chromium.org/1408273002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc
(right):

https://codereview.chromium.org/1408273002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:27: return
views::Label(base::ASCIIToUTF16(" ")).GetPreferredSize().width();
Can we cache this? We don't really support changing the fonts on the fly.

[Evan Stade, 2015-10-27 17:05:42]

https://codereview.chromium.org/1408273002/diff/100001/chrome/browser/ui/view...
File chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc
(right):

https://codereview.chromium.org/1408273002/diff/100001/chrome/browser/ui/view...
chrome/browser/ui/views/bookmarks/bookmark_bar_instructions_view.cc:27: return
views::Label(base::ASCIIToUTF16(" ")).GetPreferredSize().width();
On 2015/10/26 23:56:38, sky wrote:
> Can we cache this? We don't really support changing the fonts on the fly.

hmm, you're right. I thought we did support live font size changes but it
appears not. Done.

[sky, 2015-10-27 21:10:04]

LGTM

[Evan Stade, 2015-10-27 21:12:01]

The CQ bit was checked by estade@chromium.org

[commit-bot: I haz the power, 2015-10-27 21:12:16]

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

[commit-bot: I haz the power, 2015-10-27 21:51:40]

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

[commit-bot: I haz the power, 2015-10-27 21:51:41]

Try jobs failed on following builders:
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)

[Evan Stade, 2015-10-28 00:58:31]

The CQ bit was checked by estade@chromium.org

[Evan Stade, 2015-10-28 00:58:32]

The patchset sent to the CQ was uploaded after l-g-t-m from sky@chromium.org
Link to the patchset: https://codereview.chromium.org/1408273002/#ps140001
(title: "eschew redundancy")

[commit-bot: I haz the power, 2015-10-28 00:59:34]

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

[commit-bot: I haz the power, 2015-10-28 02:26:11]

Committed patchset #8 (id:140001)

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

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