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

calixteman · 2021-02-20T16:04:14Z

aims to fix PDF rendering problem: Checkboxes have black background #13003;
pass the colors to js sandbox;
fix checkbox color.

src/core/annotation.js

calixteman · 2021-02-20T16:26:39Z

/botio test

pdfjsbot · 2021-02-20T16:26:40Z

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/51ac9b765ebaba0/output.txt

calixteman · 2021-02-20T16:28:44Z

/botio-windows test

pdfjsbot · 2021-02-20T16:28:45Z

From: Bot.io (Windows)

Received

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

Live output at: http://3.101.106.178:8877/1aef28a4ddcac8d/output.txt

pdfjsbot · 2021-02-20T16:50:24Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/51ac9b765ebaba0/output.txt

Total script time: 23.72 mins

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

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

pdfjsbot · 2021-02-20T16:58:27Z

From: Bot.io (Windows)

Failed

Full output at http://3.101.106.178:8877/1aef28a4ddcac8d/output.txt

Total script time: 29.69 mins

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

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

timvandermeij · 2021-02-20T18:26:52Z

/botio-linux preview

pdfjsbot · 2021-02-20T18:26:53Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/a72984ad6b77272/output.txt

pdfjsbot · 2021-02-20T18:31:01Z

From: Bot.io (Linux m4)

Success

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

Total script time: 4.13 mins

Published

Viewer: http://54.67.70.0:8877/a72984ad6b77272/web/viewer.html
Viewer (legacy): http://54.67.70.0:8877/a72984ad6b77272/legacy/web/viewer.html

timvandermeij · 2021-02-21T13:51:33Z

This PR needs a rebase. I'm also not really sure what to make of some of the reference test failures, mainly firefox-issue12716-page1. Does it also render like that in Adobe Reader/Acrobat, i.e., that only some checkboxes are now white but not all of them?

calixteman · 2021-02-21T14:54:23Z

I played with acrobat DC and the background color is used when the widget has the focus.
When unfocused, an opaque (blue/violet) is used when the bg color is opaque and a transparent (still blue/violet) one when the bg color is transparent.

calixteman · 2021-02-21T17:41:43Z

/botio test

pdfjsbot · 2021-02-21T17:41:44Z

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

pdfjsbot · 2021-02-21T17:41:45Z

From: Bot.io (Windows)

Received

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

Live output at: http://3.101.106.178:8877/293f14d24d00886/output.txt

pdfjsbot · 2021-02-21T18:05:36Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 23.84 mins

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

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

pdfjsbot · 2021-02-21T18:11:32Z

From: Bot.io (Windows)

Failed

Full output at http://3.101.106.178:8877/293f14d24d00886/output.txt

Total script time: 29.79 mins

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

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

Snuffleupagus · 2021-02-21T18:16:44Z

Sorry to step in here, but that last version of this patch feels somewhat unfortunate in general, since:

It moves appearance from the CSS files and into the JS files, which is less maintainable.
It requires no less than two event listeners per element, just to manage the colours correctly.

I suppose that it might make sense to take a step back and consider how other PDF viewers (e.g. Adobe Reader and PDFium) handles fillable fields in general, since it seems that they don't necessarily always outline all fields the way that we currently do (and it rather seem to depend on the PDF file itself)!?

calixteman · 2021-02-21T18:40:39Z

In either acrobat dc or pdfium, the bg color of an unfocused rendered input has alpha or not depending if bg color of the annotation has alpha. So I don't see how avoid to have default colors in js instead of css, please tell me if it's possible. Your point is fair and I tried to figure out a solution to avoid that but I didn't manage to.
About listeners, I agree too but I don't see how to do differently: any good ideas are welcome.
Anyway I can remove listeners and remove the color in js but then as pointed by Tim the rendering is different from what we've in Acrobat: some annotations are blue and some others are not.

brendandahl · 2021-02-24T21:17:25Z

Not sure if this is really better, but we could leave defaults in css and use getComputedStyle. @Snuffleupagus any thoughts on a better way to do this?

Snuffleupagus · 2021-02-25T14:58:32Z

When looking at how e.g. Adobe Reader handles forms, it seems (at least to me) that things like e.g. the background of form fields are generally controlled by the PDF document itself rather than the viewer.

Hence my suggestion would be that we could perhaps simplify things overall by doing that in PDF.js as well, and stop setting e.g. a default background-color (in web/annotation_layer_builder.css or elsewhere) and instead defer that to the PDF document!?

