Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upwindows: fix line height handling #17340
Conversation
highfive
commented
Jun 15, 2017
|
Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @asajeffrey (or someone else) soon. |
highfive
commented
Jun 15, 2017
|
|
|
@bors-servo try |
windows: fix line height handling hi there! on windows, this change properly scales a font's line height by its size. before, this was not being done at all, which resulted in super ugly text at larger em sizes.  i'm not sure what to do about tests: i wasn't able to find any similar tests, and the previous change (#16034) did not include any tests either. i believe this also solves #16476. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17340) <!-- Reviewable:end -->
|
@stshine Not sure that a try build is very meaningful, since we don't run tests on Windows. |
|
@jonathandturner Are you able to try this out? |
|
@jdm whoops, my bad. |
|
|
|
I tested this PR out and it appears to fix the spacing issues on Hacker News as well. Looks good! |
jonathandturner
commented
Jun 15, 2017
|
@jdm - building it now |
jonathandturner
commented
Jun 15, 2017
|
Hmmm, looks like my main computer (which is getting repaired as we speak) has my working Windows Servo environment. Seeing if I can put another one together |
jonathandturner
commented
Jun 15, 2017
|
@bors-servo: r+ |
|
|
windows: fix line height handling hi there! on windows, this change properly scales a font's line height by its size. previously, line height was not scaled to the font's size at all, which meant line heights become worse and worse the further you scaled away from the font's design size (in either direction, larger or smaller). this change makes the `line_gap` ratio and size scale with the font size. i've hand checked that the new computed `line_gap` matches the effective heights in chrome and firefox when `line-height = normal` for a bunch of system fonts. (servo's rendering quality on windows is a different story, though).  i believe this also solves #16476. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors <!-- Reviewable:start --> --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17340) <!-- Reviewable:end -->
|
|
hinaria commentedJun 15, 2017
•
edited
hi there!
on windows, this change properly scales a font's line height by its size.
previously, line height was not scaled to the font's size at all, which meant line heights become worse and worse the further you scaled away from the font's design size (in either direction, larger or smaller).
this change makes the
line_gapratio and size scale with the font size. i've hand checked that the new computedline_gapmatches the effective heights in chrome and firefox whenline-height = normalfor a bunch of system fonts. (servo's rendering quality on windows is a different story, though).i believe this also solves #16476.
./mach build -ddoes not report any errors./mach test-tidydoes not report any errorsThis change is