-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Text baseline alignment issues #1562
base: master
Are you sure you want to change the base?
Conversation
@@ -947,7 +947,7 @@ describe('Canvas', function () { | |||
assert.ok(metrics.alphabeticBaseline > 0) // ~4-5 | |||
assert.ok(metrics.actualBoundingBoxAscent > 0) | |||
// On the baseline or slightly above | |||
assert.ok(metrics.actualBoundingBoxDescent <= 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i tweaked the baseline offset, so i expect some text measurement to change slightly. Unsure if this is ok or not. Any thought anyone?
cairo_matrix_t matrix; | ||
cairo_get_matrix(ctx, &matrix); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
matrix does not seem to be used in the function. Is cairo_get_matrix
having some side effect that is necessary for the code below to work?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like it is used for alphabeticBaseline
below
@zbjornson @LinusU any comment / recomendation? |
This is way outside my area of expertise 😅 I'm always a bit afraid of magic numbers, but since this clearly makes it better I think it's a good change 👍 Hopefully @zbjornson & @chearon will have some better input 😄 |
You should be able to get the right X-height via pango_font_metrics_get_strikethrough_position (for the "middle" baseline). For the other ones I guess some kind of hack is reasonable. I don't quite understand it yet and I'll try to give a fuller review later. Ideally we would extract the FreeType face and look at ideographic/hanging/etc from the font tables. But Pango doesn't use FreeType on macOS/Windows so there'd have to be platform-specific code. More reasons I should remove Pango and use lower-level libraries... |
i m willing to write more code if it does not hinder performances. |
sorry i forgot about this PR completly. |
@asturur no worries, if you feel that you don't have time to improve further, I'm open to merging it as is! Thank you for spending time on this project |
I'll give a stab tomorrow! |
@charon, i tried this: PangoRectangle logical_rect;
PangoLayout* measureLayout = pango_layout_copy(layout);
PangoFontMetrics *metrics;
pango_layout_set_text(measureLayout, "gjĮ測試ÅÊ", -1);
pango_layout_line_get_extents(pango_layout_get_line(measureLayout, 0), NULL, &logical_rect);
metrics = PANGO_LAYOUT_GET_METRICS(measureLayout);
int strike_thickness = pango_font_metrics_get_strikethrough_thickness(metrics);
int strike_position = pango_font_metrics_get_strikethrough_position(metrics);
double scale = 1.0 / PANGO_SCALE;
double ascent = scale * pango_layout_get_baseline(measureLayout);
double descent = scale * logical_rect.height - ascent;
double middle = ascent - (scale * strike_position) + (scale * strike_thickness / 2);
// 0.072 is a constant that has been chosen comparing the canvas output
// if some code change, this constant can be changed too to keep results aligned
double correction_factor = scale * logical_rect.height * 0.072; EDIT: tested on the macbook where i developed the code, this seems good: my new macbook cannot compile node-canvas correctly, i get the text scaling bug, node12, catalina 10.15.5 |
no as not said. |
ed370b7
to
2b5e1f3
Compare
The tests aren't passing anymore. While before they passed. What could it be? is something changed in tests? I reverted and push forced to the passing commit. |
So this PR should be updated to use (edit: and not sure about the tests, sorry!) |
i did update it, but the results with scaling where compleltely off, so i reverted back.
The result was OK for middle when used at scale 1, but bad when used with any other scaling. |
Any updates on this? This is an issue that I'd really like to see fixed. |
i did not have time to investigate why the test started failing, i can try again rebasing the pr on a fresh branch |
This was a minor fix, wasn't a final solution for 1:1 match with canvas. |
This PR tries to improve the precision with which different text baselines are calculated.
With node-canvas the alphabetic and bottom baseline is always perfect, it works good.
top, middle, ideographic and hanging are a bit off.
The tweak is a 2 part change.
In the function
getBaselineAdjustment
we copy the pangoLayout and we swap the text with a string that uses all the extent of the selected font, this improve automatically themiddle
use case.Then a correction factor is applied to the positions that do not seem correct, this correction factor is found with trial and error.
AFTER FIX
FIREFOX
CHROME
BEFORE FIX
FIREFOX
CHROME