Try to fix the font rendering issue on redit on Windows #15937

codec-abc · 2017-03-14T09:39:05Z

This is quick "fix" for #15698. I am quite sure that the fix only fix the effect and not the cause. As such the PR serve more as a discussion as jonathandturner suggested. I suggest reviewer to start reading the short issue's history first.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #15698

About the test I do not have one but I was able to reproduce the issue with the following :

<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml">
   <head>
      <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
   </head>
   <body>
        <div>a</div>
        <div>b</div>
        <div>c</div>
   </body>
</html>

Whereas in FF and Chrome each character a,b and c are one below the others, in Servo on Windows they are overlapping.
So if the problem come from the line gap with the default font I think that we can add some test to ensure that line gaps for default font are not 0.

This change is

EDIT : issue number


        Try to fix the font rendering issue on redit on Windows

highfive · 2017-03-14T09:39:10Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @KiChjang (or someone else) soon.

highfive · 2017-03-14T09:39:11Z

Warning

These commits modify gfx code, but no tests are modified. Please consider adding a test!

KiChjang · 2017-03-14T09:40:25Z

r? @emilio

nox · 2017-03-14T10:39:31Z

I've removed @jonathandturner's mention from the PR message to avoid too numerous email notifications when this gets propagated back into Gecko's repositories.

emilio · 2017-03-14T12:49:06Z

@vvuk does this PR look reasonable to you? I think it's really wallpaper-ish.

codec-abc · 2017-03-14T15:12:42Z

I copied and paste the wrong issue number when creating the pull request. Sorry about that.

codec-abc · 2017-03-14T18:19:20Z

Working on another bug related to fonts in Windows I think I understand the problem better. If we take a look at the integration of freetype we can see that the line_gap value is the height value coming from freetype. This comment says 'leading' is supposed to be the vertical distance between two baselines, reflected by the height attribute in freetype. While the DirectWrite Documentation state in the lineGap paragraph that "The recommended line spacing (baseline-to-baseline distance) is the sum of ascent, descent, and lineGap". To sump up I think the variables names does not match across the APIs and the line_gap value (for the FontMetrics) for Windows should be computed as dm.ascent + dm.descent+ dm.lineGap. I have yet to test this and will do tomorrow.

jonathandturner · 2017-03-16T01:30:18Z

@codec-abc - I tried your suggestion, doing something like:

-            line_gap:         au_from_du(dm.lineGap as i32),
+            line_gap:         au_from_du((dm.ascent + dm.descent + dm.lineGap as u16) as i32),

Which gives:

Not sure if that's correct (the gap looks a little bigger than the equiv in firefox), but it does look better.

codec-abc · 2017-03-16T09:44:30Z

Nice! I think the best thing to do now is to run servo on simple web page (like the one provided with the first comment of this PR) under Linux and Windows and compare the values of the structures coming from FreeType and DirectWrite. If the settings are the same (same font, same display dpi, etc...) we should get the same results and we should be able to find out the correct maths.

Random notes:

After more thinking It is weird that if the problem come from the baseline to baseline being wrong on Windows, it only break the layout on Reddit. I expect that any page having text to be messed up.
Is the font spacing on Linux and MacOS matching the ones in FF & Chrome ?
On a Totally not related subject, I cannot find a way to print debug instruction in Servo which make the debugging a bit more difficult. I suspect the standard output being redirected but I have found nothing about it in the wiki. Am I missing something obvious?

jdm · 2017-03-16T10:06:42Z

@codec-abc println! goes directly to the terminal. What are you trying that does not work?

codec-abc · 2017-03-16T10:45:16Z

@jdm I think I was fooled by the debugger (Visual Studio). I set a println! instruction before a line were a breakpoint was hit each time but I got nothing on the console. Thus I infer it did not worked. Putting println! instruction elsewhere worked. So I think that Visual Studio hit breakpoint on parts on code that are not actually run through at runtime.

