Should be fixed on ToT with Evan's recent changes to the user row layout.

I'm not so sure, looks like hshi@ has those changes (you can see the text is the right size/style/etc)

My mistake, yes the screenshot does show your latest changes. They looked fine to me when I checked ToT earlier today (Linux build).

hshi@, is this a build on device (if so, which device) or a Linux build?

re:#4 this is on samus

Actually I did some bisecting and it looks like this problem started right at the CL that made the user-row adjustments.

This is the CL I'm referring to: https://chromium.googlesource.com/chromium/src/+/87842b52fc6b0a6343afeffc8a07307849d1e98c

right, I'm not surprised, but I don't know how to debug this if I can't repro it. Clearly we're somehow miscalculating the required width for the button, but in my local build we're giving it the exact amount it asks for. Are the fonts somehow changing after the layout has taken place? Is there a rounding error in RenderText that makes a centered label one pixel too far to the right (triggering the ellipsis)?

This is a LabelButton which I know +msw has dealt with in the past, perhaps he has some insight. Mike, have you seen this before and do you have any clues what could cause it? (you can see how it's set up here --- https://cs.chromium.org/chromium/src/ash/common/system/user/user_view.cc?rcl=1479284732&l=422 --- the text is centered and the button is sized to its preferred width)

It's hard to tell which border this button is using, looks like one of three options:
- TrayPopupLabelButtonBorder
- CreateEmptyBorder(gfx::Insets(0, kHorizontalPadding))
- TrayPopupLabelButtonBorder

Regardless, it looks like UserView::Layout is giving it the preferred size; I don't spot a logic error.
If it consistently repros on a device, you could follow up there, I doubt it's a LabelButton defect.

Hi estade@ --- do you have a device (samus or some other HiDPI CrOS device) to test this?

I have attempted to go back to the previous version of user_view.cc and user_card_view.* but they do not seem to make any difference.

*HOWEVER* if I just revert your change in ash/common/system/tray/tray_popup_utils.cc (see https://codereview.chromium.org/2498293003/diff/80001/ash/common/system/tray/tray_popup_utils.cc) then the "Sign Out" button becomes normal.

I hope this information helps.

that does help narrow it down, and it's not entirely surprising --- it seems like maybe the button is being sized based on the normal font, not the medium font. I would suspect it's an issue with the fonts on your device rather than it being hidpi.

Still, when I read through the code it seems like it should be resetting the size appropriately, so it's hard to remotely diagnose (no, I don't have one of the devices you mention).

>> it seems like maybe the button is being sized based on the
>> normal font, not the medium font

You're probably right. If I use the normal font style (TrayPopupItemStyle::FontStyle::CAPTION) then the button is sized correctly.

diff --git a/ash/common/system/tray/tray_popup_utils.cc b/ash/common/system/tray/tray_popup_utils.cc
index b36733f..0dc0406 100644
--- a/ash/common/system/tray/tray_popup_utils.cc
+++ b/ash/common/system/tray/tray_popup_utils.cc
@@ -112,7 +112,7 @@ class BorderlessLabelButton : public views::LabelButton {
       set_ink_drop_visible_opacity(kTrayPopupInkDropRippleOpacity);
       const int kHorizontalPadding = 8;
       SetBorder(views::CreateEmptyBorder(gfx::Insets(0, kHorizontalPadding)));
-      TrayPopupItemStyle style(nullptr, TrayPopupItemStyle::FontStyle::BUTTON);
+      TrayPopupItemStyle style(nullptr, TrayPopupItemStyle::FontStyle::CAPTION);
       style.SetupLabel(label());
       // TODO(tdanderson): Update focus rect for material design. See
       // crbug.com/615892

Here's what "normal" font style looks like.

Deleted: Screenshot 2016-11-18 at 1.31.50 PM.png 62.1 KB

	Screenshot 2016-11-18 at 1.31.50 PM.png 62.1 KB View Download

There seems to be really a bug with the label layout. When we set the label font style and then update the preferred size, the size is calculated correctly, but re-layout didn't really take effect.

(1) The RenderTextHarfBuzz object "Sign out" for this label initially reports GetStringSize() as gfx::Size(44, 15) (the actual floating-point width is 43.x) for the default font weight.
(2) Upon style.SetupLabel() call with TrayPopupItemStyle::FontStyle::BUTTON, we get a call to Label::ResetLayout() where PreferredSizeChanged() is called, and RenderTextHarfBuzz::GetStringSize() returns the new size gfx::Size(45, 15) (actual floating point width is 44.xx), corresponding to the MEDIUM weight.

Experiment A: If I force the size in (1) to (45, 15), then the text will not be elided.
Experiment B: If I force the size in (2) to a much wider value, say even (50, 15), the text will still be elided.

Based on A and B it seems the PreferredSizeChanged() in (2) results in the correct preferred size, but it had no effect on the relayout.

can you try removing the SetHorizontalAlignment part? I'm going to wager that fixes it.

re:#14 sorry it didn't work. I tried to comment out every render_text_->SetHorizontalAlignment() call in ui/views/controls/label.cc and it doesn't make any difference.

Like I said in #13, I really don't think it is a matter of CENTER vs LEFT horizontal alignmentment issue. I could make RenderTextHarfBuzz to return 20 extra pixels width for MEDIUM weight label, and it still won't fix the "Sign out" button.

Evan -- I found a way for you to reproduce this without an actual device. Just modify tray_popup_item_style.cc to use the heaviest weight:

diff --git a/ash/common/system/tray/tray_popup_item_style.cc b/ash/common/system/tray/tray_popup_item_style.cc
index 0921d4e..76ee711 100644
--- a/ash/common/system/tray/tray_popup_item_style.cc
+++ b/ash/common/system/tray/tray_popup_item_style.cc
@@ -114,7 +114,7 @@ void TrayPopupItemStyle::SetupLabel(views::Label* label) const {
       break;
     case FontStyle::BUTTON:
       label->SetFontList(base_font_list.Derive(0, gfx::Font::NORMAL,
-                                               gfx::Font::Weight::MEDIUM));
+                                               gfx::Font::Weight::BLACK));
       break;
   }
 }

