`WebDriver:Print` implementation missing `background` argument
Categories
(Remote Protocol :: Marionette, defect, P1)
Tracking
(firefox113 fixed)
Tracking | Status | |
---|---|---|
firefox113 | --- | fixed |
People
(Reporter: kaylafire, Assigned: Sasha)
References
(Blocks 2 open bugs)
Details
(Whiteboard: [webdriver:m6], [wptsync upstream] [webdriver:relnote])
Attachments
(8 files)
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
1.95 KB,
application/zip
|
Details | |
1.94 KB,
application/zip
|
Details | |
1.92 KB,
application/zip
|
Details | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review | |
48 bytes,
text/x-phabricator-request
|
Details | Review |
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.
Reporter | ||
Comment 1•2 years ago
|
||
I've fixed this in my local checkout of mozilla-central
, I'm hoping to have a patch ready for review soon.
Reporter | ||
Comment 2•2 years ago
|
||
Rename the printBackground
parameter to background
to conform to the
WebDriver:Print
spec
Updated•2 years ago
|
Updated•2 years ago
|
Comment 3•2 years ago
|
||
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.
Updated•2 years ago
|
Reporter | ||
Comment 4•2 years ago
|
||
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 ofWebDriver:Print({})
-
Second, let
background
be the result ofWebDriver:Print({ background: True })
-
Third, let
identity_check
be the result ofWebDriver:Print({})
-
Fourth, assert that
identity != background
-
Fifth, assert that
identity == identity_check
Depends on D153903
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Updated•2 years ago
|
Reporter | ||
Comment 5•2 years ago
|
||
Reporter | ||
Comment 6•2 years ago
|
||
Reporter | ||
Comment 7•2 years ago
|
||
Reporter | ||
Comment 8•2 years ago
|
||
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•2 years ago
|
||
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!
Reporter | ||
Comment 10•2 years ago
|
||
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•2 years ago
|
||
No worries. Thanks for letting us know!
Updated•2 years ago
|
Comment 12•2 years ago
|
||
(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!
Reporter | ||
Comment 13•2 years ago
|
||
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•2 years ago
|
||
Thanks for letting us know and your patch for the print command! We will have to see how to get this finished and landed.
Updated•2 years ago
|
Comment 15•1 year ago
|
||
Setting to P2 given that we most likely want to work on that feature for BiDi in the next milestone.
Updated•1 year ago
|
Assignee | ||
Updated•1 year ago
|
Updated•1 year ago
|
Assignee | ||
Comment 16•1 year ago
|
||
Depends on D171083
Assignee | ||
Comment 17•1 year ago
|
||
Depends on D171693
Assignee | ||
Comment 18•1 year ago
|
||
Depends on D171694
Updated•1 year ago
|
Comment 19•1 year ago
|
||
Pushed by aborovova@mozilla.com: 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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/38958 for changes under testing/web-platform/tests
Comment 21•1 year ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a5f7eb5de08b
https://hg.mozilla.org/mozilla-central/rev/46736eb834cb
https://hg.mozilla.org/mozilla-central/rev/95635d61a8fa
Upstream PR merged by jgraham
Updated•1 year ago
|
Description
•