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

nicolo-ribaudo · 2024-06-18T18:11:36Z

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 #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.~~

src/display/text_layer.js

nicolo-ribaudo · 2024-06-19T13:19:12Z

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 · 2024-06-19T14:55:50Z

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

Let's trigger a round of tests:

/botio test

moz-tools-bot · 2024-06-19T14:55:52Z

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 · 2024-06-19T14:55:52Z

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 · 2024-06-19T15:24:48Z

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 · 2024-06-19T15:40:39Z

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 · 2024-06-19T15:59:45Z

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

timvandermeij · 2024-06-20T13:42:07Z

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 · 2024-06-21T09:31:15Z

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 · 2024-06-21T12:47:26Z

/botio integrationtest

moz-tools-bot · 2024-06-21T12:47:29Z

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 · 2024-06-21T12:47:29Z

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 · 2024-06-21T12:55:24Z

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 · 2024-06-21T13:07:11Z

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 · 2024-06-21T13:09:51Z

One of the two remaining windows failures is a flaky tests (#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 · 2024-06-21T13:17:58Z

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

timvandermeij

The change looks good to me, but I'd also prefer a review from @Snuffleupagus to make sure that the comments/concerns from the original PR #14427 are fully resolved here. Thanks!

Snuffleupagus · 2024-06-22T12:33:12Z

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 · 2024-06-22T17:56:44Z

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:

pdf.js/test/integration/test_utils.mjs

Lines 83 to 94 in 658b5fa

    
           function closePages(pages) { 
        
             return Promise.all( 
        
               pages.map(async ([_, page]) => { 
        
                 // Avoid to keep something from a previous test. 
        
                 await page.evaluate(async () => { 
        
                   await window.PDFViewerApplication.testingClose(); 
        
                   window.localStorage.clear(); 
        
                 }); 
        
                 await page.close({ runBeforeUnload: false }); 
        
               }) 
        
             ); 
        
           }

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 · 2024-06-23T14:16:39Z

I have fixed the issue for the existing test in f4053c2, 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 · 2024-06-23T14:37:07Z

Thank you! I'll rebase and fix it.

timvandermeij · 2024-06-24T13:31:28Z

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 · 2024-06-24T20:34:16Z

Done 👍

timvandermeij · 2024-06-24T20:41:45Z

/botio integrationtest

moz-tools-bot · 2024-06-24T20:41:47Z

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 · 2024-06-24T20:41:47Z

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 · 2024-06-24T20:53:20Z

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 · 2024-06-24T21:15:24Z

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 · 2024-06-25T08:00:48Z

The remaining failure on Linux is #17931.

src/display/text_layer.js

Snuffleupagus

r=me, with the final comments addressed and all tests passing; thank you!

src/display/text_layer.js

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 <span>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-size`s in the text layer by minFontSize, and then scaling all the `<span>`s down by 1/minFontSize.

Snuffleupagus · 2024-06-25T13:04:33Z

/botio test

moz-tools-bot · 2024-06-25T13:04:35Z

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 · 2024-06-25T13:04:35Z

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 · 2024-06-25T13:33:26Z

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 · 2024-06-25T13:49:49Z

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 · 2024-06-25T13:57:23Z

Thank you for fixing this!

nicolo-ribaudo · 2024-06-25T14:05:23Z

Thanks for the reviews!

nicolo-ribaudo force-pushed the ignore-browser-min-font-size branch from e2d9297 to 8c11334 Compare June 18, 2024 18:17

timvandermeij added the text-selection label Jun 18, 2024

Snuffleupagus reviewed Jun 18, 2024

View reviewed changes

src/display/text_layer.js Outdated Show resolved Hide resolved

nicolo-ribaudo commented Jun 19, 2024

View reviewed changes

src/display/text_layer.js Outdated Show resolved Hide resolved

nicolo-ribaudo force-pushed the ignore-browser-min-font-size branch from 3c603a2 to c6a3bbd Compare June 19, 2024 13:17

nicolo-ribaudo marked this pull request as ready for review June 19, 2024 13:19

marco-c changed the title ~~Overrride the minimum font size when rendering the text layer~~ Override the minimum font size when rendering the text layer Jun 19, 2024

nicolo-ribaudo force-pushed the ignore-browser-min-font-size branch from c6a3bbd to 23f3cb7 Compare June 21, 2024 09:29

timvandermeij approved these changes Jun 21, 2024

View reviewed changes

timvandermeij requested a review from Snuffleupagus June 21, 2024 13:54

nicolo-ribaudo force-pushed the ignore-browser-min-font-size branch from 23f3cb7 to 221518f Compare June 23, 2024 15:32

nicolo-ribaudo force-pushed the ignore-browser-min-font-size branch from 221518f to ffcefe2 Compare June 24, 2024 20:34

Snuffleupagus reviewed Jun 25, 2024

View reviewed changes

src/display/text_layer.js Outdated Show resolved Hide resolved

src/display/text_layer.js Outdated Show resolved Hide resolved

nicolo-ribaudo force-pushed the ignore-browser-min-font-size branch from 2f1b526 to b0bcd06 Compare June 25, 2024 09:34

Snuffleupagus approved these changes Jun 25, 2024

View reviewed changes

src/display/text_layer.js Outdated Show resolved Hide resolved

src/display/text_layer.js Outdated Show resolved Hide resolved

nicolo-ribaudo force-pushed the ignore-browser-min-font-size branch from b0bcd06 to 5b29e93 Compare June 25, 2024 12:58

timvandermeij approved these changes Jun 25, 2024

View reviewed changes

timvandermeij merged commit 11cb3a8 into mozilla:master Jun 25, 2024
9 checks passed

nicolo-ribaudo deleted the ignore-browser-min-font-size branch June 25, 2024 13:57

ZeroXClem mentioned this pull request Aug 12, 2024

[Snyk] Upgrade pdfjs-dist from 4.0.269 to 4.4.168 ZeroXClem/ChunkViz#4

Closed

t29mato mentioned this pull request Sep 3, 2024

Render Text issues when using tabs in Chrome lfoppiano/streamlit-pdf-viewer#60

Closed

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

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

Conversation

nicolo-ribaudo commented Jun 18, 2024 • edited Loading

nicolo-ribaudo commented Jun 19, 2024

timvandermeij commented Jun 19, 2024

moz-tools-bot commented Jun 19, 2024

From: Bot.io (Linux m4)

Received

moz-tools-bot commented Jun 19, 2024

From: Bot.io (Windows)

Received

moz-tools-bot commented Jun 19, 2024

From: Bot.io (Linux m4)

Failed

moz-tools-bot commented Jun 19, 2024

From: Bot.io (Windows)

Failed

nicolo-ribaudo commented Jun 19, 2024 • edited Loading

timvandermeij commented Jun 20, 2024

nicolo-ribaudo commented Jun 21, 2024

timvandermeij commented Jun 21, 2024

moz-tools-bot commented Jun 21, 2024

From: Bot.io (Linux m4)

Received

moz-tools-bot commented Jun 21, 2024

From: Bot.io (Windows)

Received

moz-tools-bot commented Jun 21, 2024

From: Bot.io (Linux m4)

Success

moz-tools-bot commented Jun 21, 2024

From: Bot.io (Windows)

Failed

nicolo-ribaudo commented Jun 21, 2024

timvandermeij commented Jun 21, 2024

timvandermeij left a comment • edited Loading

Choose a reason for hiding this comment

Snuffleupagus commented Jun 22, 2024

timvandermeij commented Jun 22, 2024 • edited Loading

timvandermeij commented Jun 23, 2024

nicolo-ribaudo commented Jun 23, 2024

timvandermeij commented Jun 24, 2024

nicolo-ribaudo commented Jun 24, 2024

timvandermeij commented Jun 24, 2024

moz-tools-bot commented Jun 24, 2024

From: Bot.io (Linux m4)

Received

moz-tools-bot commented Jun 24, 2024

From: Bot.io (Windows)

Received

moz-tools-bot commented Jun 24, 2024

From: Bot.io (Linux m4)

Failed

moz-tools-bot commented Jun 24, 2024

From: Bot.io (Windows)

Success

nicolo-ribaudo commented Jun 25, 2024

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus commented Jun 25, 2024

moz-tools-bot commented Jun 25, 2024

From: Bot.io (Linux m4)

Received

moz-tools-bot commented Jun 25, 2024

From: Bot.io (Windows)

Received

moz-tools-bot commented Jun 25, 2024

From: Bot.io (Linux m4)

Failed

moz-tools-bot commented Jun 25, 2024

From: Bot.io (Windows)

Failed

timvandermeij commented Jun 25, 2024

nicolo-ribaudo commented Jun 25, 2024

nicolo-ribaudo commented Jun 18, 2024 •

edited

Loading

nicolo-ribaudo commented Jun 19, 2024 •

edited

Loading

timvandermeij left a comment •

edited

Loading

timvandermeij commented Jun 22, 2024 •

edited

Loading