Skip to content

Commit

Permalink
Merge 110 | ax_ui: chromium hangs on user input when dealing with lar…
Browse files Browse the repository at this point in the history
…ge documents

SnapToMaxTextOffsetIfBeyond can be a heavy operation when triggered
on ax position creation. Disable it for now since it regresses the user
experience harsh. Later, all callers should be reworked, the method call
removed and only the IsValid() assertion should be left in place.

This is a partial revert of crrev.com/c/3810575, and todo-marked tests
from crrev.com/c/3961717 which was landed on top of the original
culprit CL.

Regressed by: crrev.com/c/3810575

(cherry picked from commit 5685924)

Bug: 1401591
Bug: 1396198
Change-Id: I448cdc4135a362f708882c0223a01507a6dbb7b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4126876
Reviewed-by: Aaron Leventhal <aleventhal@chromium.org>
Commit-Queue: Aaron Leventhal <aleventhal@chromium.org>
Cr-Original-Commit-Position: refs/heads/main@{#1088332}
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4134030
Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com>
Cr-Commit-Position: refs/branch-heads/5481@{#132}
Cr-Branched-From: 130f3e4-refs/heads/main@{#1084008}
  • Loading branch information
asurkov authored and Chromium LUCI CQ committed Jan 4, 2023
1 parent 394ab94 commit c2ef2d3
Show file tree
Hide file tree
Showing 3 changed files with 23 additions and 8 deletions.
4 changes: 3 additions & 1 deletion ui/accessibility/ax_node_position_unittest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -9234,9 +9234,11 @@ TEST_F(AXPositionTest,
ax::mojom::TextAffinity::kDownstream);
ASSERT_NE(nullptr, text_position);
ASSERT_TRUE(text_position->IsTextPosition());
// TODO: the created position is invalid, it should be a null position
// instead.
test_position = text_position->AsLeafTextPositionBeforeCharacter();
EXPECT_NE(nullptr, test_position);
EXPECT_TRUE(test_position->IsNullPosition());
EXPECT_FALSE(test_position->IsNullPosition());
}

TEST_F(AXPositionTest, AsLeafTextPositionAfterCharacter) {
Expand Down
21 changes: 16 additions & 5 deletions ui/accessibility/ax_position.h
Original file line number Diff line number Diff line change
Expand Up @@ -2554,7 +2554,11 @@ class AXPosition {
// position were at the start of the inline text box for "Line two".

const int max_text_offset = MaxTextOffset();
DCHECK_LE(text_offset_, max_text_offset);

// TODO(crbug.com/1404289): temporary disabled until ax position
// autocorrection issue is fixed.
// DCHECK_LE(text_offset_, max_text_offset);

const int max_text_offset_in_parent =
IsEmbeddedObjectInParent()
? AXNode::kEmbeddedObjectCharacterLengthUTF16
Expand Down Expand Up @@ -4527,11 +4531,13 @@ class AXPosition {
<< "Creating a position without an anchor is disallowed:\n"
<< ToDebugString();

// TODO(accessibility) Remove this line and let the below IsValid()
// TODO(crbug.com/1404289) Remove this line and let the below IsValid()
// assertion get triggered instead. We shouldn't be creating test positions
// with offsets that are too large. This seems to occur when the anchor node
// is ignored, and leads to a number of failing tests.
SnapToMaxTextOffsetIfBeyond();
// Comment this line out as a known performance culprit (also see
// crbug.com/1401591).
// SnapToMaxTextOffsetIfBeyond();

#if defined(AX_EXTRA_MAC_NODES)
// Temporary hack to constrain child index when extra mac nodes are present.
Expand All @@ -4547,8 +4553,13 @@ class AXPosition {
}
#endif

SANITIZER_CHECK(IsValid()) << "Creating invalid positions is disallowed:\n"
<< ToDebugString();
// TODO(crbug.com/1404289) see TODO above.
// Also look for the failures in
// AXPositionTest.AsLeafTextPositionBeforeCharacterIncludingGeneratedNewlines,
// AXPlatformNodeTextRangeProviderTest.TestNormalizeTextRangeForceSameAnchorOnDegenerateRange.
// SANITIZER_CHECK(IsValid()) << "Creating invalid positions is
// disallowed:\n"
// << ToDebugString();
}

int AnchorChildCount() const {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6438,9 +6438,11 @@ TEST_F(AXPlatformNodeTextRangeProviderTest,

EXPECT_EQ(*normalized_start, *normalized_start);

EXPECT_TRUE(normalized_start->AtStartOfAnchor());
// TODO: the start position is wrong.
EXPECT_FALSE(normalized_start->AtStartOfAnchor());
EXPECT_EQ(3, normalized_start->anchor_id());

EXPECT_TRUE(normalized_end->AtStartOfAnchor());
EXPECT_EQ(7, normalized_start->anchor_id());
EXPECT_EQ(7, normalized_end->anchor_id());
}

Expand Down

0 comments on commit c2ef2d3

Please sign in to comment.