1552781 - Font inflation on einthusan.tv

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, enhancement, P3)

Description

[Jan Henning [:JanH]]

As reported at https://github.com/mozilla-mobile/fenix/issues/2606, font inflation doesn't work properly on https://einthusan.tv/movie/browse/?lang=hindi.

Bits of the header, footer and the text in the ad place holders are inflated, even though the individual elements visually don't exceed our line threshold for the minimum amount of text required before we start inflation. That would suggest that somehow all those elements end up under the same font inflation flow root, and of course their sum total of text is then likely large enough to trigger inflation.

If that indeed turns out to be the case then,

1. Investigate whether the algorithm for determining font inflation flow roots could be improved (block frames that establish a new block formatting context also establish a font inflation flow root - I have a suspicion that the same might currently not be true for non-inline flex container frames, even though those establish a new formatting context, too), or
2. If that doesn't lead anywhere useful, given that this page heavily uses flexboxes, take up the suggestion from bug 1142461 comment 15 and disable font inflation for flexboxes, on the basis that those are a newish CSS feature and anybody who uses them should also test their page on small screens, too.

Comment 1

[Jan Henning [:JanH]]

Option 1.) looks promising, now just need to turn that into some additional reftests as well...

Comment 2

[Hiroyuki Ikezoe (:hiro)]

CCing :cpeterson just in case he wasn't aware of this issue.

Comment 3

[Chris Peterson [:cpeterson]]

