Issue 1020853018: DNCI [RenderText] Added font size options in RenderText.

dschuyler

dschuyler@chromium.org changed reviewers: + msw@chromium.org

5 years, 9 months ago (2015-03-25 21:46:57 UTC) #1

msw

https://codereview.chromium.org/1020853018/diff/20001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/1020853018/diff/20001/ui/gfx/render_text.h#newcode221 ui/gfx/render_text.h:221: VerticalAlignment vertical_alignment() const { return vertical_alignment_; } You should ...

5 years, 9 months ago (2015-03-26 23:07:28 UTC) #3

dschuyler

https://codereview.chromium.org/1020853018/diff/20001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/1020853018/diff/20001/ui/gfx/render_text.h#newcode221 ui/gfx/render_text.h:221: VerticalAlignment vertical_alignment() const { return vertical_alignment_; } On 2015/03/26 ...

5 years, 9 months ago (2015-03-27 21:32:14 UTC) #4

msw

https://codereview.chromium.org/1020853018/diff/80001/ui/gfx/render_text.h File ui/gfx/render_text.h (right): https://codereview.chromium.org/1020853018/diff/80001/ui/gfx/render_text.h#newcode112 ui/gfx/render_text.h:112: // Internal helper class used to iterate colors, baselines, ...

5 years, 8 months ago (2015-03-30 21:14:35 UTC) #5

dschuyler

> https://codereview.chromium.org/1020853018/diff/80001/ui/gfx/render_text.h#newcode350 > ui/gfx/render_text.h:350: // A |value| of zero will expand to the default font ...

5 years, 8 months ago (2015-03-31 19:05:41 UTC) #6

msw

msw@chromium.org changed reviewers: + pkasting@chromium.org, yukishiino@chromium.org

5 years, 8 months ago (2015-04-01 17:52:03 UTC) #7

msw

On 2015/03/31 19:05:41, dschuyler wrote: > > > https://codereview.chromium.org/1020853018/diff/80001/ui/gfx/render_text.h#newcode350 > > ui/gfx/render_text.h:350: // A |value| ...

5 years, 8 months ago (2015-04-01 17:52:03 UTC) #8

On 2015/03/31 19:05:41, dschuyler wrote:
> >
>
https://codereview.chromium.org/1020853018/diff/80001/ui/gfx/render_text.h#ne...
> > ui/gfx/render_text.h:350: // A |value| of zero will expand to the default
font
> size.
> > I believe that font size literal values (15pt, 17px, etc.) are generally
> disouraged, and relative sizes (+4, -2) used to derive sizes from the base
> system font size are encouraged. In this vein, I suspect these should be
signed
> font size deltas that apply to the instance's FontList; that would be
consistent
> with the 0=base notion.
> 
> Is there something I can read to better understand the reasoning for this? 
> Using font deltas doesn't seem like a good plan on first glance, so I'm
guessing
> that I'm missing something.  With larger fonts used (such as for
accessibility)
> the deltas won't perform well.  I can see using ratios/proportions of a base
> font (e.g. 1/2 or 6/5 or whatnot) at a high level.  Also, at some (low) level
> font sizes will be required -- is RenderText low level enough to use the
honest
> font sizes or should it still isolate the programmer from fine grained
control?

+CC Yuki and Peter, for opinions or additional info/guidelines/docs.

Generally, our UI attempts to honor the system font size, using the
ui::ResourceBundle::FontStyle enum as a way to offer consistent relative (to the
system/base) sizes for similar UI elements. Derivation using size deltas
elsewhere is common (see gfx::Font[List]::Derive* functions, and the infrequent
use of constructors that set absolute sizes). Deriving by size deltas ensures
that deviations from the initial font match the semantic intention (ie. give me
a bigger font for all users, not just my setup); whereas absolute sizes might
not relate to the base font size as you'd expect (ie. on systems with large
fonts, |big_title_font| at 16px is smaller than the base font, and now my dialog
looks weird). Using ratios instead of fixed deltas might make sense, but if that
were generally the case, I'd expect the Derive functions and the ResourceBundle
FontLists to already be using that technique (or changed to this new method if
it is, in fact, better).

Now, I actually wonder if this break list to apply ranged text size differences
within a RenderText should use ui::ResourceBundle::FontStyle instead of sizes or
deltas.

Perhaps RenderText is low-level enough to allow absolute sizes, but the clients
would have to be smart enough to either (1) specify explicit size values over
the entire range of text (and all related text elements outside the RenderText)
to ensure their semantics are honored or (2) ensure that the explicitly sized
ranges are comparable with the base font size of the RenderText instance's
FontList.

