Skip to content
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

Add some unicode mapping for ligatures when writing the cmap table in the font (bug 1946181) #19427

Merged
merged 1 commit into from
Feb 14, 2025

Conversation

calixteman
Copy link
Contributor

No description provided.

… the font (bug 1946181)
@Snuffleupagus
Copy link
Collaborator

Can we add some kind of test-case for this, to avoid it regressing?
Also, please run all tests before I review this.

@calixteman
Copy link
Contributor Author

In thinking about that this morning, I wasn't able to have an idea.
But then I remembered we have some tests with fonts and ttx, so I suppose it could work here, wdyt ?

@Snuffleupagus
Copy link
Collaborator

But then I remembered we have some tests with fonts and ttx, so I suppose it could work here, wdyt ?

Yes, there's a few tests in https://github.com/mozilla/pdf.js/tree/master/test/font
Maybe that could work, but I unfortunately don't know how difficult it'd be to test this particular case.

@calixteman
Copy link
Contributor Author

I've tried to write a test but I didn't manage to and I don't really have time for the moment.
@Snuffleupagus could review it ? and I'll file a bug in order to add a test later.

@Snuffleupagus
Copy link
Collaborator

/botio browsertest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_browsertest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/d799316d3ceb4ea/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_browsertest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/2d7bf952384bd95/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/2d7bf952384bd95/output.txt

Total script time: 17.15 mins

  • Regression tests: FAILED
  different ref/snapshot: 93

Image differences available at: http://54.241.84.105:8877/2d7bf952384bd95/reftest-analyzer.html#web=eq.log

@calixteman
Copy link
Contributor Author

As far as I can tell we see the same kind of "errors" with saw when we added this unicode mapping in the font (iirc, the linux font renderer makes some adjustments based on the unicode value associated with a glyph).

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/d799316d3ceb4ea/output.txt

Total script time: 30.66 mins

  • Regression tests: Passed

@Snuffleupagus
Copy link
Collaborator

When we started moving all glyphs to a Private Use Area it was to prevent issues with particularly FreeType on Linux affecting glyph-shaping if we left glyphs at more "normal" positions in the cmap table.

With this patch we seem to be essentially regressing that, which might be problematical!?

@calixteman
Copy link
Contributor Author

When we started moving all glyphs to a Private Use Area it was to prevent issues with particularly FreeType on Linux affecting glyph-shaping if we left glyphs at more "normal" positions in the cmap table.

With this patch we seem to be essentially regressing that, which might be problematical!?

It was the same thing in #15157.
And here's the Jonathan's answer:
#15157 (comment)

The diffs in the ref tests (at least for the one I checked) only appear for ligatures and the variation are very small.
And I'd say the same thing I said:
#15157 (comment)
Let's see if some people complain and if the case we'll try to figure out an other solution.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's try this, while keeping an eye on any future Linux-only font bugs.

r=me, thank you.

@calixteman calixteman merged commit 92ff26e into mozilla:master Feb 14, 2025
9 checks passed
@calixteman
Copy link
Contributor Author

/botio-linux makeref

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_makeref from @calixteman received. Current queue size: 0

Live output at: http://54.241.84.105:8877/d0a49b3cd1ad185/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/d0a49b3cd1ad185/output.txt

Total script time: 16.77 mins

  • Make references: Passed
  • Check references: Passed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants