Skip to content

Commit

Permalink
Ensures that text positions are at a grapheme boundary and not at a l…
Browse files Browse the repository at this point in the history 
…ow surrogate pair


This patch changes the way that CreateNext/PreviousCharacterPosition work so that they don't move by a single UTF16 code point but,
similar to how the left / right cursor keys work, by a grapheme cluster.
A grapheme cluster is what a person familiar with a particular language would call a single character.
For example, it could include a UTF16 code point for a letter and more UTF16 code points for diacritics,
or it could include a surrogate pair for characters outside the basic multilingual plain.

This patch also disallows creating text positions that are not at a grapheme cluster boundary,
so that text positions can never point in the middle of a character such as at
a low surrogate pair.

To avoid introducing performance regressions, this patch caches
the inner text of the position's anchor node, after it has been retrieved for the first time.
The break iterator is also cached.

An upcoming patch will ensure that AXPositions are invalidated when the AXTree changes.
This has always been the case, but the upcoming patch will ensure that this is captured and enforced in code too,
especially now that AXPosition includes a handful of cached members.

Furthermore, I took the opportunity to correct the copy constructors which cannot be defaulted since
AXTreeID is non-copiable and improve the assignment operator using the copy and swap idiom.

R=dmazzoni@chromium.org, kbabbitt@microsoft.com

Change-Id: Iba12e0d214cd0d4a604597f9bb17a1a78992584f
Bug: 720370
Change-Id: Iba12e0d214cd0d4a604597f9bb17a1a78992584f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1796047
Reviewed-by: Nektarios Paisios <nektar@chromium.org>
Reviewed-by: Kevin Babbitt <kbabbitt@microsoft.com>
Reviewed-by: Kurt Catti-Schmidt <kschmi@microsoft.com>
Commit-Queue: Nektarios Paisios <nektar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#711983}
  • Loading branch information
Nektarios Paisios authored and Commit Bot committed Nov 2, 2019
1 parent e4ff718 commit 376c84c
Show file tree
Hide file tree
Showing 10 changed files with 866 additions and 260 deletions.
62 changes: 34 additions & 28 deletions content/browser/accessibility/browser_accessibility_position.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,12 +10,18 @@
#include "content/browser/accessibility/browser_accessibility.h"
#include "content/browser/accessibility/browser_accessibility_manager.h"
#include "ui/accessibility/ax_enums.mojom.h"
#include "ui/accessibility/ax_node_data.h"

