Last Comment Bug 711418 - Font inflation has no effect on sites with html, body { height: 100% } or similar (e.g., cnn.com)
: Font inflation has no effect on sites with html, body { height: 100% } or sim...
Status: VERIFIED FIXED
[readability], [short-list]
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: ARM Android
: P1 normal with 1 vote (vote)
: mozilla14
Assigned To: Scott Johnson (:jwir3)
:
Mentors:
http://www.cnn.com/
: 721000 722700 (view as bug list)
Depends on:
Blocks: font-inflation
  Show dependency treegraph
 
Reported: 2011-12-16 06:50 PST by Aaron Train [:aaronmt]
Modified: 2012-05-18 00:38 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
beta+
11+


Attachments
Mobile NYTIMES using small inflation size pref. (714.20 KB, image/jpeg)
2011-12-16 10:38 PST, Patryk Adamczyk [:patryk] UX
no flags Details
screenshot (234.01 KB, image/png)
2011-12-19 15:19 PST, Matt Brubeck (:mbrubeck)
no flags Details
b711418 (8.33 KB, patch)
2012-03-22 12:37 PDT, Scott Johnson (:jwir3)
dbaron: review-
Details | Diff | Review
b711418 (v2) (8.57 KB, patch)
2012-03-23 12:37 PDT, Scott Johnson (:jwir3)
dbaron: review+
akeybl: approval‑mozilla‑aurora+
Details | Diff | Review

Description Aaron Train [:aaronmt] 2011-12-16 06:50:22 PST
bug 703029 exposed a font size adjustment [1]. The first site I tested out was CNN.com (desktop version). I adjusted font-size from small to extra large, backed out of preferences and noticed no difference. I reloaded the page to verify and the result was the same.

This exposed preference does not make it clear to the user on what sites it affects or wether it works at all.

[1] https://hg.mozilla.org/mozilla-central/rev/cf70852dfbd9

--
Samsung Nexus S (Android 2.3.6)
20111216052315
http://hg.mozilla.org/mozilla-central/rev/80ac06ad280d
Comment 1 Matt Brubeck (:mbrubeck) 2011-12-16 08:33:51 PST
Let's focus first on fixing the sites where text inflation doesn't work (in this case, cnn.com).  A reduced test case might help.

We can also file a separate bug if we want to change the wording in the preference UI.
Comment 2 Patryk Adamczyk [:patryk] UX 2011-12-16 10:38:54 PST
Created attachment 582315 [details]
Mobile NYTIMES using small inflation size pref.

