746966 - Main body text on randsinrepose.com is not enlarged by font inflation

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (04/19) - Screenshot

Was demonstrating Nightly to a passenger on the subway last night and the first thing they asked when browsing was "Can you increase the text size?". I switched the text-size from the default 'Medium' to 'Extra Large', and was disappointed to find that this nearly did nothing other than inflate the size of undesired areas of text such as an article on a page. 

All text-content should be effected by text-size change.

See screenshot for before and after

--
Nightly (04/19)
Galaxy Nexus (Android 4.0.4)

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

David, Scott: Just wanted to check with you. Is this the expected behavior? We thought it might be, since the sidebar has slightly smaller text and got bumped. But the main body text was not found to be small enough to be affected. Is this right?

Comment 2

[Matt Brubeck (:mbrubeck)]

The main body text is definitely small enough that it should be inflated (especially with the pref set to "Extra Large").  That text must be triggering one of our heuristics that disables font inflation.

(In reply to Aaron Train [:aaronmt] from comment #0)
> All text-content should be effected by text-size change.

Inflating *all* text content breaks the layout on too many sites, so we have various rules for which text is or isn't inflated.  Sometimes those rules make the wrong decision, as in this bug.  Hopefully they can be fixed.

Comment 3

[Aaron Train [:aaronmt]]

(I'm CC'ing Madhava here for thoughts because I think it's bad UX to advertise a pref that doesn't do what you think it should do, if such turns out the case to be that it's working correctly.)

Comment 4

[JP Rosevear [:jpr]]

It seems like we are not inflating the main body text when zoomed in.

Comment 5

[Aaron Train [:aaronmt]]

(In reply to JP Rosevear [:jpr] from comment #4)
> It seems like we are not inflating the main body text when zoomed in.

There is no zooming; just toggling Font-Size in the settings doesn't do what's expected. Chrome Beta/Stock scale the body text properly on self-adjustment in settings.

Comment 6

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

Presumably, the function ShouldInflateFontsForContainer in nsLayoutUtils.cpp is returning false, most likely I'd guess because of NS_FRAME_IN_CONSTRAINED_HEIGHT is set (though that's worth checking).  The thing to do here is:
 * debug which condition is returning false (which is likely to fall back to debugging the conditions where we set NS_FRAME_IN_CONSTRAINED_HEIGHT)
 * see if there's some condition that gives us a reasonable way to fix this (i.e., make it not return false)

Comment 7

[Scott Johnson (:jwir3)]

So, I think there are two problems here:

1) The font-inflation isn't working correctly on the specified site. This might be difficult to fix, but I'm going to look into what dbaron talked about. 

2) The font-size control in the settings doesn't do anything. 

#2 seems like a bigger problem to me. My understanding was that font-size from within the preferences menu on fennec native was akin to Preferences->Content->Font Size on desktop. That is, it unilaterally changes the font sizes on the page, making things easier to read by increasing things by a large multiple. I would expect this is going to break layout.

