Skip to content

GitLab

T
tangled
Commit a1a7bcba authored by Nektarios Paisios's avatar Nektarios Paisios Committed by Commit Bot
Browse files
Options

Recompute affinity if parent equivalent position is ambiguous. 

If a line break is introduced implicitly, i.e. because there is one block layout node next to the other, there would be two leaf positions:
One is at the end of the first layout node at the end of the line and the other at the beginning of the second block node at the start of the next line.
Both positions will have a downstream affinity, because there is no ambiguity as to which position is at the end vs. the start of the line.
However, when computing the parent equivalent position, and since there is no line break in the text of the parent, affinity will need to be computed on the browser side.
Also, I made some fixes which maintain predictability as to which affinity will be assigned to computed positions, regardless of the input position.

R=dmazzoni@chromium.org

Bug: 831179
Change-Id: I425223bee1f53654ddaaa045a375c6da0367b7a8
Tested: Unit tests
Reviewed-on: https://chromium-review.googlesource.com/1028181Reviewed-by: default avatarDominic Mazzoni <dmazzoni@chromium.org>
Commit-Queue: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#553794}
parent 02c5156d
Hide whitespace changes
Inline Side-by-side
Showing with 102 additions and 31 deletions
ui/accessibility/ax_node_position_unittest.cc
View file @ a1a7bcba
......@@ -627,6 +627,10 @@ TEST_F(AXPositionTest, AsTextPositionWithTreePosition) {
EXPECT_EQ(6, test_position->text_offset());
// But its child index should be unchanged.
EXPECT_EQ(1, test_position->child_index());
// And the affinity cannot be anything other than downstream because we
// haven't moved up the tree and so there was no opportunity to introduce any
// ambiguity regarding the new position.
EXPECT_EQ(ax::mojom::TextAffinity::kDownstream, test_position->affinity());
// Test for a "before text" position.
tree_position = AXNodePosition::CreateTreePosition(
......@@ -639,6 +643,7 @@ TEST_F(AXPositionTest, AsTextPositionWithTreePosition) {
EXPECT_EQ(inline_box1_.id, test_position->anchor_id());
EXPECT_EQ(0, test_position->text_offset());
EXPECT_EQ(AXNodePosition::BEFORE_TEXT, test_position->child_index());
EXPECT_EQ(ax::mojom::TextAffinity::kDownstream, test_position->affinity());
// Test for an "after text" position.
tree_position = AXNodePosition::CreateTreePosition(
......@@ -651,6 +656,7 @@ TEST_F(AXPositionTest, AsTextPositionWithTreePosition) {
EXPECT_EQ(inline_box1_.id, test_position->anchor_id());
EXPECT_EQ(6, test_position->text_offset());
EXPECT_EQ(0, test_position->child_index());
EXPECT_EQ(ax::mojom::TextAffinity::kDownstream, test_position->affinity());
}
TEST_F(AXPositionTest, AsTextPositionWithTextPosition) {
......@@ -1021,6 +1027,7 @@ TEST_F(AXPositionTest, CreateParentPositionWithTreePosition) {
EXPECT_EQ(root_.id, test_position->anchor_id());
// |child_index| should point to the check box node.
EXPECT_EQ(1, test_position->child_index());
EXPECT_EQ(ax::mojom::TextAffinity::kDownstream, test_position->affinity());
tree_position = AXNodePosition::CreateTreePosition(
tree_.data().tree_id, root_.id, 1 /* child_index */);
......@@ -1031,15 +1038,32 @@ TEST_F(AXPositionTest, CreateParentPositionWithTreePosition) {
}
TEST_F(AXPositionTest, CreateParentPositionWithTextPosition) {
// Create a position that points at the end of the first line, right after the
// check box.
TestPositionType text_position = AXNodePosition::CreateTextPosition(
tree_.data().tree_id, inline_box2_.id, 5 /* text_offset */,
tree_.data().tree_id, check_box_.id, 9 /* text_offset */,
ax::mojom::TextAffinity::kDownstream);
ASSERT_NE(nullptr, text_position);
TestPositionType test_position = text_position->CreateParentPosition();
EXPECT_NE(nullptr, test_position);
EXPECT_TRUE(test_position->IsTextPosition());
EXPECT_EQ(root_.id, test_position->anchor_id());
EXPECT_EQ(15, test_position->text_offset());
// Since the same text offset in the root could be used to point to the
// beginning of the second line, affinity should have been adjusted to
// upstream.
EXPECT_EQ(ax::mojom::TextAffinity::kUpstream, test_position->affinity());
text_position = AXNodePosition::CreateTextPosition(
tree_.data().tree_id, inline_box2_.id, 5 /* text_offset */,
ax::mojom::TextAffinity::kDownstream);
ASSERT_NE(nullptr, text_position);
test_position = text_position->CreateParentPosition();
EXPECT_NE(nullptr, test_position);
EXPECT_TRUE(test_position->IsTextPosition());
EXPECT_EQ(static_text2_.id, test_position->anchor_id());
EXPECT_EQ(5, test_position->text_offset());
EXPECT_EQ(ax::mojom::TextAffinity::kDownstream, test_position->affinity());
test_position = test_position->CreateParentPosition();
EXPECT_NE(nullptr, test_position);
......@@ -1048,6 +1072,7 @@ TEST_F(AXPositionTest, CreateParentPositionWithTextPosition) {
// |text_offset| should point to the same offset on the second line where the
// static text node position was pointing at.
EXPECT_EQ(12, test_position->text_offset());
EXPECT_EQ(ax::mojom::TextAffinity::kDownstream, test_position->affinity());
}
TEST_F(AXPositionTest,
......@@ -2010,7 +2035,7 @@ INSTANTIATE_TEST_CASE_P(
"affinity=downstream annotated_text=ButtonCheck< >boxLine "
"1\nLine 2",
"TextPosition tree_id=0 anchor_id=1 text_offset=15 "
"affinity=downstream annotated_text=ButtonCheck box<L>ine "
"affinity=upstream annotated_text=ButtonCheck box<L>ine "
"1\nLine 2",
"TextPosition tree_id=0 anchor_id=1 text_offset=19 "
"affinity=downstream annotated_text=ButtonCheck boxLine< "
......@@ -2082,7 +2107,7 @@ INSTANTIATE_TEST_CASE_P(
"affinity=downstream annotated_text=ButtonCheck< >boxLine "
"1\nLine 2",
"TextPosition tree_id=0 anchor_id=1 text_offset=15 "
"affinity=downstream annotated_text=ButtonCheck box<L>ine "
"affinity=upstream annotated_text=ButtonCheck box<L>ine "
"1\nLine 2",
"TextPosition tree_id=0 anchor_id=1 text_offset=19 "
"affinity=downstream annotated_text=ButtonCheck boxLine< "
......@@ -2203,7 +2228,7 @@ INSTANTIATE_TEST_CASE_P(
"affinity=downstream annotated_text=ButtonCheck boxLine< "
">1\nLine 2",
"TextPosition tree_id=0 anchor_id=1 text_offset=15 "
"affinity=downstream annotated_text=ButtonCheck box<L>ine "
"affinity=upstream annotated_text=ButtonCheck box<L>ine "
"1\nLine 2",
"TextPosition tree_id=0 anchor_id=1 text_offset=11 "
"affinity=downstream annotated_text=ButtonCheck< >boxLine "
......@@ -2284,7 +2309,7 @@ INSTANTIATE_TEST_CASE_P(
"affinity=downstream annotated_text=ButtonCheck boxLine< "
">1\nLine 2",
"TextPosition tree_id=0 anchor_id=1 text_offset=15 "
"affinity=downstream annotated_text=ButtonCheck box<L>ine "
"affinity=upstream annotated_text=ButtonCheck box<L>ine "
"1\nLine 2",
"TextPosition tree_id=0 anchor_id=1 text_offset=11 "
"affinity=downstream annotated_text=ButtonCheck< >boxLine "
......@@ -2667,7 +2692,7 @@ INSTANTIATE_TEST_CASE_P(
ROOT_ID,
0 /* text_offset */,
{"TextPosition tree_id=0 anchor_id=1 text_offset=15 "
"affinity=downstream annotated_text=ButtonCheck box<L>ine "
"affinity=upstream annotated_text=ButtonCheck box<L>ine "
"1\nLine 2",
"TextPosition tree_id=0 anchor_id=1 text_offset=21 "
"affinity=downstream annotated_text=ButtonCheck boxLine 1"
......@@ -2719,7 +2744,7 @@ INSTANTIATE_TEST_CASE_P(
ROOT_ID,
0 /* text_offset */,
{"TextPosition tree_id=0 anchor_id=1 text_offset=15 "
"affinity=downstream annotated_text=ButtonCheck box<L>ine "
"affinity=upstream annotated_text=ButtonCheck box<L>ine "
"1\nLine 2",
"TextPosition tree_id=0 anchor_id=1 text_offset=21 "
"affinity=downstream annotated_text=ButtonCheck boxLine 1"
......@@ -2774,14 +2799,11 @@ INSTANTIATE_TEST_CASE_P(
ROOT_ID,
0 /* text_offset */,
{"TextPosition tree_id=0 anchor_id=1 text_offset=15 "
"affinity=downstream annotated_text=ButtonCheck box<L>ine "
"affinity=upstream annotated_text=ButtonCheck box<L>ine "
"1\nLine 2",
"TextPosition tree_id=0 anchor_id=1 text_offset=21 "
"affinity=downstream annotated_text=ButtonCheck boxLine "
"1<\n>Line 2",
"TextPosition tree_id=0 anchor_id=1 text_offset=21 "
"affinity=downstream annotated_text=ButtonCheck boxLine "
"1<\n>Line 2"}},
"TextPosition tree_id=0 anchor_id=1 text_offset=15 "
"affinity=upstream annotated_text=ButtonCheck box<L>ine "
"1\nLine 2"}},
TestParam{base::BindRepeating([](const TestPositionType& position) {
return position->CreateNextLineEndPosition(
AXBoundaryBehavior::StopIfAlreadyAtBoundary);
......@@ -2850,11 +2872,9 @@ INSTANTIATE_TEST_CASE_P(
ROOT_ID,
20 /* text_offset on the last character of "line 1". */,
{"TextPosition tree_id=0 anchor_id=1 text_offset=15 "
"affinity=downstream annotated_text=ButtonCheck box<L>ine "
"affinity=upstream annotated_text=ButtonCheck box<L>ine "
"1\nLine 2",
"TextPosition tree_id=0 anchor_id=1 text_offset=15 "
"affinity=downstream annotated_text=ButtonCheck box<L>ine "
"1\nLine 2"}},
"NullPosition"}},
TestParam{base::BindRepeating([](const TestPositionType& position) {
return position->CreatePreviousLineEndPosition(
AXBoundaryBehavior::CrossBoundary);
......@@ -2928,10 +2948,10 @@ INSTANTIATE_TEST_CASE_P(
ROOT_ID,
20 /* text_offset on the last character of "line 1". */,
{"TextPosition tree_id=0 anchor_id=1 text_offset=15 "
"affinity=downstream annotated_text=ButtonCheck box<L>ine "
"affinity=upstream annotated_text=ButtonCheck box<L>ine "
"1\nLine 2",
"TextPosition tree_id=0 anchor_id=1 text_offset=15 "
"affinity=downstream annotated_text=ButtonCheck box<L>ine "
"TextPosition tree_id=0 anchor_id=1 text_offset=0 "
"affinity=downstream annotated_text=<B>uttonCheck boxLine "
"1\nLine 2"}},
TestParam{base::BindRepeating([](const TestPositionType& position) {
return position->CreatePreviousLineEndPosition(
......
ui/accessibility/ax_position.h
View file @ a1a7bcba
......@@ -302,15 +302,17 @@ class AXPosition {
case AXPositionKind::TEXT_POSITION:
// If affinity has been used to specify whether the caret is at the end
// of a line or at the start of the next one, this should have been
// reflected in the text position we got. In other cases, we assume that
// white space is being used to separate lines.
// reflected in the leaf text position we got. In other cases, we
// assume that white space is being used to separate lines.
// Note that we don't treat a position that is at the start of a line
// break that is on a line by itself as being at the end of the line.
if (GetNextOnLineID(text_position->anchor_id_) == INVALID_ANCHOR_ID) {
if (text_position->IsInWhiteSpace()) {
return !text_position->AtStartOfLine() &&
text_position->AtStartOfAnchor();
} else {
return text_position->AtEndOfAnchor();
}
return text_position->AtEndOfAnchor();
}
// The current anchor might be followed by a soft line break.
......@@ -404,7 +406,9 @@ class AXPosition {
if (copy->child_index_ == BEFORE_TEXT) {
// "Before text" positions can only appear on leaf nodes.
DCHECK(!copy->AnchorChildCount());
// If the current text offset is valid, we don't touch it.
// If the current text offset is valid, we don't touch it to potentially
// allow converting from a text position to a tree position and back
// without losing information.
if (copy->text_offset_ < 0 || copy->text_offset_ >= copy->MaxTextOffset())
copy->text_offset_ = 0;
} else if (copy->child_index_ == copy->AnchorChildCount()) {
......@@ -418,8 +422,10 @@ class AXPosition {
DCHECK(child);
int child_length = child->MaxTextOffsetInParent();
// If the current text offset is valid, we don't touch it.
// Otherwise, we reset it to the beginning of the current child node.
// If the current text offset is valid, we don't touch it to potentially
// allow converting from a text position to a tree position and back
// without losing information. Otherwise, we reset it to the beginning
// of the current child node.
if (i == child_index_ &&
(copy->text_offset_ < new_offset ||
copy->text_offset_ > (new_offset + child_length) ||
......@@ -435,6 +441,12 @@ class AXPosition {
}
}
// Affinity should always be left as downstream. The only case when the
// resulting text position is at the end of the line is when we get an
// "after text" leaf position, but even in this case downstream is
// appropriate because there is no ambiguity whetehr the position is at the
// end of the current line vs. the start of the next line. It would always
// be the former.
copy->kind_ = AXPositionKind::TEXT_POSITION;
return copy;
}
......@@ -547,8 +559,12 @@ class AXPosition {
case AXPositionKind::TEXT_POSITION: {
// If our parent contains all our text, we need to maintain the affinity
// and the text offset. Otherwise, we return a position that is either
// before or after the child and we don't maintain the affinity when the
// before or after the child. We always recompute the affinity when the
// position is after the child.
// Recomputing the affinity is important because even though a text
// position might unambiguously be at the end of a line, its parent
// position might be the same as the parent position of the position
// representing the start of the next line.
int parent_offset = AnchorTextOffsetInParent();
ax::mojom::TextAffinity parent_affinity = affinity_;
if (MaxTextOffset() == MaxTextOffsetInParent()) {
......@@ -557,8 +573,19 @@ class AXPosition {
parent_offset += MaxTextOffsetInParent();
parent_affinity = ax::mojom::TextAffinity::kDownstream;
}
return CreateTextPosition(tree_id, parent_id, parent_offset,
parent_affinity);
AXPositionInstance parent_position = CreateTextPosition(
tree_id, parent_id, parent_offset, parent_affinity);
// We check if the parent position has introduced ambiguity as to
// whether it refers to the end of the current or the start of the next
// line. We do this check by creating the parent position and testing if
// it is erroneously at the start of the next line. We could not have
// checked if the child was at the end of the line, because our line end
// testing logic takes into account line breaks, which don't apply in
// this situation.
if (text_offset_ == MaxTextOffset() && parent_position->AtStartOfLine())
parent_position->affinity_ = ax::mojom::TextAffinity::kUpstream;
return parent_position;
}
}
......@@ -786,7 +813,13 @@ class AXPosition {
if (boundary_behavior == AXBoundaryBehavior::StopIfAlreadyAtBoundary &&
text_position->AtEndOfWord()) {
AXPositionInstance clone = Clone();
// If there is no ambiguity as to whether the position is at the end of
// the current line or the start of the next line, affinity should be
// reset in order to get consistent output from this function regardless
// of input affinity.
clone->affinity_ = ax::mojom::TextAffinity::kDownstream;
if (clone->AtStartOfLine())
clone->affinity_ = ax::mojom::TextAffinity::kUpstream;
return clone;
}
......@@ -849,7 +882,13 @@ class AXPosition {
if (boundary_behavior == AXBoundaryBehavior::StopIfAlreadyAtBoundary &&
text_position->AtEndOfWord()) {
AXPositionInstance clone = Clone();
// If there is no ambiguity as to whether the position is at the end of
// the current line or the start of the next line, affinity should be
// reset in order to get consistent output from this function regardless
// of input affinity.
clone->affinity_ = ax::mojom::TextAffinity::kDownstream;
if (clone->AtStartOfLine())
clone->affinity_ = ax::mojom::TextAffinity::kUpstream;
return clone;
}
......@@ -1000,7 +1039,13 @@ class AXPosition {
if (boundary_behavior == AXBoundaryBehavior::StopIfAlreadyAtBoundary &&
text_position->AtEndOfLine()) {
AXPositionInstance clone = Clone();
// If there is no ambiguity as to whether the position is at the end of
// the current line or the start of the next line, affinity should be
// reset in order to get consistent output from this function regardless
// of input affinity.
clone->affinity_ = ax::mojom::TextAffinity::kDownstream;
if (clone->AtStartOfLine())
clone->affinity_ = ax::mojom::TextAffinity::kUpstream;
return clone;
}
......@@ -1051,7 +1096,13 @@ class AXPosition {
if (boundary_behavior == AXBoundaryBehavior::StopIfAlreadyAtBoundary &&
text_position->AtEndOfLine()) {
AXPositionInstance clone = Clone();
// If there is no ambiguity as to whether the position is at the end of
// the current line or the start of the next line, affinity should be
// reset in order to get consistent output from this function regardless
// of input affinity.
clone->affinity_ = ax::mojom::TextAffinity::kDownstream;
if (clone->AtStartOfLine())
clone->affinity_ = ax::mojom::TextAffinity::kUpstream;
return clone;
}
......
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