Issue 2801583002: Use views::style for buttons, bootstrap ash_typography to do so.

tapted

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

3 years, 8 months ago (2017-04-05 07:46:31 UTC) #1

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/1

3 years, 8 months ago (2017-04-05 07:46:47 UTC) #2

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-05 07:48:40 UTC) #3

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds/184175) ios-device-xcode-clang on ...

3 years, 8 months ago (2017-04-05 07:48:41 UTC) #4

tapted

Description was changed from ========== cl format Do BUG= ========== to ========== Use views::style for ...

3 years, 8 months ago (2017-04-05 07:49:01 UTC) #5

tapted

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

3 years, 8 months ago (2017-04-06 03:52:24 UTC) #6

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/60001

3 years, 8 months ago (2017-04-06 03:52:42 UTC) #7

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-06 04:02:12 UTC) #8

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/341576)

3 years, 8 months ago (2017-04-06 04:02:13 UTC) #9

tapted

Description was changed from ========== Use views::style for MDTextButton and Touch-Selection Menu Buttons This makes ...

3 years, 8 months ago (2017-04-06 04:02:42 UTC) #10

tapted

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

3 years, 8 months ago (2017-04-06 04:06:05 UTC) #11

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/80001

3 years, 8 months ago (2017-04-06 04:06:13 UTC) #12

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-06 04:20:43 UTC) #13

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_chromium_compile_only_ng/builds/313521) linux_chromium_chromeos_rel_ng on ...

3 years, 8 months ago (2017-04-06 04:20:43 UTC) #14

tapted

Description was changed from ========== Use views::style for MDTextButton and Touch-Selection Menu Buttons This makes ...

3 years, 8 months ago (2017-04-06 05:26:32 UTC) #15

tapted

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

3 years, 8 months ago (2017-04-06 05:34:47 UTC) #16

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/100001

3 years, 8 months ago (2017-04-06 05:34:57 UTC) #17

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-06 05:55:33 UTC) #18

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/313688) linux_chromium_chromeos_ozone_rel_ng on ...

3 years, 8 months ago (2017-04-06 05:55:34 UTC) #19

tapted

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

3 years, 8 months ago (2017-04-06 09:54:10 UTC) #20

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/110015

3 years, 8 months ago (2017-04-06 09:54:32 UTC) #21

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-06 10:27:55 UTC) #22

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_chromeos_ozone_rel_ng/builds/356324)

3 years, 8 months ago (2017-04-06 10:27:56 UTC) #23

tapted

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

3 years, 8 months ago (2017-04-07 06:03:46 UTC) #24

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/130001

3 years, 8 months ago (2017-04-07 06:03:56 UTC) #25

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-07 06:40:48 UTC) #26

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_ng/builds/424456)

3 years, 8 months ago (2017-04-07 06:40:49 UTC) #27

tapted

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

3 years, 8 months ago (2017-04-07 07:14:32 UTC) #28

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/190001

3 years, 8 months ago (2017-04-07 07:14:54 UTC) #29

tapted

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

3 years, 8 months ago (2017-04-07 07:27:36 UTC) #32

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/210001

3 years, 8 months ago (2017-04-07 07:27:58 UTC) #33

tapted

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

3 years, 8 months ago (2017-04-07 07:36:29 UTC) #34

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/230001

3 years, 8 months ago (2017-04-07 07:36:49 UTC) #35

tapted

Description was changed from ========== Use views::style for MDTextButton and Touch-Selection Menu Buttons This makes ...

3 years, 8 months ago (2017-04-07 07:39:25 UTC) #36

tapted

tapted@chromium.org changed reviewers: + pkasting@chromium.org

3 years, 8 months ago (2017-04-07 07:39:26 UTC) #37

tapted

Description was changed from ========== Use views::style for buttons, bootstrap ash_typography to do so. This ...

3 years, 8 months ago (2017-04-07 07:40:02 UTC) #38

tapted

Hi Peter, please take a look. I think this is ready for review, but I'll ...

3 years, 8 months ago (2017-04-07 07:49:52 UTC) #42

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-07 09:25:47 UTC) #43

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 8 months ago (2017-04-07 09:25:48 UTC) #44

Peter Kasting

Do we have any opportunity to reduce the number of new contexts by making small ...

3 years, 8 months ago (2017-04-08 01:38:54 UTC) #45

tapted

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

3 years, 8 months ago (2017-04-10 05:54:14 UTC) #46

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/250001

3 years, 8 months ago (2017-04-10 05:54:27 UTC) #47

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-10 05:56:56 UTC) #48

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/builds/190258) ios-simulator-xcode-clang on ...

3 years, 8 months ago (2017-04-10 05:56:57 UTC) #49

tapted

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

3 years, 8 months ago (2017-04-10 06:34:23 UTC) #50

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/270001

3 years, 8 months ago (2017-04-10 06:34:34 UTC) #51

tapted

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

3 years, 8 months ago (2017-04-10 06:35:59 UTC) #52

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/290001

3 years, 8 months ago (2017-04-10 06:36:11 UTC) #53

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-10 06:50:44 UTC) #55

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/315605)

3 years, 8 months ago (2017-04-10 06:50:45 UTC) #56

tapted

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

3 years, 8 months ago (2017-04-10 07:11:34 UTC) #58

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/310001

3 years, 8 months ago (2017-04-10 07:11:55 UTC) #59

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-10 08:17:51 UTC) #60

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 8 months ago (2017-04-10 08:17:52 UTC) #61

tapted

On 2017/04/08 01:38:54, Peter Kasting wrote: > Do we have any opportunity to reduce the ...

3 years, 8 months ago (2017-04-10 12:27:33 UTC) #62

On 2017/04/08 01:38:54, Peter Kasting wrote:
> Do we have any opportunity to reduce the number of new contexts by making
small
> changes to the existing behavior?  This would also allow reducing the number
of
> distinct font sizes in use in the UI.

I think I can get rid of the MD button context by making some assumptions (done
- see comments).

The others (CONTEXT_TOUCH_MENU, ash::CONTEXT_LAUNCHER_BUTTON and
ash::CONTEXT_TOAST_OVERLAY are not behind any flags and in-use on Ash they're
not related to dialogs/Harmony so I don't think changes should be rolled into
this CL. Note there's ash::TrayPopupItemStyle::FontStyle which can be folded
into ash_typography.h in a follow-up too -- ash::CONTEXT_LAUNCHER_BUTTON may
overlap the enums there but isn't a direct match to any currently (maybe with
some assumptions..).

CONTEXT_TOAST_OVERLAY is coincidentally the same as style::CONTEXT_DIALOG_TITLE,
but that doesn't seem a good fit either.

https://codereview.chromium.org/2801583002/diff/230001/ash/common/ash_typogra...
File ash/common/ash_typography.h (right):

https://codereview.chromium.org/2801583002/diff/230001/ash/common/ash_typogra...
ash/common/ash_typography.h:26: enum AshTextStyle {
On 2017/04/08 01:38:54, Peter Kasting wrote:
> Do you need to add this enum at all?  It seems like someone who needed to add
> this later could figure out how to do so.

removed.

https://codereview.chromium.org/2801583002/diff/230001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right):

https://codereview.chromium.org/2801583002/diff/230001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_typography_provider.cc:51:
ash::GetDefaultAshFont(text_context, text_style, &size_delta, &font_weight);
On 2017/04/08 01:38:54, Peter Kasting wrote:
> Nit: I liked doing this above the switch, as you did in chrome_typography.cc,
> better

Done.

https://codereview.chromium.org/2801583002/diff/230001/chrome/browser/ui/view...
File chrome/browser/ui/views/profiles/new_avatar_button.cc (right):

https://codereview.chromium.org/2801583002/diff/230001/chrome/browser/ui/view...
chrome/browser/ui/views/profiles/new_avatar_button.cc:78:
label()->font_list().DeriveWithHeightUpperBound(kDisplayFontHeight));
On 2017/04/08 01:38:54, Peter Kasting wrote:
> On 2017/04/07 07:49:52, tapted wrote:
> > This `DeriveWithHeightUpperBound` method is horrible...
> 
> Long-term, anywhere that uses this should really probably be placing the text
> inside an object that can auto-expand vertically.

Acknowledged.

https://codereview.chromium.org/2801583002/diff/230001/chrome/browser/ui/view...
File chrome/browser/ui/views/toolbar/app_menu.cc (right):

https://codereview.chromium.org/2801583002/diff/230001/chrome/browser/ui/view...
chrome/browser/ui/views/toolbar/app_menu.cc:262:
SetHorizontalAlignment(gfx::ALIGN_CENTER);
On 2017/04/08 01:38:54, Peter Kasting wrote:
> On 2017/04/07 07:49:52, tapted wrote:
> > (I wonder if horizontal alignment should be part of the typography spec
> too...)
> 
> My instinct is no, but not sure.

