Closed Bug 1406552 Opened 7 years ago Closed 6 years ago

Giving a div overflow:hidden can chop off its text's descenders in cases where other browsers do not do so.

Categories

(Core :: Graphics: Text, defect, P3)

Product:
Core
Component:
Graphics: Text
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- wontfix
firefox61 --- fixed

People

(Reporter: wisniewskit, Assigned: xidorn)

References

Details

(Whiteboard: [webcompat:p1])

User Story

Business driver: Achieve tier-1 Google Search experience for Gecko on Android

Note: Compat risk depends on if we change layout or painting

Attachments

(5 files)

testcase.html
169 bytes, text/html
Details
testcase2.html
152 bytes, text/html
Details
Roboto-Regular.ttf
298.45 KB, application/x-font-ttf
Details
modified testcase
313 bytes, text/html
Details
Bug 1406552 - Only update max ascent / descent with em ones when ascent and descent are zero.
59 bytes, text/x-review-board-request
jfkthame
: review+
Details
Attached file testcase.html — 
Firefox does not seem to place text the same way inside of the div in this testcase as other browsers do (Blink, WebKit and Edge), and so the overflow:hidden combined with a line-height that's too narrow means the descenders of the text are partly hidden.

This seems font-sensitive, as the default Firefox chooses on OSX is not cut off this way (Times). I also barely see any text cut off on my Quantum reference laptop in Windows 10. However, it's quite obvious on Linux and with Roboto, which is used by Google on their tier-1 mobile site.

To my untrained eye it seems as though other browsers are just selecting a baseline for the text which ensures that the descenders do not get cut off in such an scenario.

In addition, I'm uncertain if this is related to bug 1404442, but it could be.
Flags: webcompat?
status-firefox57: --- → wontfix
Priority: -- → P3
Attached file testcase2.html — 
It isn't just divs, it's also possible to have this happen with an input that is turned into a non-replaced element (border:0, etc). Here's another testcase.
See Also: → https://webcompat.com/issues/8773
I think https://github.com/webcompat/web-bugs/issues/8642 is also related. http://jsbin.com/poqifa is a very simple repro where text is positioned lower in Firefox than in all other browsers (https://www.browserstack.com/screenshots/563f71c84e09cb07b9f241835df88bf67a454f49), leading to some UI elements not being vertically centered in Google search on Firefox.

I don't know if this is specified anywhere, or if it reasonably could be specified.  But one way or another it seems like a recurring compat problem fairly unique to Firefox and so seems important for us figure out how to address.
CC'ing Mike on this one just in case.
More data and discussion with Chrome's text layout expert here: https://github.com/webcompat/web-bugs/issues/8642#issuecomment-345207779

