Closed Bug 1571341 Opened 1 year ago Closed 1 year ago

drawSnapshot() renders content outside of viewport as gray area

Categories

(Core :: Graphics, defect, P3)

70 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla70
Tracking Status
firefox70 --- fixed

People

(Reporter: whimboo, Assigned: mattwoodrow)

References

Details

Attachments

(3 files)

Attached image 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.

Flags: needinfo?(matt.woodrow)

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.

Flags: needinfo?(matt.woodrow)

(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.

Priority: -- → P3

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.

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: nobody → matt.woodrow
Status: NEW → ASSIGNED
Flags: needinfo?(matt.woodrow)

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.

Flags: needinfo?(matt.woodrow)

(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.

Attached image screenshot.png

So something doesn't really work for the following case when scrolling is involved:

  1. Open https://www.golem.de/
  2. Take a screenshot of window.DOMRect(100, 600, 1280, 2062).
  3. 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:

  1. Open https://www.hskupin.info/
  2. Get a snapshot of the current viewport
  3. The background with the windmill is not part of the screenshot. I see an empty area.

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.

Blocks: 1570147

I can't reproduce either of those failures using the browser scratchpad.

What were you using to trigger the snapshot?

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()
  );
});
Pushed by mwoodrow@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b8583d3aa37
Default to using DocumentRelative for the root document being drawn, since callers generally expect coordinates relative to the document. r=rhunt
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla70
You need to log in before you can comment on or make changes to this bug.