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

RESOLVED FIXED in Firefox 24

Status

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: jwir3, Assigned: jwir3)

Tracking

unspecified
Firefox 25
Dependency tree / graph

Firefox Tracking Flags

(firefox24 fixed, firefox25 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

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.
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).
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.
Summary: Reflow-on-zoom should utilize the font-inflation minTwips and emPerLine preferences instead of its own → Reflow-on-zoom should utilize the font-inflation minTwips preference instead of its own
Posted patch b878931 (obsolete) — Splinter Review
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.
Attachment #760526 - Flags: review?(Ms2ger)
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?
(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...
Attachment #760526 - Flags: review?(Ms2ger)
Posted patch b878931 (v2) (obsolete) — Splinter Review
Fixed comments made by Ms2ger, and now transferring review to an actual content peer. ;)
Attachment #760526 - Attachment is obsolete: true
Attachment #761675 - Flags: review?(khuey)
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.
Attachment #761675 - Flags: review?(khuey)
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. ;)
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/sjohnson@mozilla.com-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/sjohnson@mozilla.com-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?
Flags: needinfo?(ibarlow)
Flags: needinfo?(blassey.bugs)
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
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).
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.
Flags: needinfo?(blassey.bugs)
Posted patch b878931 (v3) (obsolete) — Splinter Review
(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.
Attachment #761675 - Attachment is obsolete: true
Attachment #767792 - Flags: review?(bugmail.mozilla)
Flags: needinfo?(ibarlow)
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
Attachment #767792 - Flags: review?(bugmail.mozilla) → review+
Posted patch b878931 (v4)Splinter Review
Fixed the issues you were concerned about, kats. Thanks for the preliminary review.
Attachment #767792 - Attachment is obsolete: true
Attachment #768352 - Flags: review+
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!
Attachment #768352 - Flags: review?(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);
Attachment #768352 - Flags: review?(mounir) → review+
(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!
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.
(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?
(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?
(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.
Cool, I'll go ahead and land it then. Ran through try-server:
https://tbpl.mozilla.org/?tree=Try&rev=ced15f9c05fa
Inbound, with changes requested by Mounir:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76b183c0ef36
Duplicate of this bug: 876161
https://hg.mozilla.org/mozilla-central/rev/76b183c0ef36
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Duplicate of this bug: 878928
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.
Attachment #768352 - Flags: approval-mozilla-aurora?
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.
Attachment #768352 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Depends on: 890309
Product: Firefox for Android → Fennec Graveyard
You need to log in before you can comment on or make changes to this bug.