878931 - Reflow-on-zoom should utilize the font-inflation minTwips preference instead of its own

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Readability, defect)

Description

[Scott Johnson (:jwir3)]

Currently, reflow-on-zoom has its own font size preferences. Instead of this, we should utilize the font size inflation preferences so the user only has to specify their desired font size once.

Comment 1

[Scott Johnson (:jwir3)]

Hm, so one thing I forgot was that font size inflation isn't supposed to be detectable by the content, I don't think. AFAICT, it's not detectable from within browser.js directly, either... I can detect the value of the preference, and the font size of a block of text that the author intended (from the computed style), but I don't think we can query Gecko for the current font inflation for a given element. 

Thus, I can't determine if a given element on which a double-tap event occurs should trigger the reflow-on-zoom, dependent on whether it's already font-inflated or not. It looks like what we do right now is to zoom in a proportional amount, based on 1) what the author's requested font size was and 2) what the font size would be, if font inflated (by applying the minTwips preference).

Comment 2

[Scott Johnson (:jwir3)]

Also, I don't think we should utilize the 'emPerLine' preference. Since the user-visible preference only affects 'minTwips', I think we should stick with this one.

Comment 3

[Scott Johnson (:jwir3)]

Attachment: b878931

This patch adds a font size inflation API, accessible from chrome-only javascript, that retrieves the inflated font size for a given element. This is then used in coordination with reflow-on-zoom to determine if zooming on double-tap should be enabled, and if so, by how much after the event takes place.

Ms2ger, I'm requesting review from you initially as a content peer, to verify the actual API. I'm also going to request review from either blassey or kats on the mobile team, so don't worry too much about the browser.js changes.

Comment 4

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 760526 [details] [diff] [review]
b878931

Review of attachment 760526 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/webidl/Node.webidl
@@ +103,5 @@
> +   * @returns The font size inflation ratio (inflated font size to uninflated
> +   *          font size) for the primary frame of this element. Returns 1.0
> +   *          by default if font size inflation is not enabled.
> +   *
> +   * @throws NS_ERROR_DOM_INVALID_NODE_TYPE_ERR if this node is not an element.

That doesn't make sense. If you only want it on elements, put it on the Element interface.

