"Bootstrap" a toolkit-views Typography spec.

ui/views/style/typography.h

Index: ui/views/style/typography.h
diff --git a/ui/views/style/typography.h b/ui/views/style/typography.h
new file mode 100644
index 0000000000000000000000000000000000000000..845a593fe56d5a6a1ec544ae483d47ede9856a58
--- /dev/null
+++ b/ui/views/style/typography.h
@@ -0,0 +1,100 @@
+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef UI_VIEWS_STYLE_TYPOGRAPHY_H_
+#define UI_VIEWS_STYLE_TYPOGRAPHY_H_
+
+#include <memory>
+
+#include "base/macros.h"
+#include "third_party/skia/include/core/SkColor.h"
+#include "ui/views/views_export.h"
+
+namespace gfx {
+class FontList;
+}
+
+namespace views {
+
+// Where a piece of text appears in the UI. This influences size and weight, but
+// typically not style or color.
+enum class TextContext {

[sky, 2017/03/14 17:31:47]

It seems fair to enumerate the places in views that contain text, but I would
not want this to grow to include contexts outside of views. How would that be
prevented? For example, in this patch DEPRECATED_SMALL is only used outside of
views. I do not want to see that happen. Same thing with DIALOG_TEXT_SMALL.

[Peter Kasting, 2017/03/14 19:35:49]

I chatted briefly with Scott about this, since it was sounding like some of the
split class stuff we've been talking about with ViewsDelegate, LayoutDelegate,
etc.

One way to implement this might be to copy how content/ and chrome/ split the
NotificationType enum.  Basically, content/ declares a NotificationType enum
inside the content:: namespace, and puts a signal NOTIFICATION_CONTENT_END value
at the end.  Then chrome/ declares a separate enum in a separate naming scope,
which sets the value of the first value to NOTIFICATION_CONTENT_END.

Then functions which want to take a notification type pass ints around instead,
so callers can provide something from either enum, and it works.  You don't get
compile-time typechecking from this, and the signature is uninformative compared
to if it actually said "NotificationType".

We could do something similar here.  A small improvement on the above might be
to create a type alias instead of using "int" in function signatures, except
that that alias would have to not conflict with the actual enum names (ugh). 
Another possibility, I suppose, would be to declare the core class as an enum
class and give it a specified underlying type known to be large enough to hold
all desired values, then do something similar on the chrome/ side and cast (or
write a wrapper that casts) from the "derived" enum to the "base" enum.  This
allows signatures to use the actual enum type, at the cost of making
chrome/-side users use the coercion mechanism everywhere they need to refer to a
real value.

Finally, you could make a counterargument that all this stuff should stay here
if it's conceptually correct for it to be in views/, e.g. because these styles
are implemented in terms of constants or concepts that are internal to
typography.cc and shouldn't be exposed outside views/.

[tapted, 2017/03/15 08:26:24]

Done.

+  HEADLINE,        // Headline text. Usually 20pt. Never multi-line.
+  DIALOG_TITLE,    // A title for a dialog window. Usually 15pt. Multi-line OK.
+  DIALOG_MESSAGE,  // "Body 1". Usually 13pt.
+  DIALOG_TEXT_SMALL,  // "Body 2". Usually 12pt.
+  CONTROL_LABEL,      // "Body 2". Usually 12pt.
+  FIELD,              // A text field. Usually matches CONTROL_LABEL.
+  BUTTON_TEXT,        // Text that appears on a button control. Usually 12pt.

[tapted, 2017/03/14 09:56:18]

BUTTON_TEXT is different in the spec, but DIALOG_TEXT_SMALL, CONTROL_LABEL,
FIELD are all referred to as "Body 2" in the spec. I.e. they are the same thing.
 I just don't really like the name "BODY2" as a constant here, which means
there's some repetition. The advantage is that (maybe) someone reading the code
at the callsite doesn't have to look up what "Body2" means, and for dialogs that
aren't fully specced out a dev can make a judgement call without having a
designer say "that should be body1" or "body2".

The more detailed context information may be useful for other things too - e.g.
views::Label inherits views::View::SetEnabled(bool): something in context
"FIELD" or "BUTTON_TEXT" could use this to switch between PRIMARY/DISABLED
styles, but that might not make sense in other TextContexts.

+
+  // Transitional contexts.
+  DEPRECATED_SMALL,  // ResourceBundle::SmallFont (11 pt).
+};
+
+// How a piece of text should be presented. This influences color and style, but
+// typically not size.
+enum class TextStyle {

[sky, 2017/03/14 17:31:47]

Similar comment as to TextContext. I'm ok with enumerating styles used in views,
nothing else. 

[tapted, 2017/03/15 08:26:24]

Done.

+  // Primary text: solid black, normal weight. Converts to DISABLED in some
+  // contexts (e.g. BUTTON_TEXT, FIELD).
+  PRIMARY,
+
+  // Secondary text: may be lighter.
+  SECONDARY,
+
+  // "Hint" text, usually a line that gives context to something more important.
+  HINT,
+
+  DISABLED,  // Disabled "greyed out" text.
+  RED,       // A solid shade of red.
+  GREEN,     // A solid shade of green.
+  LINK,      // A solid shade of blue.
+
+  // Legacy styles start here.

[Peter Kasting, 2017/03/14 23:53:33]

Are these intended to be "deprecated, don't add uses" and we'll have a TODO
about how to convert them to something else?

[tapted, 2017/03/15 08:26:24]

It's more about a set of styles that just don't exist in the Harmony Typography
spec :/ (which I've tried to avoid focusing on under ui/views).

this is less important to call out now things are split anyway.

+
+  ACTIVE_TAB,      // Active tab in a tabbed pane.
+  INACTIVE_TAB,    // Inactive tab in a tabbed pane.
+  HOVERED_TAB,     // Hovered tab in a tabbed pane.
+  WEB_DOMAIN,      // Used to draw attention to a web domain.
+  EXTENSION_NAME,  // Used to draw attention to an extension name.
+};
+
+// Provides fonts to use in toolkit-views UI.
+class VIEWS_EXPORT TypographyProvider {
+ public:
+  // Gets the FontList for the given |context| and |style|.
+  virtual const gfx::FontList& GetFont(TextContext context,
+                                       TextStyle style) = 0;
+
+  // Gets the color for the given |context| and |style|. This may consult
+  // ui::NativeTheme.
+  virtual SkColor GetColor(TextContext context, TextStyle style) = 0;
+
+  // Gets the line spacing, or 0 if it should be provided by gfx::FontList.

[Peter Kasting, 2017/03/14 23:53:33]

I admit it's not totally clear to me how this works.

From what I know, real-life line spacing in printing is typically "baseline to
baseline".  In views, we're typically laying out a single line of text, with
other things around it spaced some distance away from the top or bottom. 
Occasionally we actually use a multi-line label, which could use this value
internally, but in the other (common) cases, does this value influence
surrounding layout at all?  Should it?  Maybe that's beyond the scope of the
typography provider to deal with?

(I guess I'm still wrestling with how "the dialog title has X pt leading, the
body text below it has Y pt leading" means the body text should be positioned
relative to the title, and who should be responsible for that positioning. 
Maybe everything goes in a single RenderText?)

[tapted, 2017/03/15 08:26:24]

I think it does and should. views::Label has a line_height() property, which is
fed through to gfx::RenderText::SetMinLineHeight(). Label::GetHeightForWidth and
GetTextSize() cater for line_height() in both the single-line and multi-line
cases. And RenderText always tries to center the text its rendering in the
display rectangle its given.
Yeah, the plan would just be to let Label::SetLineHeight() deal with how it
should be interpreted.
RenderText will pick an *actual* height for each line that depends on the
GetHeight() of every gfx::Font, the MinLineHeight(), as well as the tallest font
glyph that actually got rendered on that line. But then it just gives bounding
boxes to the things using it and centers text within those - I think it will all
work out..

[Peter Kasting, 2017/03/15 10:01:48]

My worry here is that that means this line spacing is "the box height, within
which we should vertically center single lines of text".  Which is quite
different from "the spacing to place between the baseline of this font and the
font on the next line".

For example, a font of height 10, with line spacing of 20, placed atop a dialog,
will have 5 px of padding atop it in the first case, and 0 in the second.

[tapted, 2017/03/15 11:04:06]

(disclaimer: I'm not a typesetting expert) I think RenderText tries to do
something smarter in the second case - there's a method with lots of arithmetic
- DetermineBaselineCenteringText -
https://cs.chromium.org/chromium/src/ui/gfx/render_text.cc?q=DetermineBaselin...

it doesn't seem to take into account any line_height "override" that we'd be
doing by making the line spacing 20px rather than the default (i.e.
Font::GetHeight()) -- the line_height override just affects the bounding box
within which the text is centered.

(of course multiline makes things more complicated)

[tapted, 2017/03/15 22:43:34]

bettes@ recently pointed me to https://goto.google.com/ukysc which talks about
line spacing in the MD spec and may have some guidance here

+  virtual int GetLineSpacing(TextContext context, TextStyle style) = 0;
+
+  virtual ~TypographyProvider() {}

[Peter Kasting, 2017/03/14 23:53:34]

Nit: I tend to prefer "= default" to "{}" as the way of saying "I didn't forget
this, but it doesn't need to do anything special".  (2 places)

[tapted, 2017/03/15 08:26:24]

Done.

+
+ protected:
+  TypographyProvider() {}
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(TypographyProvider);
+};
+
+// Helper class to query (or change) the current TypographyProvider.

[Peter Kasting, 2017/03/14 23:53:34]

Hmm.  Is this really just a way to group some related functions together and not
an actual class?  If so, use a namespace instead; see
http://google.github.io/styleguide/cppguide.html#Nonmember,_Static_Member,_an...
.  Or make these static on another class, e.g. TypographyProvider (might require
some name changes? e.g. making the non-static methods there private and
suffixing them "Internal").

[tapted, 2017/03/15 08:26:24]

Hm - conceptually, `Typography` also encapsulates the "Storage" defined in the
anonymous namespace in the .cc. The methods are tightly bound to that storage,
it just happens that the actual data storage doesn't need to be declared in the
header.

A class also bundles linkage (VIEWS_EXPORT) and allows declaring `friends`
(although the latter doesn't apply here).

But this is just a convenience to explore ways to avoid the syntactic mess we
get around things like ResourceBundle (`FooProvider::Get().GetBar` feels really
awkward to me). However, this doesn't scale well with more methods. Barely
anything actually needs to get the provider anyway, so I've split things up and
gone for the more traditional approach.

+class VIEWS_EXPORT Typography {
+ public:
+  static void SetProvider(std::unique_ptr<TypographyProvider> provider);
+
+  static const gfx::FontList& GetFont(TextContext context, TextStyle style);
+  static SkColor GetColor(TextContext context, TextStyle style);
+  static int GetLineSpacing(TextContext context, TextStyle style);
+
+ private:
+  DISALLOW_IMPLICIT_CONSTRUCTORS(Typography);
+};
+
+}  // namespace views
+
+#endif  // UI_VIEWS_STYLE_TYPOGRAPHY_PROVIDER_H_