In other words, I think we should spin off the UI issue (#2) into a separate bug.

Comment 8

[Scott Johnson (:jwir3)]

Ok, after speaking with mbrubeck and AaronMT about this, I think I was incorrect in comment 7. I think we should transition away from "Font Size" as a name, and perhaps make it something like "Text Inflation Factor" (although that sounds pretty sterile and tech-y). It's unclear (to me, at least), that it actually has anything to do with font inflation rather than just something like increasing the minimum font size across all pages.

Comment 9

[Matt Brubeck (:mbrubeck)]

(In reply to Scott Johnson (:jwir3) from comment #7)
> #2 seems like a bigger problem to me. My understanding was that font-size
> from within the preferences menu on fennec native was akin to
> Preferences->Content->Font Size on desktop. That is, it unilaterally changes
> the font sizes on the page, making things easier to read by increasing
> things by a large multiple.

And just to confuse things, that preference on desktop does not affect the body text on randsinrepose.com (or many other sites) either.  It affects only text whose font-size is specified in multiples of the default size (e.g. percent or em units).

The closest thing in desktop Firefox to the expected behavior is to set browser.zoom.full to "false" and then use ctrl-plus to zoom in.  This is disabled by default and available only through a hidden pref because - again - enlarging all fonts on a page will often mangle layouts and make pages unreadable or unusable.

Comment 10

[Scott Johnson (:jwir3)]

After looking at this a bit, NS_FRAME_IN_CONSTRAINED_HEIGHT doesn't appear to be getting set. The minFontSize is coming out as 0, but I'm not exactly sure why yet. It _is_ due to the height: 80% set in the style, though. I'm still digging into this a bit to determine the root cause, because it doesn't seem like it should have constrained height to me...

Comment 11

[Scott Johnson (:jwir3)]

I have more information about this now. It's a combination of a couple of things. NS_FRAME_IN_CONSTRAINED_HEIGHT is getting set (I was looking at the wrong state bit previously). Additionally, the minimum font size that's calculated in this code:

>  float ratio = float(styleFontSize) / float(aMinFontSize);
>  if (ratio >= 1.5f) {
>    // If we're already at 1.5 or more times the minimum, don't scale.
>    return 1.0;
>  }

Gives us a ratio that's already greater than 1.5f. I'm going to fiddle with this a little bit and see if I can come up with a solution.

Comment 12

[Scott Johnson (:jwir3)]

Attachment: testcase

I'm attaching my simplified test case.

Also, I previously missed that in this code (nsHTMLReflowState.cpp):

>  if (parent &&
>      (parent->GetStateBits() & NS_FRAME_IN_CONSTRAINED_HEIGHT) &&
>      !(parent->GetType() == nsGkAtoms::scrollFrame &&
>        parent->GetStyleDisplay()->mOverflowY != NS_STYLE_OVERFLOW_HIDDEN)) {
>    frame->AddStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
>  } else if ((mStylePosition->mHeight.GetUnit() != eStyleUnit_Auto ||
>              mStylePosition->mMaxHeight.GetUnit() != eStyleUnit_None) &&
>              // Don't set NS_FRAME_IN_CONSTRAINED_HEIGHT on body or html
>              // elements.
>             (frame->GetContent() &&
>            !(frame->GetContent()->IsHTML(nsGkAtoms::body) ||
>              frame->GetContent()->IsHTML(nsGkAtoms::html)))) {
>    frame->AddStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
>  } else {
>    frame->RemoveStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
>  }

mStylePosition->mHeight.GetUnit() returns 'eStyleUnit_Percent' in a few cases (if I set a breakpoint there, it's only hit about 2 or 3 times during a page reload). So, it looks like I was incorrect when I previously said that it was becoming eStyleUnit_Auto. That said, if I change this so that it also ignores eStyleUnit_Percent (which isn't the correct solution, but I wanted to see if the problem is dependent solely on NS_FRAME_IN_CONSTRAINED_HEIGHT, or whether there was more to it), but this doesn't resolve the problem in the attached test case.

Still trying to see where the strange min size is coming from...

Comment 13

[Scott Johnson (:jwir3)]

Attachment: b746966

I added code to conditionally determine if we have a restricted height (for purposes of font inflation) when height is specified as a percentage. This will detect if the height will actually ultimately be resolved as 'auto' based on http://www.w3.org/TR/CSS21/visudet.html#the-height-property

Comment 14

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

Comment on attachment 619610 [details] [diff] [review]
b746966

>+    // If our height was specified as a percentage, then this could
>+    // actually resolve to 'auto', based on:
>+    // http://www.w3.org/TR/CSS21/visudet.html#the-height-property
>+    if (mStylePosition->mMaxHeight.GetUnit() == eStyleUnit_Percent) {

So this patch uses an odd mix of checking height and max-height.  I think you should generally check both -- i.e., if either one of them is known to be constrained, then you have a constrained height.  So basically you should stop if you hit a coord height or a coord max-height.  There's also the issue of calc(), which means that instead of the tests you're using you should try to constrain yourself to nsStyleCoord::IsCoordPercentCalcUnit() and nsStyleCoord::HasPercent().

I still want to look at the rest of the patch a bit more, but that's a good bit to get started on.

Comment 15

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

Comment on attachment 619610 [details] [diff] [review]
b746966

>+      bool shouldStop = false;
>+      bool contentDependent = false;
>+      while (containingBlk && !shouldStop) {
>+        if (containingBlk &&
>+            containingBlk->GetStylePosition()->mHeight.GetUnit() ==
>+              eStyleUnit_Auto ||
>+            containingBlk->GetStylePosition()->mHeight.GetUnit() ==
>+              eStyleUnit_Normal) {

Instead of |shouldStop|, use |break|.

no need to null-check containingBlk inside the loop since you check in the condition.

But it's probably also clearer as a for() with the containingBlk = containingBlk->GetContainingBlock() in the loop header.

Also, pull containingBlk->GetStylePosition() into a local variable.  (You'll need to look at its mHeight and mMaxHeight, per the previous comment.


Otherwise I think the approach seems reasonable; I haven't yet thought of a better way to do this.  But that's a bunch of comments to consider (see also previous comment), so I'll want to look again once they're taken into account.  (It could probably also use some testcases to exercise those issues.)

Comment 16

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

Comment on attachment 619610 [details] [diff] [review]
b746966

Also, I'd suggest naming the reftests something more related to the situation, e.g., height-constraint-percent-1?

Comment 17

[Scott Johnson (:jwir3)]

Attachment: b746966 (v2)

Second attempt.

Comment 18

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

Comment on attachment 619678 [details] [diff] [review]
b746966 (v2)

># HG changeset patch
># Parent 0d6b3c17b8398654e7cf9b8c367e02cd775ba841
># User Scott Johnson <[e-mail]>
>Bug 746966: Conditionally set NS_FRAME_IN_CONSTRAINED_HEIGHT for font inflation on percentage-based height so fonts with room to inflate can do so.
>
>diff --git a/layout/generic/nsHTMLReflowState.cpp b/layout/generic/nsHTMLReflowState.cpp
>--- a/layout/generic/nsHTMLReflowState.cpp
>+++ b/layout/generic/nsHTMLReflowState.cpp
>@@ -298,17 +298,47 @@ nsHTMLReflowState::Init(nsPresContext* a
>     frame->AddStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
>   } else if ((mStylePosition->mHeight.GetUnit() != eStyleUnit_Auto ||
>               mStylePosition->mMaxHeight.GetUnit() != eStyleUnit_None) &&
>               // Don't set NS_FRAME_IN_CONSTRAINED_HEIGHT on body or html
>               // elements.
>              (frame->GetContent() &&
>             !(frame->GetContent()->IsHTML(nsGkAtoms::body) ||
>               frame->GetContent()->IsHTML(nsGkAtoms::html)))) {
>-    frame->AddStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
>+
>+    // If our height was specified as a percentage, then this could
>+    // actually resolve to 'auto', based on:
>+    // http://www.w3.org/TR/CSS21/visudet.html#the-height-property
>+    if (mStylePosition->mMaxHeight.IsCoordPercentCalcUnit() ||
>+        mStylePosition->mHeight.IsCoordPercentCalcUnit()) {

It looks to me like this test never fails, given the conditions above and the values mHeight and mMaxHeight can take.  (Though if that isn't the case, then, this means we don't do anything at all to the NS_FRAME_IN_CONSTRAINED_HEIGHT bit if neither of these is true, which also seems wrong.)

>+      bool contentDependent = false;
>+      for (nsIFrame* containingBlk = frame->GetContainingBlock(); containingBlk;
>+           containingBlk = containingBlk->GetContainingBlock()) {
>+        const nsStylePosition* stylePos = containingBlk->GetStylePosition();
>+        if (stylePos->mHeight.GetUnit() == eStyleUnit_Auto ||
>+            stylePos->mMaxHeight.GetUnit() == eStyleUnit_Auto ||
>+            stylePos->mHeight.GetUnit() == eStyleUnit_Normal ||
>+            stylePos->mMaxHeight.GetUnit() == eStyleUnit_Normal) {
>+          contentDependent = true;
>+          break;
>+        } else if (stylePos->mHeight.IsCoordPercentCalcUnit() ||
>+                   stylePos->mMaxHeight.IsCoordPercentCalcUnit()) {
>+          continue;
>+        } else {
>+          contentDependent = false;
>+          break;
>+        }

I think the logic here is still wrong.  I think what you want is basically:

 * if either value is explicitly constrained (i.e., IsCoordPercentCalcUnit() && !HasPercent()), then you're done and it's a constrained height
 * otherwise, if either value is percentage based (i.e., IsCoordPercentCalcUnit() && HasPercent()), then continue up to the next containing block
 * otherwise (i.e., !IsCoordPercentCalcUnit(), or auto/normal for *both* values) you're done and it's not a constrained height

The order matters here since you're testing 2 values.

Comment 19

[Scott Johnson (:jwir3)]

Attachment: Truth table of possibilities for the test

(In reply to David Baron [:dbaron] from comment #18)
> Comment on attachment 619678 [details] [diff] [review]
> b746966 (v2)
> 
> ># HG changeset patch
> ># Parent 0d6b3c17b8398654e7cf9b8c367e02cd775ba841
> ># User Scott Johnson <[e-mail]>
> >Bug 746966: Conditionally set NS_FRAME_IN_CONSTRAINED_HEIGHT for font inflation on percentage-based height so fonts with room to inflate can do so.
> >
> >diff --git a/layout/generic/nsHTMLReflowState.cpp b/layout/generic/nsHTMLReflowState.cpp
> >--- a/layout/generic/nsHTMLReflowState.cpp
> >+++ b/layout/generic/nsHTMLReflowState.cpp
> >@@ -298,17 +298,47 @@ nsHTMLReflowState::Init(nsPresContext* a
> >     frame->AddStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
> >   } else if ((mStylePosition->mHeight.GetUnit() != eStyleUnit_Auto ||
> >               mStylePosition->mMaxHeight.GetUnit() != eStyleUnit_None) &&
> >               // Don't set NS_FRAME_IN_CONSTRAINED_HEIGHT on body or html
> >               // elements.
> >              (frame->GetContent() &&
> >             !(frame->GetContent()->IsHTML(nsGkAtoms::body) ||
> >               frame->GetContent()->IsHTML(nsGkAtoms::html)))) {
> >-    frame->AddStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
> >+
> >+    // If our height was specified as a percentage, then this could
> >+    // actually resolve to 'auto', based on:
> >+    // http://www.w3.org/TR/CSS21/visudet.html#the-height-property
> >+    if (mStylePosition->mMaxHeight.IsCoordPercentCalcUnit() ||
> >+        mStylePosition->mHeight.IsCoordPercentCalcUnit()) {
> 
> It looks to me like this test never fails, given the conditions above and
> the values mHeight and mMaxHeight can take.  (Though if that isn't the case,
> then, this means we don't do anything at all to the
> NS_FRAME_IN_CONSTRAINED_HEIGHT bit if neither of these is true, which also
> seems wrong.)

Hmm... I'm not sure I agree here, but perhaps I'm looking at something incorrect. See the truth table that I added - each of mMaxHeight and mHeight can take the values Auto, Percent, Coord, and Calc. The sequence of tests tests to see if (mHeight != Auto OR mMaxHeight != None) AND (mMaxHeight is Coord, Percent, or Calc OR mHeight is Coord, Percent, or Calc). 

It looks to me that there are two cases, specifically when mHeight == auto and mMaxHeight == none and when mHeight == mMaxHeight == auto when this series of tests will fail. In both of these cases, the last test fails, but only in the case when mMaxHeight == mHeight == auto is it necessary.

Comment 20

[Scott Johnson (:jwir3)]

Attachment: Truth table of possibilities for the test (v2)

Sorry, that last table was difficult to read. I've made a better version.

Comment 21

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

So I think the two issues there are:
 * max-height doesn't take 'auto', only 'none'
 * I'm asserting that E == H, not that E && H

Comment 22

[Scott Johnson (:jwir3)]

Attachment: b746966 (v3)

New version of patch based on review comments. I removed the inner 'if' statement, as I now agree that it never fails.

Comment 23

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

Comment on attachment 620328 [details] [diff] [review]
b746966 (v3)

This looks a lot closer.  However, still two problems that I see:

 (1) it doesn't set the constrained height bit on a frame whose height or max-height is set to a coord unit, since it skips straight to the containing block without testing the frame itself

 (2) it doesn't do anything to the state if it gets all the way to the top of the tree

Comment 24

[Scott Johnson (:jwir3)]

Attachment: b746966 (v4)

(In reply to David Baron [:dbaron] from comment #23)
>  (1) it doesn't set the constrained height bit on a frame whose height or
> max-height is set to a coord unit, since it skips straight to the containing
> block without testing the frame itself

I've corrected this oversight by starting the loop at frame, rather than frame's containing block.

>  (2) it doesn't do anything to the state if it gets all the way to the top
> of the tree

I've corrected this in the latest version by adding another test to the second 'else if' clause, inside the loop, that essentially tests to see if we are at the top of the tree. If we are, and we haven't yet found a constrained height, then I clear the constrained height bit on frame.

Comment 25

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

Comment on attachment 620457 [details] [diff] [review]
b746966 (v4)

r=dbaron, though I think it might be a little better if you just:
 * switched to a while() loop
 * converted the:
+                                        ; containingBlk;
+         containingBlk = containingBlk->GetContainingBlock()
   part of the for() into code here:
+        if (!containingBlk->GetParent()) {
+          // If we've reached the top of the tree, then we don't have
+          // a constrained height.
+          frame->RemoveStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
+          break;
+        }
   since right now you're null-checking twice, except with slightly different conditions (GetParent() vs. GetContainingBlock()), and you're probably better off null-checking once on the same condition (GetContainingBlock()).

Comment 26

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

It might also be good to have some tests for the cases that are constrained.

Comment 27

[Scott Johnson (:jwir3)]

(In reply to David Baron [:dbaron] from comment #25)
> Comment on attachment 620457 [details] [diff] [review]
> b746966 (v4)
> 
> r=dbaron, though I think it might be a little better if you just:
>  * switched to a while() loop

Sure.

>  * converted the:
> +                                        ; containingBlk;
> +         containingBlk = containingBlk->GetContainingBlock()
>    part of the for() into code here:
> +        if (!containingBlk->GetParent()) {
> +          // If we've reached the top of the tree, then we don't have
> +          // a constrained height.
> +          frame->RemoveStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
> +          break;
> +        }
>    since right now you're null-checking twice, except with slightly
> different conditions (GetParent() vs. GetContainingBlock()), and you're
> probably better off null-checking once on the same condition
> (GetContainingBlock()).

Ok... just so I understand what you're saying, you mean something like this?

    // If our height was specified as a percentage, then this could
    // actually resolve to 'auto', based on:
    // http://www.w3.org/TR/CSS21/visudet.html#the-height-property
    nsIFrame* containingBlk = frame;
    while (containingBlk) {
      const nsStylePosition* stylePos = containingBlk->GetStylePosition();
      if ((stylePos->mHeight.IsCoordPercentCalcUnit() &&
           !stylePos->mHeight.HasPercent()) ||
          (stylePos->mMaxHeight.IsCoordPercentCalcUnit() &&
           !stylePos->mMaxHeight.HasPercent())) {
        frame->AddStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
        break;
      } else if ((stylePos->mHeight.IsCoordPercentCalcUnit() &&
                  stylePos->mHeight.HasPercent()) ||
                 (stylePos->mMaxHeight.IsCoordPercentCalcUnit() &&
                  stylePos->mMaxHeight.HasPercent())) {
        if (!(containingBlk = containingBlk->GetContainingBlock())) {
          // If we've reached the top of the tree, then we don't have
          // a constrained height.
          frame->RemoveStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
          break;
        }

        continue;
      } else {
        frame->RemoveStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
        break;
      }
    }
  } else {
    frame->RemoveStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
  }

or something more like this?

    // If our height was specified as a percentage, then this could
    // actually resolve to 'auto', based on:
    // http://www.w3.org/TR/CSS21/visudet.html#the-height-property
    nsIFrame* containingBlk = frame;
    while (containingBlk) {
      const nsStylePosition* stylePos = containingBlk->GetStylePosition();
      if ((stylePos->mHeight.IsCoordPercentCalcUnit() &&
           !stylePos->mHeight.HasPercent()) ||
          (stylePos->mMaxHeight.IsCoordPercentCalcUnit() &&
           !stylePos->mMaxHeight.HasPercent())) {
        frame->AddStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
        break;
      } else if ((stylePos->mHeight.IsCoordPercentCalcUnit() &&
                  stylePos->mHeight.HasPercent()) ||
                 (stylePos->mMaxHeight.IsCoordPercentCalcUnit() &&
                  stylePos->mMaxHeight.HasPercent())) {
        containingBlk = containingBlk->GetContainingBlock();
      } else {
        frame->RemoveStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
        break;
      }
    }

    if (!containingBlk) {
      // If we've reached the top of the tree, then we don't have
      // a constrained height.
      frame->RemoveStateBits(NS_FRAME_IN_CONSTRAINED_HEIGHT);
    }

(In reply to David Baron [:dbaron] from comment #26)
> It might also be good to have some tests for the cases that are constrained.

Sure, I'll add some tests as well.

Comment 28

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

The first.

Comment 29

[Scott Johnson (:jwir3)]

Pushed to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5697962128e4

Comment 30

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

https://hg.mozilla.org/mozilla-central/rev/5697962128e4
https://hg.mozilla.org/mozilla-central/rev/eeba341b7e45

Comment 31

[Scott Johnson (:jwir3)]

Comment on attachment 620457 [details] [diff] [review]
b746966 (v4)

[Approval Request Comment]
Regression caused by (bug #): n/a
User impact if declined: Some websites will not have font inflation that should have it.
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): low risk - The only source of risk is that some frames will have NS_FRAME_IN_CONSTRAINED_HEIGHT not set on them, but this state bit only controls font inflation, so worst-case would be that some small number of things that shouldn't be getting inflated, are getting inflated.
String changes made by this patch: none

Comment 32

[Aaron Train [:aaronmt]]

Verified Fixed on Nightly (05/07) (Gecko 15.0a1)

Comment 33

[Johnathan Nightingale [:johnath]]

Comment on attachment 620457 [details] [diff] [review]
b746966 (v4)

Discussed in triage today - approved - git 'er in.

Comment 34

[Scott Johnson (:jwir3)]

Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/3f16abb633ee

Comment 35

[Catalin Suciu [:csuciu]]

Verified fixed on Aurora 14.0a2 (2012-05-20)