namespace content {

BrowserAccessibilityPosition::BrowserAccessibilityPosition() {}
BrowserAccessibilityPosition::BrowserAccessibilityPosition() = default;

BrowserAccessibilityPosition::~BrowserAccessibilityPosition() {}
BrowserAccessibilityPosition::~BrowserAccessibilityPosition() = default;

BrowserAccessibilityPosition::BrowserAccessibilityPosition(
const BrowserAccessibilityPosition& other)
: ui::AXPosition<BrowserAccessibilityPosition, BrowserAccessibility>(
other) {}

BrowserAccessibilityPosition::AXPositionInstance
BrowserAccessibilityPosition::Clone() const {
Expand All @@ -24,11 +30,33 @@ BrowserAccessibilityPosition::Clone() const {

base::string16 BrowserAccessibilityPosition::GetText() const {
if (IsNullPosition())
return base::string16();
return {};
DCHECK(GetAnchor());
return GetAnchor()->GetText();
}

bool BrowserAccessibilityPosition::IsInLineBreak() const {
if (IsNullPosition())
return false;
DCHECK(GetAnchor());
return GetAnchor()->IsLineBreakObject();
}

bool BrowserAccessibilityPosition::IsInTextObject() const {
if (IsNullPosition())
return false;
DCHECK(GetAnchor());
return GetAnchor()->IsTextOnlyObject();
}

bool BrowserAccessibilityPosition::IsInWhiteSpace() const {
if (IsNullPosition())
return false;
DCHECK(GetAnchor());
return GetAnchor()->IsLineBreakObject() ||
base::ContainsOnlyChars(GetText(), base::kWhitespaceUTF16);
}

void BrowserAccessibilityPosition::AnchorChild(
int child_index,
AXTreeID* tree_id,
Expand Down Expand Up @@ -110,10 +138,10 @@ BrowserAccessibility* BrowserAccessibilityPosition::GetNodeInTree(
return manager->GetFromID(node_id);
}

// On some platforms, most objects are represented in the text of their parents
// with a special (embedded object) character and not with their actual text
// contents.
bool BrowserAccessibilityPosition::IsEmbeddedObjectInParent() const {
// On some platforms, most objects are represented in the text of their
// parents with a special (embedded object) character and not with their
// actual text contents.
#if defined(OS_WIN) || BUILDFLAG(USE_ATK)
// Not all objects in the internal accessibility tree are exposed to platform
// APIs.
Expand All @@ -124,28 +152,6 @@ bool BrowserAccessibilityPosition::IsEmbeddedObjectInParent() const {
#endif
}

bool BrowserAccessibilityPosition::IsInLineBreak() const {
if (IsNullPosition())
return false;
DCHECK(GetAnchor());
return GetAnchor()->IsLineBreakObject();
}

bool BrowserAccessibilityPosition::IsInTextObject() const {
if (IsNullPosition())
return false;
DCHECK(GetAnchor());
return GetAnchor()->IsTextOnlyObject();
}

bool BrowserAccessibilityPosition::IsInWhiteSpace() const {
if (IsNullPosition())
return false;
DCHECK(GetAnchor());
return GetAnchor()->IsLineBreakObject() ||
base::ContainsOnlyChars(GetText(), base::kWhitespaceUTF16);
}

bool BrowserAccessibilityPosition::IsInLineBreakingObject() const {
if (IsNullPosition())
return false;
Expand Down
6 changes: 3 additions & 3 deletions content/browser/accessibility/browser_accessibility_position.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <stdint.h>

#include <string>
#include <vector>

#include "base/strings/string16.h"
Expand All @@ -26,17 +27,16 @@ class CONTENT_EXPORT BrowserAccessibilityPosition
public:
BrowserAccessibilityPosition();
~BrowserAccessibilityPosition() override;
BrowserAccessibilityPosition(const BrowserAccessibilityPosition& other);

AXPositionInstance Clone() const override;

base::string16 GetText() const override;
bool IsInLineBreak() const override;
bool IsInTextObject() const override;
bool IsInWhiteSpace() const override;
base::string16 GetText() const override;

protected:
BrowserAccessibilityPosition(const BrowserAccessibilityPosition& other) =
default;
void AnchorChild(int child_index,
AXTreeID* tree_id,
ui::AXNode::AXID* child_id) const override;
Expand Down
102 changes: 53 additions & 49 deletions ui/accessibility/ax_node_position.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,43 +7,24 @@
#include "base/strings/string_util.h"
#include "ui/accessibility/accessibility_features.h"
#include "ui/accessibility/ax_enums.mojom.h"
#include "ui/accessibility/ax_node_data.h"
#include "ui/accessibility/ax_tree_manager_map.h"

namespace ui {

AXTree* AXNodePosition::tree_ = nullptr;

AXNodePosition::AXNodePosition() {}
AXNodePosition::AXNodePosition() = default;

AXNodePosition::~AXNodePosition() {}
AXNodePosition::~AXNodePosition() = default;

AXNodePosition::AXNodePosition(const AXNodePosition& other)
: AXPosition<AXNodePosition, AXNode>(other) {}

AXNodePosition::AXPositionInstance AXNodePosition::Clone() const {
return AXPositionInstance(new AXNodePosition(*this));
}

base::string16 AXNodePosition::GetText() const {
if (IsNullPosition())
return base::string16();

const AXNode* anchor = GetAnchor();
DCHECK(anchor);
base::string16 value = GetAnchor()->data().GetString16Attribute(
ax::mojom::StringAttribute::kValue);
if (!value.empty())
return value;

if (anchor->IsText()) {
return anchor->data().GetString16Attribute(
ax::mojom::StringAttribute::kName);
}

base::string16 text;
for (int i = 0; i < AnchorChildCount(); ++i)
text += CreateChildPositionAt(i)->GetText();

return text;
}

// static
AXNodePosition::AXPositionInstance AXNodePosition::CreatePosition(
AXTreeID tree_id,
Expand Down Expand Up @@ -113,30 +94,6 @@ AXNodePosition::AXPositionInstance AXNodePosition::AsUnignoredTextPosition(
return unignored_position;
}

int AXNodePosition::MaxTextOffset() const {
if (IsNullPosition())
return INVALID_OFFSET;

const AXNode* anchor = GetAnchor();
DCHECK(anchor);
const std::string& value =
anchor->data().GetStringAttribute(ax::mojom::StringAttribute::kValue);
if (!value.empty())
return value.length();

if (anchor->IsText()) {
return anchor->data()
.GetStringAttribute(ax::mojom::StringAttribute::kName)
.length();
}

int max_text_offset = 0;
for (int i = 0; i < AnchorChildCount(); ++i)
max_text_offset += CreateChildPositionAt(i)->MaxTextOffset();

return max_text_offset;
}

void AXNodePosition::AnchorChild(int child_index,
AXTreeID* tree_id,
AXNode::AXID* child_id) const {
Expand Down Expand Up @@ -242,6 +199,29 @@ AXNode* AXNodePosition::GetNodeInTree(AXTreeID tree_id,
return nullptr;
}

base::string16 AXNodePosition::GetText() const {
if (IsNullPosition())
return {};

const AXNode* anchor = GetAnchor();
DCHECK(anchor);
base::string16 value = GetAnchor()->data().GetString16Attribute(
ax::mojom::StringAttribute::kValue);
if (!value.empty())
return value;

if (anchor->IsText()) {
return anchor->data().GetString16Attribute(
ax::mojom::StringAttribute::kName);
}

base::string16 text;
for (int i = 0; i < AnchorChildCount(); ++i)
text += CreateChildPositionAt(i)->GetText();

return text;
}

bool AXNodePosition::IsInLineBreak() const {
if (IsNullPosition())
return false;
Expand All @@ -264,6 +244,30 @@ bool AXNodePosition::IsInWhiteSpace() const {
base::ContainsOnlyChars(GetText(), base::kWhitespaceUTF16);
}

int AXNodePosition::MaxTextOffset() const {
if (IsNullPosition())
return INVALID_OFFSET;

const AXNode* anchor = GetAnchor();
DCHECK(anchor);
base::string16 value = GetAnchor()->data().GetString16Attribute(
ax::mojom::StringAttribute::kValue);
if (!value.empty())
return value.length();

if (anchor->IsText()) {
return anchor->data()
.GetString16Attribute(ax::mojom::StringAttribute::kName)
.length();
}

int text_length = 0;
for (int i = 0; i < AnchorChildCount(); ++i)
text_length += CreateChildPositionAt(i)->MaxTextOffset();

return text_length;
}

bool AXNodePosition::IsInLineBreakingObject() const {
if (IsNullPosition())
return false;
Expand Down
19 changes: 10 additions & 9 deletions ui/accessibility/ax_node_position.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

#include <stdint.h>

#include <string>
#include <vector>

#include "base/strings/string16.h"
Expand All @@ -21,30 +22,30 @@ namespace ui {
// knowledge of the AXPosition AXNodeType (which is unknown by AXPosition).
class AX_EXPORT AXNodePosition : public AXPosition<AXNodePosition, AXNode> {
public:
AXNodePosition();
~AXNodePosition() override;

static AXPositionInstance CreatePosition(AXTreeID tree_id,
const AXNode& node,
int offset,
ax::mojom::TextAffinity affinity);

static void SetTree(AXTree* tree) { tree_ = tree; }

AXNodePosition();
~AXNodePosition() override;
AXNodePosition(const AXNodePosition& other);

AXPositionInstance Clone() const override;

int MaxTextOffset() const override;
base::string16 GetText() const override;
bool IsInLineBreak() const override;
bool IsInTextObject() const override;
bool IsInWhiteSpace() const override;
base::string16 GetText() const override;
int MaxTextOffset() const override;

bool IsIgnoredPosition() const override;
AXPositionInstance AsUnignoredTextPosition(
AdjustmentBehavior adjustment_behavior) const override;

protected:
AXNodePosition(const AXNodePosition& other) = default;
void AnchorChild(int child_index,
AXTreeID* tree_id,
AXNode::AXID* child_id) const override;
Expand All @@ -63,18 +64,18 @@ class AX_EXPORT AXNodePosition : public AXPosition<AXNodePosition, AXNode> {
AXNode::AXID GetPreviousOnLineID(AXNode::AXID node_id) const override;

private:
static AXTree* tree_;

// Returns the parent node of the provided child. Returns the parent
// node's tree id and node id through the provided output parameters,
// parent_tree_id and parent_id.
// |parent_tree_id| and |parent_id|.
static AXNode* GetParent(AXNode* child,
AXTreeID child_tree_id,
AXTreeID* parent_tree_id,
AXNode::AXID* parent_id);

AXPositionInstance CreateUnignoredPositionFromLeafTextPosition(
AdjustmentBehavior adjustment_behavior) const;

static AXTree* tree_;
};

} // namespace ui
Expand Down

0 comments on commit 376c84c

Please sign in to comment.