This was taken on the mobile version of nytimes.com using today's nightly. The font inflation setting was set to "small" but the text renders far too large. We need a way to limit the text rendering so it doesn't scale up beyond the user specified size.
Comment 3 Matt Brubeck (:mbrubeck) 2011-12-16 10:41:57 PST
(In reply to Patryk Adamczyk from comment #2)
> This was taken on the mobile version of nytimes.com using today's nightly.
> The font inflation setting was set to "small" but the text renders far too
> large. We need a way to limit the text rendering so it doesn't scale up
> beyond the user specified size.

Bug 706198 discusses fixing (perhaps disabling) text inflation on mobile sites.

This bug is about desktop www.cnn.com; please file new bugs for separate cases.
Comment 4 Brad Lassey [:blassey] (use needinfo?) 2011-12-19 15:12:44 PST
what I see on cnn.com is expected behavior, the text is readable
Comment 5 Matt Brubeck (:mbrubeck) 2011-12-19 15:19:56 PST
Created attachment 582980 [details]
screenshot

I am still seeing this.  This is a screenshot of the widest column on the desktop version of http://www.cnn.com/.  This is on an HDPI device, so the actual size of the text is about 1mm.  Changing the font inflation preference to "extra large" has no effect on the font size.
Comment 6 JP Rosevear [:jpr] 2012-02-23 19:27:27 PST
Beta blockers.
Comment 7 JP Rosevear [:jpr] 2012-03-12 04:25:34 PDT
Scott, would this have been impacted by any recent changes?
Comment 8 Scott Johnson (:jwir3) 2012-03-12 09:35:50 PDT
(In reply to JP Rosevear [:jpr] from comment #7)
> Scott, would this have been impacted by any recent changes?

It's possible, let me check and see if it's still an issue.
Comment 9 Scott Johnson (:jwir3) 2012-03-12 13:38:57 PDT
(In reply to JP Rosevear [:jpr] from comment #7)
> Scott, would this have been impacted by any recent changes?

So, after some investigation, I don't think so. Specifically, this bug might be on the fence as to whether or not it's valid. The bug is stating that we don't inflate any text on cnn.com, but this is probably intentional, since there doesn't seem to be an easy way to inflate text sizes without a drastic change to the page layout. It does look like, however, that Safari on iOS does inflate the text of the center (main) article on the full site, but only slightly.

I guess there are two things we could do here:

1) See if we can make the font inflation only inflate the text in the center/main article. This could take a lot of work, and probably wouldn't generate a lot of benefit. It looks like the text in this article is about 1pt larger than on Firefox for android.

2) Advise users to use m.cnn.com on a mobile device, which would be uninflated, but already optimized for mobile, and close this bug as WONTFIX.

I recommend option 2, because even if we were able to get font inflation to inflate text on this page, it's likely going to cause cascading overinflation effects on other pages, so the tradeoff just doesn't seem to be there.
Comment 10 Jet Villegas (:jet) 2012-03-12 14:32:56 PDT
This looks like a case of user-agent-sniffing giving us the desktop site vs. the m.cnn.com site. Can we reach out to cnn.com to fix that?
Comment 11 Scott Johnson (:jwir3) 2012-03-20 10:04:36 PDT
Reassigning to evangelism as per comment 10.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-03-20 16:25:41 PDT
I think it looks like we're not doing inflation here because body has a fixed (100%) height.

I suspect we want to change the code that disables inflation on descendants of things that have fixed heights to either:
 (a) not count fixed heights on body or html, or
 (b) not count fixed heights on elements that have no later siblings and no ancestors with later siblings

I probably prefer (a) since I think it's a bit more understandable for authors and body and html are a bit special.
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-03-20 16:37:40 PDT
*** Bug 722700 has been marked as a duplicate of this bug. ***
Comment 14 Scott Johnson (:jwir3) 2012-03-22 12:37:15 PDT
Created attachment 608427 [details] [diff] [review]
b711418

(In reply to David Baron [:dbaron] from comment #12)
> I probably prefer (a) since I think it's a bit more understandable for
> authors and body and html are a bit special.

I asked in #developers if there was a way to get the information about whether a particular frame represented a body or html element from the frame directly, without accessing the content tree, but it seems that it doesn't exist.  Hence the GetContent()->IsHTML() calls.
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-03-22 13:42:18 PDT
Comment on attachment 608427 [details] [diff] [review]
b711418

This isn't going to work for a paragraph inside of the <body>, since then the paragraph will be the container.

You need to change the logic that sets the NS_FRAME_IN_CONSTRAINED_HEIGHT flag instead.

(And you should add a test that covers that case.)


(Good choice of bug to work on, though... I was actually going to suggest this one.)
Comment 16 Scott Johnson (:jwir3) 2012-03-23 12:37:05 PDT
Created attachment 608823 [details] [diff] [review]
b711418 (v2)

Ok, I think I fixed the items you commented about. I'm still using, in general, the same method to determine if we are representing a <body> or <html> element, but now it has conditional logic to trigger whether this sets the NS_FRAME_IN_CONSTRAINED_HEIGHT flag.
Comment 17 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-03-23 12:45:52 PDT
Comment on attachment 608823 [details] [diff] [review]
b711418 (v2)

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

Could you:
 (1) swap the order of the tests, since the height/max-height check is going to fail most of the time (then we won't bother with this more-expensive check that's also likely to succeed almost all the time)
 (2) parenthesize the existing || -- right now your code is wrong since && binds tighter than ||.




Also, please change the comment in nsIFrame.h where we define NS_FRAME_IN_CONSTRAINED_HEIGHT to explain this change.

r=dbaron with those things fixed
Comment 19 Ed Morley [:emorley] 2012-03-24 13:39:31 PDT
https://hg.mozilla.org/mozilla-central/rev/164c27136716
Comment 20 Scott Johnson (:jwir3) 2012-03-24 17:00:55 PDT
Comment on attachment 608823 [details] [diff] [review]
b711418 (v2)

[Approval Request Comment]
Regression caused by (bug #): font-inflation
User impact if declined: Limited readability on mobile.
Testing completed (on m-c, etc.): mozilla central, try
Risk to taking this patch (and alternatives if risky): very low
String changes made by this patch: none
Comment 21 Scott Johnson (:jwir3) 2012-03-24 17:01:52 PDT
Reopening for aurora approval.
Comment 22 Matt Brubeck (:mbrubeck) 2012-03-24 17:34:46 PDT
Bugs don't need to be reopened for Aurora/Beta approval.
Comment 23 Alex Keybl [:akeybl] 2012-03-26 15:29:42 PDT
Comment on attachment 608823 [details] [diff] [review]
b711418 (v2)

[Triage Comment]
In support of mobile and deemed very low risk - approved for Aurora 13.
Comment 24 Scott Johnson (:jwir3) 2012-03-26 16:07:30 PDT
Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/465933626096
Comment 25 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-05-17 05:45:22 PDT
*** Bug 721000 has been marked as a duplicate of this bug. ***
Comment 26 Adrian Tamas (:AdrianT) 2012-05-18 00:38:45 PDT
Verified fix on:
Build: Nightly 15.0a1 2012-05-17 Aurora 14.0a2 2012-05-17
Device: HTC Desire Z
OS: Android 2.3.3

The middle article font is inflated at cnn.com. Also bug 722700 and bug 712000 which are duplicated after this issue are no longer reproducible. Font inflation works on the websites in the bugs also.

Note You need to log in before you can comment on or make changes to this bug.