Acknowledged.

https://codereview.chromium.org/2801583002/diff/230001/ui/views/controls/butt...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/2801583002/diff/230001/ui/views/controls/butt...
ui/views/controls/button/label_button.cc:50: style::GetFont(button_context,
style::STYLE_DEFAULT_DIALOG_BUTTON)),
On 2017/04/08 01:38:54, Peter Kasting wrote:
> This isn't always bold, so does naming the variable "bold" make sense? 
> |cached_default_button_font_list_|?

Done.

https://codereview.chromium.org/2801583002/diff/230001/ui/views/controls/butt...
ui/views/controls/button/label_button.cc:469:
cached_normal_font_list_.GetFontSize());
On 2017/04/08 01:38:54, Peter Kasting wrote:
> Nit: (expected, actual)

Done.

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
ui/views/style/typography.h:41: CONTEXT_BUTTON_MD,
On 2017/04/08 01:38:54, Peter Kasting wrote:
> I don't think I understand why we have to introduce this distinctly in the
base
> class instead of letting the harmony provider override the meaning of
> CONTEXT_BUTTON.

The problem is that CONTEXT_BUTTON would also used for things like Checkbox (and
15 other subclasses of LabelButton).

But I think we can default to CONTEXT_LABEL in place of CONTEXT_BUTTON pretty
safely. (i.e. make that default: only pass CONTEXT_BUTTON in places where this
CL was passing CONTEXT_BUTTON_MD).

I considered whether we should use CONTEXT_BUTTON for LabelButtons created by
MdTextButton::CreateSecondaryUIButton() and decided "no". This is because MD
Browser UI (e.g. the download shelf) already uses MdTextButton::Create(..)
directly, even when --secondary-ui-md is DISABLED. The non-Harmony typography
provider needs a way to distinguish between MD and non-MD buttons when it sees
STYLE_DEFAULT_DIALOG_BUTTON since MD buttons should never "increase" in
boldness.

We may still be able to make the assumption that MdTextButton should only ever
be combined with STYLE_DEFAULT_DIALOG_BUTTON when --secondary-ui-md is enabled
(in which case the HarmonyTypographyProvider will just ignore it). But that
feels more risky, and MdTextButton::CreateSecondaryUIButton() will eventually
never create a LabelButton anyway.

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
ui/views/style/typography.h:81: STYLE_DEFAULT_DIALOG_BUTTON,
On 2017/04/08 01:38:54, Peter Kasting wrote:
> Nit: I'm inclined to name this STYLE_BUTTON_DIALOG_DEFAULT in keeping with
e.g.
> STYLE_TAB_XYZ.

Done. I caved in and sorted some stuff too ;)

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
File ui/views/style/typography_provider.h (right):

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
ui/views/style/typography_provider.h:36: // returns NORMAL, if the NORMAL font
is at least as bold as |weight|.
On 2017/04/08 01:38:54, Peter Kasting wrote:
> I'm confused.  There are fonts where asking for a lower weight (e.g. 400) gets
> you a bolder font than asking for a higher weight (e.g. 500)?

Yup.

The windows font settings have a "Bold font" checkbox. Checking this will put a
bold font into ResourceBundle when asking for gfx::Weight::NORMAL. Fonts at a
given *size* are derived from the unstyled font at the normal size, so this
boldness is preserved when changing _size_.

However, deriving a Weight::MEDIUM font from a bold font (i.e. the one in the
NORMAL slot) will cause the weight to decrease.

https://codereview.chromium.org/2801583002/diff/310001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right):

https://codereview.chromium.org/2801583002/diff/310001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_typography_provider.cc:120: case
views::style::CONTEXT_DIALOG_BUTTON:
There may still be a downside here to having LabelButton using CONTEXT_LABEL
rather than CONTEXT_DIALOG_BUTTON :/

GetLineHeight() can no longer distinguish LabelButton (and its subclasses) from
views::Label, and so it may provide a line spacing here that buttons never want.
E.g. this could change the minimum height a LabelButton can have, since its
label will now always want to have the harmony-specified leading/line-height.

Peter Kasting

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typography.h File ui/views/style/typography.h (right): https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typography.h#newcode41 ui/views/style/typography.h:41: CONTEXT_BUTTON_MD, On 2017/04/10 12:27:33, tapted wrote: > On 2017/04/08 ...

3 years, 8 months ago (2017-04-10 22:56:02 UTC) #63

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
ui/views/style/typography.h:41: CONTEXT_BUTTON_MD,
On 2017/04/10 12:27:33, tapted wrote:
> On 2017/04/08 01:38:54, Peter Kasting wrote:
> > I don't think I understand why we have to introduce this distinctly in the
> base
> > class instead of letting the harmony provider override the meaning of
> > CONTEXT_BUTTON.
> 
> The problem is that CONTEXT_BUTTON would also used for things like Checkbox
(and
> 15 other subclasses of LabelButton).
> 
> But I think we can default to CONTEXT_LABEL in place of CONTEXT_BUTTON pretty
> safely. (i.e. make that default: only pass CONTEXT_BUTTON in places where this
> CL was passing CONTEXT_BUTTON_MD).
> 
> I considered whether we should use CONTEXT_BUTTON for LabelButtons created by
> MdTextButton::CreateSecondaryUIButton() and decided "no". This is because MD
> Browser UI (e.g. the download shelf) already uses MdTextButton::Create(..)
> directly, even when --secondary-ui-md is DISABLED. The non-Harmony typography
> provider needs a way to distinguish between MD and non-MD buttons when it sees
> STYLE_DEFAULT_DIALOG_BUTTON since MD buttons should never "increase" in
> boldness.
> 
> We may still be able to make the assumption that MdTextButton should only ever
> be combined with STYLE_DEFAULT_DIALOG_BUTTON when --secondary-ui-md is enabled
> (in which case the HarmonyTypographyProvider will just ignore it). But that
> feels more risky, and MdTextButton::CreateSecondaryUIButton() will eventually
> never create a LabelButton anyway.

But the downside of this is that these are now the buttons which don't get a 0
line height, right?  (See your comment elsewhere.)

If so, that seems worse than the downsides you describe here.  It also seems
less semantically correct: the buttons you describe really are more "button
context" than "label context" in principle, and if we can't handle the
consequences of that, we should cave in and temporarily add the third context
value like you did at first.

What is so bad about letting MD buttons increase in boldness pre-Harmony? 
That's the pre-MD way of indicating a default button.  Do you have some
comparison screenshots so I can see how the two choices there would look?

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
File ui/views/style/typography_provider.h (right):

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
ui/views/style/typography_provider.h:36: // returns NORMAL, if the NORMAL font
is at least as bold as |weight|.
On 2017/04/10 12:27:33, tapted wrote:
> On 2017/04/08 01:38:54, Peter Kasting wrote:
> > I'm confused.  There are fonts where asking for a lower weight (e.g. 400)
gets
> > you a bolder font than asking for a higher weight (e.g. 500)?
> 
> Yup.
> 
> The windows font settings have a "Bold font" checkbox. Checking this will put
a
> bold font into ResourceBundle when asking for gfx::Weight::NORMAL. Fonts at a
> given *size* are derived from the unstyled font at the normal size, so this
> boldness is preserved when changing _size_.
> 
> However, deriving a Weight::MEDIUM font from a bold font (i.e. the one in the
> NORMAL slot) will cause the weight to decrease.

OK, so effectively, the "bold font" checkbox really means "when someone calls
the font APIs requesting NORMAL, return something that's really BOLD; for any
other weight, just do what they asked"?

If so, (a) thanks Microsoft, that's a great way to implement this :P, and (b)
can you make the comments more specific to describe this particular scenario,
rather than leaving them sort of vague and general as today?

https://codereview.chromium.org/2801583002/diff/310001/chrome/browser/ui/view...
File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right):

https://codereview.chromium.org/2801583002/diff/310001/chrome/browser/ui/view...
chrome/browser/ui/views/harmony/harmony_typography_provider.cc:120: case
views::style::CONTEXT_DIALOG_BUTTON:
On 2017/04/10 12:27:33, tapted wrote:
> There may still be a downside here to having LabelButton using CONTEXT_LABEL
> rather than CONTEXT_DIALOG_BUTTON :/
> 
> GetLineHeight() can no longer distinguish LabelButton (and its subclasses)
from
> views::Label, and so it may provide a line spacing here that buttons never
want.
> E.g. this could change the minimum height a LabelButton can have, since its
> label will now always want to have the harmony-specified leading/line-height.

Yeah, it seems desirable to return 0 here for LabelButton.

tapted

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

3 years, 8 months ago (2017-04-10 23:59:15 UTC) #64

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/350001

