[api-minor] Improve pdf reading in high contrast mode #14874

calixteman · 2022-05-04T13:43:19Z

Use Canvas & CanvasText color when they don't have their default value
as background and foreground colors.
The colors used to draw (stroke/fill) in a pdf are replaced by the bg/fg
ones according to their luminance.

calixteman · 2022-05-04T13:52:00Z

With tracemonkey.pdf there no noticeable difference in term of performance when rendering the 14 pages at zoom level set to 1%.

Snuffleupagus · 2022-05-04T14:03:51Z

Use Canvas & CanvasText color

What does the browser support situation look like for those values, since the only information I could find is https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#system_colors and that page doesn't (as far as I can tell) say anything about the compatibility situation?

calixteman · 2022-05-04T14:40:21Z

Good question.
I tested CSS.supports("color", "Canvas") in Chrome 103.0.5028.0, Firefox nightly and Safari 15.3 and it always returns true.

About Chrome I found this bug:
https://bugs.chromium.org/p/chromium/issues/detail?id=1015968

About Safari:
https://bugs.webkit.org/show_bug.cgi?id=203319
but I found a reference in the source code:
https://webkit-search.igalia.com/webkit/source/Source/WebCore/rendering/RenderTheme.cpp#1475
I'm not able to say if it's really where this property is bound... but at least it exists.

src/display/canvas.js

Snuffleupagus · 2022-05-04T14:47:35Z

For the GENERIC PDF.js library, we currently try to "support" the following browsers https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#faq-support

What happens in a browser without System Colors support, does it fallback gracefully or does rendering just break?
As long as this doesn't outright break anything, I don't believe that we'll need to add code specifically for older browsers (and this new feature will thus be unsupported in those browser).

Snuffleupagus · 2022-05-04T14:52:24Z

With tracemonkey.pdf there no noticeable difference in term of performance when rendering the 14 pages at zoom level set to 1%.

That document is probably simple enough to not really stress those canvas-methods all that much. What about a larger and more complex one, e.g. this map https://github.com/mozilla/pdf.js/files/1522715/wuppertal_2012.pdf?

calixteman · 2022-05-04T15:05:28Z

With tracemonkey.pdf there no noticeable difference in term of performance when rendering the 14 pages at zoom level set to 1%.

That document is probably simple enough to not really stress those canvas-methods all that much. What about a larger and more complex one, e.g. this map https://github.com/mozilla/pdf.js/files/1522715/wuppertal_2012.pdf?

In using perf tool in devtools in Firefox beta, there are nothing noticeable with this pdf without the patch, with the patch but the default black and white, with the patch with overwritten colors.

For the GENERIC PDF.js library, we currently try to "support" the following browsers https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#faq-support

What happens in a browser without System Colors support, does it fallback gracefully or does rendering just break? As long as this doesn't outright break anything, I don't believe that we'll need to add code specifically for older browsers (and this new feature will thus be unsupported in those browser).

To avoid any bad things with these values (i.e. Canvas & CanvasText) we could add a check in using CSS.supports when the viewer is created to avoid any redundant checks at page creation. Wdyt ?

Snuffleupagus · 2022-05-04T15:15:56Z

In using perf tool in devtools in Firefox beta, there are nothing noticeable with this pdf without the patch, with the patch but the default black and white, with the patch with overwritten colors.

Thanks for checking!

Also, just a FYI: Note that you can use the PDF.js debugging tools for basic performance measurements. Set pdfjs.pdfBugEnabled = true in about:config and then load https://github.com/mozilla/pdf.js/files/1522715/wuppertal_2012.pdf#pdfBug=Stats

To avoid any bad things with these values (i.e. Canvas & CanvasText) we could add a check in using CSS.supports when the viewer is created to avoid any redundant checks at page creation. Wdyt ?

Sure, that could work. In that case it's probably best to add that code in the BaseViewer-constructor, and inside of a pre-processor block like this:

if (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")) {
  ...
}

