777089 - consider changing size mapping function for font inflation

Status: RESOLVED FIXED

(Core :: Layout, defect, P3)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

One piece of feedback from Madhava and Ian Barlow that I got in a discussion last week was that our font inflation algorithm tends to reduce the distinctions in the visual hierarchy of a page.

One thing we can that may address this is to change the mapping function that maps the specified font size to the inflated font size, which is currently somewhat flat, but which I haven't done much experimentation with.

Currently the way font inflation maps font sizes is that:
 (1) We compute the minimum readable font size (m) for a particular region of text so that it will be readable when the container is zoomed to the width of the device.  (>99% of the complexity lives here, but it's not relevant to this bug.)

 (2) We map specified font sizes (s) to inflated font sizes (i) using the function:
       i = m + s/3, when s <= 3·m/2
       i = s      , when s >= 3·m/2


We should compare this behavior to the following possible replacements for step 2:

  (2b):
       i = m + 2·s/3, when s <= 3·m
       i = s        , when s >= 3·m

  (2c):
       i = m + s

I'd like to not have to change this behavior too often, because we have a bunch of tests that depend on it.  Alternatively, we could make it pref-controlled, which might be better, though it's not clear to me what the ideal way to pref-control it is.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The code in question is currently:
https://hg.mozilla.org/mozilla-central/file/34b14c220817/layout/base/nsLayoutUtils.cpp#l4826

Comment 2

[Jet Villegas (inactive)]

P3 per font inflation scrub.

Comment 3

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] from comment #0)
> I'd like to not have to change this behavior too often, because we have a
> bunch of tests that depend on it.  Alternatively, we could make it
> pref-controlled, which might be better, though it's not clear to me what the
> ideal way to pref-control it is.

We could pref-control this by having preferences for the constants in the equation. So, in other words, we could change our equations to:

i = (X)m + (Y) (s/3), when s <= 3·m
i = s            , when s >  3·m

and then have the prefs be something like:

X = font.size.inflation.minConstant
Y = font.size.inflation.sizeConstant

(or something similar with more intuitive names) so that UX can iterate on this more quickly and determine the ideal prefs.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] from comment #1)
> The code in question is currently:
> https://hg.mozilla.org/mozilla-central/file/34b14c220817/layout/base/
> nsLayoutUtils.cpp#l4826

To be clear, this code returns i/s, not i.

Comment 5

[Scott Johnson (:jwir3)]

Attachment: b777089

Added prefs to control the coefficients of the scaling algorithm. I didn't initially cache these in the pres shell, but I can if you think it would be better.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 648443 [details] [diff] [review]
b777089

Two things here:

(1) I think we probably do need to cache the pref somewhere for performance.  The pres shell would probably be good in terms of matching our change handling for the other prefs.

(2) I don't think the two-parameter set of prefs you've chosen is the right thing here:  I don't think it actually lets us test the possibilities that we want to test (see comment 0), and I think we also only need a single parameter.

Also, this code:
   if (ratio >= 1.5f) {
     // If we're already at 1.5 or more times the minimum, don't scale.
     return 1.0;
   }
is a part of the formula -- it's the part that implements the set of conditions.



I think this is better implemented as a single integer parameter P, which is interpreted as:
 (a) if P >= 0, then make the mapping function intersect i=s at s=(1+(P/2))·m
 (b) if P < 0, then make the mapping function just be i = m + s (this essentially implements P=∞)

This would allow our current behavior to be P=1, (2b) in comment 0 to be P=4, and (2c) in comment 0 to be P=-1 (or any other negative value).

Comment 7

[Scott Johnson (:jwir3)]

Attachment: b777089 (v2)

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 649022 [details] [diff] [review]
b777089 (v2)

