Commit f1f9eac7 authored by Daniel Hosseinian's avatar Daniel Hosseinian Committed by Chromium LUCI CQ

Reland "Reload the PDF accessibility tree on layout changes"

This is a reland of 2d0cae27.

The original change was reverted because a pre-existing bug in which
some IPC messages between the plugin and the renderer were dropped due
to garbage text rendering info attempted to be sent. That bug was fixed
in crrev.com/833065, so this CL should be safe to reland.

Original change's description:
> Reload the PDF accessibility tree on layout changes
>
> Currently, any layout changes applied to the PDF document (i.e. page
> rotation or two-up view) after the accessibility tree has loaded does
> not update the page bounds.
>
> Because the accessiblity tree has read-only nodes, the only way to
> resolve the issue is to reload all the nodes of the tree on each layout
> change.
>
> Note: This CL only fixes page bounds and positions. The bounds of page
> content such as text or annotations still do not rotate with the rest
> of the document. Those issues will be fixed in a follow-up.
>
> Bug: 1150665
> Change-Id: I2fbf2cb26f38ee3fed076333c0b224a8ede7947e
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2553217
> Commit-Queue: Daniel Hosseinian <dhoss@chromium.org>
> Reviewed-by: Ankit Kumar 🌪️ <ankk@microsoft.com>
> Reviewed-by: Raymes Khoury <raymes@chromium.org>
> Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org>
> Reviewed-by: Tom Sepez <tsepez@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#830918}

