Skip to content
This repository has been archived by the owner on Apr 3, 2020. It is now read-only.

Commit

Permalink
[Win, Linux] Always display the avatar button text at the same size.
Browse files Browse the repository at this point in the history 
The problem is that some locales (Hindi) or platforms (Linux) have a
bigger base font size for the default font (crbug.com/405994). If this
is the case, we should decrease the font size to the one we expect,
so that the font fits in the button correctly.

Just adjusting the baseline is not always enough, as the new avatar
button has a fixed size, and the text might not fit correctly
regardless of the baseline adjustments.

Screenshots: https://drive.google.com/folderview?id=0B1B1Up4p2NRMRHc5WTBxZWNrRm8&usp=sharing

BUG=403466
TEST=Start Chrome with --enable-new-avatar-menu on Linux, or with --enable-new-avatar-menu and --lang-hi on Windows. The avatar button text should fit nicely in the button.

Review URL: https://codereview.chromium.org/470053004

Cr-Commit-Position: refs/heads/master@{#292715}
  • Loading branch information
@notwaldorf
notwaldorf authored and Commit bot committed Aug 29, 2014
1 parent 65a60fc commit 899ae44
Show file tree
Hide file tree
Showing 5 changed files with 78 additions and 50 deletions.
53 changes: 3 additions & 50 deletions chrome/browser/ui/views/location_bar/location_bar_view.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,52 +110,6 @@ const gfx::Tween::Type kHideTweenType = gfx::Tween::FAST_OUT_LINEAR_IN;
// by 1 px.
const int kSearchButtonInset = 1;

// Given a containing |height| and a |base_font_list|, shrinks the font size
// until the font list will fit within |height| while having its cap height
// vertically centered. Returns the correctly-sized font list.
//
// The expected layout:
// +--------+-----------------------------------------------+------------+
// | | y offset | space |
// | +--------+-------------------+------------------+ above |
// | | | | internal leading | cap height |
// | box | font | ascent (baseline) +------------------+------------+
// | height | height | | cap height |
// | | |-------------------+------------------+------------+
// | | | descent (height - baseline) | space |
// | +--------+--------------------------------------+ below |
// | | space at bottom | cap height |
// +--------+-----------------------------------------------+------------+
// Goal:
// center of box height == center of cap height
// (i.e. space above cap height == space below cap height)
// Restrictions:
// y offset >= 0
// space at bottom >= 0
// (i.e. Entire font must be visible inside the box.)
gfx::FontList GetLargestFontListWithHeightBound(
const gfx::FontList& base_font_list,
int height) {
gfx::FontList font_list = base_font_list;
for (int font_size = font_list.GetFontSize(); font_size > 1; --font_size) {
const int internal_leading =
font_list.GetBaseline() - font_list.GetCapHeight();
// Some platforms don't support getting the cap height, and simply return
// the entire font ascent from GetCapHeight(). Centering the ascent makes
// the font look too low, so if GetCapHeight() returns the ascent, center
// the entire font height instead.
const int space =
height - ((internal_leading != 0) ?
font_list.GetCapHeight() : font_list.GetHeight());
const int y_offset = space / 2 - internal_leading;
const int space_at_bottom = height - (y_offset + font_list.GetHeight());
if ((y_offset >= 0) && (space_at_bottom >= 0))
break;
font_list = font_list.DeriveWithSizeDelta(-1);
}
return font_list;
}

int GetEditLeadingInternalSpace() {
// The textfield has 1 px of whitespace before the text in the RTL case only.
return base::i18n::IsRTL() ? 1 : 0;
Expand Down Expand Up @@ -277,16 +231,15 @@ void LocationBarView::Init() {
// Shrink large fonts to make them fit.
// TODO(pkasting): Stretch the location bar instead in this case.
const int location_height = GetInternalHeight(true);
font_list = GetLargestFontListWithHeightBound(font_list, location_height);
font_list = font_list.DeriveWithHeightUpperBound(location_height);

// Determine the font for use inside the bubbles. The bubble background
// images have 1 px thick edges, which we don't want to overlap.
const int kBubbleInteriorVerticalPadding = 1;
const int bubble_vertical_padding =
(kBubblePadding + kBubbleInteriorVerticalPadding) * 2;
const gfx::FontList bubble_font_list(
GetLargestFontListWithHeightBound(
font_list, location_height - bubble_vertical_padding));
const gfx::FontList bubble_font_list(font_list.DeriveWithHeightUpperBound(
location_height - bubble_vertical_padding));

const SkColor background_color =
GetColor(ToolbarModel::NONE, LocationBarView::BACKGROUND);
Expand Down
6 changes: 6 additions & 0 deletions chrome/browser/ui/views/profiles/new_avatar_button.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,12 @@ NewAvatarButton::NewAvatarButton(
gfx::ShadowValue(gfx::Point(), 1.0f, SK_ColorDKGRAY)));
SetTextSubpixelRenderingEnabled(false);

// The largest text height that fits in the button. If the font list height
// is larger than this, it will be shrunk to match it.
// TODO(noms): Calculate this constant algorithmically.
const int kDisplayFontHeight = 15;
SetFontList(GetFontList().DeriveWithHeightUpperBound(kDisplayFontHeight));

ui::ResourceBundle* rb = &ui::ResourceBundle::GetSharedInstance();
if (button_style == THEMED_BUTTON) {
const int kNormalImageSet[] = IMAGE_GRID(IDR_AVATAR_THEMED_BUTTON_NORMAL);
Expand Down
21 changes: 21 additions & 0 deletions ui/gfx/font_list.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,27 @@ FontList FontList::DeriveWithStyle(int font_style) const {
return Derive(0, font_style);
}

gfx::FontList FontList::DeriveWithHeightUpperBound(int height) const {
gfx::FontList font_list(*this);
for (int font_size = font_list.GetFontSize(); font_size > 1; --font_size) {
const int internal_leading =
font_list.GetBaseline() - font_list.GetCapHeight();
// Some platforms don't support getting the cap height, and simply return
// the entire font ascent from GetCapHeight(). Centering the ascent makes
// the font look too low, so if GetCapHeight() returns the ascent, center
// the entire font height instead.
const int space =
height - ((internal_leading != 0) ?
font_list.GetCapHeight() : font_list.GetHeight());
const int y_offset = space / 2 - internal_leading;
const int space_at_bottom = height - (y_offset + font_list.GetHeight());
if ((y_offset >= 0) && (space_at_bottom >= 0))
break;
font_list = font_list.DeriveWithSizeDelta(-1);
}
return font_list;
}

int FontList::GetHeight() const {
return impl_->GetHeight();
}
Expand Down
25 changes: 25 additions & 0 deletions ui/gfx/font_list.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,31 @@ class GFX_EXPORT FontList {
// values: Font::BOLD, Font::ITALIC and Font::UNDERLINE.
FontList DeriveWithStyle(int font_style) const;

// Shrinks the font size until the font list fits within |height| while
// having its cap height vertically centered. Returns a new FontList with
// the correct height.
//
// The expected layout:
// +--------+-----------------------------------------------+------------+
// | | y offset | space |
// | +--------+-------------------+------------------+ above |
// | | | | internal leading | cap height |
// | box | font | ascent (baseline) +------------------+------------+
// | height | height | | cap height |
// | | |-------------------+------------------+------------+
// | | | descent (height - baseline) | space |
// | +--------+--------------------------------------+ below |
// | | space at bottom | cap height |
// +--------+-----------------------------------------------+------------+
// Goal:
// center of box height == center of cap height
// (i.e. space above cap height == space below cap height)
// Restrictions:
// y offset >= 0
// space at bottom >= 0
// (i.e. Entire font must be visible inside the box.)
gfx::FontList DeriveWithHeightUpperBound(int height) const;

// Returns the height of this font list, which is max(ascent) + max(descent)
// for all the fonts in the font list.
int GetHeight() const;
Expand Down
23 changes: 23 additions & 0 deletions ui/gfx/font_list_unittest.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -286,4 +286,27 @@ TEST(FontListTest, Fonts_GetHeight_GetBaseline) {
font_list_mix.GetBaseline());
}

TEST(FontListTest, Fonts_DeriveWithHeightUpperBound) {
std::vector<Font> fonts;

fonts.push_back(gfx::Font("Arial", 18));
fonts.push_back(gfx::Font("Sans serif", 18));
fonts.push_back(gfx::Font("Symbol", 18));
FontList font_list = FontList(fonts);

// A smaller upper bound should derive a font list with a smaller height.
const int height_1 = font_list.GetHeight() - 5;
FontList derived_1 = font_list.DeriveWithHeightUpperBound(height_1);
EXPECT_LE(derived_1.GetHeight(), height_1);
EXPECT_LT(derived_1.GetHeight(), font_list.GetHeight());
EXPECT_LT(derived_1.GetFontSize(), font_list.GetFontSize());

// A larger upper bound should not change the height of the font list.
const int height_2 = font_list.GetHeight() + 5;
FontList derived_2 = font_list.DeriveWithHeightUpperBound(height_2);
EXPECT_LE(derived_2.GetHeight(), height_2);
EXPECT_EQ(font_list.GetHeight(), derived_2.GetHeight());
EXPECT_EQ(font_list.GetFontSize(), derived_2.GetFontSize());
}

} // namespace gfx

0 comments on commit 899ae44

Please sign in to comment.