calixteman · 2021-02-27T12:17:12Z

For me the "blue" background is just added by the reader to indicate that there is a fillable form element so I think it's useful for the user.
@timvandermeij what do you think ?

Snuffleupagus · 2021-02-27T13:00:35Z

How about the following compromise, then:

Leave the default styling in the CSS, as before.
Let PDF documents with field colours, set in /MK dictionaries, simply override the defaults when present.

That should hopefully reduce the overall complexity somewhat here, and also guarantee that PDF documents renders as intended (by the document author).

timvandermeij · 2021-02-28T13:54:11Z

I'll have a look at this once it's rebased, since then the patch will be simpler as all color function related changes have already been made in the square/circle annotation PR that was just merged, and with the comment above implemented.

calixteman · 2021-02-28T18:40:26Z

How about the following compromise, then:
* Leave the _default styling_ in the CSS, as before.

* Let PDF documents with field colours, set in /MK dictionaries, simply _override_ the defaults when present.
That should hopefully reduce the overall complexity somewhat here, and also guarantee that PDF documents renders as intended (by the document author).

Ok so if I understand correctly:

no bg color in MK: use "blue" one when unfocused and white when focused (current behaviour)
else: use the color as bg when unfocused and focused.

But if bg color is white or transparent and there is no border, there is no way for the user that there is something modifiable.
And we'll probably have the same issue as pointed by @timvandermeij: #13006 (comment).

My goal was to have exactly the same behaviour as in either Acrobat or chrome.

timvandermeij · 2021-02-28T18:38:14Z

src/core/annotation.js

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


Transparent is indicated by null instead of undefined as indicated on line 202.

I didn't find in the spec what's the default color (transparent or whatever), so undefined stands for "use HTML default".

Wait, does that mean that we now use both undefined and null, but they have very slightly different meaning in the display-layer?
If so, that sounds like a recipe for confusion...

timvandermeij · 2021-02-28T18:40:01Z

src/display/annotation_layer.js

@@ -976,6 +1016,15 @@ class CheckboxWidgetAnnotationElement extends WidgetAnnotationElement {
      );
    }

+    this._setBorderAndBackgroundColors(element);
+
+    const { fontColor } = this.data.defaultAppearanceData;


Why is this necessary only here and not elsewhere? Moreover, checkbox widgets don't contain text AFAIK, so what does this do?

Checkboxes contains text for the on state: most of the time one char using ZapfDingbats font.

Snuffleupagus · 2021-02-28T19:08:13Z

But if bg color is white or transparent and there is no border, there is no way for the user that there is something modifiable.

In that case the PDF document itself is responsible for that behaviour, so I don't really see this as a big problem.
Hence my suggestion above to not mess with "default" colours in the JS files, to avoid e.g. the focus/blur event listeners.

My goal was to have exactly the same behaviour as in either Acrobat or chrome.

Of course I understand that, but there's something to be said for simplicity of the overall implementation as well (in my opinion).

calixteman · 2021-03-06T15:29:24Z

But if bg color is white or transparent and there is no border, there is no way for the user that there is something modifiable.

In that case the PDF document itself is responsible for that behaviour, so I don't really see this as a big problem.

Yes you're right.
But when a form has been created and tested with Acrobat, some creators saw the blue background and so maybe for this reason they didn't set a background and/or border.
I've the impress (maybe I'm wrong) that removing this blue bg can diminish the quality of the user xp.
I really understand your concern and I don't want to bikeshed but I'm really not sure it's good for the user.

Hence my suggestion above to not mess with "default" colours in the JS files, to avoid e.g. the focus/blur event listeners.

My goal was to have exactly the same behaviour as in either Acrobat or chrome.

Of course I understand that, but there's something to be said for simplicity of the overall implementation as well (in my opinion).

Snuffleupagus · 2021-03-06T16:13:10Z

But when a form has been created and tested with Acrobat, some creators saw the blue background and so maybe for this reason they didn't set a background and/or border.

Please keep in mind that the "default" blue background, in Adobe Reader, mentioned is actually a setting which can be toggled on/off in the options (and that the particular colour used is also customizable); hence I'm not entirely convinced by this argument.