Bug: 1150665
Change-Id: Icf7a1fb3868560dee2f3d6c75dae83c128c4c0f4
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2570391
Auto-Submit: Daniel Hosseinian <dhoss@chromium.org>
Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org>
Reviewed-by: default avatarRaymes Khoury <raymes@chromium.org>
Reviewed-by: default avatarDominic Mazzoni <dmazzoni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833134}
parent aaec6b2d
......@@ -1302,6 +1302,7 @@ void PdfAccessibilityTree::SetAccessibilityDocInfo(
if (!render_accessibility)
return;
ClearAccessibilityNodes();
doc_info_ = doc_info;
doc_node_ =
CreateNode(ax::mojom::Role::kDocument, ax::mojom::Restriction::kReadOnly,
......@@ -1323,6 +1324,11 @@ void PdfAccessibilityTree::SetAccessibilityPageInfo(
const std::vector<ppapi::PdfAccessibilityTextRunInfo>& text_runs,
const std::vector<PP_PrivateAccessibilityCharInfo>& chars,
const ppapi::PdfAccessibilityPageObjects& page_objects) {
// Outdated calls are ignored.
uint32_t page_index = page_info.page_index;
if (page_index != next_page_index_)
return;
content::RenderAccessibility* render_accessibility = GetRenderAccessibility();
if (!render_accessibility)
return;
......@@ -1336,9 +1342,9 @@ void PdfAccessibilityTree::SetAccessibilityPageInfo(
if (invalid_plugin_message_received_)
return;
uint32_t page_index = page_info.page_index;
CHECK_GE(page_index, 0U);
CHECK_LT(page_index, doc_info_.page_count);
++next_page_index_;
ui::AXNodeData* page_node =
CreateNode(ax::mojom::Role::kRegion, ax::mojom::Restriction::kReadOnly,
......@@ -1460,6 +1466,13 @@ bool PdfAccessibilityTree::FindCharacterOffset(
return true;
}
void PdfAccessibilityTree::ClearAccessibilityNodes() {
next_page_index_ = 0;
nodes_.clear();
node_id_to_page_char_index_.clear();
node_id_to_annotation_info_.clear();
}
content::RenderAccessibility* PdfAccessibilityTree::GetRenderAccessibility() {
content::RenderFrame* render_frame =
host_->GetRenderFrameForInstance(instance_);
......
......@@ -121,6 +121,10 @@ class PdfAccessibilityTree : public content::PluginAXTreeSource {
const ppapi::PdfAccessibilityPageObjects& page_objects,
content::RenderAccessibility* render_accessibility);
// Clears the local cache of node data used to create the tree so that
// replacement node data can be introduced.
void ClearAccessibilityNodes();
content::RenderAccessibility* GetRenderAccessibility();
std::unique_ptr<gfx::Transform> MakeTransformFromViewInfo() const;
......@@ -163,6 +167,10 @@ class PdfAccessibilityTree : public content::PluginAXTreeSource {
// object to which an action can be passed.
std::map<int32_t, AnnotationInfo> node_id_to_annotation_info_;
bool invalid_plugin_message_received_ = false;
// Index of the next expected PDF accessibility page info, used to ignore
// outdated calls of SetAccessibilityPageInfo().
uint32_t next_page_index_ = 0;
};
} // namespace pdf
......
......@@ -233,6 +233,45 @@ TEST_F(PdfAccessibilityTreeTest, TestAccessibilityDisabledDuringPDFLoad) {
chars_, page_objects_);
}
TEST_F(PdfAccessibilityTreeTest, TestPdfAccessibilityTreeReload) {
content::RenderFrame* render_frame = view_->GetMainRenderFrame();
ASSERT_TRUE(render_frame);
render_frame->SetAccessibilityModeForTest(ui::AXMode::kWebContents);
ASSERT_TRUE(render_frame->GetRenderAccessibility());
FakeRendererPpapiHost host(view_->GetMainRenderFrame());
PP_Instance instance = 0;
pdf::PdfAccessibilityTree pdf_accessibility_tree(&host, instance);
// Make the accessibility tree with a portrait page and then remake with a
// landscape page.
gfx::RectF page_bounds = gfx::RectF(1, 2);
for (size_t i = 1; i <= 2; ++i) {
if (i == 2)
page_bounds.Transpose();
page_info_.bounds =
PP_MakeRectFromXYWH(page_bounds.x(), page_bounds.y(),
page_bounds.width(), page_bounds.height());
pdf_accessibility_tree.SetAccessibilityViewportInfo(viewport_info_);
pdf_accessibility_tree.SetAccessibilityDocInfo(doc_info_);
pdf_accessibility_tree.SetAccessibilityPageInfo(page_info_, text_runs_,
chars_, page_objects_);
ui::AXNode* root_node = pdf_accessibility_tree.GetRoot();
ASSERT_TRUE(root_node);
EXPECT_EQ(ax::mojom::Role::kDocument, root_node->data().role);
// There should only be one page node.
ASSERT_EQ(1u, root_node->children().size());
ui::AXNode* page_node = root_node->children()[0];
ASSERT_TRUE(page_node);
EXPECT_EQ(ax::mojom::Role::kRegion, page_node->data().role);
EXPECT_EQ(page_bounds, page_node->data().relative_bounds.bounds);
}
}
TEST_F(PdfAccessibilityTreeTest, TestPdfAccessibilityTreeCreation) {
static const char kTestAltText[] = "Alternate text for image";
......
......@@ -816,6 +816,10 @@ void OutOfProcessInstance::LoadAccessibility() {
doc_info.text_copyable =
PP_FromBool(engine()->HasPermission(PDFEngine::PERMISSION_COPY));
// A new document layout will trigger the creation of a new accessibility
// tree, so |next_accessibility_page_index_| should be reset to ignore
// outdated asynchronous calls of SendNextAccessibilityPage().
next_accessibility_page_index_ = 0;
pp::PDF::SetAccessibilityDocInfo(GetPluginInstance(), &doc_info);
// If the document contents isn't accessible, don't send anything more.
......@@ -836,6 +840,11 @@ void OutOfProcessInstance::LoadAccessibility() {
}
void OutOfProcessInstance::SendNextAccessibilityPage(int32_t page_index) {
// Outdated calls are ignored.
if (page_index != next_accessibility_page_index_)
return;
++next_accessibility_page_index_;
AccessibilityPageInfo page_info;
std::vector<pp::PDF::PrivateAccessibilityTextRunInfo> text_runs;
std::vector<AccessibilityCharInfo> chars;
......@@ -1236,6 +1245,11 @@ void OutOfProcessInstance::ProposeDocumentLayout(const DocumentLayout& layout) {
}
dimensions.Set(kJSPageDimensions, page_dimensions_array);
PostMessage(dimensions);
// Reload the accessibility tree on layout changes because the relative page
// bounds are no longer valid.
if (layout.dirty() && accessibility_state_ == ACCESSIBILITY_STATE_LOADED)
LoadAccessibility();
}
void OutOfProcessInstance::Invalidate(const gfx::Rect& rect) {
......
......@@ -523,6 +523,10 @@ class OutOfProcessInstance : public PdfViewPluginBase,
ACCESSIBILITY_STATE_LOADED
} accessibility_state_ = ACCESSIBILITY_STATE_OFF;
// The next accessibility page index, used to track interprocess calls when
// reconstructing the tree for new document layouts.
int32_t next_accessibility_page_index_ = 0;
base::WeakPtrFactory<OutOfProcessInstance> weak_factory_{this};
};
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment