drawSnapshot() renders content outside of viewport as gray area
Categories
(Core :: Graphics, defect, P3)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox70 | --- | fixed |
People
(Reporter: whimboo, Assigned: mattwoodrow)
References
Details
Attachments
(3 files)
screenshot.png
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:70.0) Gecko/20100101 Firefox/70.0 ID:20190804094752
Using drawSnapshot() with a rect larger than the current viewport all the content from outside the viewport is rendered as a large gray area instead of the actual content. See the attached screenshot which is the result for the current dataURL:
data:text/html;charset=utf-8,<body style='margin-top: 200vh'>foo
None of the elements outside the viewport are visible.
This blocks the full screenshot capability from Marionette.
|
Assignee | |
Comment 1•5 years ago
|
||
Does this not also happen with drawWindow?
drawWindow by default interprets coordinates as relative to the top-left of the document, ignores the current scroll position, and doesn't clip the result to what's visible in the root scroll frame.
It also has the DRAWWINDOW_DRAW_VIEW flag which lets you specify that you want coordinates relative to the window (scroll position taken into account) and clipped to what's actually visible in the root scroll frame.
It seems like marionette now always passes that flag, so I'd expect us to only get content within the viewport drawn.
drawSnapshot doesn't have any flag options yet, and is defaulting to the DRAWWINDOW_DRAW_VIEW behaviour unconditionally. Adding a flag for that seems reasonable.
|
Reporter | |
Comment 2•5 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #1)
drawWindow by default interprets coordinates as relative to the top-left of the document, ignores the current scroll position, and doesn't clip the result to what's visible in the root scroll frame.
It also has the DRAWWINDOW_DRAW_VIEW flag which lets you specify that you want coordinates relative to the window (scroll position taken into account) and clipped to what's actually visible in the root scroll frame.
It seems like marionette now always passes that flag, so I'd expect us to only get content within the viewport drawn.
Oh, that is a regression then. In case of full screenshots we shouldn't restrict it to just the current viewport. I filed bug 1571659 for that specific case. Thanks for noticing that!
Leaving out the flag, I perfectly get the full document drawn.
drawSnapshot doesn't have any flag options yet, and is defaulting to the DRAWWINDOW_DRAW_VIEW behaviour unconditionally. Adding a flag for that seems reasonable.
Getting such a flag for being able to create a screenshot of the full page will be required by Marionette before we can switch over to the new API. Thanks.
|
||
Updated•5 years ago
|
|
|
Assignee | |
Comment 3•5 years ago
|
||
I had a go at fixing this: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e6fdacbf2678eddd20e5f32a235237f9f6abbab
That has a fix for bug 1569930 (works locally), and this bug (haven't tested).
The default behaviour is now to interpret rects as relative to the top-left of the document, and not clip to the scrollport. You can pass the new DRAWSNAPSHOT_DRAW_VIEW to instead have rects interpreted relative to the scrollport (what's actually visible), have the scroll position applied, and have it clipped to the scrollport.
The flag only affects the root document that you're drawing, embedded subdocs (in-process, or out) will always use the DRAW_VIEW behaviour.
|
|
Reporter | |
Comment 4•5 years ago
|
||
Matt I have seen that in WindowGlobalActors.webidl you added optional unsigned long flags while in PWindowGlobal.ipdl it is bool aDrawView. Does it mean we no longer need the other flags which were in use so far like for the caret and widget layer?
|
|
Assignee | |
Comment 5•5 years ago
|
||
Widget layers doesn't make sense for drawSnapshot, which is explicitly not for doing a readback of what's on the screen/widget.
Caret is up to you I guess, do you need it? It's there for drawWindow, since we use it for internal reftests to confirm that caret drawing actually works. Those tests also adjust the caret timing (using prefs), since otherwise it's blinking and it'll essentially be random as to whether you get one or not.
I used a bool for the internal code being optimistic that we wouldn't need more than that, but used an int for the public API so that when we eventually do need more we can do so without breaking compat.
|
|
Reporter | |
Comment 6•5 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #5)
Caret is up to you I guess, do you need it? It's there for drawWindow, since we use it for internal reftests to confirm that caret drawing actually works. Those tests also adjust the caret timing (using prefs), since otherwise it's blinking and it'll essentially be random as to whether you get one or not.
We have a test for flags in Marionette whereby I'm not sure anymore what exactly this is testing:
https://searchfox.org/mozilla-central/rev/9775cca0a10a9b5c5f4e15c8f7b3eff5bf91bbd0/testing/marionette/harness/marionette_harness/tests/unit/test_screenshot.py#166
But that actually might test for the focus ring as drawn around the input field when focused.
Given that the WebDriver specification at least for now doesn't say anything about the caret, we most likely won't need it. So lets leave it as is. Thanks.
I used a bool for the internal code being optimistic that we wouldn't need more than that, but used an int for the public API so that when we eventually do need more we can do so without breaking compat.
That is good! Thanks for the explanation.
I just triggered some Mac builds based on your try push, which I'm going to test now.
|
|
Reporter | |
Comment 7•5 years ago
|
||
So something doesn't really work for the following case when scrolling is involved:
- Open https://www.golem.de/
- Take a screenshot of
window.DOMRect(100, 600, 1280, 2062). - As result I would expect a rect of 1180x1462
Interestingly only a small portion of the widget ends-up in the snapshot.
Also here anohter one:
- Open https://www.hskupin.info/
- Get a snapshot of the current viewport
- The background with the windmill is not part of the screenshot. I see an empty area.
|
|
Reporter | |
Comment 8•5 years ago
|
||
Would you mind to add tests for this specific addition? I think there are a couple of cases which don't seem to work, and manually testing them isn't that great, and I'm unsure when someone could work on bug 1570147.
|
|
Assignee | |
Comment 9•5 years ago
|
||
I can't reproduce either of those failures using the browser scratchpad.
What were you using to trigger the snapshot?
|
|
Reporter | |
Comment 10•5 years ago
|
||
Strange, I actually also cannot reproduce it, and it works pretty fine at the moment. Not sure what was happening on Friday. :/ At that time I clearly used a fission enabled profile. The I used code looks like:
var MAX_SKIA_DIMENSIONS = 32767;
function createCanvas(win, width, height, scale) {
let canvasWidth = width * scale;
let canvasHeight = height * scale;
if (canvasWidth > MAX_SKIA_DIMENSIONS) {
logger.warn(
"Reducing screenshot width because it exceeds " +
MAX_SKIA_DIMENSIONS +
" pixels"
);
canvasWidth = MAX_SKIA_DIMENSIONS;
}
if (canvasHeight > MAX_SKIA_DIMENSIONS) {
logger.warn(
"Reducing screenshot height because it exceeds " +
MAX_SKIA_DIMENSIONS +
" pixels"
);
canvasHeight = MAX_SKIA_DIMENSIONS;
}
let canvas = win.document.createElement("canvas");
canvas.width = canvasWidth;
canvas.height = canvasHeight;
return canvas;
}
var context = window.gBrowser.selectedTab.linkedBrowser.browsingContext;
var windowGlobal = context.currentWindowGlobal;
//windowGlobal = window.docShell.browsingContext.currentWindowGlobal;
var rect = new window.DOMRect(100, 600, 1280, 2062);
var scale = window.devicePixelRatio;
var canvas = createCanvas(window, rect.width, rect.height, scale);
var ctx = canvas.getContext('2d');
windowGlobal.drawSnapshot(rect, scale, "white").then(snapshot => {
ctx.drawImage(snapshot, 0, 0);
window.loadURI(
canvas.toDataURL("image/png"),
null, null, null, null, null, null, null,
Services.scriptSecurityManager.getSystemPrincipal()
);
});
|
|
Assignee | |
Comment 11•5 years ago
|
||
Depends on D41727
|
||
Comment 12•5 years ago
|
||
|
||
Comment 13•5 years ago
|
||
| bugherder | ||
Description
•