3 years, 8 months ago (2017-04-11 00:00:08 UTC) #65

tapted

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

3 years, 8 months ago (2017-04-11 01:56:49 UTC) #67

tapted

+sky for src/ash and ui/views OWNERS. And any other thoughts :). thanks! https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typography.h File ui/views/style/typography.h ...

3 years, 8 months ago (2017-04-11 01:56:50 UTC) #68

+sky for src/ash and ui/views OWNERS. And any other thoughts :). thanks!

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
ui/views/style/typography.h:41: CONTEXT_BUTTON_MD,
On 2017/04/10 22:56:02, Peter Kasting wrote:
> On 2017/04/10 12:27:33, tapted wrote:
> > On 2017/04/08 01:38:54, Peter Kasting wrote:
> > > I don't think I understand why we have to introduce this distinctly in the
> > base
> > > class instead of letting the harmony provider override the meaning of
> > > CONTEXT_BUTTON.
> > 
> > The problem is that CONTEXT_BUTTON would also used for things like Checkbox
> (and
> > 15 other subclasses of LabelButton).
> > 
> > But I think we can default to CONTEXT_LABEL in place of CONTEXT_BUTTON
pretty
> > safely. (i.e. make that default: only pass CONTEXT_BUTTON in places where
this
> > CL was passing CONTEXT_BUTTON_MD).
> > 
> > I considered whether we should use CONTEXT_BUTTON for LabelButtons created
by
> > MdTextButton::CreateSecondaryUIButton() and decided "no". This is because MD
> > Browser UI (e.g. the download shelf) already uses MdTextButton::Create(..)
> > directly, even when --secondary-ui-md is DISABLED. The non-Harmony
typography
> > provider needs a way to distinguish between MD and non-MD buttons when it
sees
> > STYLE_DEFAULT_DIALOG_BUTTON since MD buttons should never "increase" in
> > boldness.
> > 
> > We may still be able to make the assumption that MdTextButton should only
ever
> > be combined with STYLE_DEFAULT_DIALOG_BUTTON when --secondary-ui-md is
enabled
> > (in which case the HarmonyTypographyProvider will just ignore it). But that
> > feels more risky, and MdTextButton::CreateSecondaryUIButton() will
eventually
> > never create a LabelButton anyway.
> 
> But the downside of this is that these are now the buttons which don't get a 0
> line height, right?  (See your comment elsewhere.)

Yup. Have restored the separate MD context. I agree the CONTEXT_LABEL approach
didn't feel as correct semantically either.

 
> What is so bad about letting MD buttons increase in boldness pre-Harmony? 
> That's the pre-MD way of indicating a default button.  Do you have some
> comparison screenshots so I can see how the two choices there would look?

This was just a case of MdTextButton::CreateSecondaryUiButton() having to do
"something reasonable" when it lacked CONTEXT_BUTTON_MD. Now it has that, we
don't have to worry about tradeoffs there.

And I don't think any of the places that MdTextButton is _currently_ used when
--secondary-ui-md is NOT enabled will ever be "default" buttons. But it's an
obscure thing to have to reason about. Semantics of
CONTEXT_BUTTON+CONTEXT_BUTTON_MD are much clearer.

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
File ui/views/style/typography_provider.h (right):

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
ui/views/style/typography_provider.h:36: // returns NORMAL, if the NORMAL font
is at least as bold as |weight|.
On 2017/04/10 22:56:02, Peter Kasting wrote:
> On 2017/04/10 12:27:33, tapted wrote:
> > On 2017/04/08 01:38:54, Peter Kasting wrote:
> > > I'm confused.  There are fonts where asking for a lower weight (e.g. 400)
> gets
> > > you a bolder font than asking for a higher weight (e.g. 500)?
> > 
> > Yup.
> > 
> > The windows font settings have a "Bold font" checkbox. Checking this will
put
> a
> > bold font into ResourceBundle when asking for gfx::Weight::NORMAL. Fonts at
a
> > given *size* are derived from the unstyled font at the normal size, so this
> > boldness is preserved when changing _size_.
> > 
> > However, deriving a Weight::MEDIUM font from a bold font (i.e. the one in
the
> > NORMAL slot) will cause the weight to decrease.
> 
> OK, so effectively, the "bold font" checkbox really means "when someone calls
> the font APIs requesting NORMAL, return something that's really BOLD; for any
> other weight, just do what they asked"?

It's not quite that bad :). The logic for this is in

PlatformFontWin::HFontRef* PlatformFontWin::GetBaseFontRef() 

It calls base::win::GetNonClientMetrics(..) and creates a gfx::Font using the
HFONT that the WinAPI puts into NONCLIENTMETRICS_XP::lfMessageFont

> If so, (a) thanks Microsoft, that's a great way to implement this :P, and (b)
> can you make the comments more specific to describe this particular scenario,
> rather than leaving them sort of vague and general as today?

Done.

Peter Kasting

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typography.h File ui/views/style/typography.h (right): https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typography.h#newcode41 ui/views/style/typography.h:41: CONTEXT_BUTTON_MD, On 2017/04/11 01:56:50, tapted wrote: > On 2017/04/10 ...

3 years, 8 months ago (2017-04-11 02:24:20 UTC) #69

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
ui/views/style/typography.h:41: CONTEXT_BUTTON_MD,
On 2017/04/11 01:56:50, tapted wrote:
> On 2017/04/10 22:56:02, Peter Kasting wrote:
> > On 2017/04/10 12:27:33, tapted wrote:
> > > On 2017/04/08 01:38:54, Peter Kasting wrote:
> > > > I don't think I understand why we have to introduce this distinctly in
the
> > > base
> > > > class instead of letting the harmony provider override the meaning of
> > > > CONTEXT_BUTTON.
> > > 
> > > The problem is that CONTEXT_BUTTON would also used for things like
Checkbox
> > (and
> > > 15 other subclasses of LabelButton).
> > > 
> > > But I think we can default to CONTEXT_LABEL in place of CONTEXT_BUTTON
> pretty
> > > safely. (i.e. make that default: only pass CONTEXT_BUTTON in places where
> this
> > > CL was passing CONTEXT_BUTTON_MD).
> > > 
> > > I considered whether we should use CONTEXT_BUTTON for LabelButtons created
> by
> > > MdTextButton::CreateSecondaryUIButton() and decided "no". This is because
MD
> > > Browser UI (e.g. the download shelf) already uses MdTextButton::Create(..)
> > > directly, even when --secondary-ui-md is DISABLED. The non-Harmony
> typography
> > > provider needs a way to distinguish between MD and non-MD buttons when it
> sees
> > > STYLE_DEFAULT_DIALOG_BUTTON since MD buttons should never "increase" in
> > > boldness.
> > > 
> > > We may still be able to make the assumption that MdTextButton should only
> ever
> > > be combined with STYLE_DEFAULT_DIALOG_BUTTON when --secondary-ui-md is
> enabled
> > > (in which case the HarmonyTypographyProvider will just ignore it). But
that
> > > feels more risky, and MdTextButton::CreateSecondaryUIButton() will
> eventually
> > > never create a LabelButton anyway.
> > 
> > But the downside of this is that these are now the buttons which don't get a
0
> > line height, right?  (See your comment elsewhere.)
> 
> Yup. Have restored the separate MD context. I agree the CONTEXT_LABEL approach
> didn't feel as correct semantically either.
> 
>  
> > What is so bad about letting MD buttons increase in boldness pre-Harmony? 
> > That's the pre-MD way of indicating a default button.  Do you have some
> > comparison screenshots so I can see how the two choices there would look?
> 
> This was just a case of MdTextButton::CreateSecondaryUiButton() having to do
> "something reasonable" when it lacked CONTEXT_BUTTON_MD. Now it has that, we
> don't have to worry about tradeoffs there.

Right, but if we can eliminate that behavioral special-case, can we eliminate
having CONTEXT_BUTTON_MD versus CONTEXT_BUTTON?  If so that's even better.

If not, what's the long-term plan?  Can we combine these once Harmony ships?  In
the meantime, can we comment more clearly at the declaration here what the
practical reason is for the distinction and when it will be safe to remove?

