Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2765883004:
Implement Harmony typography spec. (Closed)
DescriptionImplement Harmony typography spec.
Currently does font size and line height.
Color is next. And there is still plumbing required to ensure more
controls/dialogs have access to views::style (and stop using
SetFontList). This CL establishes a core requirement to let us iterate
on layout for specific dialogs.
Example screenshots at https://crbug.com/691891#c10
BUG=691891
Review-Url: https://codereview.chromium.org/2765883004
Cr-Commit-Position: refs/heads/master@{#462676}
Committed: https://chromium.googlesource.com/chromium/src/+/620e80b9d16fa22fb95269718c23e7f632cf9b76
Patch Set 1 : Experiments with color - Linux themes make this super hard #Patch Set 2 : Rollback color, fix buttons #Patch Set 3 : test #Patch Set 4 : test, nit comment #
Total comments: 24
Patch Set 5 : respond to comments, but i bet MSVC does not like my static constexpr #
Total comments: 2
Patch Set 6 : rebase #Patch Set 7 : respond to comments #Messages
Total messages: 37 (28 generated)
Description was changed from ========== cl format fix compile cl format Implement harmony typography spec BUG= ========== to ========== Implement harmony typography spec. Currently just does line height. Next is color. BUG=691891 ==========
Description was changed from ========== Implement harmony typography spec. Currently just does line height. Next is color. BUG=691891 ========== to ========== Implement harmony typography spec. Currently does line height and color. BUG=691891 ==========
Description was changed from ========== Implement harmony typography spec. Currently does line height and color. BUG=691891 ========== to ========== Implement harmony typography spec. Currently does line height. Color is next. BUG=691891 ==========
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #3 (id:40001) has been deleted
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Implement harmony typography spec. Currently does line height. Color is next. BUG=691891 ========== to ========== Implement harmony typography spec. Currently does font size and line height. Color is next. And there is still plumbing required to ensure more controls/dialogs have access to views::style (and stop using SetFontList). This CL establishes a core requirement to let us iterate on layout for specific dialogs. Example screenshots at https://crbug.com/691891#c10 BUG=691891 ==========
tapted@chromium.org changed reviewers: + pkasting@chromium.org
Description was changed from ========== Implement harmony typography spec. Currently does font size and line height. Color is next. And there is still plumbing required to ensure more controls/dialogs have access to views::style (and stop using SetFontList). This CL establishes a core requirement to let us iterate on layout for specific dialogs. Example screenshots at https://crbug.com/691891#c10 BUG=691891 ========== to ========== Implement Harmony typography spec. Currently does font size and line height. Color is next. And there is still plumbing required to ensure more controls/dialogs have access to views::style (and stop using SetFontList). This CL establishes a core requirement to let us iterate on layout for specific dialogs. Example screenshots at https://crbug.com/691891#c10 BUG=691891 ==========
Hi Peter, please take a look. https://codereview.chromium.org/2765883004/diff/120001/ui/views/controls/butt... File ui/views/controls/button/label_button.cc (right): https://codereview.chromium.org/2765883004/diff/120001/ui/views/controls/butt... ui/views/controls/button/label_button.cc:244: if (style_ == STYLE_BUTTON && PlatformStyle::kDefaultLabelButtonHasBoldFont) { This ButtonHasBoldFont thing is going away eventually... which will make the whole temporary label thing unnecessary and make all the other traps obsolete. But, since button subclasses or client code can call SetFontList() or AdjustFontSize() on any Button (i.e. rather than just SetStyle) we can't switch directly to having LabelButton pass STYLE_BUTTON through to the Label constructor when it creates the button style. And although views::style *does* give us a way to remove both SetFontList() and AdjustFontSize(), but that's getting into chicken-and-egg territory. So, for now, the SetLineHeight() call above is needed, even though the typography spec returns `0` for button line heights. This is because STYLE_PRIMARY calls for a line height under Harmony, and the calculations in MdTextButton::UpdatePadding() will incorporate it, so if it's not _also_ incorporated here, then MD buttons will lose height and look wrong.
The CQ bit was unchecked by commit-bot@chromium.org
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_...)
https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right): https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:12: // "Target" font size constants from the Harmony spec. Nit: I feel weakly like it might be better to define this in the cases below that actually use them, but not sure. https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:25: // OS configuration. Ultimately, can this be computed (once) at runtime, or centralized to PlatformFont or something? Having it here feels a bit magic/layer-violationy, and it seems like some of the (disabled) tests you previously added want this as well. https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:45: font_weight = kButtonFontWeight; Nit: Put break; after (no functional effect now, could cause a bug later) https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:57: return SK_ColorBLACK; Nit: Maybe a TODO? Prolly not necessary I guess https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:77: #if defined(OS_MACOSX) For all platforms, I wonder if these values are really true across OS versions. For example, do Win 7 and Win 10 behave the same, do the earliest and latest OS X versions we support behave the same, etc. Again, if it's possible to compute this at runtime somehow (have PlatformFont construct a font with a specific face and size and then measure its height?), that would be safer than hardcoding these. https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:97: static const int headline_height = These statics worry me w.r.t. handling native theme changes that happen while Chrome is running. I wonder if instead we need to compute and store these in some kind of native theme changed listener, and trigger that on startup? https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate_unittest.cc (right): https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:234: // Ensure that line height is overridden under Harmony for the standard set of Nit: If there's a way to word comments/test names using "material" or "MD" or something instead of "Harmony", I'd try to do so, as "Harmony" is really an internal code name that's going to be meaningless to public readers and increasingly meaningless someday even to Googlers reading this stuff. (I haven't really been holding the line on this, but most places this occurs today will be ripped out at some point when Harmony is on by default.) https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:248: const struct { Nit: Can this be constexpr? https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:259: SCOPED_TRACE(testing::Message() << "Testing index: " << i++); Nit: If we're going to have a counting index anyway maybe we should just use that to index into the array instead of having what amounts to two loop indexes. https://codereview.chromium.org/2765883004/diff/120001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2765883004/diff/120001/ui/views/controls/labe... ui/views/controls/label.h:293: void Init(const base::string16& text); Why scrap the second arg? At least for now it looks like it just results in code duplication.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
PTAL https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right): https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:12: // "Target" font size constants from the Harmony spec. On 2017/04/04 00:47:58, Peter Kasting wrote: > Nit: I feel weakly like it might be better to define this in the cases below > that actually use them, but not sure. The goal here is to call out the magic numbers that come directly from the spec, and give them a name. (the pessimist/pragmatist in me is expecting that there may be a number of cases coming out in the wash that the spec doesn't cater for, which may be derived from these. E.g. TouchSelectionMenuRunnerViews::Menu::CreateButton jumps through hoops to get an 11pt font) https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:25: // OS configuration. On 2017/04/04 00:47:58, Peter Kasting wrote: > Ultimately, can this be computed (once) at runtime, or centralized to > PlatformFont or something? Having it here feels a bit magic/layer-violationy, > and it seems like some of the (disabled) tests you previously added want this as > well. I don't think it can be computed at runtime, since it's really an estimate of what's "most popular" (see next comment), but we can move it to PlatformFont (done). https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:45: font_weight = kButtonFontWeight; On 2017/04/04 00:47:58, Peter Kasting wrote: > Nit: Put break; after (no functional effect now, could cause a bug later) Done. (I am ashamed to admit that I don't think this was intentional..) https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:57: return SK_ColorBLACK; On 2017/04/04 00:47:58, Peter Kasting wrote: > Nit: Maybe a TODO? Prolly not necessary I guess Done. https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:77: #if defined(OS_MACOSX) On 2017/04/04 00:47:58, Peter Kasting wrote: > For all platforms, I wonder if these values are really true across OS versions. > For example, do Win 7 and Win 10 behave the same, do the earliest and latest OS > X versions we support behave the same, etc. > > Again, if it's possible to compute this at runtime somehow (have PlatformFont > construct a font with a specific face and size and then measure its height?), > that would be safer than hardcoding these. So, like you say, I'm quite certain that these are not consistent across OS versions, and we certainly can't assume it's the case for all future OS versions. Added a TODO to fill this out with OS point version selection. But we do need a starting point that "looks right" for a majority of users. per the comment at the top of GetLineHeight -- it's trying to add the same "extra" space between lines that the default configuration would have when asking for the line height from the spec. I can't think of a way to tell by asking the OS at runtime whether their system font has changed properties because that's what the OS now prefers or that's what the user prefers. E.g. on Mac we use [NSFont systemFontSize]. Usually that returns "13". But if a user has changed the preference, I don't know of another method we could call that returns "13" to provide what would have been provided by the current OS version if the user had not overridden it. And for the font height it's going to also depend on the particular font size - i.e. what are the characteristics of glyphs in the system font at a particular size. Since it depends on the font sizes specifically used for Harmony, I think this is the right place. E.g. on PlatformFont, it might look something like: kHeightForPopularDefaultFontOfSize20 kHeightForPopularDefaultFontOfSize15 kHeightForPopularDefaultFontOfSize13 kHeightForPopularDefaultFontOfSize12 But if the Harmony spec changes, we shouldn't have to change PlatformFont. And - for a similar reason - we can't look it up. If a user has configured a different typeface (e.g. exchanged for one with the same font size), it could return different results. E.g. we might still end up with a line spacing of "20pt" but say a users particular configured font _already_ have a line spacing of 20pt -- the text would be all squishy since we wouldn't be adding any extra line spacing. Or say the system font in a particular language is a blockish font that never has ascenders or descenders: we'd want a smaller line spacing there, but if we looked it up and it says "height=12pt" we'd add too much. Aaand, this all assumes there's useful information in the font height when taken separately from the font size. E.g. an alternative could be just to consider the fontsize. E.g. return GetFont(CONTEXT_HEADLINE, kTemplateStyle).GetFontSize() - kHeadlineSize + kHeadlineHeight; but this would do nothing to adapt to locales. https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:97: static const int headline_height = On 2017/04/04 00:47:58, Peter Kasting wrote: > These statics worry me w.r.t. handling native theme changes that happen while > Chrome is running. > > I wonder if instead we need to compute and store these in some kind of native > theme changed listener, and trigger that on startup? We'd need to clear a lot more than these statics - all of ResourceBundle::font_cache_ as well. Currently that's only supported on ChromeOS via chromeos::locale_util::SwitchLanguage(..) - no other platform calls ResourceBundle::ReloadFonts() (and I think ChromeOS only invokes that during a login flow). (if nothing invokes ResourceBundle::ReloadFonts, but we tried to re-query these, then a cached font would be returned by ResourceBundle and give exactly the same response) Some kind of ResourceBundleObserver could do this. Added a TODO(). Should we add a tracking bug for Harmony-on-ChromeOS? https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/layout_delegate_unittest.cc (right): https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:234: // Ensure that line height is overridden under Harmony for the standard set of On 2017/04/04 00:47:58, Peter Kasting wrote: > Nit: If there's a way to word comments/test names using "material" or "MD" or > something instead of "Harmony", I'd try to do so, as "Harmony" is really an > internal code name that's going to be meaningless to public readers and > increasingly meaningless someday even to Googlers reading this stuff. > > (I haven't really been holding the line on this, but most places this occurs > today will be ripped out at some point when Harmony is on by default.) Done. https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:248: const struct { On 2017/04/04 00:47:58, Peter Kasting wrote: > Nit: Can this be constexpr? Done. https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/layout_delegate_unittest.cc:259: SCOPED_TRACE(testing::Message() << "Testing index: " << i++); On 2017/04/04 00:47:58, Peter Kasting wrote: > Nit: If we're going to have a counting index anyway maybe we should just use > that to index into the array instead of having what amounts to two loop indexes. Done. https://codereview.chromium.org/2765883004/diff/120001/ui/views/controls/label.h File ui/views/controls/label.h (right): https://codereview.chromium.org/2765883004/diff/120001/ui/views/controls/labe... ui/views/controls/label.h:293: void Init(const base::string16& text); On 2017/04/04 00:47:58, Peter Kasting wrote: > Why scrap the second arg? At least for now it looks like it just results in > code duplication. Ah, this made more sense back in Patchset1 where I had a `SetStyle` call, but it's too early for that. UnDone.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right): https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:77: #if defined(OS_MACOSX) I sent email to Alan & co. asking how, theoretically, they'd like us to handle font size variations, since I feel like I don't sufficiently understand the design goals.
LGTM for now. https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right): https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:97: static const int headline_height = On 2017/04/04 06:23:00, tapted wrote: > On 2017/04/04 00:47:58, Peter Kasting wrote: > > These statics worry me w.r.t. handling native theme changes that happen while > > Chrome is running. > > > > I wonder if instead we need to compute and store these in some kind of native > > theme changed listener, and trigger that on startup? > > We'd need to clear a lot more than these statics - all of > ResourceBundle::font_cache_ as well. Currently that's only supported on ChromeOS > via chromeos::locale_util::SwitchLanguage(..) - no other platform calls > ResourceBundle::ReloadFonts() (and I think ChromeOS only invokes that during a > login flow). > > (if nothing invokes ResourceBundle::ReloadFonts, but we tried to re-query these, > then a cached font would be returned by ResourceBundle and give exactly the same > response) > > Some kind of ResourceBundleObserver could do this. Added a TODO(). Should we add > a tracking bug for Harmony-on-ChromeOS? I think we should file a bug saying that when we get a theme change, the resource bundle should ReloadFonts(), and we should also reload here (either by noticing the theme change, or by the resource bundle telling some observers that fonts were reloaded -- possibly the latter just to prevent ordering/race condition-type problems). https://codereview.chromium.org/2765883004/diff/140001/ui/gfx/platform_font.h File ui/gfx/platform_font.h (right): https://codereview.chromium.org/2765883004/diff/140001/ui/gfx/platform_font.h... ui/gfx/platform_font.h:29: static constexpr int kPopularDefaultFontSize = 13; Nit: I have no idea where the word "popular" in this name comes from. Maybe kDefaultFontSize or kDefaultBaseFontSize or something.
The CQ bit was checked by tapted@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
tapted@chromium.org changed reviewers: + asvitkine@chromium.org
+asvitkine for ui/gfx/platform_font.h OWNERS (feel free to chime in on any of the other fonty stuff too though :) Thanks! https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... File chrome/browser/ui/views/harmony/harmony_typography_provider.cc (right): https://codereview.chromium.org/2765883004/diff/120001/chrome/browser/ui/view... chrome/browser/ui/views/harmony/harmony_typography_provider.cc:97: static const int headline_height = On 2017/04/06 06:17:07, Peter Kasting wrote: > On 2017/04/04 06:23:00, tapted wrote: > > On 2017/04/04 00:47:58, Peter Kasting wrote: > > > These statics worry me w.r.t. handling native theme changes that happen > while > > > Chrome is running. > > > > > > I wonder if instead we need to compute and store these in some kind of > native > > > theme changed listener, and trigger that on startup? > > > > We'd need to clear a lot more than these statics - all of > > ResourceBundle::font_cache_ as well. Currently that's only supported on > ChromeOS > > via chromeos::locale_util::SwitchLanguage(..) - no other platform calls > > ResourceBundle::ReloadFonts() (and I think ChromeOS only invokes that during a > > login flow). > > > > (if nothing invokes ResourceBundle::ReloadFonts, but we tried to re-query > these, > > then a cached font would be returned by ResourceBundle and give exactly the > same > > response) > > > > Some kind of ResourceBundleObserver could do this. Added a TODO(). Should we > add > > a tracking bug for Harmony-on-ChromeOS? > > I think we should file a bug saying that when we get a theme change, the > resource bundle should ReloadFonts(), and we should also reload here (either by > noticing the theme change, or by the resource bundle telling some observers that > fonts were reloaded -- possibly the latter just to prevent ordering/race > condition-type problems). Filed http://crbug.com/708943 and noted it here. https://codereview.chromium.org/2765883004/diff/140001/ui/gfx/platform_font.h File ui/gfx/platform_font.h (right): https://codereview.chromium.org/2765883004/diff/140001/ui/gfx/platform_font.h... ui/gfx/platform_font.h:29: static constexpr int kPopularDefaultFontSize = 13; On 2017/04/06 06:17:07, Peter Kasting wrote: > Nit: I have no idea where the word "popular" in this name comes from. Maybe > kDefaultFontSize or kDefaultBaseFontSize or something. heh, that was to try to distinguish it from "CreateDefault()" -- I didn't want to imply that CreateDefault() is expected to return a font of kDefaultFontSize. But I guess they are still related - I went for kDefaultBaseFontSize.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by tapted@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org Link to the patchset: https://codereview.chromium.org/2765883004/#ps180001 (title: "respond to comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 180001, "attempt_start_ts": 1491520155894160,
"parent_rev": "cafbdba2f02dddaa020d228b591051692bcae535", "commit_rev":
"620e80b9d16fa22fb95269718c23e7f632cf9b76"}
Message was sent while issue was closed.
Description was changed from ========== Implement Harmony typography spec. Currently does font size and line height. Color is next. And there is still plumbing required to ensure more controls/dialogs have access to views::style (and stop using SetFontList). This CL establishes a core requirement to let us iterate on layout for specific dialogs. Example screenshots at https://crbug.com/691891#c10 BUG=691891 ========== to ========== Implement Harmony typography spec. Currently does font size and line height. Color is next. And there is still plumbing required to ensure more controls/dialogs have access to views::style (and stop using SetFontList). This CL establishes a core requirement to let us iterate on layout for specific dialogs. Example screenshots at https://crbug.com/691891#c10 BUG=691891 Review-Url: https://codereview.chromium.org/2765883004 Cr-Commit-Position: refs/heads/master@{#462676} Committed: https://chromium.googlesource.com/chromium/src/+/620e80b9d16fa22fb95269718c23... ==========
Message was sent while issue was closed.
Committed patchset #7 (id:180001) as https://chromium.googlesource.com/chromium/src/+/620e80b9d16fa22fb95269718c23... |
