Last Comment Bug 747720 - Strange text rendering on phoronix.com because font inflation uses different width basis within the same flow (including across overflow!=visible)
: Strange text rendering on phoronix.com because font inflation uses different ...
Status: RESOLVED FIXED
readability
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 14 Branch
: ARM Android
: -- normal (vote)
: mozilla15
Assigned To: David Baron :dbaron: ⌚️UTC+1 (busy September 14-25)
:
Mentors:
: 748446 (view as bug list)
Depends on: 749186 760098 763702
Blocks: 707195 font-inflation 752429 754456 856199
  Show dependency treegraph
 
Reported: 2012-04-22 02:54 PDT by Michael Monreal [:monreal]
Modified: 2013-03-29 14:56 PDT (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
fixed
+


Attachments
Screenshot (216.49 KB, image/png)
2012-04-22 02:54 PDT, Michael Monreal [:monreal]
no flags Details
Save and expose on nsFontInflationData the width of the nearest common ancestor of the inflated descendants. (, patch 1) (2.13 KB, patch)
2012-05-18 13:45 PDT, David Baron :dbaron: ⌚️UTC+1 (busy September 14-25)
roc: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Replace AutoMaybeNullInflationContainer with AutoMaybeDisableInflationForShrinkWrap since the concept of the pres context's current inflation container will be going away. (, patch 2) (15.87 KB, patch)
2012-05-18 13:45 PDT, David Baron :dbaron: ⌚️UTC+1 (busy September 14-25)
roc: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Use the same width basis for font inflation throughout a font inflation flow root. (, patch 3) (8.68 KB, patch)
2012-05-18 13:46 PDT, David Baron :dbaron: ⌚️UTC+1 (busy September 14-25)
roc: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Fix call to wrong method (passing nscoord as enum). (, patch 4) (1.16 KB, patch)
2012-05-18 13:46 PDT, David Baron :dbaron: ⌚️UTC+1 (busy September 14-25)
roc: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Remove width determination parameters from font inflation methods. (, patch 5) (32.41 KB, patch)
2012-05-18 13:46 PDT, David Baron :dbaron: ⌚️UTC+1 (busy September 14-25)
roc: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Remove caching of current inflation container and its width from the pres context. (, patch 6) (9.54 KB, patch)
2012-05-18 13:46 PDT, David Baron :dbaron: ⌚️UTC+1 (busy September 14-25)
roc: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
Make scroll frames (i.e., overflow != visible) no longer be font size inflation flow roots. This reverts part of 9499f6b28addcbcd9c480eb80cfe6c4c63a4a3a1. (, patch 7) (8.26 KB, patch)
2012-05-18 13:46 PDT, David Baron :dbaron: ⌚️UTC+1 (busy September 14-25)
roc: review+
blassey.bugs: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
screenshot 2 (270.05 KB, image/png)
2012-06-01 07:32 PDT, Cristian Nicolae (:xti)
no flags Details

Description Michael Monreal [:monreal] 2012-04-22 02:54:22 PDT
Created attachment 617300 [details]
Screenshot

On the phoronix.com main page, news items with short summaries are displayed using a much smaller font compared to those with longer summaries. The small summaries are barely readable. On desktop Firefox, this doesn't happen.
Comment 1 Aaron Train [:aaronmt] 2012-04-22 09:47:26 PDT
It's font-inflation at work here. I'm hesitant to believe that it's working properly in this case. Scott or David, can you confirm?
Comment 2 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2012-05-18 12:04:07 PDT
I'm going to put a series of patches in this bug that's essentially the first phase of bug 707195.  Since it doesn't fully fix bug 707195, I don't want to put it there, but it does fully fix this bug.
Comment 3 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2012-05-18 13:45:42 PDT
Created attachment 625233 [details] [diff] [review]
Save and expose on nsFontInflationData the width of the nearest common ancestor of the inflated descendants.  (, patch 1)
Comment 4 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2012-05-18 13:45:54 PDT
Created attachment 625234 [details] [diff] [review]
Replace AutoMaybeNullInflationContainer with AutoMaybeDisableInflationForShrinkWrap since the concept of the pres context's current inflation container will be going away.  (, patch 2)
Comment 5 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2012-05-18 13:46:05 PDT
Created attachment 625235 [details] [diff] [review]
Use the same width basis for font inflation throughout a font inflation flow root.  (, patch 3)
Comment 6 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2012-05-18 13:46:17 PDT
Created attachment 625236 [details] [diff] [review]
Fix call to wrong method (passing nscoord as enum).  (, patch 4)
Comment 7 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2012-05-18 13:46:29 PDT
Created attachment 625237 [details] [diff] [review]
Remove width determination parameters from font inflation methods.  (, patch 5)
Comment 8 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2012-05-18 13:46:41 PDT
Created attachment 625238 [details] [diff] [review]
Remove caching of current inflation container and its width from the pres context.  (, patch 6)
Comment 9 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2012-05-18 13:46:53 PDT
Created attachment 625239 [details] [diff] [review]
Make scroll frames (i.e., overflow != visible) no longer be font size inflation flow roots.  This reverts part of 9499f6b28addcbcd9c480eb80cfe6c4c63a4a3a1.  (, patch 7)
Comment 11 Matt Brubeck (:mbrubeck) 2012-05-20 22:29:56 PDT
Nominating for blocking-fennec because this is a dependency of bug 707195 which is blocking-fennec1.0+
Comment 12 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-21 11:58:25 PDT
Daivd - How much testing do you think this needs on trunk before getting to release?
Comment 13 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2012-05-21 15:18:23 PDT
I'd like it to get at least a few days on trunk before pushing it in a beta.

Also, I realize I might need to adjust the fix for bug 708175 to walk further up the tree, or something like that, to deal with inflation containers being less important a concept as a result of the patches here.  (Or maybe not.  I need to investigate, though.)
Comment 15 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2012-05-22 10:57:14 PDT
Comment on attachment 625233 [details] [diff] [review]
Save and expose on nsFontInflationData the width of the nearest common ancestor of the inflated descendants.  (, patch 1)

This comment applies to all 7 patches.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 627842 / bug 706193
User impact if declined: font inflation is less consistent across neighboring areas of text
Testing completed (on m-c, etc.): on mozilla-central, tested against a few sites with reported problems
Risk to taking this patch (and alternatives if risky): This set of patches makes pretty significant changes to the way we do font inflation -- in general, to make things within the same container more likely to behave the same way rather than differently.  I'm reasonably confident the behavior changes will be a net improvement, though it's certainly possible that some pages might be worse as a result.
String or UUID changes made by this patch: None, though it changes the object layout of nsPresContext, which should basically be layout-internal although it's possible some external code depends on it.
Comment 16 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2012-05-22 15:35:59 PDT
(In reply to David Baron [:dbaron] from comment #13)
> Also, I realize I might need to adjust the fix for bug 708175 to walk
> further up the tree, or something like that, to deal with inflation
> containers being less important a concept as a result of the patches here. 
> (Or maybe not.  I need to investigate, though.)

Actually, things are fine as they are now, since nsLayoutUtils::InflationMinFontSizeFor calls ShouldInflateFontsForContainer on the container before getting the nsFontInflationData for the flow root.
Comment 18 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2012-05-25 12:14:23 PDT
*** Bug 748446 has been marked as a duplicate of this bug. ***
Comment 19 Cristian Nicolae (:xti) 2012-06-01 07:32:34 PDT
Created attachment 629180 [details]
screenshot 2

I still see this issue on the latest Nightly and Aurora builds. Reopening bug

--
Firefox 15.0a1 (2012-06-01)
Firefox 14.0a2 (2012-06-01)
Device: Samsung Galaxy Nexus:
OS: Android 4.0.2
Comment 20 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2012-06-01 14:26:19 PDT
That's a screenshot of Google News, not phoronix.com.
Comment 21 David Baron :dbaron: ⌚️UTC+1 (busy September 14-25) 2012-06-07 12:15:05 PDT
I backed part of this out to fix bug 760098.
Comment 22 Jet Villegas (:jet) 2012-06-11 12:23:17 PDT
re-nomming for the blocking-
Comment 23 Mathieu Pellerin 2012-06-11 21:42:59 PDT
Backing out part of this bug resulted in reddit.com font inflation issues, as well as re-emergence of the initial issue on phoronix.com.

This is temporary until the depending crasher is fixed, right?
Comment 24 JP Rosevear [:jpr] 2012-06-12 12:02:57 PDT
Still a blocker, going to backout the change in bug 760098

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