[unseasoned-pdf] Fixed issue that the link area doesn't match the text.
On a display with device scale larger than 1.0, a link's clickable area does not match where it is displayed in the Pepper-free PDF viewer due to the wrong scale level is applied to the input event. The Pepper implementation has the correct behavior, which applies the viewport-to-DIP scale to the input event in PepperPluginInstanceImpl::HandleInputEvent(), then applies the device scale to the input event in PdfViewPluginBase::HandleInputEvent(). However, the Pepper-free implementation applies device scale to the input event twice, in both PdfViewWebPlugin::HandleInputEvent() and PdfViewPluginBase::HandleInputEvent(). This CL corrects the scale level applied to the input event in PdfViewWebPlugin::HandleInputEvent() and adds unit tests for this issue. Bug: 1240395 Change-Id: Ic0dc20d9c989896325edbae07e5b7ace3b76d2db Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3101860 Reviewed-by: Robert Flack <flackr@chromium.org> Reviewed-by: K. Moon <kmoon@chromium.org> Commit-Queue: Hui Yingst <nigi@chromium.org> Cr-Commit-Position: refs/heads/main@{#915298}
This commit is contained in:

committed by
Chromium LUCI CQ

parent
6fc36f1f36
commit
f8165598a0
@ -413,8 +413,10 @@ if (enable_pdf) {
|
|||||||
"//base",
|
"//base",
|
||||||
"//ppapi/cpp:objects",
|
"//ppapi/cpp:objects",
|
||||||
"//testing/gtest",
|
"//testing/gtest",
|
||||||
|
"//third_party/blink/public/common:headers",
|
||||||
"//ui/gfx:geometry_skia",
|
"//ui/gfx:geometry_skia",
|
||||||
"//ui/gfx/range",
|
"//ui/gfx/range",
|
||||||
|
"//ui/latency:latency",
|
||||||
]
|
]
|
||||||
}
|
}
|
||||||
|
|
||||||
@ -500,6 +502,7 @@ if (enable_pdf) {
|
|||||||
"//third_party/blink/public/common:headers",
|
"//third_party/blink/public/common:headers",
|
||||||
"//third_party/pdfium",
|
"//third_party/pdfium",
|
||||||
"//ui/base",
|
"//ui/base",
|
||||||
|
"//ui/events/blink:blink",
|
||||||
"//ui/gfx:test_support",
|
"//ui/gfx:test_support",
|
||||||
"//ui/gfx/geometry",
|
"//ui/gfx/geometry",
|
||||||
"//ui/gfx/range",
|
"//ui/gfx/range",
|
||||||
|
1
pdf/DEPS
1
pdf/DEPS
@ -20,5 +20,6 @@ include_rules = [
|
|||||||
specific_include_rules = {
|
specific_include_rules = {
|
||||||
".*_unittest.*\.cc": [
|
".*_unittest.*\.cc": [
|
||||||
"+cc/test",
|
"+cc/test",
|
||||||
|
"+ui/latency/latency_info.h",
|
||||||
],
|
],
|
||||||
}
|
}
|
||||||
|
@ -429,11 +429,11 @@ blink::WebInputEventResult PdfViewWebPlugin::HandleInputEvent(
|
|||||||
const blink::WebCoalescedInputEvent& event,
|
const blink::WebCoalescedInputEvent& event,
|
||||||
ui::Cursor* cursor) {
|
ui::Cursor* cursor) {
|
||||||
// TODO(crbug.com/702993): The input events received by the Pepper plugin
|
// TODO(crbug.com/702993): The input events received by the Pepper plugin
|
||||||
// already have the device scale applied. The scaling done here should be
|
// already have the viewport-to-DIP scale applied. The scaling done here
|
||||||
// moved into `PdfViewPluginBase::HandleInputEvent()` once the Pepper plugin
|
// should be moved into `PdfViewPluginBase::HandleInputEvent()` once the
|
||||||
// is removed.
|
// Pepper plugin is removed.
|
||||||
std::unique_ptr<blink::WebInputEvent> scaled_event =
|
std::unique_ptr<blink::WebInputEvent> scaled_event =
|
||||||
ui::ScaleWebInputEvent(event.Event(), device_scale());
|
ui::ScaleWebInputEvent(event.Event(), viewport_to_dip_scale_);
|
||||||
|
|
||||||
const blink::WebInputEvent& event_to_handle =
|
const blink::WebInputEvent& event_to_handle =
|
||||||
scaled_event ? *scaled_event : event.Event();
|
scaled_event ? *scaled_event : event.Event();
|
||||||
|
@ -16,6 +16,9 @@
|
|||||||
#include "testing/gmock/include/gmock/gmock-matchers.h"
|
#include "testing/gmock/include/gmock/gmock-matchers.h"
|
||||||
#include "testing/gmock/include/gmock/gmock.h"
|
#include "testing/gmock/include/gmock/gmock.h"
|
||||||
#include "testing/gtest/include/gtest/gtest.h"
|
#include "testing/gtest/include/gtest/gtest.h"
|
||||||
|
#include "third_party/blink/public/common/input/web_coalesced_input_event.h"
|
||||||
|
#include "third_party/blink/public/common/input/web_input_event.h"
|
||||||
|
#include "third_party/blink/public/common/input/web_pointer_properties.h"
|
||||||
#include "third_party/blink/public/platform/web_string.h"
|
#include "third_party/blink/public/platform/web_string.h"
|
||||||
#include "third_party/blink/public/platform/web_text_input_type.h"
|
#include "third_party/blink/public/platform/web_text_input_type.h"
|
||||||
#include "third_party/blink/public/platform/web_vector.h"
|
#include "third_party/blink/public/platform/web_vector.h"
|
||||||
@ -23,10 +26,14 @@
|
|||||||
#include "third_party/blink/public/web/web_plugin_params.h"
|
#include "third_party/blink/public/web/web_plugin_params.h"
|
||||||
#include "third_party/skia/include/core/SkBitmap.h"
|
#include "third_party/skia/include/core/SkBitmap.h"
|
||||||
#include "third_party/skia/include/core/SkColor.h"
|
#include "third_party/skia/include/core/SkColor.h"
|
||||||
|
#include "ui/base/cursor/cursor.h"
|
||||||
|
#include "ui/events/blink/blink_event_util.h"
|
||||||
#include "ui/gfx/canvas.h"
|
#include "ui/gfx/canvas.h"
|
||||||
|
#include "ui/gfx/geometry/point_f.h"
|
||||||
#include "ui/gfx/geometry/rect.h"
|
#include "ui/gfx/geometry/rect.h"
|
||||||
#include "ui/gfx/geometry/size.h"
|
#include "ui/gfx/geometry/size.h"
|
||||||
#include "ui/gfx/skia_util.h"
|
#include "ui/gfx/skia_util.h"
|
||||||
|
#include "ui/latency/latency_info.h"
|
||||||
|
|
||||||
namespace chrome_pdf {
|
namespace chrome_pdf {
|
||||||
|
|
||||||
@ -43,6 +50,9 @@ using ::testing::Return;
|
|||||||
// testing.
|
// testing.
|
||||||
constexpr gfx::Size kCanvasSize(100, 100);
|
constexpr gfx::Size kCanvasSize(100, 100);
|
||||||
|
|
||||||
|
// A common device scale for high DPI displays.
|
||||||
|
constexpr float kDeviceScale = 2.0f;
|
||||||
|
|
||||||
// Note: Make sure `kDefaultColor` is different from `kPaintColor` and the
|
// Note: Make sure `kDefaultColor` is different from `kPaintColor` and the
|
||||||
// plugin's background color. This will help identify bitmap changes after
|
// plugin's background color. This will help identify bitmap changes after
|
||||||
// painting.
|
// painting.
|
||||||
@ -86,6 +96,18 @@ SkBitmap GenerateExpectedBitmapForPaint(float device_scale,
|
|||||||
return expected_bitmap;
|
return expected_bitmap;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
blink::WebMouseEvent CreateDefaultMouseDownEvent() {
|
||||||
|
blink::WebMouseEvent web_event(
|
||||||
|
blink::WebInputEvent::Type::kMouseDown,
|
||||||
|
/*position=*/gfx::PointF(),
|
||||||
|
/*global_position=*/gfx::PointF(),
|
||||||
|
blink::WebPointerProperties::Button::kLeft,
|
||||||
|
/*click_count_param=*/1, blink::WebInputEvent::Modifiers::kLeftButtonDown,
|
||||||
|
blink::WebInputEvent::GetStaticTimeStampForTests());
|
||||||
|
web_event.SetFrameScale(1);
|
||||||
|
return web_event;
|
||||||
|
}
|
||||||
|
|
||||||
class FakeContainerWrapper final : public PdfViewWebPlugin::ContainerWrapper {
|
class FakeContainerWrapper final : public PdfViewWebPlugin::ContainerWrapper {
|
||||||
public:
|
public:
|
||||||
explicit FakeContainerWrapper(PdfViewWebPlugin* web_plugin)
|
explicit FakeContainerWrapper(PdfViewWebPlugin* web_plugin)
|
||||||
@ -386,6 +408,42 @@ TEST_F(PdfViewWebPluginTest, PaintSnapshots) {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
TEST_F(PdfViewWebPluginTest, HandleInputEventWithUseZoomForDSFEnabled) {
|
||||||
|
// Test when using zoom for DSF is enabled.
|
||||||
|
EXPECT_CALL(*client_ptr_, IsUseZoomForDSFEnabled)
|
||||||
|
.WillRepeatedly(Return(true));
|
||||||
|
wrapper_ptr_->set_device_scale(kDeviceScale);
|
||||||
|
UpdatePluginGeometry(kDeviceScale, gfx::Rect(20, 20));
|
||||||
|
|
||||||
|
ui::Cursor dummy_cursor;
|
||||||
|
plugin_->HandleInputEvent(
|
||||||
|
blink::WebCoalescedInputEvent(CreateDefaultMouseDownEvent(),
|
||||||
|
ui::LatencyInfo()),
|
||||||
|
&dummy_cursor);
|
||||||
|
|
||||||
|
ASSERT_TRUE(engine_ptr_->GetScaledMouseEvent());
|
||||||
|
EXPECT_EQ(gfx::PointF(-10.0f, 0.0f),
|
||||||
|
engine_ptr_->GetScaledMouseEvent()->PositionInWidget());
|
||||||
|
}
|
||||||
|
|
||||||
|
TEST_F(PdfViewWebPluginTest, HandleInputEventWithUseZoomForDSFDisabled) {
|
||||||
|
// Test when using zoom for DSF is disabled.
|
||||||
|
EXPECT_CALL(*client_ptr_, IsUseZoomForDSFEnabled)
|
||||||
|
.WillRepeatedly(Return(false));
|
||||||
|
wrapper_ptr_->set_device_scale(kDeviceScale);
|
||||||
|
UpdatePluginGeometry(kDeviceScale, gfx::Rect(20, 20));
|
||||||
|
|
||||||
|
ui::Cursor dummy_cursor;
|
||||||
|
plugin_->HandleInputEvent(
|
||||||
|
blink::WebCoalescedInputEvent(CreateDefaultMouseDownEvent(),
|
||||||
|
ui::LatencyInfo()),
|
||||||
|
&dummy_cursor);
|
||||||
|
|
||||||
|
ASSERT_TRUE(engine_ptr_->GetScaledMouseEvent());
|
||||||
|
EXPECT_EQ(gfx::PointF(-20.0f, 0.0f),
|
||||||
|
engine_ptr_->GetScaledMouseEvent()->PositionInWidget());
|
||||||
|
}
|
||||||
|
|
||||||
TEST_F(PdfViewWebPluginTest, ChangeTextSelection) {
|
TEST_F(PdfViewWebPluginTest, ChangeTextSelection) {
|
||||||
ASSERT_FALSE(plugin_->HasSelection());
|
ASSERT_FALSE(plugin_->HasSelection());
|
||||||
ASSERT_TRUE(plugin_->SelectionAsText().IsEmpty());
|
ASSERT_TRUE(plugin_->SelectionAsText().IsEmpty());
|
||||||
|
@ -5,6 +5,8 @@
|
|||||||
#include "pdf/test/test_pdfium_engine.h"
|
#include "pdf/test/test_pdfium_engine.h"
|
||||||
|
|
||||||
#include <string.h>
|
#include <string.h>
|
||||||
|
|
||||||
|
#include <memory>
|
||||||
#include <vector>
|
#include <vector>
|
||||||
|
|
||||||
#include "base/check_op.h"
|
#include "base/check_op.h"
|
||||||
@ -16,6 +18,8 @@
|
|||||||
#include "pdf/pdf_engine.h"
|
#include "pdf/pdf_engine.h"
|
||||||
#include "pdf/pdfium/pdfium_engine.h"
|
#include "pdf/pdfium/pdfium_engine.h"
|
||||||
#include "pdf/pdfium/pdfium_form_filler.h"
|
#include "pdf/pdfium/pdfium_form_filler.h"
|
||||||
|
#include "third_party/blink/public/common/input/web_input_event.h"
|
||||||
|
#include "third_party/blink/public/common/input/web_mouse_event.h"
|
||||||
|
|
||||||
namespace chrome_pdf {
|
namespace chrome_pdf {
|
||||||
|
|
||||||
@ -30,6 +34,20 @@ TestPDFiumEngine::TestPDFiumEngine(PDFEngine::Client* client)
|
|||||||
|
|
||||||
TestPDFiumEngine::~TestPDFiumEngine() = default;
|
TestPDFiumEngine::~TestPDFiumEngine() = default;
|
||||||
|
|
||||||
|
bool TestPDFiumEngine::HandleInputEvent(
|
||||||
|
const blink::WebInputEvent& scaled_event) {
|
||||||
|
// Since blink::WebInputEvent is an abstract class, we cannot use equal
|
||||||
|
// matcher to verify its value. Here we test with blink::WebMouseEvent
|
||||||
|
// specifically.
|
||||||
|
if (!blink::WebInputEvent::IsMouseEventType(scaled_event.GetType()))
|
||||||
|
return false;
|
||||||
|
|
||||||
|
blink::WebMouseEvent* mouse_event = new blink::WebMouseEvent;
|
||||||
|
scaled_mouse_event_.reset(mouse_event);
|
||||||
|
*mouse_event = static_cast<const blink::WebMouseEvent&>(scaled_event);
|
||||||
|
return true;
|
||||||
|
}
|
||||||
|
|
||||||
bool TestPDFiumEngine::HasPermission(DocumentPermission permission) const {
|
bool TestPDFiumEngine::HasPermission(DocumentPermission permission) const {
|
||||||
return base::Contains(permissions_, permission);
|
return base::Contains(permissions_, permission);
|
||||||
}
|
}
|
||||||
@ -65,6 +83,10 @@ std::vector<uint8_t> TestPDFiumEngine::GetSaveData() {
|
|||||||
return std::vector<uint8_t>(std::begin(kSaveData), std::end(kSaveData));
|
return std::vector<uint8_t>(std::begin(kSaveData), std::end(kSaveData));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
const blink::WebMouseEvent* TestPDFiumEngine::GetScaledMouseEvent() const {
|
||||||
|
return scaled_mouse_event_ ? scaled_mouse_event_.get() : nullptr;
|
||||||
|
}
|
||||||
|
|
||||||
void TestPDFiumEngine::SetPermissions(
|
void TestPDFiumEngine::SetPermissions(
|
||||||
const std::vector<DocumentPermission>& permissions) {
|
const std::vector<DocumentPermission>& permissions) {
|
||||||
permissions_.clear();
|
permissions_.clear();
|
||||||
|
@ -7,6 +7,7 @@
|
|||||||
|
|
||||||
#include <stdint.h>
|
#include <stdint.h>
|
||||||
|
|
||||||
|
#include <memory>
|
||||||
#include <vector>
|
#include <vector>
|
||||||
|
|
||||||
#include "base/containers/flat_set.h"
|
#include "base/containers/flat_set.h"
|
||||||
@ -15,6 +16,11 @@
|
|||||||
#include "pdf/document_metadata.h"
|
#include "pdf/document_metadata.h"
|
||||||
#include "pdf/pdf_engine.h"
|
#include "pdf/pdf_engine.h"
|
||||||
#include "pdf/pdfium/pdfium_engine.h"
|
#include "pdf/pdfium/pdfium_engine.h"
|
||||||
|
#include "third_party/blink/public/common/input/web_mouse_event.h"
|
||||||
|
|
||||||
|
namespace blink {
|
||||||
|
class WebInputEvent;
|
||||||
|
} // namespace blink
|
||||||
|
|
||||||
namespace chrome_pdf {
|
namespace chrome_pdf {
|
||||||
|
|
||||||
@ -34,6 +40,9 @@ class TestPDFiumEngine : public PDFiumEngine {
|
|||||||
|
|
||||||
~TestPDFiumEngine() override;
|
~TestPDFiumEngine() override;
|
||||||
|
|
||||||
|
// Sets a scaled mouse event for testing.
|
||||||
|
bool HandleInputEvent(const blink::WebInputEvent& scaled_event) override;
|
||||||
|
|
||||||
bool HasPermission(DocumentPermission permission) const override;
|
bool HasPermission(DocumentPermission permission) const override;
|
||||||
|
|
||||||
const std::vector<DocumentAttachmentInfo>& GetDocumentAttachmentInfoList()
|
const std::vector<DocumentAttachmentInfo>& GetDocumentAttachmentInfoList()
|
||||||
@ -52,6 +61,8 @@ class TestPDFiumEngine : public PDFiumEngine {
|
|||||||
|
|
||||||
std::vector<uint8_t> GetSaveData() override;
|
std::vector<uint8_t> GetSaveData() override;
|
||||||
|
|
||||||
|
const blink::WebMouseEvent* GetScaledMouseEvent() const;
|
||||||
|
|
||||||
void SetPermissions(const std::vector<DocumentPermission>& permissions);
|
void SetPermissions(const std::vector<DocumentPermission>& permissions);
|
||||||
|
|
||||||
protected:
|
protected:
|
||||||
@ -66,6 +77,8 @@ class TestPDFiumEngine : public PDFiumEngine {
|
|||||||
|
|
||||||
DocumentMetadata metadata_;
|
DocumentMetadata metadata_;
|
||||||
|
|
||||||
|
std::unique_ptr<blink::WebMouseEvent> scaled_mouse_event_;
|
||||||
|
|
||||||
base::flat_set<DocumentPermission> permissions_;
|
base::flat_set<DocumentPermission> permissions_;
|
||||||
};
|
};
|
||||||
|
|
||||||
|
Reference in New Issue
Block a user