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

XFA - Improve text layout #13653

Merged
merged 1 commit into from
Jul 5, 2021
Merged

XFA - Improve text layout #13653

merged 1 commit into from
Jul 5, 2021

Conversation

calixteman
Copy link
Contributor

  • support paragraph margins, line height, letter spacing, ...
  • compute missing dimensions from fields based almost on the dimensions of caption contents.

  - support paragraph margins, line height, letter spacing, ...
  - compute missing dimensions from fields based almost on the dimensions of caption contents.
@calixteman
Copy link
Contributor Author

It's expected to have a lot of test failures.
Overall, the rendering looks better (there is one exception in an Italian document where some different texts are not correctly vertically aligned: it's a matter of one pixel and I'll investigate that later).

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Jul 2, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/682a72b13106a36/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 2, 2021

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/dda743adf47650a/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 2, 2021

From: Bot.io (Linux m4)


Failed

Full output at http://54.67.70.0:8877/682a72b13106a36/output.txt

Total script time: 30.35 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 242
  different first/second rendering: 1

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

@pdfjsbot
Copy link

pdfjsbot commented Jul 2, 2021

From: Bot.io (Windows)


Failed

Full output at http://3.101.106.178:8877/dda743adf47650a/output.txt

Total script time: 35.55 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 242
  different first/second rendering: 2

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

@calixteman calixteman merged commit 02c0694 into mozilla:master Jul 5, 2021
@Snuffleupagus
Copy link
Collaborator

This might be a known issue, but looking at e.g. xfa_bug17176688_3 and comparing with Adobe Reader some of the changes with this patch are improvements and others are regressions.

@calixteman
Copy link
Contributor Author

/botio makeref

@pdfjsbot
Copy link

pdfjsbot commented Jul 5, 2021

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.67.70.0:8877/416b96e0cf115c2/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 5, 2021

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/33799e783cb71a6/output.txt

@marco-c
Copy link
Contributor

marco-c commented Jul 5, 2021

This might be a known issue, but looking at e.g. xfa_bug17176688_3 and comparing with Adobe Reader some of the changes with this patch are improvements and others are regressions.

Most of the changes there looked irrelevant to me, do you have an example of a regression?

@calixteman
Copy link
Contributor Author

I know we've still some issues with line height.

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Jul 5, 2021

Most of the changes there looked irrelevant to me, do you have an example of a regression?

One example: Compare the bottom of the first page and the top of the second page, in PDF.js and Adobe Reader.

(If necessary I can attach screen-shots later, but based on #13653 (comment) I guess this is indeed a known issue.)

@pdfjsbot
Copy link

pdfjsbot commented Jul 5, 2021

From: Bot.io (Windows)


Failed

Full output at http://3.101.106.178:8877/33799e783cb71a6/output.txt

Total script time: 24.72 mins

  • Lint: Passed
  • Make references: FAILED

@pdfjsbot
Copy link

pdfjsbot commented Jul 5, 2021

From: Bot.io (Linux m4)


Success

Full output at http://54.67.70.0:8877/416b96e0cf115c2/output.txt

Total script time: 27.12 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@calixteman
Copy link
Contributor Author

/botio-windows makeref

@pdfjsbot
Copy link

pdfjsbot commented Jul 5, 2021

From: Bot.io (Windows)


Received

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

Live output at: http://3.101.106.178:8877/e61f792d9d5ada4/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 5, 2021

From: Bot.io (Windows)


Success

Full output at http://3.101.106.178:8877/e61f792d9d5ada4/output.txt

Total script time: 31.43 mins

  • Lint: Passed
  • Make references: Passed
  • Check references: Passed

@calixteman
Copy link
Contributor Author

sadly we had an intermittent failure here...
The message TEST-UNEXPECTED-FAIL | test failed firefox has not responded in 120s likely means that firefox crashed, is it correct ?
In comparing logs from the two bots, I've the impress the first test not appearing is xfa_bug1716980 which could be the one which led to the issue.
Unfortunately I don't manage to reproduce locally.

@Snuffleupagus
Copy link
Collaborator

The message TEST-UNEXPECTED-FAIL | test failed firefox has not responded in 120s likely means that firefox crashed, is it correct ?

Possibly it could also mean that the browser hangs, but I also don't really know :-(

This type of error looks very similar, or even identical, to the intermittent ones that made me abandon PR #13649.

In comparing logs from the two bots, I've the impress the first test not appearing is xfa_bug1716980 which could be the one which led to the issue.

In the PR mentioned above, initially the failures occurred for another test-case; hence I'm not sure if the problem is with a specific file. However, it seems to me that it's the XFA test-cases which are (intermittently) affected by this. This is pure speculation on my part, but I cannot help wonder if maybe there's cases where some of the image resources somehow fail to load (without an error being throw), causing the resolveImages helper (in test/driver.js) to never resolve!?

Unfortunately I don't manage to reproduce locally.

Me neither, and I really tried in PR #13649.
I wonder if adding more logging around the resolveImages, for the enableXfa case, could maybe help?

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

Successfully merging this pull request may close these issues.

4 participants