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.

Sign up for GitHub

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

Jump to bottom

XFA - Fix text positions (bug 1718741) #13705

Merged
merged 1 commit into from
Jul 10, 2021
Merged

XFA - Fix text positions (bug 1718741) #13705

merged 1 commit into from
Jul 10, 2021

Conversation

calixteman
Copy link
Contributor

  • font line height is taken into account by acrobat when it isn't with masterpdfeditor: I extracted a font from a pdf, modified some ascent/descent properties thanks to ttx and the reinjected the font in the pdf: only Acrobat is taken it into account. So in this patch, line heights for some substituted fonts are added.
  • it seems that Acrobat is using a line height of 1.2 when the line height in the font is not enough (it's the only way I found to fix correctly bug 1718741).
  • don't use flex in wrapper container (which was causing an horizontal overflow in the above bug).
  • consequently, the above fixes introduced a lot of small regressions, so in order to see real improvements on reftests, I fixed the regressions in this patch:
    • replace margin by padding in some case where padding is a part of a container dimensions;
    • remove some flex display: some containers are wrongly sized when rendered;
    • set letter-spacing to 0.01px: it helps to be sure that text is not broken because of not enough width in Firefox.

@calixteman
Copy link
Contributor Author

Almost all the reftests are broken: I checked locally all of them and overall it's an improvement, I compared a lot of output with Acrobat and for most of them we've very close rendering.

/botio test

@calixteman
XFA - Fix text positions (bug 1718741)
58e1f51 
  - font line height is taken into account by acrobat when it isn't with masterpdfeditor: I extracted a font from a pdf, modified some ascent/descent properties thanks to ttx and the reinjected the font in the pdf: only Acrobat is taken it into account. So in this patch, line heights for some substituted fonts are added.
  - it seems that Acrobat is using a line height of 1.2 when the line height in the font is not enough (it's the only way I found to fix correctly bug 1718741).
   - don't use flex in wrapper container (which was causing an horizontal overflow in the above bug).
   - consequently, the above fixes introduced a lot of small regressions, so in order to see real improvements on reftests, I fixed the regressions in this patch:
     - replace margin by padding in some case where padding is a part of a container dimensions;
     - remove some flex display: some containers are wrongly sized when rendered;
     - set letter-spacing to 0.01px: it helps to be sure that text is not broken because of not enough width in Firefox.
@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

pdfjsbot commented Jul 9, 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/976360880962879/output.txt

@pdfjsbot
Copy link

pdfjsbot commented Jul 9, 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/d2408edec9e8e39/output.txt

@mozilla mozilla deleted a comment from pdfjsbot Jul 9, 2021
@mozilla mozilla deleted a comment from pdfjsbot Jul 9, 2021
@mozilla mozilla deleted a comment from pdfjsbot Jul 9, 2021
@mozilla mozilla deleted a comment from pdfjsbot Jul 9, 2021
@pdfjsbot
Copy link

pdfjsbot commented Jul 9, 2021

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/976360880962879/output.txt

Total script time: 31.70 mins

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

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

@pdfjsbot
Copy link

pdfjsbot commented Jul 9, 2021

From: Bot.io (Windows)

Failed

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

Total script time: 36.01 mins

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

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

@marco-c
Copy link
Contributor

marco-c commented Jul 10, 2021

A few observations:

  • In firefox-xfa_bug1718670_1-page1, towards the end of the page there is some text moving a bit behind a field.
  • In firefox-hsbc_closure-page1, towards the bottom of the page, the "IBAN" text disappeared.
  • In "firefox-xfa_bug1716816-page1", the bottom of the red rectangle is now behind "Numero de Referencia".
  • In "firefox-xfa_bug1717668_4-page1" the header of the table at the bottom is repeated on page 2 too.

@calixteman
Copy link
Contributor Author

about:

  • firefox-xfa_bug1718670_1-page1, it's firefox-linux specific
  • firefox-hsbc_closure-page1: I think you made a confusion: "IBAN" disappeared in the previous version
  • firefox-xfa_bug1717668_4-page1: it's by design: when there is an overflow there is a leader. I open this doc in acrobat and the delete is on the first page because there is no delete in table I.B. (thanks to JS).
  • firefox-xfa_bug1716816-page1: I noticed that too.

Since we've an overall improvement, I'd prefer fixing the 2 small regressions in a follow-up: it'd make my life easier.

@marco-c
Copy link
Contributor

marco-c commented Jul 10, 2021

Since we've an overall improvement, I'd prefer fixing the 2 small regressions in a follow-up: it'd make my life easier.

WFM, can you file bugs for the regressions?

@calixteman calixteman mentioned this pull request Jul 10, 2021
Open
@calixteman calixteman merged commit d416b23 into mozilla:master Jul 10, 2021
@calixteman
Copy link
Contributor Author

/botio makeref

@pdfjsbot
Copy link

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/095ff6109104529/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)

Received

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

Live output at: http://3.101.106.178:8877/2cd4550bf418a68/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)

Success

Full output at http://54.67.70.0:8877/095ff6109104529/output.txt

Total script time: 28.69 mins

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

@pdfjsbot
Copy link

From: Bot.io (Windows)

Success

Full output at http://3.101.106.178:8877/2cd4550bf418a68/output.txt

Total script time: 33.61 mins

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marco-c marco-c marco-c approved these changes

Assignees
No one assigned
Labels
form-xfa
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@calixteman @pdfjsbot @marco-c @timvandermeij