Dave, I wonder if you can give us some context for adding the ranged font size
differences in the first place. The bug doesn't actually state what this feature
will be used for and how that use would guide our approach to implementation.

dschuyler

On 2015/04/01 at 17:52:03, msw wrote: > On 2015/03/31 19:05:41, dschuyler wrote: > > > ...

5 years, 8 months ago (2015-04-01 18:44:51 UTC) #9

On 2015/04/01 at 17:52:03, msw wrote:
> On 2015/03/31 19:05:41, dschuyler wrote:
> > >
> >
https://codereview.chromium.org/1020853018/diff/80001/ui/gfx/render_text.h#ne...
> > > ui/gfx/render_text.h:350: // A |value| of zero will expand to the default
font
> > size.
> > > I believe that font size literal values (15pt, 17px, etc.) are generally
> > disouraged, and relative sizes (+4, -2) used to derive sizes from the base
> > system font size are encouraged. In this vein, I suspect these should be
signed
> > font size deltas that apply to the instance's FontList; that would be
consistent
> > with the 0=base notion.
> > 
> > Is there something I can read to better understand the reasoning for this? 
> > Using font deltas doesn't seem like a good plan on first glance, so I'm
guessing
> > that I'm missing something.  With larger fonts used (such as for
accessibility)
> > the deltas won't perform well.  I can see using ratios/proportions of a base
> > font (e.g. 1/2 or 6/5 or whatnot) at a high level.  Also, at some (low)
level
> > font sizes will be required -- is RenderText low level enough to use the
honest
> > font sizes or should it still isolate the programmer from fine grained
control?
> 
> +CC Yuki and Peter, for opinions or additional info/guidelines/docs.
> 
> Generally, our UI attempts to honor the system font size, using the
ui::ResourceBundle::FontStyle enum as a way to offer consistent relative (to the
system/base) sizes for similar UI elements. 

Awesome, that's pretty much what I was expecting.

> Derivation using size deltas elsewhere is common (see gfx::Font[List]::Derive*
functions, and the infrequent use of constructors that set absolute sizes).
Deriving by size deltas ensures that deviations from the initial font match the
semantic intention (ie. give me a bigger font for all users, not just my setup);
whereas absolute sizes might not relate to the base font size as you'd expect
(ie. on systems with large fonts, |big_title_font| at 16px is smaller than the
base font, and now my dialog looks weird). Using ratios instead of fixed deltas
might make sense, but if that were generally the case, I'd expect the Derive
functions and the ResourceBundle FontLists to already be using that technique
(or changed to this new method if it is, in fact, better).

Makes some sense, but I'd like to propose that ratio (scalar) is a better
technique.  By better I mean more likely to behave as intended in a wider
variety of situations.  (All three methods: absolute sizes, or deltas, or
ratios; all have drawbacks in some cases).

> 
> Now, I actually wonder if this break list to apply ranged text size
differences within a RenderText should use ui::ResourceBundle::FontStyle instead
of sizes or deltas.

I can see using ui::ResourceBundle::FontStyle as the right thing to do at some
level.  I feel that I'd need more experience with the code as a whole to know if
RenderText is the right level to use ui::ResourceBundle::FontStyle,
deltas/ratios, or absolute font sizes.  I'd like to get thoughts from others on
that - what do you think? :)

> 
> Perhaps RenderText is low-level enough to allow absolute sizes, but the
clients would have to be smart enough to either (1) specify explicit size values
over the entire range of text (and all related text elements outside the
RenderText) to ensure their semantics are honored or (2) ensure that the
explicitly sized ranges are comparable with the base font size of the RenderText
instance's FontList.

IIUC, that makes sense.

> 
> Dave, I wonder if you can give us some context for adding the ranged font size
differences in the first place. The bug doesn't actually state what this feature
will be used for and how that use would guide our approach to implementation.

The intended use is future support of the Answers in Suggest spec.  It's an
internal doc, I can send you a link.  But the gist is that it defines around a
dozen predefined text style/property selections (e.g. type 1 text may be 12pt
font in blue; type 2 may be 18pt font in black and right justified; and so on). 
We've already taken some liberties to translate those into
ui::ResourceBundle::FontStyle font sizes rather than using the exact font pt
sizes.  That's at a higher level though, within omnibox_result_view.cc for
example.  I'm looking to get the font size from ui::ResourceBundle::FontStyle at
the high level and tell RenderText to use font size N at the low level.