jonathandturner · 2017-03-16T23:26:15Z

@codec-abc - I looked at the font metrics for macos and windows, but it's too difficult to line up the numbers to see where one is different. There are dozens of log messages just for font metrics alone for opening reddit.

Like I said, it looks close. Perhaps we can land it and then tweak.

codec-abc · 2017-03-17T00:12:52Z

I think it should be manageable is you use something like that because you should get only one value for the FontMetric struct.

<!DOCTYPE html>
<html xmlns="http://www.w3.org/1999/xhtml">
   <head>
      <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
   </head>
   <body style="font-family:'Times New Roman';">
<div>A line</div>
<div>Another line</div>
   </body>
</html>

When rendering this simple web page on Servo I got the 2nd line on top on the first, which is I believe the exact same thing happening on Reddit.

For debugging the other font issues on Windows I usually use http-server or live-server to serve a tiny web page. That way I can easily focus on the part that I want while not being overwhelmed by data.

Concerning the pull request it may be better the to be sure to have the math right changing the code as it will surely reduce the risk to have to deal with many other issues later.

jonathandturner · 2017-03-17T01:09:35Z

I'm not sure I'll have time to keep looking into this until possibly sometime next week, but please do keep going. I'd be interested to see if you can get the platforms lining up.


        Auto merge of #16034 - jonathandturner:fix_font, r=metajack

