Annotation - Use border and background colors from MK dictionary #14049

calixteman · 2021-09-18T18:27:06Z

it aims to fix PDF rendering problem: Checkboxes have black background #13003;
set the bg and fg colors as they're in the pdf;
put a transparent overlay to help to see the fields.

src/core/annotation.js

web/annotation_layer_builder.css

calixteman · 2021-09-25T14:02:47Z

/botio test

pdfjsbot · 2021-09-25T14:02:48Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2021-09-25T14:02:48Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2021-09-25T14:44:05Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 20.25 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  errors: 604
  different ref/snapshot: 17
  different first/second rendering: 2

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

calixteman · 2021-09-25T14:58:43Z

/botio-linux test

pdfjsbot · 2021-09-25T14:58:44Z

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/344baaf712bab05/output.txt

Snuffleupagus · 2021-09-25T15:11:15Z

@timvandermeij Since you reviewed the previous PR, i.e. #13006, mind taking a look at this one when you've got time (so I'm not overlooking anything, given the previous discussion in that PR)?

pdfjsbot · 2021-09-25T15:20:47Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.241.84.105:8877/344baaf712bab05/output.txt

Total script time: 22.03 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  errors: 4
  different ref/snapshot: 28
  different first/second rendering: 2

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

pdfjsbot · 2021-09-25T15:22:01Z

From: Bot.io (Windows)

Failed

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

Total script time: 40.07 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: FAILED
Regression tests: FAILED

  errors: 4
  different ref/snapshot: 24
  different first/second rendering: 1

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

Snuffleupagus

Looking at the latest round of test results, there's also a couple of breaking errors during the reference tests; one example being:
TEST-UNEXPECTED-FAIL | test failed issue13823 | in firefox | page1 round 1 | render : Error: rasterizeAnnotationLayer: "can't access property 0, color is undefined".

calixteman · 2021-09-25T15:54:21Z

@Snuffleupagus I forgot to initialize the colors:
https://github.com/mozilla/pdf.js/pull/14049/files#diff-a7e409abd3d3209d32d3adc8457c5e23feb4c06113a2e065b0c6a9322a3a3369R622

calixteman · 2021-09-25T19:41:34Z

/botio test

pdfjsbot · 2021-09-25T19:41:35Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2021-09-25T19:41:36Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/3d92a6b327e5941/output.txt

pdfjsbot · 2021-09-25T20:22:16Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 22.03 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 29
  different first/second rendering: 2

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

pdfjsbot · 2021-09-25T21:00:11Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/3d92a6b327e5941/output.txt

Total script time: 39.82 mins

Font tests: Passed
Unit tests: Passed
Integration Tests: Passed
Regression tests: FAILED

  different ref/snapshot: 34

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

timvandermeij · 2021-09-26T10:57:45Z

@Snuffleupagus Thank you for the ping; I'll have a look at this PR today so we can hopefully get it merged soon.

timvandermeij · 2021-09-26T12:02:23Z

/botio-linux preview

pdfjsbot · 2021-09-26T12:02:24Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/60d0559f5dcb868/output.txt

pdfjsbot · 2021-09-26T12:06:56Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/60d0559f5dcb868/output.txt

Total script time: 4.51 mins

Published

Viewer: http://54.241.84.105:8877/60d0559f5dcb868/web/viewer.html
Viewer (legacy): http://54.241.84.105:8877/60d0559f5dcb868/legacy/web/viewer.html

timvandermeij

Looks good to me with these two comments addressed.

timvandermeij · 2021-09-26T12:31:40Z

src/core/annotation.js

+   */
+  setBorderAndBackgroundColors(mk) {
+    if (mk instanceof Dict) {
+      this.borderColor = getRgbColor(mk.getArray("BC"), null);


The borders seem thicker now in the reference tests; it looks like the borders are applied twice now, once here and once in existing CSS perhaps?

Moreover, when opening annotation-text-widget.pdf from the test suite and clicking the combs field, the border is now extended behind the field (due to the way padding works for the combs fields). Would it maybe be a solution to make the border, just like the background, transparent on focus?

The container and the element had a border and so I just removed the one from the element.
About the focus issue on the comb field, I just removed

pdf.js/web/annotation_layer_builder.css

Lines 144 to 152 in 93ed4bf

.annotationLayer .textWidgetAnnotation input.comb:focus {

/*

* Letter spacing is placed on the right side of each character. Hence, the

* letter spacing of the last character may be placed outside the visible

* area, causing horizontal scrolling. We avoid this by extending the width

* when the element has focus and revert this when it loses focus.

*/

width: 103%;

}

which seems to be useless now (something has likely been fixed in Firefox) and without any consequence with the pdf in #12865.

src/core/annotation.js

- it aims to fix mozilla#13003; - set the bg and fg colors as they're in the pdf; - put a transparent overlay to help to see the fields.

timvandermeij · 2021-09-29T18:51:27Z

/botio-linux preview

pdfjsbot · 2021-09-29T18:51:28Z

From: Bot.io (Linux m4)

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.241.84.105:8877/2b5da8bcc1c35a9/output.txt

pdfjsbot · 2021-09-29T18:55:49Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/2b5da8bcc1c35a9/output.txt

Total script time: 4.34 mins

Published

Viewer: http://54.241.84.105:8877/2b5da8bcc1c35a9/web/viewer.html
Viewer (legacy): http://54.241.84.105:8877/2b5da8bcc1c35a9/legacy/web/viewer.html

timvandermeij · 2021-09-29T19:13:34Z

Thank you for improving this!

/botio makeref

pdfjsbot · 2021-09-29T19:13:35Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2021-09-29T19:13:35Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2021-09-29T19:20:23Z

From: Bot.io (Windows)

Failed

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

Total script time: 1.06 mins

Lint: FAILED
Make references: FAILED

timvandermeij · 2021-09-29T19:25:10Z

Euh... what? Let's try that again.

/botio-windows makeref

pdfjsbot · 2021-09-29T19:25:11Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/01d9ffc1e72b281/output.txt

pdfjsbot · 2021-09-29T19:32:07Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 18.52 mins

Lint: Passed
Make references: FAILED

timvandermeij · 2021-09-29T19:37:19Z

Windows works, now Linux...

/botio-linux makeref

pdfjsbot · 2021-09-29T19:37:20Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2021-09-29T19:56:18Z

From: Bot.io (Windows)

Failed

Full output at http://54.193.163.58:8877/01d9ffc1e72b281/output.txt

Total script time: 31.09 mins

Lint: Passed
Make references: FAILED

timvandermeij · 2021-09-29T19:57:42Z

/botio-windows makeref

pdfjsbot · 2021-09-29T19:57:43Z

From: Bot.io (Linux m4)

Success

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

Total script time: 20.37 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2021-09-29T19:57:43Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/21c0a2c6b97b557/output.txt

Snuffleupagus · 2021-09-29T20:02:17Z

test/test_manifest.json

@@ -5975,5 +5975,12 @@
           "value": true
         }
       }
