Use default colors only in forced-colors mode (bug 1778068) #15135

calixteman · 2022-07-06T11:51:33Z

There is a patch in review for Firefox:
https://phabricator.services.mozilla.com/D151158

Snuffleupagus

Please see questions inline.

Snuffleupagus · 2022-07-06T12:59:49Z

web/app.js

+      : {
+          background: null,
+          foreground: null,
+        };


This is overkill, since all code should already be able to deal with pageColors being unset/undefined.

Suggested change

: {

background: null,

foreground: null,

};

: null;

Snuffleupagus · 2022-07-06T13:20:36Z

web/app.js

-      background: AppOptions.get("pageColorsBackground"),
-      foreground: AppOptions.get("pageColorsForeground"),
-    };
+    const pageColors = window.matchMedia("(forced-colors: active)").matches


Generally speaking, it seems a little bit unfortunate to essentially make it impossible for users to specify custom values for these PDF.js-specific color preferences.

Looking at the bug, could we perhaps only disable this functionality when the "problematic" (general) Firefox preferences have been set?
For context: Years ago we had code which checked the browser.display.document_color_use preference; this was removed (since it'd become unused) in a977882 and mozilla/gecko-dev@7015ab6

Would it make (any) sense to re-instate similar checks here, and do something like this instead?

Suggested change

const pageColors = window.matchMedia("(forced-colors: active)").matches

const pageColors =

window.matchMedia("(forced-colors: active)").matches || this.supportsDocumentColors

Snuffleupagus · 2022-07-06T17:41:34Z

web/app.js

-      background: AppOptions.get("pageColorsBackground"),
-      foreground: AppOptions.get("pageColorsForeground"),
-    };
+    const pageColors = PDFViewerApplication.useUserDefinedColors


Doesn't the following work here?

Suggested change

const pageColors = PDFViewerApplication.useUserDefinedColors

const pageColors = this.useUserDefinedColors

Snuffleupagus · 2022-07-06T17:55:15Z

web/firefoxcom.js

+  static get documentColorUse() {
+    const support = FirefoxCom.requestSync("documentColorUse");
+    return shadow(this, "documentColorUse", support);
+  }
+


Please remember to add a suitable method here as well, such that the GENERIC viewer works as intended:

pdf.js/web/app.js

Line 185 in bde4663

Also, what is an appropriate value to use in the DefaultExternalServices-class; perhaps 2 in that case?

Snuffleupagus · 2022-07-06T18:01:20Z

web/app.js

@@ -730,6 +732,21 @@ const PDFViewerApplication = {
    return this.externalServices.supportedMouseWheelZoomModifierKeys;
  },