::: mobile/android/chrome/content/browser.js
@@ +193,5 @@
>    return (Math.abs(a - b) < 1e-6);
>  }
>  
>  /**
> + * Convert a font size to CSS pixels (px) from twenteiths-of-a-point

twenteiths?

Comment 5

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to Scott Johnson (:jwir3) from comment #3)
> Ms2ger, I'm requesting review from you initially as a content peer

Not that I'm one...

Comment 6

[Scott Johnson (:jwir3)]

Attachment: b878931 (v2)

Fixed comments made by Ms2ger, and now transferring review to an actual content peer. ;)

Comment 7

[Scott Johnson (:jwir3)]

Comment on attachment 761675 [details] [diff] [review]
b878931 (v2)

I'm canceling review for this temporarily while I sort out a couple of issues with this.

Comment 8

[Scott Johnson (:jwir3)]

So, a short synopsis of what kats and I discussed today about this behavior. The salient points are as follows:

- If we have a block of text that has a specified size of 4px which should be inflated to 8px, based on a current minTwips setting of X, but isn't due to some other constraint (e.g. text-size-adjust: none), then ideally we would like to have our getZoomToMinFontSize() function return 8/4 = 2.0. Unfortunately, it is sometimes the case that we can't retrieve 8px. We have only the specified font size, 4px, and the minimum font size tolerable, X.

- If X happens to be greater than 4px, then we would reflow-on-zoom to a new zoom level of X/4. But, if 4 is already greater than X, then it's probably ok already anyway, because we've specified that X is our minimum readable font size.

- The problem with the above approach is that it is very often the case that we fall into that latter category, even if fonts are not actually readable. One solution to this problem would be for the user to increase the minimum font size for them to be able to read the page. Another approach is to have some specified percentage, say 20% (as a preference) that will allow the user to zoom in to a zoom level where the minimum font size is readable, plus this percentage. For example, if we had a minTwips setting of 120, then we would actually be using 120*1.2 = 144 as our "minTwips" value for reflow-on-zoom. If the font size is still greater than this, then no zooming is necessary. 

I am going to post a couple of builds today to see which of these are better, and then I'll let others play with it while I'm gone next week. ;)

Comment 9

[Scott Johnson (:jwir3)]

Ok, the build that utilizes just the font inflation "minTwips" value to compare is available at:
http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/[e-mail]-53941ab50955/try-android/fennec-24.0a1.en-US.android-arm.apk

And the build that will zoom to make the text equivalent to "minTwips" + some percentage, which is determined by the "browser.zoom.reflowZoom.additionalZoomPercentage" (by default 20) is available at:
http://ftp.mozilla.org/pub/mozilla.org/mobile/try-builds/[e-mail]-0da70a0a366a/try-android/fennec-24.0a1.en-US.android-arm.apk

Ian, would you and/or Blassey try these out and see which you prefer?

Comment 10

[Scott Johnson (:jwir3)]

Blassey noted that the following site:

http://thehackernews.com/2013/06/Bitcoin-cease-order-California.html

does not work with either of these. I'm seeing the same behavior, however, I think it has something to do with the popup they have on this site. I think the double-tap behavior might be getting interfered with by this thing. If I change the font inflation sizes, I can see it zooming in after a double-tap as I would expect, although it's not clear to me that it's reflowing as I would expect, because I can't see the text underneath the popup thing.

At any rate, I suggest trying other sites: cnn.com, nytimes.com, news.ycombinator.com (as this was one of the problems with the font inflation itself), and the following "test" sites I have created:

http://people.mozilla.org/~sjohnson/fables/monkeycat.html
http://people.mozilla.org/~sjohnson/fables/monkeycat-noinfl.html (this is the same, but with font inflation disabled)
http://people.mozilla.org/~sjohnson/junkyard/lemis.html
http://people.mozilla.org/~sjohnson/junkyard/desperationSamba.html

Comment 11

[Scott Johnson (:jwir3)]

Also, I'd recommend trying (for the second build) a percentage that's a bit larger (say 100, instead of 20). I just guessed at the amount, but the problem is that we're using the minTwips value, not the actual font value. 

So, a site might already have fonts that are 120% * minTwips, but still be font-inflated (i.e. we're not zooming in 20% more than the font inflation value, but 20% more than the minTwips value). I'll make another build with the latter value, too, so you can test that one, but I think perhaps it might be too much of a change. (i.e. it'll be essentially the same as having another minTwips preference for reflow-on-zoom).

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

I 100% prefer the first build. 

A good example of why I don't like the behavior of the second build is the "The Latest" section of cnn.com. I double tap to zoom into it, and it is perfectly legible (because it is above the minTwips size) and then we go and reflow unnecessarily to make the text 20% bigger.

In the end, font inflation is the thing we want to work without the user ever thinking about it. Sometimes it isn't able to get us all the way there though, which is when double tap to reflow should kick in. No matter what, the reflow is a jarring transition (even in browsers that do it better than we do, and to be clear I think we can get there) so we shouldn't doing it if avoidable.

If the user wants a bigger text size than minTwips, they should adjust their text size pref, not mysteriously reflow when they double tap.

Comment 13

[Scott Johnson (:jwir3)]

Attachment: b878931 (v3)

(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #12)
> I 100% prefer the first build. 
> 
> A good example of why I don't like the behavior of the second build is the
> "The Latest" section of cnn.com. I double tap to zoom into it, and it is
> perfectly legible (because it is above the minTwips size) and then we go and
> reflow unnecessarily to make the text 20% bigger.
> 
> In the end, font inflation is the thing we want to work without the user
> ever thinking about it. Sometimes it isn't able to get us all the way there
> though, which is when double tap to reflow should kick in. No matter what,
> the reflow is a jarring transition (even in browsers that do it better than
> we do, and to be clear I think we can get there) so we shouldn't doing it if
> avoidable.
> 
> If the user wants a bigger text size than minTwips, they should adjust their
> text size pref, not mysteriously reflow when they double tap.

Agreed. I've integrated this into a new patch for this bug.

Kats, if you wouldn't mind doing a preliminary review to make sure this addresses the issues we talked about previously, I would appreciate it. I will forward it to a content peer once your review is complete and I address any issues you may have, so no need to worry too much about the changes to the content modules.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 767792 [details] [diff] [review]
b878931 (v3)

Review of attachment 767792 [details] [diff] [review]:
-----------------------------------------------------------------

This one makes a lot more sense to me. I didn't check the pref removal - presumably that's unused now?

::: mobile/android/chrome/content/browser.js
@@ +2618,2 @@
>      // This is in px, so we want to convert it to points then to twips.
> +    return fontSize*fontSizeInfl;

nit: add spaces around the '*'. Also remove the comment line just above about the twips which is no longer valid

@@ +2625,5 @@
>     * preference.
>     */
>    getZoomToMinFontSize: function(aElement) {
> +    let fontSizeStr = this.window.getComputedStyle(aElement)['fontSize'];
> +    let fontSize = fontSizeStr.slice(0, -2);

The above two lines are not used, can be removed

Comment 15

[Scott Johnson (:jwir3)]

Attachment: b878931 (v4)

Fixed the issues you were concerned about, kats. Thanks for the preliminary review.

Comment 16

[Scott Johnson (:jwir3)]

Comment on attachment 768352 [details] [diff] [review]
b878931 (v4)

Hm, so apparently sr? is gone now, so I'm requesting a normal review of the content changes made in this patch. Basically, I'm just looking to make sure the API is sane, that it's in the right place, and that it works as you would expect.

Thanks, Mounir!

Comment 17

[Mounir Lamouri (:mounir)]

Comment on attachment 768352 [details] [diff] [review]
b878931 (v4)

Review of attachment 768352 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the comments applied or answered.

::: content/base/public/Element.h
@@ +934,5 @@
> +   * @note The font size inflation ratio that is returned is actually the
> +   *       font size inflation data for the element's _primary frame_, not the
> +   *       element itself, but for most purposes, this should be sufficient.
> +   */
> +  virtual float GetFontSizeInflation(mozilla::ErrorResult& aError);

Don't make the method virtual unless you expect it to be overridden which doesn't seem to be the intended design here.

::: content/base/src/Element.cpp
@@ +3545,5 @@
> +{
> +  nsIFrame* frame = mPrimaryFrame;
> +  if (!frame) {
> +    aError.Throw(NS_ERROR_NULL_POINTER);
> +    return -1;

Why not returning -1 or 0 instead of throwing?

::: mobile/android/chrome/content/browser.js
@@ +2614,5 @@
>      // GetComputedStyle should always give us CSS pixels for a font size.
>      let fontSizeStr = this.window.getComputedStyle(aElement)['fontSize'];
>      let fontSize = fontSizeStr.slice(0, -2);
> +    let fontSizeInfl = aElement.getFontSizeInflation();
> +    return fontSize * fontSizeInfl;

nit:
return aElement.getFontSizeInflation() * fontSize;

@@ +2630,5 @@
> +    // effect reflow-on-zoom.
> +    let minFontSize = convertFromTwipsToPx(Services.prefs.getIntPref("font.size.inflation.minTwips"));
> +    let curFontSize = this.getInflatedFontSizeFor(aElement);
> +
> +    return minFontSize / curFontSize;

nit:
return minFontSize / this.getInflatedFontSizeFor(aElement);

Comment 18

[Scott Johnson (:jwir3)]

(In reply to Mounir Lamouri (:mounir) from comment #17)
> Comment on attachment 768352 [details] [diff] [review]
> b878931 (v4)
> 
> Review of attachment 768352 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the comments applied or answered.
> 
> ::: content/base/public/Element.h
> @@ +934,5 @@
> > +   * @note The font size inflation ratio that is returned is actually the
> > +   *       font size inflation data for the element's _primary frame_, not the
> > +   *       element itself, but for most purposes, this should be sufficient.
> > +   */
> > +  virtual float GetFontSizeInflation(mozilla::ErrorResult& aError);
> 
> Don't make the method virtual unless you expect it to be overridden which
> doesn't seem to be the intended design here.

Yep, you're right. I'll change it.

> ::: content/base/src/Element.cpp
> @@ +3545,5 @@
> > +{
> > +  nsIFrame* frame = mPrimaryFrame;
> > +  if (!frame) {
> > +    aError.Throw(NS_ERROR_NULL_POINTER);
> > +    return -1;
> 
> Why not returning -1 or 0 instead of throwing?

Well, I thought that throwing was a better method of indicating error in this case, but I can return -1 if you'd prefer. I'll change that.

> 
> ::: mobile/android/chrome/content/browser.js
> @@ +2614,5 @@
> >      // GetComputedStyle should always give us CSS pixels for a font size.
> >      let fontSizeStr = this.window.getComputedStyle(aElement)['fontSize'];
> >      let fontSize = fontSizeStr.slice(0, -2);
> > +    let fontSizeInfl = aElement.getFontSizeInflation();
> > +    return fontSize * fontSizeInfl;
> 
> nit:
> return aElement.getFontSizeInflation() * fontSize;

Changed.

> 
> @@ +2630,5 @@
> > +    // effect reflow-on-zoom.
> > +    let minFontSize = convertFromTwipsToPx(Services.prefs.getIntPref("font.size.inflation.minTwips"));
> > +    let curFontSize = this.getInflatedFontSizeFor(aElement);
> > +
> > +    return minFontSize / curFontSize;
> 
> nit:
> return minFontSize / this.getInflatedFontSizeFor(aElement);

Changed.

Thanks for the review!

Comment 19

[Mounir Lamouri (:mounir)]

By the way, Scott, I forgot that but is there a reason why getFontSizeInflation is a method and not a readonly attribute? It could be .fontSizeInflation instead of .getFontSizeInflation(). It would follow the conventions of the Web Platform better I believe.

Comment 20

[Scott Johnson (:jwir3)]

(In reply to Mounir Lamouri (:mounir) from comment #19)
> By the way, Scott, I forgot that but is there a reason why
> getFontSizeInflation is a method and not a readonly attribute? It could be
> .fontSizeInflation instead of .getFontSizeInflation(). It would follow the
> conventions of the Web Platform better I believe.

Yes, I suppose we could do this, and then cache it. I'll change it - do you want to re-review after that change is made?

Comment 21

[Scott Johnson (:jwir3)]

(In reply to Scott Johnson (:jwir3) from comment #20)
> (In reply to Mounir Lamouri (:mounir) from comment #19)
> > By the way, Scott, I forgot that but is there a reason why
> > getFontSizeInflation is a method and not a readonly attribute? It could be
> > .fontSizeInflation instead of .getFontSizeInflation(). It would follow the
> > conventions of the Web Platform better I believe.
> 
> Yes, I suppose we could do this, and then cache it. I'll change it - do you
> want to re-review after that change is made?

Ah, wait... nevermind. The reason I made it a method was so that I could add [ChromeOnly]. Can this be added to attributes as well?

Comment 22

[Mounir Lamouri (:mounir)]

(In reply to Scott Johnson (:jwir3) from comment #20)
> Yes, I suppose we could do this, and then cache it. I'll change it - do you
> want to re-review after that change is made?

If you just change the WebIDL from a method to a readonly attribute, no need for me to review the change.

> Ah, wait... nevermind. The reason I made it a method was so that I could add [ChromeOnly]. 
> Can this be added to attributes as well?

I think it should work as well.

Comment 23

[Scott Johnson (:jwir3)]

Cool, I'll go ahead and land it then. Ran through try-server:
https://tbpl.mozilla.org/?tree=Try&rev=ced15f9c05fa

Comment 24

[Scott Johnson (:jwir3)]

Inbound, with changes requested by Mounir:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76b183c0ef36

Comment 26

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/76b183c0ef36

Comment 28

[Scott Johnson (:jwir3)]

Comment on attachment 768352 [details] [diff] [review]
b878931 (v4)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): reflow-zoom
User impact if declined: Reflow on zoom will be less usable, more jumpy.
Testing completed (on m-c, etc.): Testing completed locally, has been on m-c for one day.
Risk to taking this patch (and alternatives if risky): This should be very straightforward. It does change a webidl interface, but only for chrome javascript. Other than the changes to Element.webidl, and the Element class, there are only changes to browser.js. I would argue that this is reasonably low risk.
String or IDL/UUID changes made by this patch: I didn't change any strings or UUIDs, however, I'm not sure if webidl has a generated UUID. If so, then the UUID would likely have changed, as the Element.webidl interface changed.

Comment 29

[Alex Keybl [:akeybl]]

Comment on attachment 768352 [details] [diff] [review]
b878931 (v4)

Very early in the cycle, and low risk. Let's let this skip up a version.

Comment 30

[Scott Johnson (:jwir3)]

Uplifted to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/afb602f09bc9