Skip to content

Commit

Permalink
Reland "Reland "PDF a11y: Create the status node to notify users of P…
Browse files Browse the repository at this point in the history
…DF content loading""

This is a reland of commit 86927ca

In the first reland CL, some of the PDFExtensionAccessibility* tests
when a transient node (i.e. the status node) already got removed from
the accessibility tree before calling the new helper function, which
was waiting for the node to be first added to the tree. In the second
reland CL, the status node and its wrapper node don't get removed
anymore (so no transient node in a PDF accessibility tree), but their
attributes get reset in `PdfAccessibilityTree::UnserializeNodes()`.
Then, the tree format output related to these nodes (i.e. the second
and the third lines in the tree format output) is manually deleted in
PDFExtensionAccessibility* tests.

Original change's description:
> Reland "PDF a11y: Create the status node to notify users of PDF content loading"
>
> This is a reland of commit 72776ff
>
> In the original CL, some tests failed since the new helper function
> assumed that a node with the given name already existed in an a11y
> tree. Instead of this assumption, updated the helper function to wait
> for a node with the given name to be added to the tree and then wait
> for it to be removed from the tree. This helper function has been
> renamed to `WaitForTransientNodeWithName()` and added to
> chrome/browser/pdf/pdf_extension_accessibility_test.cc as its use case
> is applied to only some of the browser tests there.
>
> Original change's description:
> > PDF a11y: Create the status node to notify users of PDF content loading
> >
> > Make the status node available regardless of the PDF OCR feature to
> > notify users of PDF content loading. This status node will be added and
> > become available when it starts to load PDF content into an a11y tree.
> > Then, the status node will be deleted once it finishes loading the
> > PDF content into the tree.
> >
> > Note that this status node will be kept when PDF OCR is on and PDF
> > doesn't have accessible text but images without alternative text.
> >
> > This notification is necessary especially when users open a large PDF
> > (e.g. 200 or more pages) as it takes one or more minutes to load such
> > a large PDF. Without notification, users won't have any accessible
> > information that helps them know what's going on after opening a PDF
> > until it finishes loading the PDF content into an a11y tree.
> >
> > AX-Relnotes: n/a.
> > Bug: 1473176
> > Change-Id: Ib51af6014ca3a195ec43eff6137eeb3bff9f41c7
> > Cq-Include-Trybots: luci.chromium.try:fuchsia-official
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4792347
> > Reviewed-by: David Tseng <dtseng@chromium.org>
> > Commit-Queue: Kyungjun Lee <kyungjunlee@google.com>
> > Reviewed-by: Lei Zhang <thestig@chromium.org>
> > Reviewed-by: Scott Violet <sky@chromium.org>
> > Cr-Commit-Position: refs/heads/main@{#1198735}
>
> Bug: 1473176, 1484882
> Change-Id: Ia9f505da01f448a80cf1ed999b1c9c7bdcb63bc9
> Cq-Include-Trybots: luci.chromium.try:fuchsia-official
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4884773
> Reviewed-by: David Tseng <dtseng@chromium.org>
> Reviewed-by: Scott Violet <sky@chromium.org>
> Reviewed-by: Lei Zhang <thestig@chromium.org>
> Commit-Queue: Kyungjun Lee <kyungjunlee@google.com>
> Cr-Commit-Position: refs/heads/main@{#1204078}

Bug: 1473176, 1484882
Change-Id: Ie3a2e0435640983611dce3de6e6e068efd10fd79
Cq-Include-Trybots: luci.chromium.try:fuchsia-official,linux_chromium_msan_rel_ng
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4908279
Commit-Queue: Kyungjun Lee <kyungjunlee@google.com>
Reviewed-by: Lei Zhang <thestig@chromium.org>
Reviewed-by: David Tseng <dtseng@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1215741}
  • Loading branch information
Kyungjun Lee authored and Chromium LUCI CQ committed Oct 26, 2023
1 parent f013a99 commit 8e16868
Show file tree
Hide file tree
Showing 5 changed files with 758 additions and 204 deletions.
82 changes: 75 additions & 7 deletions chrome/browser/pdf/pdf_extension_accessibility_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,13 @@ std::string DumpPdfAccessibilityTree(const ui::AXTreeUpdate& ax_tree) {
continue;
}

// Exclude the status node and its wrapper node from `ax_tree_dump` if they
// exist in the tree. Tests don't expect them to be included in the dump.
if (node.role == ax::mojom::Role::kBanner ||
node.role == ax::mojom::Role::kStatus) {
continue;
}

auto indent_found = id_to_indentation.find(node.id);
int indent = 0;
if (indent_found != id_to_indentation.end()) {
Expand Down Expand Up @@ -154,6 +161,16 @@ class PDFExtensionAccessibilityTest : public PDFExtensionTestBase {
~PDFExtensionAccessibilityTest() override = default;

protected:
std::vector<base::test::FeatureRef> GetDisabledFeatures() const override {
auto disabled = PDFExtensionTestBase::GetDisabledFeatures();
// PDF OCR should not be enabled in `PDFExtensionAccessibilityTest`. If a
// new test class is derived from this class and needs to test PDF OCR,
// make sure that `GetDisabledFeatures()` is overridden to exclude
// `::features::kPdfOcr` from a list of disabled features.
disabled.push_back(::features::kPdfOcr);
return disabled;
}

ui::AXTreeUpdate GetAccessibilityTreeSnapshotForPdf(
content::WebContents* web_contents) {
content::FindAccessibilityNodeCriteria find_criteria;
Expand Down Expand Up @@ -732,13 +749,6 @@ class PDFExtensionAccessibilityTreeDumpTest
return enabled;
}

std::vector<base::test::FeatureRef> GetDisabledFeatures() const override {
auto disabled = PDFExtensionAccessibilityTest::GetDisabledFeatures();
// PDF OCR should not modify the dump.
disabled.push_back(::features::kPdfOcr);
return disabled;
}

void RunPDFTest(const base::FilePath::CharType* pdf_file) {
base::FilePath test_path = ui_test_utils::GetTestFilePath(
base::FilePath(FILE_PATH_LITERAL("pdf")),
Expand Down Expand Up @@ -828,6 +838,10 @@ class PDFExtensionAccessibilityTreeDumpTest
std::vector<std::string> actual_lines =
base::SplitString(actual_contents, "\n", base::KEEP_WHITESPACE,
base::SPLIT_WANT_NONEMPTY);
// TODO(b/1473176): Either keep the banner and status node in the output or
// modify `pdf_root` above to remove the banner and status nodes from the
// tree so that they are not in the format output.
RemoveBannerAndStatusNodesFromFormatOutput(actual_lines, GetParam());

// Validate the dump against the expectation file.
EXPECT_TRUE(test_helper_.ValidateAgainstExpectation(
Expand Down Expand Up @@ -864,6 +878,53 @@ class PDFExtensionAccessibilityTreeDumpTest
return property_filters;
}

void RemoveBannerAndStatusNodesFromFormatOutput(
std::vector<std::string>& output_lines,
const ui::AXApiType::Type& platform_type) {
// The banner and status nodes will be in the second and third lines in the
// tree output, respectively. These two nodes are always added to the PDF
// accessibility tree after the PDF root node and don't get deleted. So,
// it is safe to assume that they are always there in the format output.
// Thus, delete the second and third lines from the tree format output.
ASSERT_GT(output_lines.size(), 3u);
std::string banner_role;
std::string status_role;
switch (platform_type) {
case ui::AXApiType::kBlink:
banner_role = "banner";
status_role = "status";
break;
case ui::AXApiType::kLinux:
banner_role = "landmark";
status_role = "statusbar";
break;
case ui::AXApiType::kMac:
banner_role = "AXLandmarkBanner";
status_role = "AXApplicationStatus";
break;
case ui::AXApiType::kWinIA2:
banner_role = "IA2_ROLE_LANDMARK";
status_role = "ROLE_SYSTEM_STATUSBAR";
break;
case ui::AXApiType::kWinUIA:
banner_role = "Group";
status_role = "StatusBar";
break;
case ui::AXApiType::kNone:
[[fallthrough]];
case ui::AXApiType::kAndroid:
[[fallthrough]];
case ui::AXApiType::kAndroidExternal:
[[fallthrough]];
case ui::AXApiType::kFuchsia:
return;
}
EXPECT_TRUE(base::Contains(output_lines[1], banner_role));
EXPECT_TRUE(base::Contains(output_lines[2], status_role));

output_lines.erase(output_lines.begin() + 1, output_lines.begin() + 3);
}

ui::AXInspectTestHelper test_helper_;
};

Expand Down Expand Up @@ -1002,6 +1063,13 @@ class PDFExtensionAccessibilityPdfOcrTest
return enabled;
}

std::vector<base::test::FeatureRef> GetDisabledFeatures() const override {
// `PDFExtensionAccessibilityTest` has `::features::kPdfOcr` in a list of
// disabled features. Now that `::features::kPdfOcr` is used in this test,
// just return an empty list to exclude the feature from the list.
return {};
}

void ClickPdfOcrToggleButton(MimeHandlerViewGuest* guest_view) {
content::RenderFrameHost* guest_main_frame =
guest_view->GetGuestMainFrame();
Expand Down

0 comments on commit 8e16868

Please sign in to comment.