At this point the underlying interop issue seems entirely blocked on Firefox text rendering experts chiming in.
(In reply to Rick Byers from comment #4)
> More data and discussion with Chrome's text layout expert here:
> https://github.com/webcompat/web-bugs/issues/8642#issuecomment-345207779
> 
> At this point the underlying interop issue seems entirely blocked on Firefox
> text rendering experts chiming in.

jfkthame, could you take a look?
Flags: needinfo?(jfkthame)
It's a known issue that the computation of font/line-spacing metrics is not entirely consistent between browsers, for a bunch of legacy reasons.

For the <input> example (testcase2.html), I think bug 752790 would help, at least slightly, because there's currently a difference between us and Chrome in how the clipping is handled. However, it's still perfectly possible for clipping to occur, depending on the font that's being used, because the assumption that text can be fully displayed within a height of 1em (or 1.1 em, or any other hard-coded amount!) is simply not safe.

(For an extreme example, try adding font-family:Zapfino to the testcases, and see how much clipping happens -- in all browsers.)

Incidentally, looking at the <input> example in Safari, I see even worse clipping than in Firefox: there's only about 1px of descender left visible, so the "y" looks more like a "v". (And even in Chrome, where the text is allowed to paint in the input field's top/bottom padding, there is still slight clipping, most visible as "flattening" of the tail of the "g". That's very similar to the result we'd have if we implement Mats' suggestion in bug 752790.)

Returning to the <div> testcase, as already noted, it's highly font-dependent -- and so because different browsers may use different default fonts, results are likely to differ. (Add to that the differences in precisely how metrics are handled even for the same font, because of rounding differences or different heuristics for dealing with the multiple, conflicting sets of metrics provided in some font resources.) FWIW, if I change the serif font to Baskerville, for example, here on macOS, then all 3 browsers (Firefox, Chrome, Safari) show a very similar result, with just the bottom pixel or so of the descender of "g" being clipped.

While it would be nice to work towards more interoperable behavior here, this is primarily a web authoring problem: such tightly-constrained layouts are simply not safe (in the sense of "reliably able to display the complete text"). What if the user has a different default font than the designer expected, perhaps for accessibility reasons (or doesn't have the expected font installed), and this font has taller/deeper ascenders and descenders? What happens when the content includes accents stacked above and below the letters? What if someone enters text in a non-Latin-script language where the glyphs require more vertical space?
Flags: needinfo?(jfkthame)
My concern is simply from the webcompat perspective. Web authors cannot avoid this issue in any sane way that I can see, making this *our* problem, not theirs. They will see their designs working well enough on three of four major engines, and will have little choice but to just let the fourth one look different. If we could provide a relatively simple, sane way for authors to avoid this in such common cases, then at least webcompat outreach would be trivial. Whether that means supporting a hypothetical CSS property like "font-size: fit-parent-content-box" or tweaking Gecko's font heuristics, I think it's worth investigating sooner, rather than later.
Flags: webcompat? → webcompat+
Whiteboard: [webcompat] → [webcompat:p1]
User Story: (updated)
User Story: (updated)
I was suspecting whether it may related to different font metrics different browser uses for placing the text, but it turns out it isn't. Specifically, I thought others may be using em box to center, but they are actually using max height box like us as well, because if they use different metrics, we would see larger offset as the font size grows, but it seems to me that the offset keeps being 1px.

That makes me feel that this is just some rounding difference, and when the size is small, 1px is significant enough to hide some more descender than others.

Given that descender seems to be a problem more often than ascender, maybe we can just round it in a different way?
Assignee: nobody → xidorn+moz
Status: NEW → ASSIGNED
So, I think the issue here is probably that, we compute maxAscent using this on Windows:
> mMetrics->maxAscent = ceil(fontMetrics.ascent * mFUnitsConvFactor);
where fontMetrics.ascent = 1825, and mFUnitsConvFactor = font-size / 2048, which is 18px in this case.

fontMetrics.ascent * mFUnitsConvFactor = 16.04 however we ceil it to 17px while other browsers probably all use 16px in this case.

The rounding here was added in bug 549190 to improve underline consistency, but it seems removing it doesn't regress the behavior there now. So we can just remove that.

The problem is that, this is a Windows-specific fix, and it works only on testcase 1, not testcase 2, and thus it seems to be unrelated to the User Story about Google on Android...
I spawned the Windows issue to bug 1458159 given that I have a patch for it...
I tried to run testcase 1 with Roboto font downloaded from Google Fonts (using as user font with @font-face rule), and I cannot reproduce any issue on either my Linux virtual machine (with Firefox 59.0.2 release) or my Android phone (with Firefox Android 60.0b15).

However, when I tried to look into some of the webcompat issues, I found that some of them indeed can still reproduce on my phone (but not on Linux, unfortunately). Specifically I tested https://webcompat.com/issues/10355 and https://webcompat.com/issues/7348 both have the same result.

It seems to me that both Linux and Android should be using the FT2 backend, so it's not clear to me why they have difference.
So I found something interesting that, when I use the downloaded Roboto font, everything works fine, but if I use `local(Roboto)`, then descender seems to be cropped more.

I tried to pull the Roboto font from my Android phone, and use that for the testcase instead. With that, I successfully reproduce this issue on both Linux and Android.

The Roboto font from my phone is versioned 2.138, while the one downloaded from Google Fonts is versioned 2.137, and their size is quite different as well. The former is 299KB, and the latter is only 168KB.

Interestingly, with that font, this issue is reproducible on all browsers on Windows including Chrome itself as well as a Firefox build with patch in bug 1458159. That may be one of the reasons why Google doesn't publish that version for web fonts and only use it on Android. But neither Google Android nor Chromium on Linux has this problem.

So probably there is something to do with our FreeType backend...
Attached file Roboto-Regular.ttf — 
This is the Roboto font file pulled from my Android phone, which can reproduce this issue on Linux and Android.
Attached file modified testcase — 
This is the modified testcase 1 which uses user font for Roboto, and also has <meta name="viewport"> added so that it works better on Android browsers.
Component: Layout: Block and Inline → Graphics: Text
FWIW, it seems that with that font we have similar issue on macOS, while Chrome and Safari don't.
So the max ascent and descent we initially get from the font is 17px and 5px correspondingly, which should generate a result matching other browsers (22px max height, so 1px cropped from each side of baseline for an area with 20px, leaving 16px above and 4px below the baseline).

However, em ascent we get from the OS/2 table is 18.86px, so we change the max ascent to the rounding of it[1], 19px. This makes us have 24px height, and 2px is cropped from each side, so we have only 3px below the baseline.

The related code was initially added in bug 385263, and according to the comment, is for fixing bug 279032.

Reading the code comment as well as comment in bug 279032, I suspect that the code was there just for working around a single font which has zero ascent and descent in their HHEA, which could crash Firefox 13yrs ago.

I would suggest that we change it so that we only use OS/2 metrics when ascent or descent is zero.


[1] https://searchfox.org/mozilla-central/rev/ce9ff94ffed34dc17ec0bfa406156d489eaa8ee1/gfx/thebes/gfxFT2FontBase.cpp#324-331
The change here causes css/CSS2/margin-padding-clear/padding-em-inherit-001.xht to unexpected pass? I don't get how this can happen, really... That test doesn't seem to be related to font metrics at all. It seems to purely use em unit...

Also according to the meta data, it is expected to fail on all platforms, which is surprising. At least it doesn't fail locally for me one Windows...
And the reftest
> != underline-offset-change-1-ref.html underline-offset-change-2-ref.html # Bug 534132
fails on all platforms with this change, which is probably good...
(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #18)
> Also according to the meta data, it is expected to fail on all platforms,
> which is surprising. At least it doesn't fail locally for me one Windows...

Actually... the wpt doesn't fail locally on Linux without this patch as well...
OK, so CSS tests only run on linux platform[1]... that explains why the meta shows failure on all. Anyway, it seems we can just remove that failure expectation.

[1] https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/testing/mozharness/scripts/web_platform_tests.py#201-202
Comment on attachment 8972820 [details]
Bug 1406552 - Only update max ascent / descent with em ones when ascent and descent are zero.

https://reviewboard.mozilla.org/r/241380/#review247738

OK, seems reasonable. Anything that affects how we read/compute font metrics is always a bit worrying, given the potential for unexpected effects on how particular pages render, but this looks like it should be safe enough. Though maybe worth holding off until after the merge, rather than landing right at the end of the cycle? And then if the webcompat issue is important enough to justify it, ask for uplift once it has had a week or two of bake time on Nightly. Something to consider, at least.
Attachment #8972820 - Flags: review?(jfkthame) → review+
I thought about that, and I believe it isn't very risky. Any change should mostly be just cosmetic.

Also given our Android and Linux user population, I don't expect much feedback for this kind of change from just nightly users, so I think we should just land it now.
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/355a6e3f9bed
Only update max ascent / descent with em ones when ascent and descent are zero. r=jfkthame
https://hg.mozilla.org/mozilla-central/rev/355a6e3f9bed
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
I've retested the "See also" issues and below are my results:
- https://github.com/webcompat/web-bugs/issues/10355 - fixed
- https://github.com/webcompat/web-bugs/issues/7348 - fixed
- https://github.com/webcompat/web-bugs/issues/8642 - no longer reproducible since the design has changed 
- https://github.com/webcompat/web-bugs/issues/8773 - fixed

Tested with:
Browser / Version: Firefox Nightly 62.0a1 (2018-05-09)
Operating System: Samsung Galaxy Tab S3 (Android 7) - 1536 x 2048 pixels, 4:3 ratio (~264 ppi density), Samsung Galaxy S6 (Android 7.0) - Resolution 1440 x 2560 pixels (~577 ppi pixel density), Google Pixel (Android 8.1.0) - 1080 x 1920 pixels (~441 ppi pixel density)
Add-on: Chrome UA on Google For Firefox Android
Thanks Oana!
Status: RESOLVED → VERIFIED

Hi I still see this behaviour today using macOS Firefox 76. Should we raise another bug?

(In reply to laheen from comment #30)

Hi I still see this behaviour today using macOS Firefox 76. Should we raise another bug?

Yes, please. Thank you.

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

Attachment

General

Creator:
Created:
Updated:
Size: