Commit 86b19038 authored by Pavol Marko's avatar Pavol Marko Committed by Commit Bot

Revert "Reload the PDF accessibility tree on layout changes"

This reverts commit 2d0cae27.

Reason for revert: The CL caused test failures on PDFExtensionTestWithParam.PdfAccessibilityTextRunCrash
Example failure:
https://ci.chromium.org/p/chrome/builders/ci/linux-chromeos-chrome/10202
The test is only enabled with gn arg is_chrome_branded = true which explains why it got missed in CQ.
(when reproing locally, I also ran into crbug.com/1152752 , so if you want to repro make sure to turn dchecks off :-D )

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}

TBR=raymes@chromium.org,dmazzoni@chromium.org,tsepez@chromium.org,dhoss@chromium.org,ankk@microsoft.com

Change-Id: I6c2946592cfcb496c6ed2cd3de6610e2d7c64902
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1150665
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2560263Reviewed-by: default avatarPavol Marko <pmarko@chromium.org>
Commit-Queue: Pavol Marko <pmarko@chromium.org>
Cr-Commit-Position: refs/heads/master@{#830971}
parent d43ca38c
......@@ -1302,7 +1302,6 @@ void PdfAccessibilityTree::SetAccessibilityDocInfo(
if (!render_accessibility)
return;
ClearAccessibilityNodes();
doc_info_ = doc_info;
doc_node_ =
CreateNode(ax::mojom::Role::kDocument, ax::mojom::Restriction::kReadOnly,
......@@ -1324,11 +1323,6 @@ 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;
......@@ -1342,9 +1336,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,
......@@ -1466,13 +1460,6 @@ 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,10 +121,6 @@ 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;
......@@ -167,10 +163,6 @@ 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,45 +233,6 @@ 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,10 +816,6 @@ 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.
......@@ -840,11 +836,6 @@ 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;
......@@ -1245,11 +1236,6 @@ 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,10 +523,6 @@ 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