Skip to content

Commit

Permalink
Fix node screenshot. Part 1. Add captureBeyondViewport param to CDP…
Browse files Browse the repository at this point in the history
… command `captureScreenshot`.

1. Add `captureBeyondViewport` param to CDP command `captureScreenshot`.
2. Use `WebSettingsImpl::SetMainFrameClipsContent(false)` to force the whole page to be rendered.
3. Set `hide_scrollbars` to avoid artificial scrollbars.
4. Set `record_whole_document` to force the whole page to be rendered.
5. Removed DCHECK from `third_party/blink/renderer/core/frame/visual_viewport.cc`, as soon as it didn't expect to have preferences to be changed "on the flight".

Details: https://bugs.chromium.org/p/chromium/issues/detail?id=1003629#c37

Screenshots:

- Before: https://i.imgur.com/yt6WZRx.png
- Patchset chromium#1: https://i.imgur.com/VsocJ3L.png - artificial scrollbars.
- Patchset chromium#3:
  * https://imgur.com/UVqpzUQ - line breaks are not exactly the same as in original view.
  * https://imgur.com/MXpgHOl
  * https://imgur.com/haxemcr - not aligned with the node.
  * https://imgur.com/RbdlYVT
- Patchset chromium#18:
  * https://imgur.com/EWUmn0O
  * https://imgur.com/DUrQ1yF
  * https://imgur.com/cT5oBSi - perfectly aligned.
  * https://imgur.com/Jy4UWtf - no artificial scrollbars (because of the view extended).