If we move the code to different levels (other than omnibox_result_view.cc and
RenderText), I'd like to maintain using ui::ResourceBundle::FontStyle at the
higher level and font size N at the lower level.

Peter Kasting

On 2015/04/01 18:44:51, dschuyler wrote: > The intended use is future support of the Answers ...

5 years, 8 months ago (2015-04-01 19:29:31 UTC) #10

dschuyler

On 2015/04/01 at 19:29:31, pkasting wrote: > On 2015/04/01 18:44:51, dschuyler wrote: > > The ...

5 years, 8 months ago (2015-04-01 19:45:39 UTC) #11

On 2015/04/01 at 19:29:31, pkasting wrote:
> On 2015/04/01 18:44:51, dschuyler wrote:
> > The intended use is future support of the Answers in Suggest spec.  It's an
> > internal doc, I can send you a link.  But the gist is that it defines around
a
> > dozen predefined text style/property selections (e.g. type 1 text may be
12pt
> > font in blue; type 2 may be 18pt font in black and right justified; and so
on). 
> > We've already taken some liberties to translate those into
> > ui::ResourceBundle::FontStyle font sizes rather than using the exact font pt
> > sizes.  That's at a higher level though, within omnibox_result_view.cc for
> > example.  I'm looking to get the font size from
ui::ResourceBundle::FontStyle at
> > the high level and tell RenderText to use font size N at the low level.
> > 
> > If we move the code to different levels (other than omnibox_result_view.cc
and
> > RenderText), I'd like to maintain using ui::ResourceBundle::FontStyle at the
> > higher level and font size N at the lower level.
> 
> Can you say more about why using FontStyle down in the RenderText isn't as
good as using absolute sizes?  

:) I don't think I can.  It's an open question as far as I'm concerned.  It
sounds like I misguessed RenderText to be lower level than it is and that I
should consider it higher level.  I feel that using BaseFont and LargeFont and
so on at a high level and absolute sizes at a low level is a solid design.  I'm
looking for feedback on whether RenderText is low level or high level - that's
what I'm not sure about.

> I would have instinctively gone the other way.  

Like at a different level (where RenderText is considered high level) or somehow
the opposite way around (which I'd be confused about). 

> I'm not sure what advantage we get by forcing the caller to obtain a size from
the FontStyle when in general Chromium code never wants anyone to think in
absolute sizes.
> 
> (Even thinking in relative sizes is discouraged.  We used to do more of this,
but the UI leads gave explicit direction a while back that as much as possible
we should restrict to just using the fixed BaseFont, LargeFont, etc. values and
not deriving anything from them.  Some code is more compliant with that than
others.)

That makes sense.  It sounds like we'd still have the highest level (BaseFont,
LargeFont) and the lowest level (absolute font sizes).  My question is where the
dividing line should be?  The two options I see so far are put the line above
RenderText or below RenderText.  I thought it may be right to put it above
RenderText.  It sounds like that was the wrong choice and it should be below
RenderText(?).

I'll rework this CL so that the dividing line is below RenderText (i.e. HarfBuzz
and peers would use absolute font sizes).

msw

On 2015/04/01 19:45:39, dschuyler wrote: > On 2015/04/01 at 19:29:31, pkasting wrote: > > On ...

5 years, 8 months ago (2015-04-01 20:14:25 UTC) #12