I'm also wondering if CONTEXT_BUTTON_TEXT_MD or something else more distinctive
might discourage people from using this widely -- it sort of sounds from the
name like CONTEXT_BUTTON_MD is the shiny new way of doing all buttons.

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
File ui/views/style/typography_provider.h (right):

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
ui/views/style/typography_provider.h:36: // returns NORMAL, if the NORMAL font
is at least as bold as |weight|.
On 2017/04/11 01:56:50, tapted wrote:
> On 2017/04/10 22:56:02, Peter Kasting wrote:
> > On 2017/04/10 12:27:33, tapted wrote:
> > > On 2017/04/08 01:38:54, Peter Kasting wrote:
> > > > I'm confused.  There are fonts where asking for a lower weight (e.g.
400)
> > gets
> > > > you a bolder font than asking for a higher weight (e.g. 500)?
> > > 
> > > Yup.
> > > 
> > > The windows font settings have a "Bold font" checkbox. Checking this will
> put
> > a
> > > bold font into ResourceBundle when asking for gfx::Weight::NORMAL. Fonts
at
> a
> > > given *size* are derived from the unstyled font at the normal size, so
this
> > > boldness is preserved when changing _size_.
> > > 
> > > However, deriving a Weight::MEDIUM font from a bold font (i.e. the one in
> the
> > > NORMAL slot) will cause the weight to decrease.
> > 
> > OK, so effectively, the "bold font" checkbox really means "when someone
calls
> > the font APIs requesting NORMAL, return something that's really BOLD; for
any
> > other weight, just do what they asked"?
> 
> It's not quite that bad :). The logic for this is in
> 
> PlatformFontWin::HFontRef* PlatformFontWin::GetBaseFontRef() 
> 
> It calls base::win::GetNonClientMetrics(..) and creates a gfx::Font using the
> HFONT that the WinAPI puts into NONCLIENTMETRICS_XP::lfMessageFont

OK, so the default metrics say e.g. "use weight 600", and so the issue is that
if we ask PlatformFont for "normal", it doesn't actually force the weight, it
just uses the base font, so we get bold, but if we ask for a specific weight, it
writes that into the metrics, and then we get that?

If so it sounds to me like PlatformFont should take care of this -- if we ask
for something bolder than NORMAL, but we're deriving it on a font where the base
metrics would already have given a higher weight to the NORMAL case, then we
should clamp the requested weight to at least that weight.

Doing it in the typography provider seems a bit too high-level.  Or long run
does it not matter because no one uses PlatformFont directly and everyone goes
through the typography provider?  Though even then it seems like this should
happen there, I think?

commit-bot: I haz the power

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

3 years, 8 months ago (2017-04-11 02:26:33 UTC) #70

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 8 months ago (2017-04-11 02:26:34 UTC) #71

tapted

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typography.h File ui/views/style/typography.h (right): https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typography.h#newcode41 ui/views/style/typography.h:41: CONTEXT_BUTTON_MD, On 2017/04/11 02:24:20, Peter Kasting wrote: > On ...

3 years, 8 months ago (2017-04-12 07:31:48 UTC) #72

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
ui/views/style/typography.h:41: CONTEXT_BUTTON_MD,
On 2017/04/11 02:24:20, Peter Kasting wrote:
> On 2017/04/11 01:56:50, tapted wrote:
> > On 2017/04/10 22:56:02, Peter Kasting wrote:
> > > On 2017/04/10 12:27:33, tapted wrote:
> > > > On 2017/04/08 01:38:54, Peter Kasting wrote:
> > > > > I don't think I understand why we have to introduce this distinctly in
> the
> > > > base
> > > > > class instead of letting the harmony provider override the meaning of
> > > > > CONTEXT_BUTTON.
> > > > 
> > > > The problem is that CONTEXT_BUTTON would also used for things like
> Checkbox
> > > (and
> > > > 15 other subclasses of LabelButton).
> > > > 
> > > > But I think we can default to CONTEXT_LABEL in place of CONTEXT_BUTTON
> > pretty
> > > > safely. (i.e. make that default: only pass CONTEXT_BUTTON in places
where
> > this
> > > > CL was passing CONTEXT_BUTTON_MD).
> > > > 
> > > > I considered whether we should use CONTEXT_BUTTON for LabelButtons
created
> > by
> > > > MdTextButton::CreateSecondaryUIButton() and decided "no". This is
because
> MD
> > > > Browser UI (e.g. the download shelf) already uses
MdTextButton::Create(..)
> > > > directly, even when --secondary-ui-md is DISABLED. The non-Harmony
> > typography
> > > > provider needs a way to distinguish between MD and non-MD buttons when
it
> > sees
> > > > STYLE_DEFAULT_DIALOG_BUTTON since MD buttons should never "increase" in
> > > > boldness.
> > > > 
> > > > We may still be able to make the assumption that MdTextButton should
only
> > ever
> > > > be combined with STYLE_DEFAULT_DIALOG_BUTTON when --secondary-ui-md is
> > enabled
> > > > (in which case the HarmonyTypographyProvider will just ignore it). But
> that
> > > > feels more risky, and MdTextButton::CreateSecondaryUIButton() will
> > eventually
> > > > never create a LabelButton anyway.
> > > 
> > > But the downside of this is that these are now the buttons which don't get
a
> 0
> > > line height, right?  (See your comment elsewhere.)
> > 
> > Yup. Have restored the separate MD context. I agree the CONTEXT_LABEL
approach
> > didn't feel as correct semantically either.
> > 
> >  
> > > What is so bad about letting MD buttons increase in boldness pre-Harmony? 
> > > That's the pre-MD way of indicating a default button.  Do you have some
> > > comparison screenshots so I can see how the two choices there would look?
> > 
> > This was just a case of MdTextButton::CreateSecondaryUiButton() having to do
> > "something reasonable" when it lacked CONTEXT_BUTTON_MD. Now it has that, we
> > don't have to worry about tradeoffs there.
> 
> Right, but if we can eliminate that behavioral special-case, can we eliminate
> having CONTEXT_BUTTON_MD versus CONTEXT_BUTTON?  If so that's even better.
> 
> If not, what's the long-term plan?  Can we combine these once Harmony ships? 
In
> the meantime, can we comment more clearly at the declaration here what the
> practical reason is for the distinction and when it will be safe to remove?
> 
> I'm also wondering if CONTEXT_BUTTON_TEXT_MD or something else more
distinctive
> might discourage people from using this widely -- it sort of sounds from the
> name like CONTEXT_BUTTON_MD is the shiny new way of doing all buttons.

I don't think CONTEXT_BUTTON_MD makes sense for LabelButton (I was only
considering it for the special case in MdTextButton::CreateSecondaryUiButton()
when it falls back to using LabelButton). Too much inherits off LabelButton, and
it's used to implement buttons that are not dialog buttons, such as Checkboxes.

Long term (post harmony), I think these are different contexts (possibly more if
the scope of the spec grows to cover more than just "secondary" UI - i.e.
browser window and the Ash desktop).

CONTEXT_DIALOG_BUTTON (in this CL: CONTEXT_BUTTON_MD) - Used in dialogs. gets
bold. no line spacing.

CONTEXT_BUTTON (or something - CONTEXT_CONTROL?) - Used in toolbars, checkboxes
and elsewhere, no line spacing (probably always single-line)

CONTEXT_LABEL - Gets extra line spacing, may be multi-line

Outside of views there are also the "BODY_TEXT" contexts.

That is, after Harmony, there will be no "LabelButton" that is used directly for
buttons on Dialogs. But there will still be LabelButtons used for other things
and semantically it doesn't make sense for them to be CONTEXT_DIALOG_ANYTHING or
CONTEXT_LABEL, so we need a third option.

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
File ui/views/style/typography_provider.h (right):

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
ui/views/style/typography_provider.h:36: // returns NORMAL, if the NORMAL font
is at least as bold as |weight|.
On 2017/04/11 02:24:20, Peter Kasting wrote:
> On 2017/04/11 01:56:50, tapted wrote:
> > On 2017/04/10 22:56:02, Peter Kasting wrote:
> > > On 2017/04/10 12:27:33, tapted wrote:
> > > > On 2017/04/08 01:38:54, Peter Kasting wrote:
> > > > > I'm confused.  There are fonts where asking for a lower weight (e.g.
> 400)
> > > gets
> > > > > you a bolder font than asking for a higher weight (e.g. 500)?
> > > > 
> > > > Yup.
> > > > 
> > > > The windows font settings have a "Bold font" checkbox. Checking this
will
> > put
> > > a
> > > > bold font into ResourceBundle when asking for gfx::Weight::NORMAL. Fonts
> at
> > a
> > > > given *size* are derived from the unstyled font at the normal size, so
> this
> > > > boldness is preserved when changing _size_.
> > > > 
> > > > However, deriving a Weight::MEDIUM font from a bold font (i.e. the one
in
> > the
> > > > NORMAL slot) will cause the weight to decrease.
> > > 
> > > OK, so effectively, the "bold font" checkbox really means "when someone
> calls
> > > the font APIs requesting NORMAL, return something that's really BOLD; for
> any
> > > other weight, just do what they asked"?
> > 
> > It's not quite that bad :). The logic for this is in
> > 
> > PlatformFontWin::HFontRef* PlatformFontWin::GetBaseFontRef() 
> > 
> > It calls base::win::GetNonClientMetrics(..) and creates a gfx::Font using
the
> > HFONT that the WinAPI puts into NONCLIENTMETRICS_XP::lfMessageFont
> 
> OK, so the default metrics say e.g. "use weight 600", and so the issue is that
> if we ask PlatformFont for "normal", it doesn't actually force the weight, it
> just uses the base font, so we get bold, but if we ask for a specific weight,
it
> writes that into the metrics, and then we get that?

