1458159 - Giving a div overflow:hidden can chop off its text's descenders in cases on Windows

Status: RESOLVED FIXED

(Core :: Graphics: Text, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+11]

Just a spawn of bug 1406552 since that's more related to Linux and Android, while I found it also affects Windows and I found the reason on Windows and have a patch for it.

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

Attachment: Bug 1458159 - Use rounding instead of ceiling on max{Ascent,Descent} for DWriteFont.

The ceiling was introduced in bug 549190 for improve the consistency of
underline positioning. However, removing ceiling now doesn't seem to
regress the testcases in that bug, probably thanks to improvement in
other part.

The ceiling here causes us to have different font metrics than other
browsers on Windows, and can lead to webcompat issue. We also don't do
this for other backends. So it's probably better just remove it.

Review commit: https://reviewboard.mozilla.org/r/240912/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240912/

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

Looks like it causes lots of test failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e15d653b1f08ce05e169373187205117b8f3874

Comment 3

[Jonathan Kew [:jfkthame]]

I suspect that we (at least) want some kind of rounding, so that ascent+descent is a whole number of device pixels. Whether to round each value separately or round the total and then figure out how to divide it between ascent/descent is unclear...

(Anything that changes how we compute font/line metrics has potentially far-reaching effects, as it can disturb the positioning/alignment of text in the browser UI as well as on web pages. As such, I'd be reluctant to make a change here when we're in the pre-beta freeze; it should probably be aimed at the beginning of a cycle for maximum Nightly testing time.)

Comment 4

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8972243 [details]
Bug 1458159 - Use rounding instead of ceiling on max{Ascent,Descent} for DWriteFont.

Cancel the review request for now.

I'll revisit this after we start 62 nightly and try to fix the test failures.

Comment 5

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8972243 [details]
Bug 1458159 - Use rounding instead of ceiling on max{Ascent,Descent} for DWriteFont.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240912/diff/1-2/

Comment 6

[Xidorn Quan [:xidorn] UTC+11]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=0fd73926b0a85598e1c5acc7cc6e1ae9b671f14c

Comment 7

[Jonathan Kew [:jfkthame]]

Comment on attachment 8972243 [details]
Bug 1458159 - Use rounding instead of ceiling on max{Ascent,Descent} for DWriteFont.

https://reviewboard.mozilla.org/r/240912/#review251336

OK, this seems reasonable; let's try it. Thanks for looking into the tests.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5352d48512f7
Use rounding instead of ceiling on max{Ascent,Descent} for DWriteFont. r=jfkthame

Comment 9

[Andreea Pavel [:apavel]]

Backed out for failing reftest min-intrinsic-with-percents-across-elements.html  == min-intrinsic-with-percents-across-elements-ref.html 

Push that started the failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=5352d48512f78e1dd767ce24ca6292396bb21fac

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=179463324&repo=autoland&lineNumber=4230

Backout: https://hg.mozilla.org/integration/autoland/rev/f1fe5e4c8ce5f8bab3ed63e2c751fd3a5db43a3c

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Tests relying on consistency of font metrics are so hard to get right cross platform /_\

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Comment on attachment 8972243 [details]
Bug 1458159 - Use rounding instead of ceiling on max{Ascent,Descent} for DWriteFont.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240912/diff/2-3/

Comment 12

[Xidorn Quan [:xidorn] UTC+11]

So it is mostly a test bug that the reference doesn't reflect what should happen in test by adding a wrapper but lacking an corresponding rule.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1be70a3d127f
Use rounding instead of ceiling on max{Ascent,Descent} for DWriteFont. r=jfkthame

Comment 14

[Noemi Erli[:noemi_erli]]

Backed out changeset 1be70a3d127f (bug 1458159) for reftest failures in sizing-orthog-vlr-in-htb-008.xht on a CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=1be70a3d127fa7e5cfced92c29175c5f8907cb44&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified&selectedJob=179576538

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=179576538&repo=autoland&lineNumber=56871

Backout: https://hg.mozilla.org/integration/autoland/rev/d6f131d9d8a00d576330ae7fee1ab662d6359574

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e86e1123dd47
Use rounding instead of ceiling on max{Ascent,Descent} for DWriteFont. r=jfkthame

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8eaf4eadc303
followup - Update reftest expectations in w3c-css/received/reftest.list on CLOSED TREE

Comment 17

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/e86e1123dd47
https://hg.mozilla.org/mozilla-central/rev/8eaf4eadc303