+  get useUserDefinedColors() {


Maybe allowDocumentColorUse instead here, to have slightly more consistent method names between the app.js, firefoxcom.js, and PdfStreamConverter.jsm files?

calixteman · 2022-07-07T10:04:45Z

My idea with the color change was to change the colors only in HCM (color choice in canvas is binary: either on the CanvasText side or on the Canvas one, there is no shade).
Hence I still wonder if we should just change them when in HCM whatever the setting is in Firefox.

Snuffleupagus · 2022-07-07T11:44:17Z

Hence I still wonder if we should just change them when in HCM whatever the setting is in Firefox.

Right, the situation here is a bit messy since we "advertised" the pageColors feature in the release notes of version 2.14.305 and we might not want to outright break that!?

How about we try to limit the scope of things here to address the Firefox bug, but still support the HCM-case properly, while also allowing users to override things if desired/needed?
My, very simply, idea would be something like the following (which limits the changes needed here):

Add a new boolean option/preference, e.g. forcePageColors, set to false by default.

In app.js we replace this code with

const pageColors =
   AppOptions.get("forcePageColors") ||
   window.matchMedia("(forced-colors: active)").matches
     ? {
         background: AppOptions.get("pageColorsBackground"),
         foreground: AppOptions.get("pageColorsForeground"),
       }
     : null;

calixteman · 2022-07-07T12:22:53Z

Yes I thinks it's the right solution here.
When I read stuff about Canvas & CanvasText, it was always about a11y and I considered that those values were always set to non-default values in HCM.

calixteman · 2022-07-07T17:19:14Z

@Snuffleupagus, I wrote what you proposed.
Once it's merged I'll uplift the patch in beta and release.
It's so painful to not have discovered this issue in nightly & beta.

Snuffleupagus

r=me, with missing doc-comment added; thank you!

web/app_options.js

calixteman · 2022-07-07T17:37:47Z

/botio integrationtest

calixteman · 2022-07-07T17:42:08Z

/botio integrationtest

pdfjsbot · 2022-07-07T17:42:11Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.241.84.105:8877/33b8d1be01c00ab/output.txt

pdfjsbot · 2022-07-07T17:42:11Z

From: Bot.io (Windows)

Received

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

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

pdfjsbot · 2022-07-07T17:50:44Z

From: Bot.io (Linux m4)

Success

Full output at http://54.241.84.105:8877/33b8d1be01c00ab/output.txt

Total script time: 4.71 mins

Integration Tests: Passed

pdfjsbot · 2022-07-07T17:56:47Z

From: Bot.io (Windows)

Failed

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

Total script time: 7.33 mins

Integration Tests: FAILED

calixteman added the accessibility label Jul 6, 2022

calixteman requested a review from Snuffleupagus July 6, 2022 11:51

Snuffleupagus reviewed Jul 6, 2022

View reviewed changes

calixteman force-pushed the forced_colors branch from b7b94df to 8e6c4f3 Compare July 6, 2022 17:24

Snuffleupagus reviewed Jul 6, 2022

View reviewed changes

calixteman force-pushed the forced_colors branch from 8e6c4f3 to 957b728 Compare July 7, 2022 09:37

calixteman requested a review from Snuffleupagus July 7, 2022 09:38

calixteman force-pushed the forced_colors branch from 957b728 to 32ddcb8 Compare July 7, 2022 17:10

Snuffleupagus approved these changes Jul 7, 2022

View reviewed changes

web/app_options.js Show resolved Hide resolved

Use default colors only in forced-colors mode (bug 1778068)

fe93742

calixteman force-pushed the forced_colors branch from 32ddcb8 to fe93742 Compare July 7, 2022 17:36

calixteman merged commit b0a3c9e into mozilla:master Jul 7, 2022

calixteman deleted the forced_colors branch July 7, 2022 18:30

Snuffleupagus linked an issue Jul 9, 2022 that may be closed by this pull request

Coloured image displaying in black-white #15141

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use default colors only in forced-colors mode (bug 1778068) #15135

Use default colors only in forced-colors mode (bug 1778068) #15135

calixteman commented Jul 6, 2022 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus Jul 6, 2022

Snuffleupagus Jul 6, 2022

Snuffleupagus Jul 6, 2022

Snuffleupagus Jul 6, 2022

Snuffleupagus Jul 6, 2022 •

edited

Loading

calixteman commented Jul 7, 2022

Snuffleupagus commented Jul 7, 2022

calixteman commented Jul 7, 2022

calixteman commented Jul 7, 2022

Snuffleupagus left a comment

calixteman commented Jul 7, 2022

calixteman commented Jul 7, 2022

pdfjsbot commented Jul 7, 2022

pdfjsbot commented Jul 7, 2022

pdfjsbot commented Jul 7, 2022

pdfjsbot commented Jul 7, 2022

	const pageColors = window.matchMedia("(forced-colors: active)").matches
	const pageColors =
	window.matchMedia("(forced-colors: active)").matches \|\| this.supportsDocumentColors

	const pageColors = PDFViewerApplication.useUserDefinedColors
	const pageColors = this.useUserDefinedColors

Use default colors only in forced-colors mode (bug 1778068) #15135

Use default colors only in forced-colors mode (bug 1778068) #15135

Conversation

calixteman commented Jul 6, 2022 • edited Loading

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

Snuffleupagus Jul 6, 2022

Choose a reason for hiding this comment

Snuffleupagus Jul 6, 2022

Choose a reason for hiding this comment

Snuffleupagus Jul 6, 2022

Choose a reason for hiding this comment

Snuffleupagus Jul 6, 2022

Choose a reason for hiding this comment

Snuffleupagus Jul 6, 2022 • edited Loading

Choose a reason for hiding this comment

calixteman commented Jul 7, 2022

Snuffleupagus commented Jul 7, 2022

calixteman commented Jul 7, 2022

calixteman commented Jul 7, 2022

Snuffleupagus left a comment

Choose a reason for hiding this comment

calixteman commented Jul 7, 2022

calixteman commented Jul 7, 2022

pdfjsbot commented Jul 7, 2022

From: Bot.io (Linux m4)

Received

pdfjsbot commented Jul 7, 2022

From: Bot.io (Windows)

Received

pdfjsbot commented Jul 7, 2022

From: Bot.io (Linux m4)

Success

pdfjsbot commented Jul 7, 2022

From: Bot.io (Windows)

Failed

calixteman commented Jul 6, 2022 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus Jul 6, 2022 •

edited

Loading