Yup

> If so it sounds to me like PlatformFont should take care of this -- if we ask
> for something bolder than NORMAL, but we're deriving it on a font where the
base
> metrics would already have given a higher weight to the NORMAL case, then we
> should clamp the requested weight to at least that weight.
> 
> Doing it in the typography provider seems a bit too high-level.  Or long run
> does it not matter because no one uses PlatformFont directly and everyone goes
> through the typography provider?  Though even then it seems like this should
> happen there, I think?

I don't think PlatformFont is right. It's coincidence that a default-constructed
PlatformFont gets its metrics from something that happens to be used for UI.
E.g. There's no reason why Blink should have its own font class rather than
using gfx::Font (I want to combine them!)

Another option is ResourceBundle - it's ResourceBundle::GetFontListWithDelta().
That's the method that has the logic to preserve styles on the base font when
deriving a font at the same size
https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.cc?l=582 

It also has logic to do a bitwise-OR of styles when deriving things.

But I don't think that's right either. E.g. we'd have to make a decision what to
do when deriving a _lighter_ weight to be consistent. That is, when BaseFont is
"BOLD" should gfx::Font::Weight::LIGHT be MEDIUM? If it's not, how does one get
a MEDIUM font if they really want one?

I think the root cause is that it's a bug that
ResourceBundle::GetFontListwithDelta(.., Font::Weight::NORMAL) would ever give a
BOLD font. We can't change that without breaking everything.

I think we'd want to introduce something like Font::Weight::SYSTEM to do what
Weight::NORMAL currently does: (and only that should have any special logic for
MEDIUM to still return BOLD when Weight::SYSTEM is already BOLD). and
brute-force/refactor everything currently using NORMAL to see if it should be
changed to SYSTEM instead.

Currently this confusion results from burying this weird logic around system UI
fonts deep in the system. I think adding _more_ subtle logic can only make that
worse. (Hence SYSTEM to make it explicit rather than subtle).

Peter Kasting

I gotta run for the moment, so don't have time to look closely, but I'm ...

3 years, 8 months ago (2017-04-13 00:50:00 UTC) #73

tapted

On 2017/04/13 00:50:00, Peter Kasting wrote: > I gotta run for the moment, so don't ...

3 years, 8 months ago (2017-04-13 00:56:36 UTC) #74

Peter Kasting

On 2017/04/13 00:56:36, tapted wrote: > On 2017/04/13 00:50:00, Peter Kasting wrote: > > I ...

3 years, 8 months ago (2017-04-13 03:53:17 UTC) #75

On 2017/04/13 00:56:36, tapted wrote:
> On 2017/04/13 00:50:00, Peter Kasting wrote:
> > I gotta run for the moment, so don't have time to look closely, but I'm
> > wondering how CONTEXT_BUTTON_MD differs from CONTEXT_BUTTON in the
> post-Harmony
> > world, since I thought the bolding thing would no longer differ (like it
does
> > pre-Harmony), and it seems like all the other aspects are the same.
> 
> It would no longer differ for Dialog Buttons: OK/Cancel etc in
DialogClientView.
> 
> However we also need a style for buttons that do not appear in dialogs. This
> includes: Checkboxes, Radio Buttons, Menu Items, Profile Switcher, Ash Tray,
> Bookmarks toolbar. These should not have line spacing added but they should
not
> get Bold either.

OK, I might have got my head around this.  The issue is that in contexts where a
button can be the "default" button, then on Windows, the default button should
be bold (and other "defaultable" buttons that aren't actually the default one
would be normal).  But in the other cases you mention, there is no default
button.  Right?

It seems like in that case we won't really need two contexts -- for stuff like
checkboxes and radio buttons, or buttons in menus, we simply won't have a
STYLE_DIALOG_BUTTON_DEFAULT button in the set.

And if we just go ahead and make default buttons' text bold pre-Harmony, we
don't need a different context now, either, do we?

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
File ui/views/style/typography_provider.h (right):

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
ui/views/style/typography_provider.h:36: // returns NORMAL, if the NORMAL font
is at least as bold as |weight|.
On 2017/04/12 07:31:48, tapted wrote:
> On 2017/04/11 02:24:20, Peter Kasting wrote:
> > If so it sounds to me like PlatformFont should take care of this -- if we
ask
> > for something bolder than NORMAL, but we're deriving it on a font where the
> base
> > metrics would already have given a higher weight to the NORMAL case, then we
> > should clamp the requested weight to at least that weight.
> > 
> > Doing it in the typography provider seems a bit too high-level.  Or long run
> > does it not matter because no one uses PlatformFont directly and everyone
goes
> > through the typography provider?  Though even then it seems like this should
> > happen there, I think?
> 
> I don't think PlatformFont is right. It's coincidence that a
default-constructed
> PlatformFont gets its metrics from something that happens to be used for UI.
> E.g. There's no reason why Blink should have its own font class rather than
> using gfx::Font (I want to combine them!)

Yeah, I agree this is too low-level -- the issue is with the behavior when you
ask for a specific UI font like "the base font", and that happens above this
layer.

> 
> Another option is ResourceBundle - it's
ResourceBundle::GetFontListWithDelta().
> That's the method that has the logic to preserve styles on the base font when
> deriving a font at the same size
> https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.cc?l=582

> 
> It also has logic to do a bitwise-OR of styles when deriving things.
> 
> But I don't think that's right either. E.g. we'd have to make a decision what
to
> do when deriving a _lighter_ weight to be consistent. That is, when BaseFont
is
> "BOLD" should gfx::Font::Weight::LIGHT be MEDIUM? If it's not, how does one
get
> a MEDIUM font if they really want one?

All font weights are relative to the normal appearance of that font face.  The
real problem here, IMO, is that Microsoft implemented this "bold font" checkbox
not by substituting a different font face that appeared bolder at the same
weight, but that they hacked the default weight metrics.