(In reply to Hiroyuki Ikezoe (:hiro) from comment #2)

CCing :cpeterson just in case he wasn't aware of this issue.

Thanks. I'll add the [geckoview] whiteboard tag so the GV team will review this bug during this week's triage meeting.

Comment 4

[Chris Peterson [:cpeterson]]

P3 for now because mozilla-mobile/fenix#2606 does not block Fenix MVP.

Comment 5

[Jan Henning [:JanH]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b4db32dd79d674a1c6d827e2a2213ac9aaa1641
https://treeherder.mozilla.org/#/jobs?repo=try&revision=738c6a03fe71ccc8a34e97dd7ed6eac771494aec

Comment 6

[Jan Henning [:JanH]]

Attachment: Bug 1552781 - Part 0: Clarify comment in reftest manifest. r?dbaron

The comment says that the last group of tests uses a non-zero lineThreshold,
but the last few tests actually don't.

Comment 7

[Jan Henning [:JanH]]

Attachment: Bug 1552781 - Part 1: Add reftests for flexbox/grid font inflation scope. r?dbaron

As we don't want the test samples to be inflated, test and ref files are
identical this time - the only difference is the prefs being used when loading
each file.

Comment 8

[Jan Henning [:JanH]]

Attachment: Bug 1552781 - Part 2: Flexboxes/grids should be font inflation flow roots. r?dbaron

Our algorithm for dividing a page up into separate font inflation flow roots
seems mostly based on the idea that a new Block Formatting Context (BFC) should
go hand in hand with a font inflation flow root.
Flex containers so far didn't follow that rule, since they technically create a
new Flex Formatting Context (FFC) and possibly also because nobody thought
about font inflation when implementing nsFlexContainerFrame.

This leads to all flex containers being counted against the next higher-level
flow root, meaning that a lot of small flex containers can get inflated if their
sum total of text collectively exceeds the font inflation threshold.
This alone is likely undesired most of the time, but is then also aggravated by
the fact that our flexbox behaviour under font inflation is somewhat buggy (bug
1142461).

As apart from the different layout rules inside the container, a FFC behaves
very much like a BFC, flex containers should therefore correspondingly become
font inflation flow roots, too, and therefore be considered individually for
font inflation.

As far as I can tell, with this change we'll also match Blink's behaviour in
that regard.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Should this be done for grid containers as well?

Comment 10

[Jan Henning [:JanH]]

Good point - hopefully grids on non mobile-friendly pages should be somewhat rarer than flexboxes, but who knows and I guess the same argument re formatting contexts applies there, too.
Might do it in a follow-up, though, because I'm away part of this week and again from next week and am not sure whether I have the time to add that inbetween.

Comment 11

[Jan Henning [:JanH]]

Attachment: Bug 1552781 - Part 3: Make flex/grid items font inflation flow roots, too. r?dbaron

The arguments for the respective container elements apply to their immediate
child items, too: They establish a new formatting context as well and presumably
represent page content that can be considered to be logically separate enough to
warrant individual consideration for font inflation.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/39ce05a6691d
Part 0: Clarify comment in reftest manifest. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/1a496fedfacc
Part 1: Add reftests for flexbox/grid font inflation scope. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/02d7c3c99561
Part 2: Flexboxes/grids should be font inflation flow roots. r=dbaron
https://hg.mozilla.org/integration/autoland/rev/a64db20f0ec9
Part 3: Make flex/grid items font inflation flow roots, too. r=dbaron

Comment 13

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/39ce05a6691d
https://hg.mozilla.org/mozilla-central/rev/1a496fedfacc
https://hg.mozilla.org/mozilla-central/rev/02d7c3c99561
https://hg.mozilla.org/mozilla-central/rev/a64db20f0ec9

Comment 14

[Chris Peterson [:cpeterson]]

68=wontfix because we don't need to uplift this fix for Fennec. Fenix will pick up this fix when it updates to GV 69.

Comment 15

[Jan Henning [:JanH]]

(In reply to Chris Peterson [:cpeterson] from comment #14)

68=wontfix because we don't need to uplift this fix for Fennec. Fenix will pick up this fix when it updates to GV 69.

Hmm - Fennec is affected, too, for those people who have turned on the "Use system font size" option, and as the fix for this particular problem is quite simple, I'd still prefer to get this into the next scheduled ESR release.

Comment 16

[Jan Henning [:JanH]]

Comment on attachment 9067610 [details]
Bug 1552781 - Part 1: Add reftests for flexbox/grid font inflation scope. r?dbaron

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Only way of fixing this for Fennec users now.
• User impact if declined: Desktop pages using flexboxes and/or CSS grids may appear strangely under font inflation (when "Use system font size" is turned on in Fennec), i.e. even small individual page elements which normally wouldn't have their font sizes increased are rendered with increased font sizes, leading to possibly strange looking or broken page layouts.
• Fix Landed on Version: 69
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): - Only small changes to the logic which page elements are marked up for further font inflation processing were required, but no fundamental changes to the core font inflation logic.

• The worst side effect of marking additional page elements as font inflation flow roots could be that we might now inflate too little page text in conjunction with flexboxes/grids, but a very similar logic is already successfully employed by Chrome's font inflation algorithm, and there are a few known pages where this change definitively improves our rendering under font inflation.
• Both font inflation in general and these changes in particular are covered by tests.
• Font inflation isn't used in desktop Firefox outside of testing.

• String or UUID changes made by this patch: none

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9067609 [details]
Bug 1552781 - Part 0: Clarify comment in reftest manifest. r?dbaron

Fixes some font inflation issues with flex box on mobile. Includes automated tests. We should try to have QA verify this as well. Approved for Fennec 68.1b2.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr68/rev/58cf4ae6d198
https://hg.mozilla.org/releases/mozilla-esr68/rev/9df75d7dcc93
https://hg.mozilla.org/releases/mozilla-esr68/rev/6534f2a515fd
https://hg.mozilla.org/releases/mozilla-esr68/rev/d011c5c8270b

Comment 19

[ovidiu boca[:Ovidiu]]

Hi Jan, can you please guide me and tell me what exactly needs to be verified here? Thanks

Comment 20

[Jan Henning [:JanH]]

First of all, make sure that Settings -> Accessibility -> Use system font size is enabled.

Then, the "check that it's working properly" bit is relatively easy: Visit any of for example

• https://einthusan.tv/movie/browse/?lang=hindi
• https://searchfox.org/mozilla-central/source/docshell/test/browser/browser_csp_uir.js
• https://www.ebuyer.com/
• https://www.livecoinwatch.com/

and check that you don't get font inflation (i.e. larger font size) for small bits of texts, as e.g. visible in the screenshots at https://github.com/webcompat/web-bugs/issues/34025.

The other bit regarding potential regressions would be seeing if you notice any pages (without a mobile-friendly layout, i.e. no <meta name="viewport" content="width=device-width"> tag) where font inflation used to work fine, but because of this patch we still stopped applying font inflation anyway. That's a bit more difficult though, because either it happens by chance on a page you visit often enough to notice the difference, or you'd have to try lots of pages in versions of Firefox both with and without this patch. And of course you're trying to prove a negative, which doesn't help either in deciding how many pages to test.
So basically this latter point is just something to watch out for generally, but you don't have to look too hard - if somebody's favourite page is now appearing in too tiny a font size, hopefully they'll report it somehow...

Comment 21

[Diana Rus]

Hi, issue is partially solved on Fennec 68.1b2 - Build 1 on;

• Sony Xperia Z3 (Android 5.1.1)
• Samsung Galaxy S9 (Android 8.0.0)
• Sony Xperia Z5 (Android 7.0)
• Google Pixel 3a XL (Android 9)

The rest of the elements from the page seem to be correctly displayed excluding the advertisements and the search bar. Please view the screenshot
This outcome is present on all devices.
The link with which the tests have been executed: https://searchfox.org/mozilla-central/source/docshell/test/browser/browser_csp_uir.js & https://www.livecoinwatch.com/

Note: Links which do not work or cannot use:

• https://einthusan.tv/movie/browse/?lang=hindi
• https://www.ebuyer.com/

Comment 22

[Jan Henning [:JanH]]

the search bar

Do you mean the searchbar on https://www.livecoinwatch.com/ that isn't visible in your screenshot because of the "Screenshot saved" notification, and specifically the fact that it is overhanging the end of the page somewhat? I'm getting the same behaviour without font inflation, or actually even when resizing the page on desktop, so that's simply the page not properly handling non-widescreen-desktop viewport widths.

the advertisements

Can't reproduce this, even when I'm getting what seems like the same ad as in your screenshot. If you can still reproduce this (in case the ad layout has somehow been updated in the intervening time), does this also happen in 68.1b1 for you?

Comment 23

[Chris Peterson [:cpeterson]]

Approved for Fennec 68.1b2.