Bug: 1003629
Change-Id: I6bbc85cd0995626a8b1fb748ec9048c9d586200e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2470876
Commit-Queue: Maksim Sadym <sadym@chromium.org>
Auto-Submit: Maksim Sadym <sadym@chromium.org>
Reviewed-by: Andrey Kosyakov <caseq@chromium.org>
Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org>
Cr-Commit-Position: refs/heads/master@{#827440}
  • Loading branch information
sadym-chromium authored and Commit Bot committed Nov 13, 2020
1 parent 2ca0c46 commit 114bc6a
Show file tree
Hide file tree
Showing 5 changed files with 154 additions and 18 deletions.
107 changes: 103 additions & 4 deletions content/browser/devtools/protocol/devtools_protocol_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@
#include "ui/gfx/skia_util.h"
#include "ui/snapshot/snapshot.h"

#if defined(OS_ANDROID)
#include "base/android/build_info.h"
#endif

#define EXPECT_SIZE_EQ(expected, actual) \
do { \
EXPECT_EQ((expected).width(), (actual).width()); \
Expand Down Expand Up @@ -398,11 +402,15 @@ class CaptureScreenshotTest : public DevToolsProtocolTest {
ScreenshotEncoding encoding,
bool from_surface,
const gfx::RectF& clip = gfx::RectF(),
float clip_scale = 0) {
float clip_scale = 0,
bool capture_beyond_viewport = false) {
std::unique_ptr<base::DictionaryValue> params(new base::DictionaryValue());
params->SetString("format", encoding == ENCODING_PNG ? "png" : "jpeg");
params->SetInteger("quality", 100);
params->SetBoolean("fromSurface", from_surface);
if (capture_beyond_viewport) {
params->SetBoolean("captureBeyondViewport", true);
}
if (clip_scale) {
std::unique_ptr<base::DictionaryValue> clip_value(
new base::DictionaryValue());
Expand Down Expand Up @@ -433,9 +441,10 @@ class CaptureScreenshotTest : public DevToolsProtocolTest {
bool from_surface,
float device_scale_factor = 0,
const gfx::RectF& clip = gfx::RectF(),
float clip_scale = 0) {
std::unique_ptr<SkBitmap> result_bitmap =
CaptureScreenshot(encoding, from_surface, clip, clip_scale);
float clip_scale = 0,
bool capture_beyond_viewport = false) {
std::unique_ptr<SkBitmap> result_bitmap = CaptureScreenshot(
encoding, from_surface, clip, clip_scale, capture_beyond_viewport);

gfx::Rect matching_mask(gfx::SkIRectToRect(expected_bitmap.bounds()));
#if defined(OS_MAC)
Expand Down Expand Up @@ -531,6 +540,96 @@ class CaptureScreenshotTest : public DevToolsProtocolTest {
#endif
};

IN_PROC_BROWSER_TEST_F(CaptureScreenshotTest,
CaptureScreenshotBeyondViewport_OutOfView) {
// This test fails consistently on low-end Android devices.
// See crbug.com/653637.
// TODO(eseckler): Reenable with error limit if necessary.
if (base::SysInfo::IsLowEndDevice())
return;

// Load dummy page before getting the window size.
shell()->LoadURL(GURL("data:text/html,<body></body>"));
gfx::Size window_size =
static_cast<RenderWidgetHostViewBase*>(
shell()->web_contents()->GetRenderWidgetHostView())
->GetCompositorViewportPixelSize();

// Make a page a bit bigger then the view to force scrollbar to be shown.
float content_height = window_size.height() + 10;
float content_width = window_size.width() + 10;

shell()->LoadURL(
GURL("data:text/html,<body "
"style='background:%23123456;height:" +
base::NumberToString(content_height) +
"px;width:" + base::NumberToString(content_width) + "px'></body>"));

EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
Attach();

// Generate expected screenshot without any scrollbars.
SkBitmap expected_bitmap;
expected_bitmap.allocN32Pixels(content_width, content_height);
expected_bitmap.eraseColor(SkColorSetRGB(0x12, 0x34, 0x56));

float device_scale_factor =
display::Screen::GetScreen()->GetPrimaryDisplay().device_scale_factor();

// Verify there are no scrollbars on the screenshot.
CaptureScreenshotAndCompareTo(
expected_bitmap, ENCODING_PNG, true, device_scale_factor,
gfx::RectF(0, 0, content_width, content_height), 1, true);
}

IN_PROC_BROWSER_TEST_F(
CaptureScreenshotTest,
CaptureScreenshotBeyondViewport_InnerScrollbarsAreShown) {
#if defined(OS_ANDROID)
// TODO(crbug.com/1147911): This test fails on Android Lollipop. Reenable
// after the bug is fixed.
if (base::android::BuildInfo::GetInstance()->sdk_int() <
base::android::SDK_VERSION_MARSHMALLOW) {
return;
}
#endif

// This test fails consistently on low-end Android devices.
// See crbug.com/653637.
// TODO(eseckler): Reenable with error limit if necessary.
if (base::SysInfo::IsLowEndDevice())
return;

shell()->LoadURL(GURL(
"data:text/html,<body><div style='width: 50px; height: 50px; overflow: "
"scroll;'><h3>test</h3><h3>test</h3><h3>test</h3></div></body>"));

EXPECT_TRUE(WaitForLoadStop(shell()->web_contents()));
Attach();

// We compare against the actual physical backing size rather than the
// view size, because the view size is stored adjusted for DPI and only in
// integer precision.
gfx::Size view_size = static_cast<RenderWidgetHostViewBase*>(
shell()->web_contents()->GetRenderWidgetHostView())
->GetCompositorViewportPixelSize();

// Capture a screenshot not "form surface", meaning without emulation and
// without changing preferences, as-is.
std::unique_ptr<SkBitmap> expected_bitmap =
CaptureScreenshot(ENCODING_PNG, false);

float device_scale_factor =
display::Screen::GetScreen()->GetPrimaryDisplay().device_scale_factor();

// Compare the captured screenshot with one made "from_surface", where actual
// scrollbar magic happened, and verify it looks the same, meaning the
// internal scrollbars are rendered.
CaptureScreenshotAndCompareTo(
*expected_bitmap, ENCODING_PNG, true, device_scale_factor,
gfx::RectF(0, 0, view_size.width(), view_size.height()), 1, true);
}

IN_PROC_BROWSER_TEST_F(CaptureScreenshotTest, CaptureScreenshot) {
// This test fails consistently on low-end Android devices.
// See crbug.com/653637.
Expand Down
39 changes: 37 additions & 2 deletions content/browser/devtools/protocol/page_handler.cc
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
#include "base/memory/ref_counted.h"
#include "base/memory/ref_counted_memory.h"
#include "base/numerics/safe_conversions.h"
#include "base/optional.h"
#include "base/process/process_handle.h"
#include "base/single_thread_task_runner.h"
#include "base/strings/string16.h"
Expand Down Expand Up @@ -681,6 +682,7 @@ void PageHandler::CaptureScreenshot(
Maybe<int> quality,
Maybe<Page::Viewport> clip,
Maybe<bool> from_surface,
Maybe<bool> capture_beyond_viewport,
std::unique_ptr<CaptureScreenshotCallback> callback) {
if (!host_ || !host_->GetRenderWidgetHost() ||
!host_->GetRenderWidgetHost()->GetView()) {
Expand Down Expand Up @@ -712,7 +714,8 @@ void PageHandler::CaptureScreenshot(
base::BindOnce(&PageHandler::ScreenshotCaptured,
weak_factory_.GetWeakPtr(), std::move(callback),
screenshot_format, screenshot_quality, gfx::Size(),
gfx::Size(), blink::DeviceEmulationParams()),
gfx::Size(), blink::DeviceEmulationParams(),
base::nullopt),
false);
return;
}
Expand Down Expand Up @@ -775,6 +778,30 @@ void PageHandler::CaptureScreenshot(
}
}

base::Optional<blink::web_pref::WebPreferences> original_web_prefs_if_needed;
if (capture_beyond_viewport.fromMaybe(false)) {
blink::web_pref::WebPreferences original_web_prefs =
host_->GetRenderViewHost()->GetDelegate()->GetOrCreateWebPreferences();
original_web_prefs_if_needed = original_web_prefs;

blink::web_pref::WebPreferences modified_web_prefs = original_web_prefs;
// Hiding scrollbar is needed to avoid scrollbar artefacts on the
// screenshot. Details: https://crbug.com/1003629.
modified_web_prefs.hide_scrollbars = true;
modified_web_prefs.record_whole_document = true;
host_->GetRenderViewHost()->GetDelegate()->SetWebPreferences(
modified_web_prefs);

{
// TODO(crbug.com/1141835): Remove the bug is fixed.
// Walkaround for the bug. Emulated `view_size` has to be set twice,
// otherwise the scrollbar will be on the screenshot present.
blink::DeviceEmulationParams tmp_params = modified_params;
tmp_params.view_size = gfx::Size(1, 1);
emulation_handler_->SetDeviceEmulationParams(tmp_params);
}
}

// We use DeviceEmulationParams to either emulate, set viewport or both.
emulation_handler_->SetDeviceEmulationParams(modified_params);

Expand Down Expand Up @@ -807,7 +834,8 @@ void PageHandler::CaptureScreenshot(
base::BindOnce(&PageHandler::ScreenshotCaptured,
weak_factory_.GetWeakPtr(), std::move(callback),
screenshot_format, screenshot_quality, original_view_size,
requested_image_size, original_params),
requested_image_size, original_params,
original_web_prefs_if_needed),
true);
}

Expand Down Expand Up @@ -1110,13 +1138,20 @@ void PageHandler::ScreenshotCaptured(
const gfx::Size& original_view_size,
const gfx::Size& requested_image_size,
const blink::DeviceEmulationParams& original_emulation_params,
const base::Optional<blink::web_pref::WebPreferences>&
maybe_original_web_prefs,
const gfx::Image& image) {
if (original_view_size.width()) {
RenderWidgetHostImpl* widget_host = host_->GetRenderWidgetHost();
widget_host->GetView()->SetSize(original_view_size);
emulation_handler_->SetDeviceEmulationParams(original_emulation_params);
}

if (maybe_original_web_prefs) {
host_->GetRenderViewHost()->GetDelegate()->SetWebPreferences(
maybe_original_web_prefs.value());
}

if (image.IsEmpty()) {
callback->sendFailure(
Response::ServerError("Unable to capture screenshot"));
Expand Down
17 changes: 10 additions & 7 deletions content/browser/devtools/protocol/page_handler.h
Original file line number Diff line number Diff line change
Expand Up @@ -126,6 +126,7 @@ class PageHandler : public DevToolsDomainHandler,
Maybe<int> quality,
Maybe<Page::Viewport> clip,
Maybe<bool> from_surface,
Maybe<bool> capture_beyond_viewport,
std::unique_ptr<CaptureScreenshotCallback> callback) override;
void CaptureSnapshot(
Maybe<std::string> format,
Expand Down Expand Up @@ -187,13 +188,15 @@ class PageHandler : public DevToolsDomainHandler,
std::unique_ptr<Page::ScreencastFrameMetadata> metadata,
const protocol::Binary& data);

void ScreenshotCaptured(std::unique_ptr<CaptureScreenshotCallback> callback,
const std::string& format,
int quality,
const gfx::Size& original_view_size,
const gfx::Size& requested_image_size,
const blink::DeviceEmulationParams& original_params,
const gfx::Image& image);
void ScreenshotCaptured(
std::unique_ptr<CaptureScreenshotCallback> callback,
const std::string& format,
int quality,
const gfx::Size& original_view_size,
const gfx::Size& requested_image_size,
const blink::DeviceEmulationParams& original_params,
const base::Optional<blink::web_pref::WebPreferences>& original_web_prefs,
const gfx::Image& image);

void GotManifest(std::unique_ptr<GetAppManifestCallback> callback,
const GURL& manifest_url,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6274,6 +6274,8 @@ domain Page
optional Viewport clip
# Capture the screenshot from the surface, rather than the view. Defaults to true.
experimental optional boolean fromSurface
# Capture the screenshot beyond the viewport. Defaults to false.
experimental optional boolean captureBeyondViewport
returns
# Base64-encoded image data.
binary data
Expand Down
7 changes: 2 additions & 5 deletions third_party/blink/renderer/core/frame/visual_viewport.cc
Original file line number Diff line number Diff line change
Expand Up @@ -599,15 +599,12 @@ void VisualViewport::InitializeScrollbars() {

needs_paint_property_update_ = true;

scrollbar_layer_horizontal_ = nullptr;
scrollbar_layer_vertical_ = nullptr;
if (VisualViewportSuppliesScrollbars() &&
!GetPage().GetSettings().GetHideScrollbars()) {
DCHECK(!scrollbar_layer_horizontal_);
DCHECK(!scrollbar_layer_vertical_);
UpdateScrollbarLayer(kHorizontalScrollbar);
UpdateScrollbarLayer(kVerticalScrollbar);
} else {
scrollbar_layer_horizontal_ = nullptr;
scrollbar_layer_vertical_ = nullptr;
}

// Ensure existing LocalFrameView scrollbars are removed if the visual
Expand Down

0 comments on commit 114bc6a

Please sign in to comment.