nicolo-ribaudo commented 3 months ago

Fixes https://github.com/mozilla/pdf.js/issues/14426

Commit message:

Overrride the minimum font size when rendering the text layer

Browsers have an accessibility option that allows user to enforce a minimum font size for all text rendered in the page, regardless of what the font-size CSS property says. For example, it can be found in Firefox under font.minimum-size.x-western.

When rendering the s in the text layer, this causes the text layer to not be aligned anymore with the underlying canvas. While normally accessibility features should not be worked around, in this case it is not improving accessibility:

the text is transparent, so making it bigger doesn't make it more readable

the selection UX for users with that accessibility option enabled is worse than for other users (it's basically unusable).

While there is tecnically no way to ignore that minimum font size, this commit does it by multiplying all the font-sizes in the text layer by minFontSize, and then scaling all the <span>s down by 1/minFontSize.

This PR detects the min font size similarly to https://github.com/mozilla/pdf.js/pull/14427, with a few changes:

the minimum font size is only computed once, rather than for every rendered element
I used getBoundingClientRect().height rather than getComputedStyle().fontSize, because getComputedStyle().fontSize returns the minimum font size in Chrome and Safari but not in Firefox (https://bugzilla.mozilla.org/show_bug.cgi?id=1903360)

~~Marking as draft because I still have to run all the tests locally, and add a new test.~~

nicolo-ribaudo commented 3 months ago

This is ready for review. I couldn't run the full test suite locally because makeref gives me a bunch of failures and timeouts in Chrome, but everything passed in Firefox.

Also, I couldn't add a test for Chrome because it seems like it's impossible to set its "Settings > Appearance > Customise fonts > Minimum font size" option through puppeteer (the only way to change it programmatically is through the extensions API, but writing a Chrome extension for this test seems overkill)

timvandermeij commented 3 months ago

Nit for the commit message: s/Overrride/Override.

Let's trigger a round of tests:

/botio test

moz-tools-bot commented 3 months ago

From: Bot.io (Linux m4)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/674ae2965e56596/output.txt

moz-tools-bot commented 3 months ago

From: Bot.io (Windows)

Received

Command cmd_test from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/2c26c2e6f59439f/output.txt

moz-tools-bot commented 3 months ago

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/674ae2965e56596/output.txt

Total script time: 28.92 mins

Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 16

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

moz-tools-bot commented 3 months ago

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/2c26c2e6f59439f/output.txt

Total script time: 44.77 mins

Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 2

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

nicolo-ribaudo commented 3 months ago

Uhm, taking a look at the integration failures 🤔 It didn't fail locally for some reason

timvandermeij commented 3 months ago

Note that most of them are known intermittents, see https://github.com/mozilla/pdf.js/issues?q=is%3Aopen+is%3Aissue+label%3Aintermittent for the associated issues, so I think only the Text layer when the browser enforces a minimum font size renders spans with the right size failure in the Windows logs is actually relevant for this PR.

nicolo-ribaudo commented 3 months ago

There was some variability on the exact number of pixels across different platforms (and across different browsers on the same OS, even if the test only checks Firefox), so I relaxed the assertion to be "the with must be within 3% of 315px". Without the fix the width is more than 200% of the expected with, so a 3% tolerance is ok to verify that the fix is being applied.

I don't have access to a windows machine, could you re-run the bot?

timvandermeij commented 3 months ago

/botio integrationtest

moz-tools-bot commented 3 months ago

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

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

moz-tools-bot commented 3 months ago

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 0

Live output at: http://54.193.163.58:8877/7c447b81f825640/output.txt

moz-tools-bot commented 3 months ago

From: Bot.io (Linux m4)

Success

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

Total script time: 7.91 mins

Integration Tests: Passed

moz-tools-bot commented 3 months ago

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/7c447b81f825640/output.txt

Total script time: 19.68 mins

Integration Tests: FAILED

nicolo-ribaudo commented 3 months ago

One of the two remaining windows failures is a flaky tests (https://github.com/mozilla/pdf.js/issues/17931), the other one is a timeout also related to pasting (Stamp Editor Copy and paste a stamp with an alt text must check that the pasted image has an alt text).

timvandermeij commented 3 months ago

Yes, both are flaky tests and the failures are unrelated to this PR, so the tests here should now be OK.

Snuffleupagus commented 3 months ago

Unfortunately it seems that the new integration test added in this PR has intermittent shutdown issues, based on the logs in http://54.241.84.105:8877/dbd55648b66ba65/output.txt:

[32m.[0mTEST-PASSED | renders spans with the right size
JavaScript warning: http://127.0.0.1:34837/build/generic/build/pdf.mjs, line 10681: Script terminated by timeout at:
#getMinFontSize@http://127.0.0.1:34837/build/generic/build/pdf.mjs:10681:25
#appendText@http://127.0.0.1:34837/build/generic/build/pdf.mjs:10573:71
#processItems@http://127.0.0.1:34837/build/generic/build/pdf.mjs:10534:23
render/pump/<@http://127.0.0.1:34837/build/generic/build/pdf.mjs:10453:27

timvandermeij commented 3 months ago

I think the problem is that pretty much all integration tests call closePages in the afterAll handler, which ensures that proper cleanup is done using the testingClose method; see: https://github.com/mozilla/pdf.js/blob/658b5fab5dffaf2d1a2006fb4573f2f910282452/test/integration/test_utils.mjs#L83-L94

However, in this new integration test, and also in other tests in this file, this pattern is not used and a separate browser is spawned instead. This is for good reason, but instead of only calling browser.close(), which doesn't do any cleanup of PDF.js, we should make sure the tests that spawn a new browser use the same cleanup mechanism.

We have a reference to the pages in the separate browser in these tests, so we should be able to close the page in a similar way to fix the issue.

timvandermeij commented 3 months ago

I have fixed the issue for the existing test in https://github.com/mozilla/pdf.js/pull/18317/commits/f4053c2b3e84fce7090451042193bdc01a18da87, which should make it a bit easier to mirror that approach for this test and that should solve the shutdown issue identified above.

nicolo-ribaudo commented 3 months ago

Thank you! I'll rebase and fix it.

timvandermeij commented 3 months ago

Could you rebase this patch once more onto the current master branch, now that all other test framework fixes have landed? I'll retrigger the tests here afterwards.

nicolo-ribaudo commented 3 months ago

Done 👍

timvandermeij commented 3 months ago

/botio integrationtest

moz-tools-bot commented 3 months ago

From: Bot.io (Linux m4)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 1

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

moz-tools-bot commented 3 months ago

From: Bot.io (Windows)

Received

Command cmd_integrationtest from @timvandermeij received. Current queue size: 1

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

moz-tools-bot commented 3 months ago

From: Bot.io (Linux m4)

Failed

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

Total script time: 7.91 mins

Integration Tests: FAILED

moz-tools-bot commented 3 months ago

From: Bot.io (Windows)

Success

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

Total script time: 19.06 mins

Integration Tests: Passed

nicolo-ribaudo commented 3 months ago

The remaining failure on Linux is https://github.com/mozilla/pdf.js/issues/17931.

Snuffleupagus commented 3 months ago

/botio test

moz-tools-bot commented 3 months ago

From: Bot.io (Linux m4)

Received

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

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

moz-tools-bot commented 3 months ago

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/1cee9608bb46e28/output.txt

moz-tools-bot commented 3 months ago

From: Bot.io (Linux m4)

Failed

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

Total script time: 28.84 mins

Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 11
  different first/second rendering: 1

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

moz-tools-bot commented 3 months ago

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/1cee9608bb46e28/output.txt

Total script time: 45.22 mins

Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  different ref/snapshot: 3

Image differences available at: http://54.193.163.58:8877/1cee9608bb46e28/reftest-analyzer.html#web=eq.log

timvandermeij commented 3 months ago

Thank you for fixing this!

nicolo-ribaudo commented 3 months ago

Thanks for the reviews!

mozilla / pdf.js

Override the minimum font size when rendering the text layer #18283

From: Bot.io (Linux m4)

Received

From: Bot.io (Windows)

Received

From: Bot.io (Linux m4)

Failed

From: Bot.io (Windows)

Failed

From: Bot.io (Linux m4)

Received

From: Bot.io (Windows)

Received

From: Bot.io (Linux m4)

Success

From: Bot.io (Windows)

Failed

From: Bot.io (Linux m4)

Received

From: Bot.io (Windows)

Received

From: Bot.io (Linux m4)

Failed

From: Bot.io (Windows)

Success

From: Bot.io (Linux m4)

Received

From: Bot.io (Windows)

Received

From: Bot.io (Linux m4)

Failed

From: Bot.io (Windows)

Failed