I'm mostly worried about readability/maintainability of the solution, especially since these types of changes feel like they have the potential to require additional fixes later on for various edge-cases in "weird" PDF documents.
(Since especially for Annotations it seems that many PDF generators aren't really following the specification all that well.)

- aims to fix mozilla#13003; - pass the colors to js sandbox; - fix checkbox color.

calixteman · 2021-03-06T17:08:59Z

/botio test

pdfjsbot · 2021-03-06T17:09:00Z

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

pdfjsbot · 2021-03-06T17:09:00Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2021-03-06T17:34:34Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 25.56 mins

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

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

pdfjsbot · 2021-03-06T17:39:40Z

From: Bot.io (Windows)

Failed

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

Total script time: 30.66 mins

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

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

calixteman · 2021-09-18T18:22:59Z

I'm closing this PR and I'll open a new one.

calixteman requested a review from timvandermeij February 20, 2021 16:04

Snuffleupagus reviewed Feb 20, 2021

View reviewed changes

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

calixteman force-pushed the 13003 branch from 611a26c to 81644ee Compare February 20, 2021 16:16

Snuffleupagus reviewed Feb 20, 2021

View reviewed changes

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

calixteman force-pushed the 13003 branch from 81644ee to 4622efc Compare February 20, 2021 16:26

timvandermeij added core annotations labels Feb 20, 2021

calixteman force-pushed the 13003 branch 2 times, most recently from f576b14 to c7abd6d Compare February 21, 2021 17:41

calixteman force-pushed the 13003 branch from c7abd6d to 94f5eb6 Compare February 21, 2021 18:24

calixteman force-pushed the 13003 branch from 94f5eb6 to e180c2b Compare February 28, 2021 18:02

timvandermeij reviewed Feb 28, 2021

View reviewed changes

Annotation - Use border and background colors from MK dictionary

16f2ab2

- aims to fix mozilla#13003; - pass the colors to js sandbox; - fix checkbox color.

calixteman force-pushed the 13003 branch from e180c2b to 16f2ab2 Compare March 6, 2021 17:06

calixteman mentioned this pull request Jul 28, 2021

XFA - Fix auto-sized fields (bug 1722030) #13806

Merged

calixteman closed this Sep 18, 2021

Snuffleupagus mentioned this pull request Sep 25, 2021

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

Merged

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

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

Conversation

calixteman commented Feb 20, 2021

calixteman commented Feb 20, 2021

pdfjsbot commented Feb 20, 2021

From: Bot.io (Linux m4)

Received

calixteman commented Feb 20, 2021

pdfjsbot commented Feb 20, 2021

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 20, 2021

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Feb 20, 2021

From: Bot.io (Windows)

Failed

timvandermeij commented Feb 20, 2021

pdfjsbot commented Feb 20, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Feb 20, 2021

From: Bot.io (Linux m4)

Success

Published

timvandermeij commented Feb 21, 2021 • edited Loading

calixteman commented Feb 21, 2021

calixteman commented Feb 21, 2021

pdfjsbot commented Feb 21, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Feb 21, 2021

From: Bot.io (Windows)

Received

pdfjsbot commented Feb 21, 2021

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Feb 21, 2021

From: Bot.io (Windows)

Failed

Snuffleupagus commented Feb 21, 2021

calixteman commented Feb 21, 2021

brendandahl commented Feb 24, 2021

Snuffleupagus commented Feb 25, 2021

calixteman commented Feb 27, 2021

Snuffleupagus commented Feb 27, 2021

timvandermeij commented Feb 28, 2021

calixteman commented Feb 28, 2021

timvandermeij Feb 28, 2021

Choose a reason for hiding this comment

calixteman Feb 28, 2021

Choose a reason for hiding this comment

Snuffleupagus Feb 28, 2021

Choose a reason for hiding this comment

timvandermeij Feb 28, 2021

Choose a reason for hiding this comment

calixteman Feb 28, 2021

Choose a reason for hiding this comment

Snuffleupagus commented Feb 28, 2021

calixteman commented Mar 6, 2021

Snuffleupagus commented Mar 6, 2021

calixteman commented Mar 6, 2021

pdfjsbot commented Mar 6, 2021

From: Bot.io (Linux m4)

Received

pdfjsbot commented Mar 6, 2021

From: Bot.io (Windows)

Received

pdfjsbot commented Mar 6, 2021

From: Bot.io (Linux m4)

Failed

pdfjsbot commented Mar 6, 2021

From: Bot.io (Windows)

Failed

calixteman commented Sep 18, 2021

timvandermeij commented Feb 21, 2021 •

edited

Loading