Closed Bug 747720 Opened 8 years ago Closed 7 years ago

Strange text rendering on phoronix.com because font inflation uses different width basis within the same flow (including across overflow!=visible)

Categories

(Core :: Layout, defect)

14 Branch
ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla15
Tracking Status
firefox14 --- fixed
firefox15 --- fixed
blocking-fennec1.0 --- +

People

(Reporter: micmon, Assigned: dbaron)

References

(Blocks 1 open bug)

Details

(Whiteboard: readability)

Attachments

(9 files)

216.49 KB, image/png
Details
2.13 KB, patch
roc
: review+
Details | Diff | Splinter Review
15.87 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.68 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.16 KB, patch
roc
: review+
Details | Diff | Splinter Review
32.41 KB, patch
roc
: review+
Details | Diff | Splinter Review
9.54 KB, patch
roc
: review+
Details | Diff | Splinter Review
8.26 KB, patch
roc
: review+
Details | Diff | Splinter Review
270.05 KB, image/png
Details
Attached image 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.
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?
No longer blocks: 707195
Depends on: 707195
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.
Blocks: 707195
Component: General → Layout
No longer depends on: 707195
Product: Fennec Native → Core
QA Contact: general → layout
Target Milestone: --- → mozilla15
Version: Firefox 14 → 14 Branch
Nominating for blocking-fennec because this is a dependency of bug 707195 which is blocking-fennec1.0+
blocking-fennec1.0: --- → ?
blocking-fennec1.0: ? → +
Daivd - How much testing do you think this needs on trunk before getting to release?
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 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.
Attachment #625233 - Flags: approval-mozilla-aurora?
Attachment #625234 - Flags: approval-mozilla-aurora?
Attachment #625235 - Flags: approval-mozilla-aurora?
Attachment #625236 - Flags: approval-mozilla-aurora?
Attachment #625237 - Flags: approval-mozilla-aurora?
Attachment #625238 - Flags: approval-mozilla-aurora?
Attachment #625239 - Flags: approval-mozilla-aurora?
(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.
Blocks: 754456
Attachment #625233 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #625234 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #625235 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #625236 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #625237 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #625238 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #625239 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Duplicate of this bug: 748446
Summary: Strange text rendering on phoronix.com → Strange text rendering on phoronix.com because font inflation uses different width basis within the same flow (including across overflow!=visible)
Blocks: 752429
Depends on: 760098
No longer depends on: 760098
Depends on: 760098
Attached image 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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
That's a screenshot of Google News, not phoronix.com.
Status: REOPENED → RESOLVED
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Depends on: 749186
I backed part of this out to fix bug 760098.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
re-nomming for the blocking-
blocking-fennec1.0: + → ?
Depends on: 763702
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?
Still a blocker, going to backout the change in bug 760098
blocking-fennec1.0: ? → +
You need to log in before you can comment on or make changes to this bug.