1783086 - `WebDriver:Print` implementation missing `background` argument

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Kayla Firestack [:kaylafire]]

When using WebDriver:Print though the WebDriver crate and geckodriver, marionette's current implementation ignores the parameter named background, and instead looks for a parameter named printBackground (https://searchfox.org/mozilla-central/source/remote/shared/PDF.jsm#45), therefore rendering it impossible to specify background rendering behavior for PDFs' in webdriver.

an example of this behavior in geckodriver with irreverent JSON parameters removed.

webdriver::server DEBUG -> POST /session/<SESSION_ID>/print {"background":true, <removed>, "printBackground":true, <removed>}
Marionette DEBUG 0 -> [0,5,"WebDriver:Print",{"background":true, <removed> }]
RemoteAgent DEBUG PDF output written to /build/Temp-<UUID>/marionette.pdf

when this returns, even though background is specified to be true, there are no backgrounds rendered in the resulting PDF.

Comment 1

[Kayla Firestack [:kaylafire]]

I've fixed this in my local checkout of mozilla-central, I'm hoping to have a patch ready for review soon.

Comment 2

[Kayla Firestack [:kaylafire]]

Attachment: Bug 1783086 - [marionette] Rename `printBackground` to `background` for WebDriver compatibility. r=whimboo

Rename the printBackground parameter to background to conform to the
WebDriver:Print spec

• https://github.com/w3c/webdriver/pull/1468
• https://w3c.github.io/webdriver/#print-page

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thank you for finding this incompatibility with the WebDriver spec and even working on a fix! This is great to see. I'll be able to have a look at the patch soon.

Comment 4

[Kayla Firestack [:kaylafire]]

Attachment: Bug 1783086 - Add pdf background rendering setting test to assert webdriver spec and functionality. r=whimboo

This attempts to assert the functionality of the background parameter by testing and assuming WebDriver:Print
will attempt to be somewhat deterministic.

pdf_options.py attempts to test for the functionality of the background parameter indirectly though equivalence using the following steps.

• First, let identity be the result of WebDriver:Print({})

• Second, let background be the result of WebDriver:Print({ background: True })

• Third, let identity_check be the result of WebDriver:Print({})

• Fourth, assert that identity != background

• Fifth, assert that identity == identity_check

Depends on D153903

Comment 5

[Kayla Firestack [:kaylafire]]

Attachment: bug.1783086.test_print_settings_reproducable.1.zip

Comment 6

[Kayla Firestack [:kaylafire]]

Attachment: bug.1783086.test_print_settings_reproducable.2.zip

Comment 7

[Kayla Firestack [:kaylafire]]

Attachment: bug.1783086.test_print_background_enabled.zip

Comment 8

[Kayla Firestack [:kaylafire]]

Hi there, I've added the results of running ./mach test with some extra code to dump the PDF output to files when testing.

I've run the tests multiple times, there's times where the equality test has run successfully up to 9 times in a row, but it seems to be inconsistent. These pdf's are an example of the changes the browser is making per PDF print operation.

also, I've found biodiff to be very useful in finding the specific changes between the two versions.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hi Kayla, as you got a review from James a while ago I wonder if you are still interested to get his proposal with pdf.js implemented. Please let us know if not. Thanks!

Comment 10

[Kayla Firestack [:kaylafire]]

Hi Henrik!

Yes I still intend to implement the test suggestions from the review.
I had/have gotten caught up in other pressing work, my apologies for the radio silence.

I'm hoping to have the time to implement it this week.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+2]]

No worries. Thanks for letting us know!

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2]]

(In reply to Kayla Firestack [:kaylafire] from comment #10)

Yes I still intend to implement the test suggestions from the review.
I had/have gotten caught up in other pressing work, my apologies for the radio silence.

I'm hoping to have the time to implement it this week.

Hi Kayla, we would kinda appreciate a fix here. So if you foresee that you won't have the time in the near future to continue please let us know and we might have to take over the remaining work. Thanks!

Comment 13

[Kayla Firestack [:kaylafire]]

Hi Henrik, I was unable to continue working on this due to environment issues, If someone else has time to finish the test for this I welcome it.
I'm so sorry I didn't see this sooner, I've been out of free time starting and on-boarding new job I couldn't account for before.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks for letting us know and your patch for the print command! We will have to see how to get this finished and landed.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Setting to P2 given that we most likely want to work on that feature for BiDi in the next milestone.

Comment 16

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1783086 - [marionette] Rename `printBackground` to `background` for WebDriver compatibility.

Depends on D171083

Comment 17

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1783086 - [wdspec] Add fixtures to render PDF to png and compare two pngs for marionette.

Depends on D171693

Comment 18

[Alexandra Borovova [:Sasha]]

Attachment: Bug 1783086 - [wdspec] Add tests for background argument of "Webdriver:Print" command.

Depends on D171694

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a5f7eb5de08b
[marionette] Rename `printBackground` to `background` for WebDriver compatibility. r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/46736eb834cb
[wdspec] Add fixtures to render PDF to png and compare two pngs for marionette. r=webdriver-reviewers,whimboo
https://hg.mozilla.org/integration/autoland/rev/95635d61a8fa
[wdspec] Add tests for background argument of "Webdriver:Print" command. r=webdriver-reviewers,whimboo

Comment 20

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/38958 for changes under testing/web-platform/tests

Comment 21

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/a5f7eb5de08b
https://hg.mozilla.org/mozilla-central/rev/46736eb834cb
https://hg.mozilla.org/mozilla-central/rev/95635d61a8fa

Comment 22

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by jgraham