On 2015/04/01 19:45:39, dschuyler wrote:
> On 2015/04/01 at 19:29:31, pkasting wrote:
> > On 2015/04/01 18:44:51, dschuyler wrote:
> > > The intended use is future support of the Answers in Suggest spec.  It's
an
> > > internal doc, I can send you a link.  But the gist is that it defines
around
> a
> > > dozen predefined text style/property selections (e.g. type 1 text may be
> 12pt
> > > font in blue; type 2 may be 18pt font in black and right justified; and so
> on). 
> > > We've already taken some liberties to translate those into
> > > ui::ResourceBundle::FontStyle font sizes rather than using the exact font
pt
> > > sizes.  That's at a higher level though, within omnibox_result_view.cc for
> > > example.  I'm looking to get the font size from
> ui::ResourceBundle::FontStyle at
> > > the high level and tell RenderText to use font size N at the low level.
> > > 
> > > If we move the code to different levels (other than omnibox_result_view.cc
> and
> > > RenderText), I'd like to maintain using ui::ResourceBundle::FontStyle at
the
> > > higher level and font size N at the lower level.
> > 
> > Can you say more about why using FontStyle down in the RenderText isn't as
> good as using absolute sizes?  
> 
> :) I don't think I can.  It's an open question as far as I'm concerned.  It
> sounds like I misguessed RenderText to be lower level than it is and that I
> should consider it higher level.  I feel that using BaseFont and LargeFont and
> so on at a high level and absolute sizes at a low level is a solid design. 
I'm
> looking for feedback on whether RenderText is low level or high level - that's
> what I'm not sure about.
> 
> > I would have instinctively gone the other way.  
> 
> Like at a different level (where RenderText is considered high level) or
somehow
> the opposite way around (which I'd be confused about). 
> 
> > I'm not sure what advantage we get by forcing the caller to obtain a size
from
> the FontStyle when in general Chromium code never wants anyone to think in
> absolute sizes.
> > 
> > (Even thinking in relative sizes is discouraged.  We used to do more of
this,
> but the UI leads gave explicit direction a while back that as much as possible
> we should restrict to just using the fixed BaseFont, LargeFont, etc. values
and
> not deriving anything from them.  Some code is more compliant with that than
> others.)
> 
> That makes sense.  It sounds like we'd still have the highest level (BaseFont,
> LargeFont) and the lowest level (absolute font sizes).  My question is where
the
> dividing line should be?  The two options I see so far are put the line above
> RenderText or below RenderText.  I thought it may be right to put it above
> RenderText.  It sounds like that was the wrong choice and it should be below
> RenderText(?).

I would consider RenderText fairly "low-level" and views::Label/Textfield more
"high-level"; The omnibox result view has been an unusual RenderText user, and
that has caused numerous complications. Regardless of those distinctions, I
think Peter and I are more inclined to make the "correct" behavior (LargeFont
amid BaseFont) easy, and make "incorrect" behavior ("custom_obscure_font_family,
15px" amid <system_font_family, system_font_size>) difficult. As such, I'm not
adamantly opposed to allowing direct control of font sizes through RenderText
API, but I'd encourage an API that doesn't promote/risk "incorrect" usage, if an
API that encourages "correct" behavior is equally feasible and supports our use
cases. Once the precedent is set by this CL, future changes become more
difficult, especially when going from widespread arbitrary low-level control to
more consistent high-level control (see the uphill battles for size->size_delta,
Font->FontList, etc.).

RenderText::SetFontList offer pretty low-level arbitrary font support, and a
size-delta (or maybe ratio) break list would offer pretty flexible control on
top of that; I'd be surprised if users really required additional "low-level"
controls that go against the design patterns for our UI. Further usage of
abstract font size enums (BaseFont, etc.) seems like a reasonable goal, but may
not be critical...

> I'll rework this CL so that the dividing line is below RenderText (i.e.
HarfBuzz
> and peers would use absolute font sizes).

Yuki

On 2015/04/01 20:14:25, msw wrote: > On 2015/04/01 19:45:39, dschuyler wrote: > > On 2015/04/01 ...

5 years, 8 months ago (2015-04-02 06:39:29 UTC) #13

On 2015/04/01 20:14:25, msw wrote:
> On 2015/04/01 19:45:39, dschuyler wrote:
> > On 2015/04/01 at 19:29:31, pkasting wrote:
> > > On 2015/04/01 18:44:51, dschuyler wrote:
> > > > The intended use is future support of the Answers in Suggest spec.  It's
> an
> > > > internal doc, I can send you a link.  But the gist is that it defines
> around
> > a
> > > > dozen predefined text style/property selections (e.g. type 1 text may be
> > 12pt
> > > > font in blue; type 2 may be 18pt font in black and right justified; and
so
> > on). 
> > > > We've already taken some liberties to translate those into
> > > > ui::ResourceBundle::FontStyle font sizes rather than using the exact
font
> pt
> > > > sizes.  That's at a higher level though, within omnibox_result_view.cc
for
> > > > example.  I'm looking to get the font size from
> > ui::ResourceBundle::FontStyle at
> > > > the high level and tell RenderText to use font size N at the low level.
> > > > 
> > > > If we move the code to different levels (other than
omnibox_result_view.cc
> > and
> > > > RenderText), I'd like to maintain using ui::ResourceBundle::FontStyle at
> the
> > > > higher level and font size N at the lower level.
> > > 
> > > Can you say more about why using FontStyle down in the RenderText isn't as
> > good as using absolute sizes?  
> > 
> > :) I don't think I can.  It's an open question as far as I'm concerned.  It
> > sounds like I misguessed RenderText to be lower level than it is and that I
> > should consider it higher level.  I feel that using BaseFont and LargeFont
and
> > so on at a high level and absolute sizes at a low level is a solid design. 
> I'm
> > looking for feedback on whether RenderText is low level or high level -
that's
> > what I'm not sure about.
> > 
> > > I would have instinctively gone the other way.  
> > 
> > Like at a different level (where RenderText is considered high level) or
> somehow
> > the opposite way around (which I'd be confused about). 
> > 
> > > I'm not sure what advantage we get by forcing the caller to obtain a size
> from
> > the FontStyle when in general Chromium code never wants anyone to think in
> > absolute sizes.
> > > 
> > > (Even thinking in relative sizes is discouraged.  We used to do more of
> this,
> > but the UI leads gave explicit direction a while back that as much as
possible
> > we should restrict to just using the fixed BaseFont, LargeFont, etc. values
> and
> > not deriving anything from them.  Some code is more compliant with that than
> > others.)
> > 
> > That makes sense.  It sounds like we'd still have the highest level
(BaseFont,
> > LargeFont) and the lowest level (absolute font sizes).  My question is where
> the
> > dividing line should be?  The two options I see so far are put the line
above
> > RenderText or below RenderText.  I thought it may be right to put it above
> > RenderText.  It sounds like that was the wrong choice and it should be below
> > RenderText(?).
> 
> I would consider RenderText fairly "low-level" and views::Label/Textfield more
> "high-level"; The omnibox result view has been an unusual RenderText user, and
> that has caused numerous complications. Regardless of those distinctions, I
> think Peter and I are more inclined to make the "correct" behavior (LargeFont
> amid BaseFont) easy, and make "incorrect" behavior
("custom_obscure_font_family,
> 15px" amid <system_font_family, system_font_size>) difficult. As such, I'm not
> adamantly opposed to allowing direct control of font sizes through RenderText
> API, but I'd encourage an API that doesn't promote/risk "incorrect" usage, if
an
> API that encourages "correct" behavior is equally feasible and supports our
use
> cases. Once the precedent is set by this CL, future changes become more
> difficult, especially when going from widespread arbitrary low-level control
to
> more consistent high-level control (see the uphill battles for
size->size_delta,
> Font->FontList, etc.).
> 
> RenderText::SetFontList offer pretty low-level arbitrary font support, and a
> size-delta (or maybe ratio) break list would offer pretty flexible control on
> top of that; I'd be surprised if users really required additional "low-level"
> controls that go against the design patterns for our UI. Further usage of
> abstract font size enums (BaseFont, etc.) seems like a reasonable goal, but
may
> not be critical...
> 
> > I'll rework this CL so that the dividing line is below RenderText (i.e.
> HarfBuzz
> > and peers would use absolute font sizes).

+1 for BaseFont, LargeFont, etc.  I agree to make the correct behavior easy and
incorrect ones difficult, and to have a loophole to specify the absolute size as
the last resort.

As long as we use BaseFont and LargeFont, it's easy to switch from delta to
ratio, or to a smarter/heuristic way.

dschuyler

On 2015/04/02 at 06:39:29, yukishiino wrote: > On 2015/04/01 20:14:25, msw wrote: > > On ...

5 years, 8 months ago (2015-04-02 18:53:47 UTC) #14

On 2015/04/02 at 06:39:29, yukishiino wrote:
> On 2015/04/01 20:14:25, msw wrote:
> > On 2015/04/01 19:45:39, dschuyler wrote:
> > > On 2015/04/01 at 19:29:31, pkasting wrote:
> > > > On 2015/04/01 18:44:51, dschuyler wrote:
> > > > > The intended use is future support of the Answers in Suggest spec. 
It's
> > an
> > > > > internal doc, I can send you a link.  But the gist is that it defines
> > around
> > > a
> > > > > dozen predefined text style/property selections (e.g. type 1 text may
be
> > > 12pt
> > > > > font in blue; type 2 may be 18pt font in black and right justified;
and so
> > > on). 
> > > > > We've already taken some liberties to translate those into
> > > > > ui::ResourceBundle::FontStyle font sizes rather than using the exact
font
> > pt
> > > > > sizes.  That's at a higher level though, within omnibox_result_view.cc
for
> > > > > example.  I'm looking to get the font size from
> > > ui::ResourceBundle::FontStyle at
> > > > > the high level and tell RenderText to use font size N at the low
level.
> > > > > 
> > > > > If we move the code to different levels (other than
omnibox_result_view.cc
> > > and
> > > > > RenderText), I'd like to maintain using ui::ResourceBundle::FontStyle
at
> > the
> > > > > higher level and font size N at the lower level.
> > > > 
> > > > Can you say more about why using FontStyle down in the RenderText isn't
as
> > > good as using absolute sizes?  
> > > 
> > > :) I don't think I can.  It's an open question as far as I'm concerned. 
It
> > > sounds like I misguessed RenderText to be lower level than it is and that
I
> > > should consider it higher level.  I feel that using BaseFont and LargeFont
and
> > > so on at a high level and absolute sizes at a low level is a solid design.

