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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Michael Monreal [:micmon]]

Attachment: 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]]

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-5, no longer working on Mozilla)]

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-5, no longer working on Mozilla)]

Attachment: Save and expose on nsFontInflationData the width of the nearest common ancestor of the inflated descendants. (, patch 1)

Comment 4

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: 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-5, no longer working on Mozilla)]

Attachment: Use the same width basis for font inflation throughout a font inflation flow root. (, patch 3)

Comment 6

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: Fix call to wrong method (passing nscoord as enum). (, patch 4)

Comment 7

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: Remove width determination parameters from font inflation methods. (, patch 5)

Comment 8

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: Remove caching of current inflation container and its width from the pres context. (, patch 6)

Comment 9

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Attachment: Make scroll frames (i.e., overflow != visible) no longer be font size inflation flow roots. This reverts part of 9499f6b28addcbcd9c480eb80cfe6c4c63a4a3a1. (, patch 7)

Comment 10

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 11

[Matt Brubeck (:mbrubeck)]

Nominating for blocking-fennec because this is a dependency of bug 707195 which is blocking-fennec1.0+

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

Daivd - How much testing do you think this needs on trunk before getting to release?

Comment 13

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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 14

[Ed Morley [:emorley]]

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

Comment 15

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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-5, no longer working on Mozilla)]

(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 17

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

Comment 19

[Cristian Nicolae (:xti)]

Attachment: 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-5, no longer working on Mozilla)]

That's a screenshot of Google News, not phoronix.com.

Comment 21

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

I backed part of this out to fix bug 760098.

Comment 22

[Jet Villegas (inactive)]

re-nomming for the blocking-

Comment 23

[Mathieu Pellerin]

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]]

Still a blocker, going to backout the change in bug 760098

Comment 25

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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