>+  PRUint32 interceptParam = nsLayoutUtils::FontSizeInflationInterceptValue();
...
>+  if (interceptParam >= 0) {

This needs to be signed (throughout, if the function isn't signed as well).

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 649022 [details] [diff] [review]
b777089 (v2)

(In reply to David Baron [:dbaron] from comment #8)
> This needs to be signed (throughout, if the function isn't signed as well).

And that includes the pref, and using AddIntVarCache instead of AddUintVarCache.


Maybe also change the name interceptValue -> mappingIntercept.


>BUg 777089: Add preferences to control min font size calculation function coefficients for font inflation. [r=?]

BUg -> Bug

Also, maybe change the description to:

Add preference to control the function used to map specified font sizes to inflated font sizes.

>+  PRUint32 interceptParam = nsLayoutUtils::FontSizeInflationInterceptValue();
>+
>+  // Scale everything from 0- times min to instead fit in the range
>+  // 1-(1+P/2) times min, so that we still show some distinction rather than

Just dropping the "1.5" from the first sentence here doesn't leave it quite right.  I'd suggest dropping this comment and having separate comments in the two branches of the if explaining the function used in each branch.


>+    float intercept = 1 + (interceptParam/2.0f);

I'd explicitly cast interceptParam to a float, for clarity, so it's instead:
  float intercept = 1.0f + float(interceptParam)/2.0f;

>diff --git a/modules/libpref/src/init/all.js b/modules/libpref/src/init/all.js

>+/*
>+ * Defines the font size inflation "intercept value", which is used
>+ * to determine where the font size inflation mapping function meets
>+ * the i=s line.

This sort of jumps into the middle of the problem without explaining the beginning.  I think it would be useful here to explain that font inflation computes a minimum font size, and we then need to decide how (given that minimum) to map specified font sizes to inflated font sizes.  Then explain how negative values yield i = s + m, but positive values lead to a graph that meets i = s when s is large enough (but still starts at i=m when s=0).

Other than that (which is mostly about the comments, plus the signedness issue) this looks good, though.

Comment 10

[Scott Johnson (:jwir3)]

Attachment: b777089 (v3)

Adjusted patch for review comments.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 650985 [details] [diff] [review]
b777089 (v3)

>+  if (interceptParam >= 0) {

You should start this by saying something like:

  Given a minimum inflated font size m, a specified font size s, we want to find the inflated font size i but then actually return i/s.

>+    // Since the mapping intercept is greater than zero, we use it as the point

"mapping intercept" -> "mapping intercept parameter P"

"use it as" -> "use it to determine"

>+    // where our mapping function intersects the i=s line. This means that we
>+    // have an equation of the form:
>+    // i/s = (1 - 2/(2+P))s + m, but only if s >= (1 + P/2)m

This equation should probably be written as:
  i = m + s·(P/2)/(1 + P/2), if s <= (1 + P/2)·m
  i = s, if s >= (1 + P/2)·m

>+    float intercept = 1 + float(interceptParam)/2.0f;
>+    if (ratio >= intercept) {
>+      // If we're already at 1+P/2 or more times the minimum, don't scale.
>+      return 1.0;
>+    }
>+
>+    // The point (intercept, intercept) is where the part of the graph

graph -> i vs. s graph

>+    // that's not slope 1 intersects the i=s line.  (This part of the

intersects -> meets

>+    // graph is a line from (0, m), to that point). We calculate the
>+    // intersection point to be ((1+P/2)m, (1+P/2)m), where P is the
>+    // intercept parameter above.

Probably add:
  And then we need to return i/s.

>+    return (1.0f + (ratio * (intercept - 1) / intercept)) / ratio;



>+/*
>+ * Defines the font size inflation mapping intercept.

mapping intercept -> mapping intercept parameter

>+ *
>+ * Font size inflation computes a minimum font size based on

minimum font size -> minimum font size, m,

>+ * other preferences (see font.size.inflation.minTwips and
>+ * font.size.inflation.emPerLine, above) and the width of
>+ * the frame in which the text resides. Using this minimum,
>+ * a specified font size, s, is mapped to an inflated font
>+ * size, i, using an equation that varies depending on the
>+ * value of the font size inflation mapping intercept, P:

intercept -> intercept parameter here, and below

>+ *
>+ * If the intercept is negative, then the following mapping
>+ * function is used:
>+ * 
>+ * i = m + s
>+ *
>+ * If the intercept is non-negative, then the mapping function
>+ * is a function such that its graph meets the graph of i = s
>+ * at the specified intercept point for values of s that are

the specified intercept point -> the point where both i and s are (1 + P/2)·m

>+ * large enough. For s = 0, the function i = m is used.

This isn't really a special case; I'd either drop this sentence or say instead:

This means that when s=0, i is always equal to m.


r=dbaron with that

Comment 12

[Scott Johnson (:jwir3)]

Changes made and pushed to try:
https://hg.mozilla.org/try/rev/d94094360441

Comment 13

[Scott Johnson (:jwir3)]

Inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a9be0ad3321f

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

What developer documentation do you think is needed here?  I think we mainly want to use these preferences to figure out what the right value to ship is -- we don't want to encourage people to change what we ship.

Comment 15

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] from comment #14)
> What developer documentation do you think is needed here?  I think we mainly
> want to use these preferences to figure out what the right value to ship is
> -- we don't want to encourage people to change what we ship.

Ah, whoops. I assumed that dev-doc-needed meant that we wanted to document it for internal developers (i.e. so that Madhava and Ian know how to use it). This is probably not the correct interpretation of this flag, though, now that I think about it.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a9be0ad3321f

Comment 17

[John Mellor]

Hi folks, I'm working on the upstream WebKit implementation of Text Autosizing (our equivalent to Font Inflation, following on from downstream Chrome for Android's Font Boosting, and Mobile Safari's text size adjustments; you can follow the master bug at http://webkit.org/b/84186).

I'm currently implementing our size mapping function (http://webkit.org/b/94227), and I agree that it makes sense for the function to join the i = s line for high values of s (they're already big and legible, so best not to inflate them as it just worsens the layout).

However I don't see the benefit of having i = m when s = 0. In such cases the author has explicitly chosen to make the text hard to read / illegible, and I don't see why we should override that and make the text easier to read on mobile than it was on desktop.

I posted an alternative proposal based on Chrome for Android on http://webkit.org/b/94227, including a graph visualizing this size mapping function versus Firefox's current and proposed size mapping functions, together with some explanation and discussion, and a patch to implement it in WebKit.

I recommend reading that and looking at the linked graph first, but here's my proposal in the notation used above:
      i = s·m/12                    , when s <= 16
      i = max(s, 16·m/12 + (s-16)/2), when s >= 16

Or, equivalently, factoring out the max:
      i = s·m/12            , when s <= 16
      i = 16·m/12 + (s-16)/2, when s >= 16 and s <= 16·(2·m/12 - 1)
      i = s                 , when s >= 16·(2·m/12 - 1)

The constants 12, 16 and 1/2 can be tweaked.
- "12" should be the minimum font size for non-inflated blocks,
  i.e. 1440 / sFontSizeInflationMinTwips if MinTwips is dominating
  EmPerLine, such that m/12 = aContainerWidth/deviceWidthInches.
- "16" is an arbitrary "pleasant" font size, such that all font
  sizes below it are scaled by the full m/12, but font sizes above
  it don't need to be quite as inflated.
- "1/2" as in (s-16)/2 determines the gradient of the graph between
  s = 16 and the point where the graph meets the i = s line. Note,
  that since this gradient is fixed instead of fixing the meeting
  point, the value of s at which they meet depends on how big m is.

Again, it'll probably be easier to see this on the graph linked from http://webkit.org/b/94227.

How does this sound? It would be useful for us to converge on the same mapping function, as part of eventually making font inflation compatible across browsers, to make life easier for web developers.