This will cause the label text to get elided even on 1x display and you can test this on your Linux workstation.

awesome, thanks!

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/27ec4f8bcae25e024776f5f97390f768b5b47f49

commit 27ec4f8bcae25e024776f5f97390f768b5b47f49
Author: estade <estade@chromium.org>
Date: Mon Nov 21 22:19:38 2016

Fix LabelButton size calculation for non-default fonts.

BUG= 666479 

Review-Url: https://codereview.chromium.org/2514163002
Cr-Commit-Position: refs/heads/master@{#433670}

[modify] https://crrev.com/27ec4f8bcae25e024776f5f97390f768b5b47f49/ui/views/controls/button/label_button.cc

 Issue 667509  has been merged into this issue.

Your change meets the bar and is auto-approved for M56 (branch: 2924)

Verified in 57.0.2933.0 canary. Performing the merge now.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/39088f4ac060e821a956da7ab8ca8b674f0d2430

commit 39088f4ac060e821a956da7ab8ca8b674f0d2430
Author: Terry Anderson <tdanderson@chromium.org>
Date: Mon Nov 28 19:59:04 2016

Fix LabelButton size calculation for non-default fonts.

BUG= 666479 

Review-Url: https://codereview.chromium.org/2514163002
Cr-Commit-Position: refs/heads/master@{#433670}
(cherry picked from commit 27ec4f8bcae25e024776f5f97390f768b5b47f49)

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

Cr-Commit-Position: refs/branch-heads/2924@{#122}
Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059}

[modify] https://crrev.com/39088f4ac060e821a956da7ab8ca8b674f0d2430/ui/views/controls/button/label_button.cc

Verified on ChromeOS 9000.11.0, 56.0.2924.11