extensions/chromium/preferences_schema.json

src/shared/util.js

test/test_manifest.json

web/app.js

web/app_options.js

web/base_viewer.js

src/display/canvas.js

calixteman · 2022-05-05T09:41:44Z

/botio test

pdfjsbot · 2022-05-05T09:41:46Z

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

pdfjsbot · 2022-05-05T09:41:46Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2022-05-05T10:08:26Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 26.66 mins

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

  different ref/snapshot: 7
  different first/second rendering: 1

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

pdfjsbot · 2022-05-05T10:12:34Z

From: Bot.io (Windows)

Failed

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

Total script time: 30.80 mins

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

  different ref/snapshot: 2131

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

Snuffleupagus · 2022-05-05T11:45:46Z

/botio-windows browsertest

pdfjsbot · 2022-05-05T11:45:47Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/47a826a7d55a7c2/output.txt

pdfjsbot · 2022-05-05T12:10:03Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/47a826a7d55a7c2/output.txt

Total script time: 24.24 mins

Regression tests: Passed

Snuffleupagus

r=me, with the final comments addressed; thank you!

Snuffleupagus · 2022-05-05T11:52:13Z

src/display/canvas.js

+
+    if (this.foregroundColor && this.backgroundColor) {
+      // Get the #RRGGBB value of the color. If it's a name (e.g. CanvasText)
+      // then it'll be converted to it's rgb value.


Grammar-nit: it's -> its.

Snuffleupagus · 2022-05-05T11:56:22Z

test/test_manifest.json

+         "foreground": "#00FF00"
+      },
+      "type": "eq"
+   },


Can you please add one more test here, using the same color values as in the viewer, such that we actually test that configuration as well?
E.g. something like:

{ "id": "tracemonkey-viewer-colors-eq", "file": "pdfs/tracemonkey.pdf", "md5": "9a192d8b1a7dc652a19835f6f08098bd", "rounds": 1, "pageColors": { "background": "Canvas", "foreground": "CanvasText" }, "type": "eq", "about": "Uses the same pageColors as the viewer." },

Snuffleupagus · 2022-05-05T12:15:27Z

src/display/canvas.js

+        // Then for every color in the pdf, if its rounded luminance is the
+        // same as the background one then it's replaced by the new
+        // background color else by the foreground one.
+        const cB = parseInt(this.defaultBackgroundColor.slice(1), 16);


Wait, does this actually work correctly in general given what sort of values are listed as supported for the background-parameter?
Please refer to

pdf.js/src/display/api.js

Lines 1160 to 1164 in 7d6d6f9

* @property {Object | string} [background] - Background to use for the canvas.

* Any valid `canvas.fillStyle` can be used: a `DOMString` parsed as CSS

* <color> value, a `CanvasGradient` object (a linear or radial gradient) or

* a `CanvasPattern` object (a repetitive image). The default value is

* 'rgb(255,255,255)'.

I might be overlooking something here, but should we maybe even consider deprecating the old background-parameter in favour of the new pageColors one?

If I'm correct, for Firefox, the background parameter is always null so finally the default bg is always white.
Consequently, we can add a check on the the default bg color using a if (typeof PDFJSDev === "undefined" || !PDFJSDev.test("MOZCENTRAL")).
About this parameter in general, it has been added here:
#8413
and in considering the use case shown in the 1st comment, I guess having the parameter is still relevant but in this case it's impossible to detect what is the real background color so the stuff I added is useless.
I'm wondering if I should rename the options I added here because it's really a patch about forced colors... something like pageForcedColors, wdyt ?

Right, we probably need to keep the background-parameter.

I'm not sure that we actually need to rename the new parameter, though.
However, we may want to explicitly skip the background-parameter (in the canvas-code) if pageColors have been set and also document that in the API?

