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

RESOLVED FIXED in Firefox 14

Status

()

Core
Layout
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: monreal, Assigned: dbaron)

Tracking

(Blocks: 1 bug)

14 Branch
mozilla15
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox14 fixed, firefox15 fixed, blocking-fennec1.0 +)

Details

(Whiteboard: readability)

Attachments

(9 attachments)

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
(Reporter)

Description

5 years ago
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.
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?
Blocks: 627842
Whiteboard: readability
Blocks: 707195
(Assignee)

Updated

5 years ago
No longer blocks: 707195
Depends on: 707195
(Assignee)

Comment 2

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

Comment 3

5 years ago
Created attachment 625233 [details] [diff] [review]
Save and expose on nsFontInflationData the width of the nearest common ancestor of the inflated descendants.  (, patch 1)
Attachment #625233 - Flags: review?(roc)
(Assignee)

Comment 4

5 years ago
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)
Attachment #625234 - Flags: review?(roc)
(Assignee)

Comment 5

5 years ago
Created attachment 625235 [details] [diff] [review]
Use the same width basis for font inflation throughout a font inflation flow root.  (, patch 3)
Attachment #625235 - Flags: review?(roc)
(Assignee)

Comment 6

5 years ago
Created attachment 625236 [details] [diff] [review]
Fix call to wrong method (passing nscoord as enum).  (, patch 4)
Attachment #625236 - Flags: review?(roc)
(Assignee)

Comment 7

5 years ago
Created attachment 625237 [details] [diff] [review]
Remove width determination parameters from font inflation methods.  (, patch 5)
Attachment #625237 - Flags: review?(roc)
(Assignee)

Comment 8

5 years ago
Created attachment 625238 [details] [diff] [review]
Remove caching of current inflation container and its width from the pres context.  (, patch 6)
Attachment #625238 - Flags: review?(roc)
(Assignee)

Comment 9

5 years ago
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)
Attachment #625239 - Flags: review?(roc)
Attachment #625233 - Flags: review?(roc) → review+
Attachment #625234 - Flags: review?(roc) → review+
Attachment #625235 - Flags: review?(roc) → review+
Attachment #625236 - Flags: review?(roc) → review+
Attachment #625237 - Flags: review?(roc) → review+
Attachment #625238 - Flags: review?(roc) → review+
Attachment #625239 - Flags: review?(roc) → review+
(Assignee)

Comment 10

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cbae0f1285c
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b7c2ea08f70
https://hg.mozilla.org/integration/mozilla-inbound/rev/63c2a4ad7afb
https://hg.mozilla.org/integration/mozilla-inbound/rev/d643376b35ac
https://hg.mozilla.org/integration/mozilla-inbound/rev/0805380444b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2f0d40f26b4
https://hg.mozilla.org/integration/mozilla-inbound/rev/9620f50be9fa
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?
(Assignee)

Comment 13

5 years ago
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.)
https://hg.mozilla.org/mozilla-central/rev/4cbae0f1285c
https://hg.mozilla.org/mozilla-central/rev/6b7c2ea08f70
https://hg.mozilla.org/mozilla-central/rev/63c2a4ad7afb
https://hg.mozilla.org/mozilla-central/rev/d643376b35ac
https://hg.mozilla.org/mozilla-central/rev/0805380444b2
https://hg.mozilla.org/mozilla-central/rev/e2f0d40f26b4
https://hg.mozilla.org/mozilla-central/rev/9620f50be9fa
Assignee: nobody → dbaron
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Assignee)

Comment 15

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

Updated

5 years ago
Attachment #625234 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #625235 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #625236 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #625237 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #625238 - Flags: approval-mozilla-aurora?
(Assignee)

Updated

5 years ago
Attachment #625239 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 16

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

Updated

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

Comment 17

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/562f7505891e
https://hg.mozilla.org/releases/mozilla-aurora/rev/a85cb76bb4fd
https://hg.mozilla.org/releases/mozilla-aurora/rev/750366fff45c
https://hg.mozilla.org/releases/mozilla-aurora/rev/c7c3f34a8bc3
https://hg.mozilla.org/releases/mozilla-aurora/rev/d6264b8231b7
https://hg.mozilla.org/releases/mozilla-aurora/rev/f7b35488a05b
https://hg.mozilla.org/releases/mozilla-aurora/rev/1c4729a52ff1
status-firefox14: --- → fixed
(Assignee)

Updated

5 years ago
Duplicate of this bug: 748446
(Assignee)

Updated

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

Updated

5 years ago
Blocks: 752429

Updated

5 years ago
Depends on: 760098

Updated

5 years ago
No longer depends on: 760098

Updated

5 years ago
Depends on: 760098
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
status-firefox14: fixed → affected
status-firefox15: --- → affected
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 20

5 years ago
That's a screenshot of Google News, not phoronix.com.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
status-firefox14: affected → fixed
status-firefox15: affected → ---
Resolution: --- → FIXED

Updated

5 years ago
Depends on: 749186
(Assignee)

Comment 21

5 years ago
I backed part of this out to fix bug 760098.
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 22

5 years ago
re-nomming for the blocking-
blocking-fennec1.0: + → ?

Updated

5 years ago
Depends on: 763702

Comment 23

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

5 years ago
Still a blocker, going to backout the change in bug 760098
blocking-fennec1.0: ? → +
(Assignee)

Comment 25

5 years ago
Fix relanded:
https://hg.mozilla.org/mozilla-central/rev/880037c0ff1e
https://hg.mozilla.org/releases/mozilla-aurora/rev/bbbcb681820d
https://hg.mozilla.org/releases/mozilla-beta/rev/494a2756d358
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
status-firefox15: --- → fixed
Resolution: --- → FIXED
Blocks: 856199
You need to log in before you can comment on or make changes to this bug.