But given that they did this, I think we should try and pretend they did it the
other way -- that is, we basically ask the system for (base font's base weight)
+ (requested weight - NORMAL).  When this checkbox isn't on, that does the same
thing as today; otherwise, it will add 200 to every requested font weight.  This
has a similar effect as if Microsoft was just using a font face that was
inherently bolder.

So to answer your question, one should not be able to "get a MEDIUM font if they
really want one", because the fact that the weights are being munged should be
hidden from the view of all ResourceBundle consumers.

> I think the root cause is that it's a bug that
> ResourceBundle::GetFontListwithDelta(.., Font::Weight::NORMAL) would ever give
a
> BOLD font. We can't change that without breaking everything.

> 
> I think we'd want to introduce something like Font::Weight::SYSTEM to do what
> Weight::NORMAL currently does: (and only that should have any special logic
for
> MEDIUM to still return BOLD when Weight::SYSTEM is already BOLD). and
> brute-force/refactor everything currently using NORMAL to see if it should be
> changed to SYSTEM instead.

We could sort of do that, but I claim:
(1) Everyone using NORMAL should use SYSTEM; there is no place where we should
avoid the SYSTEM behavior.
(2) And also everyone using every other font weight should have things adjusted,
because those weights were chosen relative to NORMAL, and they should stay
relative to the "NORMAL" (now SYSTEM) weight.

Hence my proposal above.

> Currently this confusion results from burying this weird logic around system
UI
> fonts deep in the system. I think adding _more_ subtle logic can only make
that
> worse. (Hence SYSTEM to make it explicit rather than subtle).

I would agree if only some consumers want this.  If every consumer wants this,
better to do it at a core level.

tapted

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

3 years, 7 months ago (2017-05-05 03:08:02 UTC) #76

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/410001

3 years, 7 months ago (2017-05-05 03:08:22 UTC) #77

tapted

On 2017/04/13 03:53:17, Peter Kasting (catching up) wrote: > On 2017/04/13 00:56:36, tapted wrote: > ...

3 years, 7 months ago (2017-05-05 03:08:49 UTC) #78

On 2017/04/13 03:53:17, Peter Kasting (catching up) wrote:
> On 2017/04/13 00:56:36, tapted wrote:
> > On 2017/04/13 00:50:00, Peter Kasting wrote:
> > > I gotta run for the moment, so don't have time to look closely, but I'm
> > > wondering how CONTEXT_BUTTON_MD differs from CONTEXT_BUTTON in the
post-Harmony
> > > world, since I thought the bolding thing would no longer differ (like it
does
> > > pre-Harmony), and it seems like all the other aspects are the same.
> > 
> > It would no longer differ for Dialog Buttons: OK/Cancel etc in
DialogClientView.
> > 
> > However we also need a style for buttons that do not appear in dialogs. This
> > includes: Checkboxes, Radio Buttons, Menu Items, Profile Switcher, Ash Tray,
> > Bookmarks toolbar. These should not have line spacing added but they should
> not get Bold either.
> 
> OK, I might have got my head around this.  The issue is that in contexts where
a
> button can be the "default" button, then on Windows, the default button should
> be bold (and other "defaultable" buttons that aren't actually the default one
> would be normal).  But in the other cases you mention, there is no default
> button.  Right?
> 
> It seems like in that case we won't really need two contexts -- for stuff like
> checkboxes and radio buttons, or buttons in menus, we simply won't have a
> STYLE_DIALOG_BUTTON_DEFAULT button in the set.
> 
> And if we just go ahead and make default buttons' text bold pre-Harmony, we
> don't need a different context now, either, do we?

The problem isn't the BOLDness from default-action buttons, but the BOLDness
from _all_ dialog buttons in the MD spec (i.e. BOLD on Windows, MEDIUM
elsewhere). MdTextButton and Checkbox can't both use CONTEXT_BUTTON if we want
one to be bold and not the other. In Patchset 10 the workaround was to keep
using CONTEXT_LABEL for Checkbox (and LabelButton, since we assume all "real"
buttons will be MdTextButton in MD). But that was confusing.

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
File ui/views/style/typography_provider.h (right):

https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
ui/views/style/typography_provider.h:36: // returns NORMAL, if the NORMAL font
is at least as bold as |weight|.
On 2017/04/13 03:53:17, Peter Kasting (catching up) wrote:
> On 2017/04/12 07:31:48, tapted wrote:
> > Another option is ResourceBundle - its
ResourceBundle::GetFontListWithDelta().
> > That's the method that has the logic to preserve styles on the base font
when
> > deriving a font at the same size
> >
https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.cc?l=582
> 
> > 
> > It also has logic to do a bitwise-OR of styles when deriving things.
> > But I don't think that's right either. E.g. we'd have to make a decision
what to
> > do when deriving a _lighter_ weight to be consistent. That is, when BaseFont
is
> > "BOLD" should gfx::Font::Weight::LIGHT be MEDIUM? If it's not, how does one
get
> > a MEDIUM font if they really want one?
> 
> All font weights are relative to the normal appearance of that font face.  The
> real problem here, IMO, is that Microsoft implemented this "bold font"
checkbox
> not by substituting a different font face that appeared bolder at the same
> weight, but that they hacked the default weight metrics.
> 
> But given that they did this, I think we should try and pretend they did it
the
> other way -- that is, we basically ask the system for (base font's base
weight)
> + (requested weight - NORMAL).  When this checkbox isn't on, that does the
same
> thing as today; otherwise, it will add 200 to every requested font weight. 
This
> has a similar effect as if Microsoft was just using a font face that was
> inherently bolder.
> 
> So to answer your question, one should not be able to "get a MEDIUM font if
they
> really want one", because the fact that the weights are being munged should be
> hidden from the view of all ResourceBundle consumers.

I've implemented this in PatchSet 13. But -- without introducing a behaviour
change -- it doesn't help simplify the logic here. The current logic returns
Weight::NORMAL if that's "bold enough" -- i.e. the weight is "capped" to bold.

In PatchSet 14 I've explored a simplification that results in a behaviour
change: MD buttons will use BLACK weight on Windows if the base font is BOLD.
(same applies to the default buttons in non-MD).

I've added some screenshots to http://crbug.com/691891#c16 . I am still unsure
about this, because:
- It assumes gfx::Font::Weight is linear, and that the system provides fonts at
all weights consistently (e.g. across typefaces where fallback fonts are
involved)
- A user may be setting their default fonts to 'BOLD' to assist with legibility,
but BLACK may be less legible than BOLD.

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-05 03:11:57 UTC) #79

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linux/builds/361234) chromeos_amd64-generic_chromium_compile_only_ng on ...

3 years, 7 months ago (2017-05-05 03:11:58 UTC) #80

tapted

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

3 years, 7 months ago (2017-05-05 03:18:27 UTC) #81

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/430001

3 years, 7 months ago (2017-05-05 03:18:42 UTC) #82

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-05 03:32:43 UTC) #83

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/332819) chromeos_daisy_chromium_compile_only_ng on ...

3 years, 7 months ago (2017-05-05 03:32:44 UTC) #84

tapted

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

3 years, 7 months ago (2017-05-05 03:37:29 UTC) #85

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/450001

3 years, 7 months ago (2017-05-05 03:37:52 UTC) #86

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-05 04:58:53 UTC) #87

commit-bot: I haz the power

Dry run: This issue passed the CQ dry run.

3 years, 7 months ago (2017-05-05 04:58:55 UTC) #88

sky

On 2017/05/05 03:08:49, tapted wrote: > On 2017/04/13 03:53:17, Peter Kasting (catching up) wrote: > ...

3 years, 7 months ago (2017-05-05 14:24:42 UTC) #89

On 2017/05/05 03:08:49, tapted wrote:
> On 2017/04/13 03:53:17, Peter Kasting (catching up) wrote:
> > On 2017/04/13 00:56:36, tapted wrote:
> > > On 2017/04/13 00:50:00, Peter Kasting wrote:
> > > > I gotta run for the moment, so don't have time to look closely, but I'm
> > > > wondering how CONTEXT_BUTTON_MD differs from CONTEXT_BUTTON in the
> post-Harmony
> > > > world, since I thought the bolding thing would no longer differ (like it
> does
> > > > pre-Harmony), and it seems like all the other aspects are the same.
> > > 
> > > It would no longer differ for Dialog Buttons: OK/Cancel etc in
> DialogClientView.
> > > 
> > > However we also need a style for buttons that do not appear in dialogs.
This
> > > includes: Checkboxes, Radio Buttons, Menu Items, Profile Switcher, Ash
Tray,
> > > Bookmarks toolbar. These should not have line spacing added but they
should
> > not get Bold either.
> > 
> > OK, I might have got my head around this.  The issue is that in contexts
where
> a
> > button can be the "default" button, then on Windows, the default button
should
> > be bold (and other "defaultable" buttons that aren't actually the default
one
> > would be normal).  But in the other cases you mention, there is no default
> > button.  Right?
> > 
> > It seems like in that case we won't really need two contexts -- for stuff
like
> > checkboxes and radio buttons, or buttons in menus, we simply won't have a
> > STYLE_DIALOG_BUTTON_DEFAULT button in the set.
> > 
> > And if we just go ahead and make default buttons' text bold pre-Harmony, we
> > don't need a different context now, either, do we?
> 
> The problem isn't the BOLDness from default-action buttons, but the BOLDness
> from _all_ dialog buttons in the MD spec (i.e. BOLD on Windows, MEDIUM
> elsewhere). MdTextButton and Checkbox can't both use CONTEXT_BUTTON if we want
> one to be bold and not the other. In Patchset 10 the workaround was to keep
> using CONTEXT_LABEL for Checkbox (and LabelButton, since we assume all "real"
> buttons will be MdTextButton in MD). But that was confusing.
> 
>
https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
> File ui/views/style/typography_provider.h (right):
> 
>
https://codereview.chromium.org/2801583002/diff/230001/ui/views/style/typogra...
> ui/views/style/typography_provider.h:36: // returns NORMAL, if the NORMAL font
> is at least as bold as |weight|.
> On 2017/04/13 03:53:17, Peter Kasting (catching up) wrote:
> > On 2017/04/12 07:31:48, tapted wrote:
> > > Another option is ResourceBundle - its
> ResourceBundle::GetFontListWithDelta().
> > > That's the method that has the logic to preserve styles on the base font
> when
> > > deriving a font at the same size
> > >
> https://cs.chromium.org/chromium/src/ui/base/resource/resource_bundle.cc?l=582
> > 
> > > 
> > > It also has logic to do a bitwise-OR of styles when deriving things.
> > > But I don't think that's right either. E.g. we'd have to make a decision
> what to
> > > do when deriving a _lighter_ weight to be consistent. That is, when
BaseFont
> is
> > > "BOLD" should gfx::Font::Weight::LIGHT be MEDIUM? If it's not, how does
one
> get
> > > a MEDIUM font if they really want one?
> > 
> > All font weights are relative to the normal appearance of that font face. 
The
> > real problem here, IMO, is that Microsoft implemented this "bold font"
> checkbox
> > not by substituting a different font face that appeared bolder at the same
> > weight, but that they hacked the default weight metrics.
> > 
> > But given that they did this, I think we should try and pretend they did it
> the
> > other way -- that is, we basically ask the system for (base font's base
> weight)
> > + (requested weight - NORMAL).  When this checkbox isn't on, that does the
> same
> > thing as today; otherwise, it will add 200 to every requested font weight. 
> This
> > has a similar effect as if Microsoft was just using a font face that was
> > inherently bolder.
> > 
> > So to answer your question, one should not be able to "get a MEDIUM font if
> they
> > really want one", because the fact that the weights are being munged should
be
> > hidden from the view of all ResourceBundle consumers.
> 
> I've implemented this in PatchSet 13. But -- without introducing a behaviour
> change -- it doesn't help simplify the logic here. The current logic returns
> Weight::NORMAL if that's "bold enough" -- i.e. the weight is "capped" to bold.
> 
> In PatchSet 14 I've explored a simplification that results in a behaviour
> change: MD buttons will use BLACK weight on Windows if the base font is BOLD.
> (same applies to the default buttons in non-MD).
> 
> I've added some screenshots to http://crbug.com/691891#c16 . I am still unsure
> about this, because:
> - It assumes gfx::Font::Weight is linear, and that the system provides fonts
at
> all weights consistently (e.g. across typefaces where fallback fonts are
> involved)
> - A user may be setting their default fonts to 'BOLD' to assist with
legibility,
> but BLACK may be less legible than BOLD.