It isn't always skipped it depends.
For example if someone set it to a light gray then it'll be really a light gray for some people but for ones who enabled high contrast mode they will get the new colors.
And my patch is using the default bg luminance to "guess" when use forced bg or fg so the parameter never ignored (except when it isn't a #RRGGBB).

OK, then how about simply adding a note like this

* * NOTE: This option may be partially, or completely, ignored when * the `pageColors`-option is used.

to the existing documentation at

pdf.js/src/display/api.js

Lines 1160 to 1164 in 7d6d6f9

* @property {Object | string} [background] - Background to use for the canvas.

* Any valid `canvas.fillStyle` can be used: a `DOMString` parsed as CSS

* <color> value, a `CanvasGradient` object (a linear or radial gradient) or

* a `CanvasPattern` object (a repetitive image). The default value is

* 'rgb(255,255,255)'.

and calling that good enough?

Snuffleupagus

r=me, with all tests passing; thanks again!

extensions/chromium/preferences_schema.json

src/display/api.js

src/display/canvas.js

- Use Canvas & CanvasText color when they don't have their default value as background and foreground colors. - The colors used to draw (stroke/fill) in a pdf are replaced by the bg/fg ones according to their luminance.

calixteman · 2022-05-05T14:36:58Z

/botio test

pdfjsbot · 2022-05-05T14:36:59Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2022-05-05T14:36:59Z

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

pdfjsbot · 2022-05-05T15:03:19Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 26.32 mins

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

  different ref/snapshot: 7
  different first/second rendering: 1

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

pdfjsbot · 2022-05-05T15:08:29Z

From: Bot.io (Windows)

Failed

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

Total script time: 31.48 mins

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

calixteman · 2022-05-05T15:16:20Z

Failures are unrelated to this change.

calixteman · 2022-05-05T15:16:32Z

/botio makeref

pdfjsbot · 2022-05-05T15:16:34Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2022-05-05T15:16:34Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/8a8f493d80a2ca3/output.txt

pdfjsbot · 2022-05-05T15:39:37Z

From: Bot.io (Linux m4)

Success

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

Total script time: 23.05 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2022-05-05T15:41:50Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/8a8f493d80a2ca3/output.txt

Total script time: 25.26 mins

Lint: Passed
Make references: Passed
Check references: Passed

calixteman · 2022-05-05T19:48:28Z

/botio makeref

pdfjsbot · 2022-05-05T19:48:29Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2022-05-05T19:48:29Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.193.163.58:8877/5aad85bab447421/output.txt

pdfjsbot · 2022-05-05T20:11:32Z

From: Bot.io (Linux m4)

Success

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

Total script time: 23.04 mins

Lint: Passed
Make references: Passed
Check references: Passed

pdfjsbot · 2022-05-05T20:16:33Z

From: Bot.io (Windows)

Success

Full output at http://54.193.163.58:8877/5aad85bab447421/output.txt

Total script time: 24.80 mins

Lint: Passed
Make references: Passed
Check references: Passed

calixteman requested a review from Snuffleupagus May 4, 2022 13:43

calixteman force-pushed the colors branch from f233fa6 to d7fd510 Compare May 4, 2022 13:49

calixteman closed this May 4, 2022

calixteman reopened this May 4, 2022

marco-c added the accessibility label May 4, 2022

Snuffleupagus reviewed May 4, 2022

View reviewed changes

src/display/canvas.js Outdated Show resolved Hide resolved

Snuffleupagus added core viewer labels May 4, 2022

calixteman force-pushed the colors branch from d7fd510 to 412d942 Compare May 4, 2022 16:31

calixteman mentioned this pull request May 4, 2022

Unable to change the text and background colors. #2071

Closed

Snuffleupagus requested changes May 4, 2022

View reviewed changes

marco-c linked an issue May 5, 2022 that may be closed by this pull request

Unable to change the text and background colors. #2071

Closed

calixteman force-pushed the colors branch from 412d942 to 0301f06 Compare May 5, 2022 09:25

calixteman requested a review from Snuffleupagus May 5, 2022 09:48

Snuffleupagus approved these changes May 5, 2022

View reviewed changes

Snuffleupagus requested changes May 5, 2022

View reviewed changes

Snuffleupagus changed the title ~~Improve pdf reading in high contrast mode~~ [api-minor] Improve pdf reading in high contrast mode May 5, 2022

calixteman force-pushed the colors branch from 0301f06 to 0da5bc0 Compare May 5, 2022 13:54

Snuffleupagus approved these changes May 5, 2022

View reviewed changes

extensions/chromium/preferences_schema.json Outdated Show resolved Hide resolved

src/display/api.js Show resolved Hide resolved

src/display/canvas.js Outdated Show resolved Hide resolved

[api-minor] Improve pdf reading in high contrast mode

c8afd6c

- Use Canvas & CanvasText color when they don't have their default value as background and foreground colors. - The colors used to draw (stroke/fill) in a pdf are replaced by the bg/fg ones according to their luminance.

calixteman force-pushed the colors branch from 0da5bc0 to c8afd6c Compare May 5, 2022 14:36

calixteman merged commit cfac6fa into mozilla:master May 5, 2022

Snuffleupagus mentioned this pull request May 8, 2022

Time for a new release? #14892

Closed

juarezr mentioned this pull request Nov 30, 2023

Feature Request: Apply dark mode to PDF m-khvoinitsky/dark-background-light-text-extension#361

Open

	* @property {Object \| string} [background] - Background to use for the canvas.
	* Any valid `canvas.fillStyle` can be used: a `DOMString` parsed as CSS
	* <color> value, a `CanvasGradient` object (a linear or radial gradient) or
	* a `CanvasPattern` object (a repetitive image). The default value is
	* 'rgb(255,255,255)'.

[api-minor] Improve pdf reading in high contrast mode #14874

[api-minor] Improve pdf reading in high contrast mode #14874

Conversation

calixteman commented May 4, 2022

calixteman commented May 4, 2022

Snuffleupagus commented May 4, 2022

calixteman commented May 4, 2022

Snuffleupagus commented May 4, 2022 • edited Loading

Snuffleupagus commented May 4, 2022

calixteman commented May 4, 2022

Snuffleupagus commented May 4, 2022

calixteman commented May 5, 2022

pdfjsbot commented May 5, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented May 5, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented May 5, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented May 5, 2022

From: Bot.io (Windows)

Failed

Snuffleupagus commented May 5, 2022

pdfjsbot commented May 5, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented May 5, 2022

From: Bot.io (Windows)

Success

Snuffleupagus left a comment

Choose a reason for hiding this comment

Snuffleupagus May 5, 2022

Choose a reason for hiding this comment

Snuffleupagus May 5, 2022

Choose a reason for hiding this comment

Snuffleupagus May 5, 2022

Choose a reason for hiding this comment

Snuffleupagus May 5, 2022

Choose a reason for hiding this comment

calixteman May 5, 2022

Choose a reason for hiding this comment

Snuffleupagus May 5, 2022 • edited Loading

Choose a reason for hiding this comment

calixteman May 5, 2022

Choose a reason for hiding this comment

Snuffleupagus May 5, 2022 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus left a comment

Choose a reason for hiding this comment

calixteman commented May 5, 2022

pdfjsbot commented May 5, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented May 5, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented May 5, 2022

From: Bot.io (Linux m4)

Failed

pdfjsbot commented May 5, 2022

From: Bot.io (Windows)

Failed

calixteman commented May 5, 2022

calixteman commented May 5, 2022

pdfjsbot commented May 5, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented May 5, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented May 5, 2022

From: Bot.io (Linux m4)

Success

pdfjsbot commented May 5, 2022

From: Bot.io (Windows)

Success

calixteman commented May 5, 2022

pdfjsbot commented May 5, 2022

Snuffleupagus commented May 4, 2022 •

edited

Loading

Snuffleupagus May 5, 2022 •

edited

Loading

Snuffleupagus May 5, 2022 •

edited

Loading