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

[TextLayer] Use Arrays to build the total transform and padding, rather than concatenating Strings, in expandTextDivs #11089

Closed

Conversation

Snuffleupagus
Copy link
Collaborator

Please refer to the individual commit messages for additional details.

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/2095d49a67f294f/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/6391fa06fdfa392/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/2095d49a67f294f/output.txt

Total script time: 17.43 mins

  • Font tests: Passed
  • Unit tests: FAILED
  • Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/2095d49a67f294f/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/6391fa06fdfa392/output.txt

Total script time: 26.12 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/6391fa06fdfa392/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator Author

Interestingly the only text failure was in Linux and Chrome, with no other configuration affected. The likely culprit should be the first patch, and I suppose that we could change things to only skip negative padding but still set the transform property in that case!?

Given that browsers will reject padding values smaller than zero (which may be caused by limited numerical precision during calculations in the `expand` code), it makes no sense to include those when expanding the `textDiv`s.
…concatenating Strings, in `expandTextDivs`

Furthermore, it's possible to re-use the same Array for all `textDiv`s on the page.
…ncatenating Strings, in `expandTextDivs`

Furthermore, it's possible to re-use the same Array for all `textDiv`s on the page and the resulting padding string is also a lot more compact.
Please note that the `paddingLeft` branch was moved, since the padding values need to be ordered as `top, right, bottom, left`.

Finally, with this re-factoring it's no longer necessary to cache the original `style` string for every `textDiv` when `enhanceTextSelection` is enabled.
@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/42a61c16baf6d1b/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

Live output at: http://54.215.176.217:8877/6cf365724c95680/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/42a61c16baf6d1b/output.txt

Total script time: 17.46 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.67.70.0:8877/42a61c16baf6d1b/reftest-analyzer.html#web=eq.log

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

Full output at http://54.215.176.217:8877/6cf365724c95680/output.txt

Total script time: 26.03 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/6cf365724c95680/reftest-analyzer.html#web=eq.log

@Snuffleupagus
Copy link
Collaborator Author

I'll try to submit these patches separately, to see if it's possible to pin-point where the movement in Chrome on Linux is coming from.

@Snuffleupagus
Copy link
Collaborator Author

I'll try to submit these patches separately, to see if it's possible to pin-point where the movement in Chrome on Linux is coming from.

This was split into PRs #11090, #11091, and #11092; please see #11092 (comment) which outlines the culprit.

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

Successfully merging this pull request may close these issues.

2 participants