Font inflation has no effect on sites with html, body { height: 100% } or similar (e.g., cnn.com)

VERIFIED FIXED in Firefox 13

Status

()

Core
Layout
P1
normal
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: aaronmt, Assigned: jwir3)

Tracking

unspecified
mozilla14
ARM
Android
Points:
---

Firefox Tracking Flags

(firefox13 fixed, blocking-fennec1.0 beta+, fennec11+)

Details

(Whiteboard: [readability], [short-list], URL)

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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
(Reporter)

Updated

6 years ago
Whiteboard: readability
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.
Blocks: 627842
No longer blocks: 703029
Summary: Font size adjustment has no affect on certain sites → Font inflation has no effect on desktop www.cnn.com home page
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.
(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.
what I see on cnn.com is expected behavior, the text is readable
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → INVALID
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.
Attachment #582315 - Attachment is obsolete: true
Status: RESOLVED → REOPENED
Resolution: INVALID → ---
Status: REOPENED → NEW
Priority: -- → P2
Whiteboard: readability → [readability]

Updated

6 years ago
Whiteboard: [readability] → [readability], [short-list]
tracking-fennec: --- → 11+
(Assignee)

Updated

5 years ago
Assignee: nobody → sjohnson
Keywords: fennecnative-releaseblocker

Updated

5 years ago
Keywords: fennecnative-releaseblocker → fennecnative-betablocker

Comment 6

5 years ago
Beta blockers.
Priority: P2 → P1
blocking-fennec1.0: --- → beta+
Status: NEW → ASSIGNED

Comment 7

5 years ago
Scott, would this have been impacted by any recent changes?
(Assignee)

Comment 8

5 years ago
(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.
(Assignee)

Comment 9

5 years ago
(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

5 years ago
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?
(Assignee)

Comment 11

5 years ago
Reassigning to evangelism as per comment 10.
Assignee: sjohnson → english-us
Component: General → English US
Product: Fennec Native → Tech Evangelism
QA Contact: general → english-us
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.
Assignee: english-us → nobody
Component: English US → Layout
Product: Tech Evangelism → Core
QA Contact: english-us → layout
Summary: Font inflation has no effect on desktop www.cnn.com home page → Font inflation has no effect on sites with html, body { height: 100% } or similar (e.g., cnn.com)
Duplicate of this bug: 722700
(Assignee)

Updated

5 years ago
Assignee: nobody → sjohnson
(Assignee)

Comment 14

5 years ago
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.
Attachment #608427 - Flags: review?(dbaron)
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.)
Attachment #608427 - Flags: review?(dbaron) → review-
(Assignee)

Comment 16

5 years ago
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.
Attachment #608427 - Attachment is obsolete: true
Attachment #608823 - Flags: review?(dbaron)
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
Attachment #608823 - Flags: review?(dbaron) → review+
(Assignee)

Comment 18

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/164c27136716
Target Milestone: --- → mozilla13
(Assignee)

Updated

5 years ago
Target Milestone: mozilla13 → mozilla14
https://hg.mozilla.org/mozilla-central/rev/164c27136716
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 20

5 years ago
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
Attachment #608823 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 21

5 years ago
Reopening for aurora approval.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Bugs don't need to be reopened for Aurora/Beta approval.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Comment on attachment 608823 [details] [diff] [review]
b711418 (v2)

[Triage Comment]
In support of mobile and deemed very low risk - approved for Aurora 13.
Attachment #608823 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Assignee)

Comment 24

5 years ago
Pushed to aurora:
https://hg.mozilla.org/releases/mozilla-aurora/rev/465933626096

Updated

5 years ago
status-firefox13: --- → fixed
Duplicate of this bug: 721000
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.