> > I'm
> > > looking for feedback on whether RenderText is low level or high level -
that's
> > > what I'm not sure about.
> > > 
> > > > I would have instinctively gone the other way.  
> > > 
> > > Like at a different level (where RenderText is considered high level) or
> > somehow
> > > the opposite way around (which I'd be confused about). 
> > > 
> > > > I'm not sure what advantage we get by forcing the caller to obtain a
size
> > from
> > > the FontStyle when in general Chromium code never wants anyone to think in
> > > absolute sizes.
> > > > 
> > > > (Even thinking in relative sizes is discouraged.  We used to do more of
> > this,
> > > but the UI leads gave explicit direction a while back that as much as
possible
> > > we should restrict to just using the fixed BaseFont, LargeFont, etc.
values
> > and
> > > not deriving anything from them.  Some code is more compliant with that
than
> > > others.)
> > > 
> > > That makes sense.  It sounds like we'd still have the highest level
(BaseFont,
> > > LargeFont) and the lowest level (absolute font sizes).  My question is
where
> > the
> > > dividing line should be?  The two options I see so far are put the line
above
> > > RenderText or below RenderText.  I thought it may be right to put it above
> > > RenderText.  It sounds like that was the wrong choice and it should be
below
> > > RenderText(?).
> > 
> > I would consider RenderText fairly "low-level" and views::Label/Textfield
more
> > "high-level"; The omnibox result view has been an unusual RenderText user,
and
> > that has caused numerous complications. Regardless of those distinctions, I
> > think Peter and I are more inclined to make the "correct" behavior
(LargeFont
> > amid BaseFont) easy, and make "incorrect" behavior
("custom_obscure_font_family,
> > 15px" amid <system_font_family, system_font_size>) difficult. As such, I'm
not
> > adamantly opposed to allowing direct control of font sizes through
RenderText
> > API, but I'd encourage an API that doesn't promote/risk "incorrect" usage,
if an
> > API that encourages "correct" behavior is equally feasible and supports our
use
> > cases. Once the precedent is set by this CL, future changes become more
> > difficult, especially when going from widespread arbitrary low-level control
to
> > more consistent high-level control (see the uphill battles for
size->size_delta,
> > Font->FontList, etc.).
> > 
> > RenderText::SetFontList offer pretty low-level arbitrary font support, and a
> > size-delta (or maybe ratio) break list would offer pretty flexible control
on
> > top of that; I'd be surprised if users really required additional
"low-level"
> > controls that go against the design patterns for our UI. Further usage of
> > abstract font size enums (BaseFont, etc.) seems like a reasonable goal, but
may
> > not be critical...
> > 
> > > I'll rework this CL so that the dividing line is below RenderText (i.e.
> > HarfBuzz
> > > and peers would use absolute font sizes).
> 
> +1 for BaseFont, LargeFont, etc.  I agree to make the correct behavior easy
and incorrect ones difficult, and to have a loophole to specify the absolute
size as the last resort.
> 
> As long as we use BaseFont and LargeFont, it's easy to switch from delta to
ratio, or to a smarter/heuristic way.

Hi,

I've been looking at setting the font size based on
ui::ResourceBundle::FontStyle.  I've found a bit of a sticky situation with
RenderText::SetFontList.  So RenderText::SetFontList can set the font to *any*
size, but the sub-string modifications can then only set values based on
ui::ResourceBundle::FontStyle.  This feels inconsistent (and it's inconvenient).

We could change RenderText so that it doesn't accept a font list and only
accepts ui::ResourceBundle::FontStyle, but that's also a partial solution
because one would need to know how big the current font is to use a smaller or
larger font within the string.

How about one of these options to keep RenderText accepting a font list:

(A) How about having RenderText continue to accept a font list and I'll change
the font sizing within RenderText to accept a relative value to that font list. 
E.g. smaller, equal, and larger enum values (relative to the base font list --
somewhat like ui::ResourceBundle::FontStyle in concept, though not literally
ui::ResourceBundle::FontStyle).  
(B) Or, maybe having RenderText continue to accept a font list and I'll change
the font sizing within RenderText to accept a relative value to that font list
as a floating point ratio (and maybe cap the extremes to something like 0.1 to
10.0).

How does (A) or (B) sound?

msw

On 2015/04/02 18:53:47, dschuyler wrote: > On 2015/04/02 at 06:39:29, yukishiino wrote: > > On ...

5 years, 8 months ago (2015-04-02 19:15:29 UTC) #15

On 2015/04/02 18:53:47, dschuyler wrote:
> On 2015/04/02 at 06:39:29, yukishiino wrote:
> > On 2015/04/01 20:14:25, msw wrote:
> > > On 2015/04/01 19:45:39, dschuyler wrote:
> > > > On 2015/04/01 at 19:29:31, pkasting wrote:
> > > > > On 2015/04/01 18:44:51, dschuyler wrote:
> > > > > > The intended use is future support of the Answers in Suggest spec. 
> It's
> > > an
> > > > > > internal doc, I can send you a link.  But the gist is that it
defines
> > > around
> > > > a
> > > > > > dozen predefined text style/property selections (e.g. type 1 text
may
> be
> > > > 12pt
> > > > > > font in blue; type 2 may be 18pt font in black and right justified;
> and so
> > > > on). 
> > > > > > We've already taken some liberties to translate those into
> > > > > > ui::ResourceBundle::FontStyle font sizes rather than using the exact
> font
> > > pt
> > > > > > sizes.  That's at a higher level though, within
omnibox_result_view.cc
> for
> > > > > > example.  I'm looking to get the font size from
> > > > ui::ResourceBundle::FontStyle at
> > > > > > the high level and tell RenderText to use font size N at the low
> level.
> > > > > > 
> > > > > > If we move the code to different levels (other than
> omnibox_result_view.cc
> > > > and
> > > > > > RenderText), I'd like to maintain using
ui::ResourceBundle::FontStyle
> at
> > > the
> > > > > > higher level and font size N at the lower level.
> > > > > 
> > > > > Can you say more about why using FontStyle down in the RenderText
isn't
> as
> > > > good as using absolute sizes?  
> > > > 
> > > > :) I don't think I can.  It's an open question as far as I'm concerned. 
> It
> > > > sounds like I misguessed RenderText to be lower level than it is and
that
> I
> > > > should consider it higher level.  I feel that using BaseFont and
LargeFont
> and
> > > > so on at a high level and absolute sizes at a low level is a solid
design.
> 
> > > I'm
> > > > looking for feedback on whether RenderText is low level or high level -
> that's
> > > > what I'm not sure about.
> > > > 
> > > > > I would have instinctively gone the other way.  
> > > > 
> > > > Like at a different level (where RenderText is considered high level) or
> > > somehow
> > > > the opposite way around (which I'd be confused about). 
> > > > 
> > > > > I'm not sure what advantage we get by forcing the caller to obtain a
> size
> > > from
> > > > the FontStyle when in general Chromium code never wants anyone to think
in
> > > > absolute sizes.
> > > > > 
> > > > > (Even thinking in relative sizes is discouraged.  We used to do more
of
> > > this,
> > > > but the UI leads gave explicit direction a while back that as much as
> possible
> > > > we should restrict to just using the fixed BaseFont, LargeFont, etc.
> values
> > > and
> > > > not deriving anything from them.  Some code is more compliant with that
> than
> > > > others.)
> > > > 
> > > > That makes sense.  It sounds like we'd still have the highest level
> (BaseFont,
> > > > LargeFont) and the lowest level (absolute font sizes).  My question is
> where
> > > the
> > > > dividing line should be?  The two options I see so far are put the line
> above
> > > > RenderText or below RenderText.  I thought it may be right to put it
above
> > > > RenderText.  It sounds like that was the wrong choice and it should be
> below
> > > > RenderText(?).
> > > 
> > > I would consider RenderText fairly "low-level" and views::Label/Textfield
> more
> > > "high-level"; The omnibox result view has been an unusual RenderText user,
> and
> > > that has caused numerous complications. Regardless of those distinctions,
I
> > > think Peter and I are more inclined to make the "correct" behavior
> (LargeFont
> > > amid BaseFont) easy, and make "incorrect" behavior
> ("custom_obscure_font_family,
> > > 15px" amid <system_font_family, system_font_size>) difficult. As such, I'm
> not
> > > adamantly opposed to allowing direct control of font sizes through
> RenderText
> > > API, but I'd encourage an API that doesn't promote/risk "incorrect" usage,
> if an
> > > API that encourages "correct" behavior is equally feasible and supports
our
> use
> > > cases. Once the precedent is set by this CL, future changes become more
> > > difficult, especially when going from widespread arbitrary low-level
control
> to
> > > more consistent high-level control (see the uphill battles for
> size->size_delta,
> > > Font->FontList, etc.).
> > > 
> > > RenderText::SetFontList offer pretty low-level arbitrary font support, and
a
> > > size-delta (or maybe ratio) break list would offer pretty flexible control
> on
> > > top of that; I'd be surprised if users really required additional
> "low-level"
> > > controls that go against the design patterns for our UI. Further usage of
> > > abstract font size enums (BaseFont, etc.) seems like a reasonable goal,
but
> may
> > > not be critical...
> > > 
> > > > I'll rework this CL so that the dividing line is below RenderText (i.e.
> > > HarfBuzz
> > > > and peers would use absolute font sizes).
> > 
> > +1 for BaseFont, LargeFont, etc.  I agree to make the correct behavior easy
> and incorrect ones difficult, and to have a loophole to specify the absolute
> size as the last resort.
> > 
> > As long as we use BaseFont and LargeFont, it's easy to switch from delta to
> ratio, or to a smarter/heuristic way.
> 
> Hi,
> 
> I've been looking at setting the font size based on
> ui::ResourceBundle::FontStyle.  I've found a bit of a sticky situation with
> RenderText::SetFontList.  So RenderText::SetFontList can set the font to *any*
> size, but the sub-string modifications can then only set values based on
> ui::ResourceBundle::FontStyle.  This feels inconsistent (and it's
inconvenient).
> 
> We could change RenderText so that it doesn't accept a font list and only
> accepts ui::ResourceBundle::FontStyle, but that's also a partial solution
> because one would need to know how big the current font is to use a smaller or
> larger font within the string.

I don't quite understand the obstacle you're describing here, can you clarify?
If RenderText uses BaseFont by default, and we deprecate SetFontList in favor of
SetFontStyle (for SmallFont, LargeFont, etc.) then shouldn't that allow the
flexibility we want? What information would RenderText users lack? We could
continue allowing SetFontList for now, and just DCHECK and no-op on use of
SetFontStyle with a custom FontList; then we can slowly convert SetFontList
users to the new pattern.

> How about one of these options to keep RenderText accepting a font list:
> 
> (A) How about having RenderText continue to accept a font list and I'll change
> the font sizing within RenderText to accept a relative value to that font
list. 
> E.g. smaller, equal, and larger enum values (relative to the base font list --
> somewhat like ui::ResourceBundle::FontStyle in concept, though not literally
> ui::ResourceBundle::FontStyle).  
> (B) Or, maybe having RenderText continue to accept a font list and I'll change
> the font sizing within RenderText to accept a relative value to that font list
> as a floating point ratio (and maybe cap the extremes to something like 0.1 to
> 10.0).
> 
> How does (A) or (B) sound?

Given those options, I think I prefer (B). I'd actually prefer using deltas
instead of ratios until we devise a more unified and well-tested DeriveFont
scheme for ratios.

Peter Kasting

Dave, this CL has been open for a year with no comments. I didn't bother ...

4 years, 7 months ago (2016-04-29 01:56:38 UTC) #16

dschuyler

dschuyler@chromium.org changed reviewers: - msw@chromium.org, pkasting@chromium.org, yukishiino@chromium.org

4 years, 4 months ago (2016-08-06 00:16:58 UTC) #17

dschuyler

4 years, 4 months ago (2016-08-06 00:19:48 UTC) #18

Message was sent while issue was closed.

On 2016/04/29 01:56:38, Peter Kasting wrote:
> Dave, this CL has been open for a year with no comments.  I didn't bother to
> read the review discussion above, so I don't know whether there's still value
> here, but please either move this forward or close as applicable.

Making a note that this CL is marked 'do not check in' and closed; so that
Peter's
message is not as prominent (and looking unanswered).

Issue 1020853018: DNCI [RenderText] Added font size options in RenderText. (Closed)

Description

Patch Set 1 #

Patch Set 2 : Added font sizes to mac font style iterator #

Patch Set 3 : fix for RenderText bounding rectangles #

Patch Set 4 : changed CL parent #

Patch Set 5 : Removed some debug code #

Messages