[api-minor] Adds additional parameter so background color of canvas can be set #8413

cgreening · 2017-05-16T13:52:00Z

Adds an additional parameter that lets you set the background color of the canvas when rendering pdf.

e.g. for a transparent background:

page.render({ canvasContext, viewport, backgroundColor: 'rgba(0,0,0,0)' });

Snuffleupagus · 2017-05-16T14:34:27Z

Isn't this basically a duplicate of PR #7911?

e.g. for a transparent background:

Based on this comment, it seems that a side-effect of a transparent canvas would be worse text rendering!?

cgreening · 2017-05-16T16:21:53Z

Didn't realise someone had already tried to get this in.

I don't completely understand why it was rejected.

yurydelendik · 2017-05-16T17:25:38Z

@cgreening there are multiple reasons, but main one (from my point of view) is we are hesitant to accept something without tests. Since we are doing refactoring sometimes, we might break some functionality we don't use. By accepting a patch, we usually committed to support it in the future, without testing it is just a burden :)

yurydelendik · 2017-05-16T17:34:24Z

src/display/canvas.js

@@ -706,7 +706,8 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
  CanvasGraphics.prototype = {

    beginDrawing: function CanvasGraphics_beginDrawing(transform, viewport,


refactor beginDrawing to destructure parameters:

beginDrawing({transform, viewport, transparency, backgroundColor})

Not sure this can be done as the transparency argument doesn't seem to come from params:

this.gfx.beginDrawing(params.transform, params.viewport, transparency, params.backgroundColor);

I'm assuming that @yurydelendik meant that you should change the method signature like this (and also change api.js as indicated elsewhere):

beginDrawing({ transform, viewport, transparency, backgroundColor = null, }) {

yurydelendik

That said. Let's add a tests for that, e.g. create a new file "custom_spec.js" to the unit tests that will read canvas pixels.

yurydelendik · 2017-05-16T17:38:01Z

src/display/api.js

+      this.gfx.beginDrawing(params.transform,
+                            params.viewport,
+                            transparency,
+                            params.backgroundColor);


Add documentation about new parameter https://github.com/MRMBRAND/pdf.js/blob/f52376929d7584d41f38b2c741b6ed29b17fa181/src/display/api.js#L700

Done - let me know if it has enough details

With the changed signature of beginDrawing, this becomes:

this.gfx.beginDrawing({ transform: params.transform, viewport: params.viewport, transparency, backgroundColor: params.backgroundColor, });

cgreening · 2017-05-17T08:42:44Z

Will update the code with requested changes and add some tests.

Snuffleupagus · 2017-05-17T09:42:09Z

src/display/api.js

@@ -717,6 +717,8 @@ var PDFDocumentProxy = (function PDFDocumentProxyClosure() {
 * @property {Object} canvasFactory - (optional) The factory that will be used
 *                    when creating canvases. The default value is
 *                    {DOMCanvasFactory}.
+ * @property {string} backgroundColor - Background color to use for the canvas.
+ *                    The default value is 'rgb(255,255,255)'


Please mark this as (optional), compare e.g. with the canvasFactory just above. Also, the The default value is 'rgb(255,255,255)' sentence is missing a period at the end.

Finally, when all review comments have been addressed, please squash the commits into one; see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits.

cgreening · 2017-05-17T10:55:52Z

Tests and documentation added - let me know if you need anything else.

Snuffleupagus

Thanks for adding unit-tests!
However, none of them currently work, since they reference a PDF file that isn't included in the patch.

Also, please use let rather than var when you're adding new code.

Snuffleupagus · 2017-05-17T10:59:43Z

src/display/api.js

@@ -717,6 +717,8 @@ var PDFDocumentProxy = (function PDFDocumentProxyClosure() {
 * @property {Object} canvasFactory - (optional) The factory that will be used
 *                    when creating canvases. The default value is
 *                    {DOMCanvasFactory}.
+ * @property {string} backgroundColor - (optional) Background color to use for the canvas.
+ *                    The default value is 'rgb(255,255,255).'


Nit: Incorrect placement of the period, it should be The default value is 'rgb(255,255,255)'.

Also, please keep the line lengths in mind, i.e. this needs to be

* @property {string} backgroundColor - (optional) Background color to use for * the canvas. The default value is 'rgb(255,255,255)'.

Snuffleupagus · 2017-05-17T11:05:45Z

test/unit/api_spec.js

@@ -12,7 +12,7 @@
 * See the License for the specific language governing permissions and
 * limitations under the License.
 */
-
+import { buildGetDocumentParams, NodeFileReaderFactory, TEST_PDFS_PATH } from './test_utils';


This line is too long, it should be:

import { buildGetDocumentParams, NodeFileReaderFactory, TEST_PDFS_PATH } from './test_utils';

And please keep one blank line between the copyright header and the first import.

Snuffleupagus · 2017-05-17T11:07:03Z

test/unit/test_utils.js

@@ -57,4 +78,6 @@ class NodeCMapReaderFactory {
 export {
  NodeFileReaderFactory,
  NodeCMapReaderFactory,
+  buildGetDocumentParams,
+  TEST_PDFS_PATH


Nit: Add a colon at the end here, i.e. TEST_PDFS_PATH,.

Snuffleupagus · 2017-05-17T11:08:40Z

test/unit/test_utils.js

+  return params;
+}
+
+


Nit: Please remove a blank line here, since one is enough.

Snuffleupagus · 2017-05-17T11:10:04Z

test/unit/custom_spec.js

@@ -0,0 +1,99 @@
+import { buildGetDocumentParams } from './test_utils';


This file is missing the required license header, see e.g. https://github.com/mozilla/pdf.js/blob/master/src/license_header.js.

Snuffleupagus · 2017-05-17T11:21:12Z

test/unit/custom_spec.js

+
+  afterAll(function(done) {
+    CanvasFactory = null;
+    done();


Please clean up the loadingTask properly here, i.e. loadingTask.destroy().then(done);.

Snuffleupagus · 2017-05-17T11:24:42Z

test/unit/custom_spec.js

+      done();
+    }).catch(function (reason) {
+      done.fail(reason);
+    });


Nit: The above can probably be simplified to (applies in the test below as well):

page.render({ canvasContext: canvasAndCtx.context, viewport, }).then(function() { var { r, g, b, a } = getTopLeftPixel(canvasAndCtx.context); CanvasFactory.destroy(canvasAndCtx); expect(r).toEqual(255); expect(g).toEqual(255); expect(b).toEqual(255); expect(a).toEqual(255); done(); }).catch(function (reason) { done.fail(reason); });

Snuffleupagus · 2017-05-17T11:27:21Z

src/display/canvas.js

@@ -706,7 +706,8 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
  CanvasGraphics.prototype = {

    beginDrawing: function CanvasGraphics_beginDrawing(transform, viewport,


I'm assuming that @yurydelendik meant that you should change the method signature like this (and also change api.js as indicated elsewhere):

beginDrawing({ transform, viewport, transparency, backgroundColor = null, }) {

Snuffleupagus · 2017-05-17T11:29:42Z

src/display/api.js

+      this.gfx.beginDrawing(params.transform,
+                            params.viewport,
+                            transparency,
+                            params.backgroundColor);


With the changed signature of beginDrawing, this becomes:

this.gfx.beginDrawing({ transform: params.transform, viewport: params.viewport, transparency, backgroundColor: params.backgroundColor, });

Snuffleupagus · 2017-05-17T11:34:21Z

test/unit/custom_spec.js

+  });
+
+  afterAll(function(done) {
+    CanvasFactory = null;


Please add page = null; here too.

cgreening · 2017-05-17T12:38:43Z

Should be all the comments addressed - let me know if I've missed one.

Snuffleupagus · 2017-05-17T12:46:21Z

src/display/canvas.js

-                                                       transparency) {
+    beginDrawing: function CanvasGraphics_beginDrawing({
+      transform, viewport, transparency, backgroundColor = null,
+    }) {


This is overly verbose, taking full advantage of ES6 notation this can be simplified to:

beginDrawing({ transform, viewport, transparency, backgroundColor = null, }) {

Refer to https://developer.mozilla.org/en/docs/Web/JavaScript/Reference/Operators/Object_initializer#New_notations_in_ECMAScript_2015.

That seems to be the style of all the functions in the canvas.js file. I'm happy to clean that up as part of this pull request - but it might be better in a new one?

We slowly (since not all the code covered by tests) refactoring that to new style. In sake of this PR, we prefer to change only the affected code.

Snuffleupagus · 2017-05-17T12:47:40Z

test/unit/custom_spec.js

+    CanvasFactory = null;
+    page = null;
+    loadingTask.destroy().then(done);
+    done();


This line needs to be removed, since the point of loadingTask.destroy().then(done); just above is to wait for everything to be terminated before continuing with the tests.

yurydelendik · 2017-05-17T14:17:03Z

src/display/api.js

@@ -717,6 +717,8 @@ var PDFDocumentProxy = (function PDFDocumentProxyClosure() {
 * @property {Object} canvasFactory - (optional) The factory that will be used
 *                    when creating canvases. The default value is
 *                    {DOMCanvasFactory}.
+ * @property {string} backgroundColor - (optional) Background color to use for
+ *                    the canvas. The default value is 'rgb(255,255,255)'.


nit: can you make a note that the color is specified in the CSS format? Actually it can be string, CanvasGradient, or CanvasPattern -- someone creative might apply paper texture as a background, no?

Have added more docs and changes the name of the parameter to "background" as it's not necessarily a color.

yurydelendik · 2017-05-17T14:39:37Z

/botio test

pdfjsbot · 2017-05-17T14:39:38Z

From: Bot.io (Linux m4)

Received

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

Live output at: http://54.67.70.0:8877/9054e4699dc1ed3/output.txt

pdfjsbot · 2017-05-17T14:39:38Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/416d5de51b82e21/output.txt

pdfjsbot · 2017-05-17T15:03:07Z

From: Bot.io (Linux m4)

Failed

Full output at http://54.67.70.0:8877/9054e4699dc1ed3/output.txt

Total script time: 23.48 mins

Font tests: Passed
Unit tests: FAILED
Regression tests: FAILED

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

pdfjsbot · 2017-05-17T15:05:25Z

From: Bot.io (Windows)

Success

Full output at http://54.215.176.217:8877/416d5de51b82e21/output.txt

Total script time: 25.76 mins

Font tests: Passed
Unit tests: Passed
Regression tests: Passed

Snuffleupagus · 2017-05-17T15:31:56Z

src/display/canvas.js

-                                                       transparency) {
+    beginDrawing({
+      transform, viewport, transparency, background = null,
+    }) {


This looks kind of weird, compared to existing method signatures with ES6 parameter destructuring (e.g. the placement of the { bracket is strange); it should be:

beginDrawing({ transform, viewport, transparency, background = null, }) {

Snuffleupagus · 2017-05-17T21:49:37Z

/botio test

pdfjsbot · 2017-05-17T21:49:38Z

From: Bot.io (Linux m4)

Received

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

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

pdfjsbot · 2017-05-17T21:49:38Z

From: Bot.io (Windows)

Received

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

Live output at: http://54.215.176.217:8877/6a35e96e41df208/output.txt

pdfjsbot · 2017-05-17T22:13:07Z

From: Bot.io (Linux m4)

Failed

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

Total script time: 23.48 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

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

pdfjsbot · 2017-05-17T22:15:09Z

From: Bot.io (Windows)

Failed

Full output at http://54.215.176.217:8877/6a35e96e41df208/output.txt

Total script time: 25.50 mins

Font tests: Passed
Unit tests: Passed
Regression tests: FAILED

Image differences available at: http://54.215.176.217:8877/6a35e96e41df208/reftest-analyzer.html#web=eq.log

Snuffleupagus · 2017-05-17T22:18:38Z

Thank you for the patch!

timvandermeij · 2017-05-17T22:21:15Z

I'm a bit confused: are the Windows failures expected here?

Ninja edit: Already answered below :)

Snuffleupagus · 2017-05-17T22:21:25Z

Please note: The test "failures" in Firefox on the Windows bot in the latest test run isn't caused by this patch, since I can replicate all of those changes locally (on Windows) when testing against master.

…olor Adds additional parameter so background color of canvas can be set

yurydelendik reviewed May 16, 2017

View reviewed changes

yurydelendik requested changes May 16, 2017

View reviewed changes

yurydelendik reviewed May 16, 2017

View reviewed changes

timvandermeij added the core label May 16, 2017

Snuffleupagus reviewed May 17, 2017

View reviewed changes

cgreening force-pushed the set-canvas-background-color branch from 963f744 to 61f894c Compare May 17, 2017 10:54

Snuffleupagus reviewed May 17, 2017

View reviewed changes

cgreening force-pushed the set-canvas-background-color branch from b96181f to f112aaa Compare May 17, 2017 12:33

Snuffleupagus reviewed May 17, 2017

View reviewed changes

cgreening force-pushed the set-canvas-background-color branch from f112aaa to 70877e8 Compare May 17, 2017 13:55

yurydelendik reviewed May 17, 2017

View reviewed changes

yurydelendik approved these changes May 17, 2017

View reviewed changes

cgreening force-pushed the set-canvas-background-color branch from 4dc5bf8 to 941a4e5 Compare May 17, 2017 14:33

Snuffleupagus reviewed May 17, 2017

View reviewed changes

Adds additional parameter so background color of canvas can be set

cfc2f36

cgreening force-pushed the set-canvas-background-color branch from 941a4e5 to cfc2f36 Compare May 17, 2017 16:07

Snuffleupagus merged commit 5958daf into mozilla:master May 17, 2017

brendandahl changed the title ~~Adds additional parameter so background color of canvas can be set~~ [api-minor] Adds additional parameter so background color of canvas can be set May 18, 2017

Snuffleupagus mentioned this pull request Jul 16, 2017

Added handler to make text div building more flexible #8656

Closed

cgreening mentioned this pull request Aug 23, 2017

When exporting pdf to canvas, can pass a background color parameter #7911

Closed

movsb pushed a commit to movsb/pdf.js that referenced this pull request Jul 14, 2018

Merge pull request mozilla#8413 from MRMBRAND/set-canvas-background-c…

2daa07f

…olor Adds additional parameter so background color of canvas can be set

timvandermeij mentioned this pull request Sep 11, 2021

It will be cool, if the method 'getPageAsImage' could return image with transparency. I mean if pdf has transparent background. Now it filling with white color stephanrauh/ngx-extended-pdf-viewer#916

Closed

calixteman mentioned this pull request May 5, 2022

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

Merged

		@@ -706,7 +706,8 @@ var CanvasGraphics = (function CanvasGraphicsClosure() {
		CanvasGraphics.prototype = {

		beginDrawing: function CanvasGraphics_beginDrawing(transform, viewport,

		@@ -0,0 +1,99 @@
		import { buildGetDocumentParams } from './test_utils';

[api-minor] Adds additional parameter so background color of canvas can be set #8413

[api-minor] Adds additional parameter so background color of canvas can be set #8413

Conversation

cgreening commented May 16, 2017

Snuffleupagus commented May 16, 2017 • edited Loading

cgreening commented May 16, 2017

yurydelendik commented May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgreening commented May 17, 2017

Snuffleupagus May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgreening commented May 17, 2017

Snuffleupagus left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Snuffleupagus May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgreening commented May 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yurydelendik commented May 17, 2017

pdfjsbot commented May 17, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented May 17, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented May 17, 2017

From: Bot.io (Linux m4)

Failed

pdfjsbot commented May 17, 2017

From: Bot.io (Windows)

Success

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Snuffleupagus commented May 17, 2017

pdfjsbot commented May 17, 2017

From: Bot.io (Linux m4)

Received

pdfjsbot commented May 17, 2017

From: Bot.io (Windows)

Received

pdfjsbot commented May 17, 2017

From: Bot.io (Linux m4)

Failed

pdfjsbot commented May 17, 2017

From: Bot.io (Windows)

Failed

Snuffleupagus commented May 17, 2017

timvandermeij commented May 17, 2017 • edited Loading

Snuffleupagus commented May 17, 2017 • edited Loading

Snuffleupagus commented May 16, 2017 •

edited

Loading

yurydelendik commented May 16, 2017 •

edited

Loading

Snuffleupagus May 17, 2017 •

edited

Loading

Snuffleupagus left a comment •

edited

Loading

Snuffleupagus May 17, 2017 •

edited

Loading

Snuffleupagus May 17, 2017 •

edited

Loading

Snuffleupagus May 17, 2017 •

edited

Loading

timvandermeij commented May 17, 2017 •

edited

Loading

Snuffleupagus commented May 17, 2017 •

edited

Loading