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

Display background when printing or saving a text widget (issue #14928) #14929

Merged
merged 1 commit into from
May 20, 2022

Conversation

calixteman
Copy link
Contributor

No description provided.

Copy link
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shorter number format in the written data, especially for integer values, is a nice bonus of these changes!

src/core/annotation.js Outdated Show resolved Hide resolved
src/core/annotation.js Show resolved Hide resolved
@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/2797961af460b08/output.txt

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/2797961af460b08/output.txt

Total script time: 25.88 mins

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

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

Total script time: 31.11 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 379

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

@Snuffleupagus
Copy link
Collaborator

There's a bunch of movement that looks unrelated to these changes, but I think that we need to run tests (and makeref) separately first such that we don't miss anything in this PR.

However, looking at the Windows results the reftest-analyzer appears to be broken since there's no clickable links anymore which makes checking for failures impossible. @calixteman Can you please see if the Windows bot need to update or restarted, or what's up with the reftest-analyzer being broken?

@calixteman
Copy link
Contributor Author

There are a lot of errors:

TypeError: Cannot read property 'html_url' of undefined
    at C:\pdfjs\botio\lib\server.js:274:37
    at callbacks (C:\pdfjs\botio\node_modules\express\lib\router\index.js:272:11)
    at param (C:\pdfjs\botio\node_modules\express\lib\router\index.js:246:11)
    at pass (C:\pdfjs\botio\node_modules\express\lib\router\index.js:253:5)
    at Router._dispatch (C:\pdfjs\botio\node_modules\express\lib\router\index.js:280:5)
    at Object.Router.middleware [as handle] (C:\pdfjs\botio\node_modules\express\lib\router\index.js:45:10)
    at next (C:\pdfjs\botio\node_modules\connect\lib\http.js:204:15)
    at IncomingMessage.<anonymous> (C:\pdfjs\botio\node_modules\connect\lib\middleware\bodyParser.js:137:7)
    at IncomingMessage.emit (events.js:388:22)
    at endReadableNT (internal/streams/readable.js:1336:12)

and this property is read:
https://github.com/arturadib/botio/blob/master/lib/server.js#L272

@calixteman
Copy link
Contributor Author

/botio test

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

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

Total script time: 19.41 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: FAILED
  • Regression tests: FAILED
  errors: 708
  different ref/snapshot: 9

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Failed

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

Total script time: 27.67 mins

  • Font tests: Passed
  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 4

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

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented May 20, 2022

(It seems that Chrome timed-out on the Linux bot...)

Are all of these changes consistent with printing from e.g. Adobe Reader?
Looking at 160F-2019-page1, that one doesn't seem to include either border or background colors when printing from Adobe Reader (and that particular field isn't editable there).

@calixteman
Copy link
Contributor Author

in 160F-2019.pdf, the colors of background and border are set thanks to JS in a WillPrint action and reset in DidPrint one.
But it won't work correctly because of:

pdf.js/web/app.js

Lines 1773 to 1774 in 3c877f9

// Given that the "beforeprint" browser event is synchronous, we
// unfortunately cannot await the scripting event dispatching here.

Anyway I began to add the color values in annotationStorage when they're modified by JS. If you're ok I'll make these changes in an other PR.
If you've a good idea to be able to wait for WillPrint results, please propose.

@calixteman
Copy link
Contributor Author

/botio-linux browsertest

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

Live output at: http://54.241.84.105:8877/8e5951ef4272478/output.txt

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/8e5951ef4272478/output.txt

Total script time: 21.66 mins

  • Regression tests: FAILED
  different ref/snapshot: 13
  different first/second rendering: 1

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

@calixteman
Copy link
Contributor Author

in 160F-2019.pdf, the colors of background and border are set thanks to JS in a WillPrint action and reset in DidPrint one. But it won't work correctly because of:

pdf.js/web/app.js

Lines 1773 to 1774 in 3c877f9

// Given that the "beforeprint" browser event is synchronous, we
// unfortunately cannot await the scripting event dispatching here.

Anyway I began to add the color values in annotationStorage when they're modified by JS. If you're ok I'll make these changes in an other PR. If you've a good idea to be able to wait for WillPrint results, please propose.

I'll try to fix that in a follow-up.

@Snuffleupagus
Copy link
Collaborator

Anyway I began to add the color values in annotationStorage when they're modified by JS. If you're ok I'll make these changes in an other PR.

Yeah, this one is probably large enough already so that sounds good to me.

If you've a good idea to be able to wait for WillPrint results, please propose.

Unfortunately not, since it seems to be a web-platform limitation given that printing is synchronous.
It seems that you'd really want async printing support (which as far as I understand doesn't exist on the web), since trying to somehow delay regular printing seems like it could easily cause lots of issues.

@Snuffleupagus Snuffleupagus merged commit 5b67202 into mozilla:master May 20, 2022
@Snuffleupagus
Copy link
Collaborator

/botio makeref

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Received

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

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Received

Command cmd_makeref from @Snuffleupagus received. Current queue size: 2

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

@pdfjsbot
Copy link

From: Bot.io (Linux m4)


Success

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

Total script time: 22.33 mins

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

@pdfjsbot
Copy link

From: Bot.io (Windows)


Success

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

Total script time: 21.72 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[annotation] Text widget background color is not taken into account when printed
3 participants