Fix Windows UI and font squishing  This PR: * updates the browserhtml dep (fixes #15255) * updates the Windows font metric calculation (fixes #15698, based on codec-abc #15937 (comment)) * may address #15933. With this PR, I was not able to repro ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because change are to UI/visual results of font drawing   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/16034)

bors-servo · 2017-03-21T10:51:41Z

☔ The latest upstream changes (presumably #16034) made this pull request unmergeable. Please resolve the merge conflicts.

jdm · 2017-03-21T11:38:17Z

#16034 has resolved this. Thanks for investigating this @codec-abc!


        servo: Merge #16034 - Fix Windows UI and font squishing (from jonatha…

…ndturner:fix_font); r=metajack  This PR: * updates the browserhtml dep (fixes servo/servo#15255) * updates the Windows font metric calculation (fixes servo/servo#15698, based on codec-abc servo/servo#15937 (comment)) * may address servo/servo#15933. With this PR, I was not able to repro ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because change are to UI/visual results of font drawing  Source-Repo: https://github.com/servo/servo Source-Revision: cea1760eb7689d88b18525fac7d53f9a12fdb8a1 --HG-- extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear extra : subtree_revision : e9654e401876e04082919212e16f185c2992cdc9


        servo: Merge #16034 - Fix Windows UI and font squishing (from jonatha…

…ndturner:fix_font); r=metajack  This PR: * updates the browserhtml dep (fixes servo/servo#15255) * updates the Windows font metric calculation (fixes servo/servo#15698, based on codec-abc servo/servo#15937 (comment)) * may address servo/servo#15933. With this PR, I was not able to repro ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because change are to UI/visual results of font drawing  Source-Repo: https://github.com/servo/servo Source-Revision: cea1760eb7689d88b18525fac7d53f9a12fdb8a1


        servo: Merge #16034 - Fix Windows UI and font squishing (from jonatha…

…ndturner:fix_font); r=metajack  This PR: * updates the browserhtml dep (fixes servo/servo#15255) * updates the Windows font metric calculation (fixes servo/servo#15698, based on codec-abc servo/servo#15937 (comment)) * may address servo/servo#15933. With this PR, I was not able to repro ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because change are to UI/visual results of font drawing  Source-Repo: https://github.com/servo/servo Source-Revision: cea1760eb7689d88b18525fac7d53f9a12fdb8a1 UltraBlame original commit: fad072f940ccdfb70bb1799bb256841070e0993c


        servo: Merge #16034 - Fix Windows UI and font squishing (from jonatha…

…ndturner:fix_font); r=metajack  This PR: * updates the browserhtml dep (fixes servo/servo#15255) * updates the Windows font metric calculation (fixes servo/servo#15698, based on codec-abc servo/servo#15937 (comment)) * may address servo/servo#15933. With this PR, I was not able to repro ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because change are to UI/visual results of font drawing  Source-Repo: https://github.com/servo/servo Source-Revision: cea1760eb7689d88b18525fac7d53f9a12fdb8a1 UltraBlame original commit: fad072f940ccdfb70bb1799bb256841070e0993c


        servo: Merge #16034 - Fix Windows UI and font squishing (from jonatha…

…ndturner:fix_font); r=metajack  This PR: * updates the browserhtml dep (fixes servo/servo#15255) * updates the Windows font metric calculation (fixes servo/servo#15698, based on codec-abc servo/servo#15937 (comment)) * may address servo/servo#15933. With this PR, I was not able to repro ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #__ (github issue number if applicable).  - [ ] There are tests for these changes OR - [X] These changes do not require tests because change are to UI/visual results of font drawing  Source-Repo: https://github.com/servo/servo Source-Revision: cea1760eb7689d88b18525fac7d53f9a12fdb8a1 UltraBlame original commit: fad072f940ccdfb70bb1799bb256841070e0993c

Try to fix the font rendering issue on redit on Windows

Loading status checks…

6bd8472

highfive assigned KiChjang Mar 14, 2017

highfive added the S-awaiting-review label Mar 14, 2017

highfive assigned emilio and unassigned KiChjang Mar 14, 2017

jonathandturner mentioned this pull request Mar 20, 2017

Fix Windows UI and font squishing #16034

Merged

4 of 5 tasks complete

highfive added the S-needs-rebase label Mar 21, 2017

jdm closed this Mar 21, 2017

codec-abc mentioned this pull request May 2, 2017

Line height is not properly calculated #16476

Closed

servo / servo

Try to fix the font rendering issue on redit on Windows #15937

Try to fix the font rendering issue on redit on Windows #15937

codec-abc commented Mar 14, 2017 •

edited

highfive commented Mar 14, 2017

highfive commented Mar 14, 2017

KiChjang commented Mar 14, 2017

nox commented Mar 14, 2017

emilio commented Mar 14, 2017

codec-abc commented Mar 14, 2017

codec-abc commented Mar 14, 2017 •

edited

jonathandturner commented Mar 16, 2017

codec-abc commented Mar 16, 2017 •

edited

jdm commented Mar 16, 2017

codec-abc commented Mar 16, 2017

jonathandturner commented Mar 16, 2017

codec-abc commented Mar 17, 2017

jonathandturner commented Mar 17, 2017

bors-servo commented Mar 21, 2017

jdm commented Mar 21, 2017

servo / servo

Join GitHub today

Try to fix the font rendering issue on redit on Windows #15937

Try to fix the font rendering issue on redit on Windows #15937

Conversation

codec-abc commented Mar 14, 2017 • edited

highfive commented Mar 14, 2017

highfive commented Mar 14, 2017

KiChjang commented Mar 14, 2017

nox commented Mar 14, 2017

emilio commented Mar 14, 2017

codec-abc commented Mar 14, 2017

codec-abc commented Mar 14, 2017 • edited

jonathandturner commented Mar 16, 2017

codec-abc commented Mar 16, 2017 • edited

jdm commented Mar 16, 2017

codec-abc commented Mar 16, 2017

jonathandturner commented Mar 16, 2017

codec-abc commented Mar 17, 2017

jonathandturner commented Mar 17, 2017

bors-servo commented Mar 21, 2017

jdm commented Mar 21, 2017

codec-abc commented Mar 14, 2017 •

edited

codec-abc commented Mar 14, 2017 •

edited

codec-abc commented Mar 16, 2017 •

edited