There seems to be an active discussion between you two. Should I wait to review
until that is resolved?

Peter Kasting

On 2017/05/05 03:08:49, tapted wrote: > On 2017/04/13 03:53:17, Peter Kasting (catching up) wrote: > ...

3 years, 7 months ago (2017-05-06 03:03:16 UTC) #90

On 2017/05/05 03:08:49, tapted wrote:
> On 2017/04/13 03:53:17, Peter Kasting (catching up) wrote:
> > OK, I might have got my head around this.  The issue is that in contexts
where
> a
> > button can be the "default" button, then on Windows, the default button
should
> > be bold (and other "defaultable" buttons that aren't actually the default
one
> > would be normal).  But in the other cases you mention, there is no default
> > button.  Right?
> > 
> > It seems like in that case we won't really need two contexts -- for stuff
like
> > checkboxes and radio buttons, or buttons in menus, we simply won't have a
> > STYLE_DIALOG_BUTTON_DEFAULT button in the set.
> > 
> > And if we just go ahead and make default buttons' text bold pre-Harmony, we
> > don't need a different context now, either, do we?
> 
> The problem isn't the BOLDness from default-action buttons, but the BOLDness
> from _all_ dialog buttons in the MD spec (i.e. BOLD on Windows, MEDIUM
> elsewhere). MdTextButton and Checkbox can't both use CONTEXT_BUTTON if we want
> one to be bold and not the other. In Patchset 10 the workaround was to keep
> using CONTEXT_LABEL for Checkbox (and LabelButton, since we assume all "real"
> buttons will be MdTextButton in MD). But that was confusing.

It seems like a lot of the complexity here comes from the Harmony spec using
bold fonts for Win button faces.  I don't really understand why we're doing that
anyway, so I just sent mail to bettes (CC chrome-harmony) about it.  If we could
drop that requirement, my hope is that a lot of this would get simpler.

> > But given that they did this, I think we should try and pretend they did it
> the
> > other way -- that is, we basically ask the system for (base font's base
> weight)
> > + (requested weight - NORMAL).  When this checkbox isn't on, that does the
> same
> > thing as today; otherwise, it will add 200 to every requested font weight. 
> This
> > has a similar effect as if Microsoft was just using a font face that was
> > inherently bolder.
> > 
> > So to answer your question, one should not be able to "get a MEDIUM font if
> they
> > really want one", because the fact that the weights are being munged should
be
> > hidden from the view of all ResourceBundle consumers.
> 
> I've implemented this in PatchSet 13. But -- without introducing a behaviour
> change -- it doesn't help simplify the logic here. The current logic returns
> Weight::NORMAL if that's "bold enough" -- i.e. the weight is "capped" to bold.
> 
> In PatchSet 14 I've explored a simplification that results in a behaviour
> change: MD buttons will use BLACK weight on Windows if the base font is BOLD.
> (same applies to the default buttons in non-MD).

IIUC, you're basically implementing in patch set 14 what I was intending to
propose, which is indeed a different behavior than what was originally
implemented (but feels saner to me).

> I've added some screenshots to http://crbug.com/691891#c16 . I am still unsure
> about this, because:
> - It assumes gfx::Font::Weight is linear, and that the system provides fonts
at
> all weights consistently (e.g. across typefaces where fallback fonts are
> involved)
> - A user may be setting their default fonts to 'BOLD' to assist with
legibility,
> but BLACK may be less legible than BOLD.

I think these concerns are legitimate.  My hope is that if we can nuke the "bold
button font" requirement on Windows they will mostly go away (as in, they'll
still be theoretically valid, but they won't be in people's faces all the time).
 And I think really the fact that they exist is something of an indictment of
Microsoft providing this option to begin with, and how they designed it.  The
whole thing feels like a hack, and the fact that the hack doesn't solve users'
needs in all cases isn't really our fault :/

tapted

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

3 years, 7 months ago (2017-05-11 01:13:18 UTC) #95

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/490001

3 years, 7 months ago (2017-05-11 01:14:50 UTC) #96

tapted

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

3 years, 7 months ago (2017-05-11 01:22:56 UTC) #97

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/510001

3 years, 7 months ago (2017-05-11 01:24:18 UTC) #98

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-11 03:17:29 UTC) #99

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/423638)

3 years, 7 months ago (2017-05-11 03:17:30 UTC) #100

tapted

Peter, PTAL. I've basically reverted to patchset 8. As discussed in the Harmony sync, it ...

3 years, 7 months ago (2017-05-11 04:01:53 UTC) #101

Peter Kasting

On 2017/05/11 04:01:53, tapted wrote: > Peter, PTAL. I've basically reverted to patchset 8. > ...

3 years, 7 months ago (2017-05-11 06:07:13 UTC) #102

tapted

On 2017/05/11 06:07:13, Peter Kasting wrote: > On 2017/05/11 04:01:53, tapted wrote: > > Peter, ...

3 years, 7 months ago (2017-05-11 07:26:17 UTC) #103

Peter Kasting

OK, if this preserves existing behavior, then LGTM for now. I didn't re-review since plenty ...

3 years, 7 months ago (2017-05-11 15:42:46 UTC) #104

tapted