+    },
+    {  "id": "issue13003",
+       "file": "pdfs/issue13003.pdf",


I'm not sure why makeref doesn't fail on the bots, since this file isn't included in anywhere in the diff (neither directly or as a linked test-case). Locally this causes makeref to fail completely for me.

@calixteman Can you please do a follow-up PR adding the missing file?

Add the missing pdf file for the test in the PR #14049

pdfjsbot · 2021-09-29T20:36:03Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/21c0a2c6b97b557/output.txt

Total script time: 38.32 mins

Lint: Passed
Make references: Passed
Check references: Passed

Snuffleupagus · 2021-09-30T06:44:01Z

src/display/annotation_layer.js

          data.color[0] | 0,
          data.color[1] | 0,


Is this makeHexColor-call supposed to use data.color now, since naively I'd assume that it should actually use borderColor (defined just above) instead?
As-is the borderColor is being ignored, which I'm assuming wasn't intended; was this perhaps fallout from addressing one of the review comments?

borderColor is set from MK-BC entry and color is set from C one.
If I understand the spec correctly, BC (see https://www.adobe.com/content/dam/acom/en/devnet/pdf/pdfs/PDF32000_2008.pdf#page=417) is optional and for widget annotations, when C (see https://www.adobe.com/content/dam/acom/en/devnet/pdf/pdfs/PDF32000_2008.pdf#page=392) is for icon, popup and link, so only one of the BC and C should be set.

If I'm correct, I should likely add a comment.

…ered (PR 14049 follow-up) Note that PR 14049 removed this, since mozilla#14049 (comment) claimed that it's not necessary anymore. Unfortunately that, in my testing on Windows, actually re-introduced exactly the issue described in the comment; more specifically once the *last* character has been entered in the comb-field it's now again incorrectly scrolled (with the first character being invisible) until focus is lost. This can be tested with e.g. `f1040.pdf` from the test-suite.

…ered (PR 14049 follow-up) Note that PR 14049 removed this, since mozilla#14049 (comment) claimed that it's not necessary anymore. Unfortunately that, in my testing on Windows, actually re-introduced exactly the issue described in the comment; more specifically once the *last* character has been entered in the comb-field it's now again incorrectly scrolled (with the first character being invisible) until focus is lost. This can be tested with e.g. `f1040.pdf`, see page 2, from the test-suite.

calixteman requested review from timvandermeij and Snuffleupagus September 18, 2021 18:27

Snuffleupagus reviewed Sep 18, 2021

View reviewed changes

src/core/annotation.js Outdated Show resolved Hide resolved

web/annotation_layer_builder.css Outdated Show resolved Hide resolved

calixteman force-pushed the bg_from_mk branch from e67971e to bc24dcf Compare September 18, 2021 19:44

timvandermeij added the annotations label Sep 19, 2021

calixteman force-pushed the bg_from_mk branch from bc24dcf to a201fce Compare September 25, 2021 13:36

Snuffleupagus requested changes Sep 25, 2021

View reviewed changes

calixteman force-pushed the bg_from_mk branch from a201fce to d8b5115 Compare September 25, 2021 15:53

calixteman force-pushed the bg_from_mk branch from d8b5115 to 82ac5a0 Compare September 25, 2021 19:40

timvandermeij reviewed Sep 26, 2021

View reviewed changes

Annotation - Use border and background colors from MK dictionary

0776cd9

- it aims to fix mozilla#13003; - set the bg and fg colors as they're in the pdf; - put a transparent overlay to help to see the fields.

timvandermeij approved these changes Sep 29, 2021

View reviewed changes

timvandermeij merged commit 9a74f3e into mozilla:master Sep 29, 2021

Snuffleupagus reviewed Sep 29, 2021

View reviewed changes

calixteman added a commit to calixteman/pdf.js that referenced this pull request Sep 29, 2021

Add the missing pdf file for the test in the PR mozilla#14049

748ab49

calixteman added a commit that referenced this pull request Sep 29, 2021

Merge pull request #14091 from calixteman/14049_fu

db7c91e

Add the missing pdf file for the test in the PR #14049

Snuffleupagus reviewed Sep 30, 2021

View reviewed changes

Snuffleupagus mentioned this pull request Oct 5, 2021

[Regression] Fix comb fields scrolling when the last character is entered (PR 14049 follow-up) #14101

Merged

bh213 pushed a commit to bh213/pdf.js that referenced this pull request Jun 3, 2022

Add the missing pdf file for the test in the PR mozilla#14049

698a833

rousek pushed a commit to signosoft/pdf.js that referenced this pull request Aug 10, 2022

Add the missing pdf file for the test in the PR mozilla#14049

1cef6a1

	.annotationLayer .textWidgetAnnotation input.comb:focus {
	/*
	* Letter spacing is placed on the right side of each character. Hence, the
	* letter spacing of the last character may be placed outside the visible
	* area, causing horizontal scrolling. We avoid this by extending the width
	* when the element has focus and revert this when it loses focus.
	*/
	width: 103%;
	}

Annotation - Use border and background colors from MK dictionary #14049

Annotation - Use border and background colors from MK dictionary #14049

Conversation

calixteman commented Sep 18, 2021

calixteman commented Sep 25, 2021

pdfjsbot commented Sep 25, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Sep 25, 2021

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 25, 2021

From: Bot.io (Linux m4)

Failed

calixteman commented Sep 25, 2021

pdfjsbot commented Sep 25, 2021

From: Bot.io (Linux m4)

Received

Snuffleupagus commented Sep 25, 2021

pdfjsbot commented Sep 25, 2021

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Sep 25, 2021

From: Bot.io (Windows)

Failed

Snuffleupagus left a comment

Choose a reason for hiding this comment

calixteman commented Sep 25, 2021

calixteman commented Sep 25, 2021

pdfjsbot commented Sep 25, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Sep 25, 2021

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 25, 2021

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Sep 25, 2021

From: Bot.io (Windows)

Failed

timvandermeij commented Sep 26, 2021

timvandermeij commented Sep 26, 2021

pdfjsbot commented Sep 26, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Sep 26, 2021

From: Bot.io (Linux m4)

Success

Published

timvandermeij left a comment

Choose a reason for hiding this comment

timvandermeij Sep 26, 2021

Choose a reason for hiding this comment

calixteman Sep 26, 2021

Choose a reason for hiding this comment

timvandermeij commented Sep 29, 2021

pdfjsbot commented Sep 29, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Sep 29, 2021

From: Bot.io (Linux m4)

Success

Published

timvandermeij commented Sep 29, 2021

pdfjsbot commented Sep 29, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Sep 29, 2021

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 29, 2021

From: Bot.io (Windows)

Failed

timvandermeij commented Sep 29, 2021

pdfjsbot commented Sep 29, 2021

From: Bot.io (Windows)

Received

pdfjsbot commented Sep 29, 2021

From: Bot.io (Linux m4)

Snuffleupagus Sep 30, 2021 •

edited

Loading