sky: please take a look for OWNERS. (principally the ash/* stuff, but it's all related) ...

3 years, 7 months ago (2017-05-12 01:57:05 UTC) #105

sky

https://codereview.chromium.org/2801583002/diff/510001/ash/ash_typography.cc File ash/ash_typography.cc (right): https://codereview.chromium.org/2801583002/diff/510001/ash/ash_typography.cc#newcode20 ash/ash_typography.cc:20: } Don't you need a default: NOTREACHED? https://codereview.chromium.org/2801583002/diff/510001/ash/ash_typography.h File ...

3 years, 7 months ago (2017-05-12 17:00:12 UTC) #106

tapted

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

3 years, 7 months ago (2017-05-15 08:35:42 UTC) #107

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/530001

3 years, 7 months ago (2017-05-15 08:36:00 UTC) #108

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-15 08:39:11 UTC) #109

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/339193) linux_chromium_rel_ng on ...

3 years, 7 months ago (2017-05-15 08:39:12 UTC) #110

tapted

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

3 years, 7 months ago (2017-05-15 11:50:17 UTC) #111

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/550001

3 years, 7 months ago (2017-05-15 11:50:32 UTC) #112

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-15 11:56:02 UTC) #113

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-generic_chromium_compile_only_ng/builds/339266)

3 years, 7 months ago (2017-05-15 11:56:03 UTC) #114

tapted

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

3 years, 7 months ago (2017-05-15 12:01:00 UTC) #115

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/570001

3 years, 7 months ago (2017-05-15 12:01:11 UTC) #116

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-15 12:16:00 UTC) #117

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_compile_dbg_ng/builds/296540)

3 years, 7 months ago (2017-05-15 12:16:01 UTC) #118

tapted

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

3 years, 7 months ago (2017-05-15 12:34:32 UTC) #119

commit-bot: I haz the power

Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/590001

3 years, 7 months ago (2017-05-15 12:34:53 UTC) #120

commit-bot: I haz the power

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

3 years, 7 months ago (2017-05-15 14:13:57 UTC) #122

commit-bot: I haz the power

Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/426502)

3 years, 7 months ago (2017-05-15 14:13:59 UTC) #123

tapted

https://codereview.chromium.org/2801583002/diff/510001/ash/ash_typography.cc File ash/ash_typography.cc (right): https://codereview.chromium.org/2801583002/diff/510001/ash/ash_typography.cc#newcode20 ash/ash_typography.cc:20: } On 2017/05/12 17:00:11, sky wrote: > Don't you ...

3 years, 7 months ago (2017-05-15 23:02:33 UTC) #124

https://codereview.chromium.org/2801583002/diff/510001/ash/ash_typography.cc
File ash/ash_typography.cc (right):

https://codereview.chromium.org/2801583002/diff/510001/ash/ash_typography.cc#...
ash/ash_typography.cc:20: }
On 2017/05/12 17:00:11, sky wrote:
> Don't you need a default: NOTREACHED?

I think this was confusing so I've renamed this method to ApplyAshFontStyles(..)

In the current setup. Only views::DefaultTypographyProvider::GetDefaultFont()
will always set values, then other providers can update it with the things they
know about.

So, then, I see that it is confusing to call this method GetDefaultFooFont() if
it has different semantics. Hence, the rename - ApplyAshFontStyles

https://codereview.chromium.org/2801583002/diff/510001/ash/ash_typography.h
File ash/ash_typography.h (right):

https://codereview.chromium.org/2801583002/diff/510001/ash/ash_typography.h#n...
ash/ash_typography.h:14: enum AshTextContext {
On 2017/05/12 17:00:11, sky wrote:
> With the mustash work we want the only headers that chrome uses from ash are
in
> ash/public/cpp. Please move this file there. Really once mustash is shipping
> chrome won't need to use this header at all, but until then putting the header
> in ash/public is preferred.

Done.

https://codereview.chromium.org/2801583002/diff/510001/ui/views/controls/butt...
File ui/views/controls/button/label_button.cc (right):

https://codereview.chromium.org/2801583002/diff/510001/ui/views/controls/butt...
ui/views/controls/button/label_button.cc:61: SetAccessibleName(text);
On 2017/05/12 17:00:11, sky wrote:
> It seems less error prone to call SetTextInternal here. Is there a compelling
> reason not to?

Went back to SetTextInternal.

This means the text is set twice - once in the initializer and once in
SetTextInternal. Performance isn't a huge deal since Label::SetText doesn't do
too much and RenderText::SetText has an early exit.  My thinking was just that
SetTextInternal could suggest the SetText call was still necessary, where it is
now not.

https://codereview.chromium.org/2801583002/diff/510001/ui/views/style/typogra...
File ui/views/style/typography.h (right):

https://codereview.chromium.org/2801583002/diff/510001/ui/views/style/typogra...
ui/views/style/typography.h:32: CONTEXT_BUTTON_MD,
On 2017/05/12 17:00:11, sky wrote:
> Will this one eventually go away and we use CONTEXT_BUTTON only?

Currently LabelButton is used for other things (subclasses that are not dialog
buttons), which are inheriting CONTEXT_BUTTON. E.g. Checkbox, RadioButton.

I think when the only buttons are MD buttons, and nobody instantiates a concrete
LabelButton, then a good approach would be to rename CONTEXT_BUTTON_MD to
CONTEXT_DIALOG_BUTTON, and rename CONTEXT_BUTTON to something more specific --
CONTEXT_CONTROL_LABEL perhaps.

(and CONTEXT_CONTROL_LABEL could possibly replace CONTEXT_LABEL as well, but I
need to get the relevant Chrome code moved over to CONTEXT_BODY_TEXT_* first )

https://codereview.chromium.org/2801583002/diff/590001/ash/public/cpp/ash_typ...
File ash/public/cpp/ash_typography.h (right):

https://codereview.chromium.org/2801583002/diff/590001/ash/public/cpp/ash_typ...
ash/public/cpp/ash_typography.h:5: #ifndef ASH_PUBLIC_CPP_ASH_TYPOGRAPHY_H_
I couldn't decide whether to drop the "ash_" and just call this typography.h -
(the ash_ prefix is consistent with chrome_typography.h, but seems to diverge a
bit from other things in ash/public/cpp).

Peter Kasting

https://codereview.chromium.org/2801583002/diff/590001/ash/public/cpp/ash_typography.h File ash/public/cpp/ash_typography.h (right): https://codereview.chromium.org/2801583002/diff/590001/ash/public/cpp/ash_typography.h#newcode5 ash/public/cpp/ash_typography.h:5: #ifndef ASH_PUBLIC_CPP_ASH_TYPOGRAPHY_H_ On 2017/05/15 23:02:33, tapted wrote: > I ...

3 years, 7 months ago (2017-05-15 23:29:47 UTC) #125

tapted

Thanks all! On 2017/05/15 23:29:47, Peter Kasting wrote: > https://codereview.chromium.org/2801583002/diff/590001/ash/public/cpp/ash_typography.h > File ash/public/cpp/ash_typography.h (right): > ...

3 years, 7 months ago (2017-05-16 04:46:25 UTC) #127

tapted

The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the ...

3 years, 7 months ago (2017-05-16 04:46:36 UTC) #129

commit-bot: I haz the power

CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2801583002/590001

3 years, 7 months ago (2017-05-16 04:47:07 UTC) #130

commit-bot: I haz the power

CQ is committing da patch. Bot data: {"patchset_id": 590001, "attempt_start_ts": 1494909995909320, "parent_rev": "c5804ea426a20b553dc33c744c48315a5c40e59d", "commit_rev": "e3880da51bf9da14f748d00ac7b5741989107696"}

3 years, 7 months ago (2017-05-16 05:35:21 UTC) #131

commit-bot: I haz the power

Description was changed from ========== Use views::style for buttons, bootstrap ash_typography to do so. This ...

3 years, 7 months ago (2017-05-16 05:35:32 UTC) #132

commit-bot: I haz the power

Committed patchset #18 (id:590001) as https://chromium.googlesource.com/chromium/src/+/e3880da51bf9da14f748d00ac7b5741989107696

3 years, 7 months ago (2017-05-16 05:35:34 UTC) #133

sky

3 years, 7 months ago (2017-05-17 15:29:04 UTC) #134

Message was sent while issue was closed.

On Mon, May 15, 2017 at 9:46 PM, <tapted@chromium.org> wrote:

> Thanks all!
>
> On 2017/05/15 23:29:47, Peter Kasting wrote:
> >
> https://codereview.chromium.org/2801583002/diff/590001/
> ash/public/cpp/ash_typography.h
> > File ash/public/cpp/ash_typography.h (right):
> >
> >
> https://codereview.chromium.org/2801583002/diff/590001/
> ash/public/cpp/ash_typography.h#newcode5
> > ash/public/cpp/ash_typography.h:5: #ifndef
> ASH_PUBLIC_CPP_ASH_TYPOGRAPHY_H_
> > On 2017/05/15 23:02:33, tapted wrote:
> > > I couldn't decide whether to drop the "ash_" and just call this
> typography.h
> -
> > > (the ash_ prefix is consistent with chrome_typography.h, but seems to
> diverge
> > a
> > > bit from other things in ash/public/cpp).
> >
> > You could solve this inconsistency by dropping it from both :)
>
> leaving as-is for now. Although it's unlikely happen in this particular
> case
> (and without source_set), I've run into same-named files causing conflicts
> when
> linking a target enough times that I'm wary of opting for that.
>

I have found that using distinct file names makes it easier in discussions,
otherwise you have to prefix that with the directory, e.g. look at the file
typography in ash, vs look at the file named ash_typography. In this case
it isn't that big of deal, but I've found it supremely confusing when we
have had two Window classes but in different packages.

  -Scott


>
> https://codereview.chromium.org/2801583002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

Issue 2801583002: Use views::style for buttons, bootstrap ash_typography to do so. (Closed)

Description

Patch Set 1 : add a test, make it pass. #

Patch Set 2 : Remove LabelButton::GetFontList, Use typography for ash toast ui #

Patch Set 3 : base off test cleanup in crrev/2799403002 #

Patch Set 4 : More self review #

Patch Set 5 : respond to comments #

Patch Set 6 : rebase for epic ash file move #

Patch Set 7 : Add back MD context #

Patch Set 8 : rebase #

Patch Set 9 : ResourceBundle: Make font weights relative to the base font weight. #

Patch Set 10 : Behaviour change: removes helper methods #

Patch Set 11 : Guh. Rebase again #

Patch Set 12 : Fix CrOS compile #

Patch Set 13 : rebase for ash/BUILD.gn #

Patch Set 14 : Revert to Patchset 8 (no relative font weights) #

Patch Set 15 : selfnits #

Patch Set 16 : Respond to comments #

Patch Set 17 : rebase for r471279 - label_button.*. use ash/public properly #

Patch Set 18 : placate gn check. new_avatar_button now just avatar_button #

Messages