Accessible boundsForRange common impl

content/browser/accessibility/browser_accessibility_manager_unittest.cc

Index: content/browser/accessibility/browser_accessibility_manager_unittest.cc
diff --git a/content/browser/accessibility/browser_accessibility_manager_unittest.cc b/content/browser/accessibility/browser_accessibility_manager_unittest.cc
index d6a9f554c8f4f99850ef91c2a52b6fea0ce3ae34..3c0af498f6c7553e9f7623bfe220416281846920 100644
--- a/content/browser/accessibility/browser_accessibility_manager_unittest.cc
+++ b/content/browser/accessibility/browser_accessibility_manager_unittest.cc
@@ -600,4 +600,79 @@ TEST(BrowserAccessibilityManagerTest, TestFatalError) {
   ASSERT_TRUE(delegate->got_fatal_error());
 }
 
+TEST(BrowserAccessibilityManagerTest, BoundsFromRange) {
+  AccessibilityNodeData root;
+  root.id = 1;
+  root.role = WebKit::WebAXRoleRootWebArea;
+  root.child_ids.push_back(2);

[aboxhall, 2013/10/04 18:53:07]

I know this will work fine, but could we do it after static_text is created
anyway, just to avoid time travel? (Similarly with the child_ids of
static_text.)

[dmazzoni, 2013/10/21 17:25:38]

Done.

+
+  AccessibilityNodeData static_text;
+  static_text.id = 2;
+  static_text.SetValue("Hello, world.");
+  static_text.role = WebKit::WebAXRoleStaticText;
+  static_text.child_ids.push_back(3);
+  static_text.child_ids.push_back(4);
+  static_text.location = gfx::Rect(100, 100, 29, 18);
+
+  AccessibilityNodeData inline_text1;
+  inline_text1.id = 3;
+  inline_text1.SetValue("Hello, ");
+  inline_text1.role = WebKit::WebAXRoleInlineTextBox;
+  inline_text1.location = gfx::Rect(100, 100, 29, 9);
+  inline_text1.AddIntAttribute(AccessibilityNodeData::ATTR_TEXT_DIRECTION,
+                               WebKit::WebAXTextDirectionLR);
+  std::vector<int32> character_offsets1;
+  character_offsets1.push_back(6);

[aboxhall, 2013/10/04 18:53:07]

It might be helpful to comment each of these with the character offset, e.g.
character_offsets1.push_back(6);   // 0
character_offsets1.push_back(11);  // 1

Also, reading this makes me feel that the name "character_offsets" is a little
misleading; that makes me expect that each value will be the offset of the start
of the character from the start of the line, rather than the end of the
character. I can't think of a better name, though... character_end_offsets?

[dmazzoni, 2013/10/21 17:25:38]

Done.
I can't think of anything that's abundantly clear. This is one of those
low-level things that you have to dig into pretty deeply to understand, it's not
going to be obvious at a glance no matter what we name it.

I added some more comments to the attribute definition in the other change
(https://codereview.chromium.org/25943003/).

[aboxhall, 2013/10/23 15:40:54]

Looks good, thanks.

+  character_offsets1.push_back(11);
+  character_offsets1.push_back(16);
+  character_offsets1.push_back(21);
+  character_offsets1.push_back(26);
+  character_offsets1.push_back(29);
+  character_offsets1.push_back(29);

[aboxhall, 2013/10/04 18:53:07]

Is this meant to be the same as the previous one?

[dmazzoni, 2013/10/21 17:25:38]

Yes. This is an example of a space character having no width.

+  inline_text1.AddIntListAttribute(
+      AccessibilityNodeData::ATTR_CHARACTER_OFFSETS, character_offsets1);
+
+  AccessibilityNodeData inline_text2;
+  inline_text2.id = 4;
+  inline_text2.SetValue("world.");
+  inline_text2.role = WebKit::WebAXRoleInlineTextBox;
+  inline_text2.location = gfx::Rect(100, 109, 28, 9);
+  inline_text2.AddIntAttribute(AccessibilityNodeData::ATTR_TEXT_DIRECTION,
+                               WebKit::WebAXTextDirectionLR);
+  std::vector<int32> character_offsets2;
+  character_offsets2.push_back(5);
+  character_offsets2.push_back(10);
+  character_offsets2.push_back(15);
+  character_offsets2.push_back(20);
+  character_offsets2.push_back(25);
+  character_offsets2.push_back(28);
+  inline_text2.AddIntListAttribute(
+      AccessibilityNodeData::ATTR_CHARACTER_OFFSETS, character_offsets2);
+
+  scoped_ptr<BrowserAccessibilityManager> manager(
+      BrowserAccessibilityManager::Create(
+          root,
+          NULL,
+          new CountedBrowserAccessibilityFactory()));
+  manager->UpdateNodesForTesting(static_text, inline_text1, inline_text2);
+
+  BrowserAccessibility* root_accessible = manager->GetRoot();
+  BrowserAccessibility* static_text_accessible = root_accessible->GetChild(0);
+
+  EXPECT_EQ(gfx::Rect(100, 100, 6, 9).ToString(),
+            static_text_accessible->GetLocalBoundsForRange(0, 1).ToString());
+
+  EXPECT_EQ(gfx::Rect(100, 100, 26, 9).ToString(),
+            static_text_accessible->GetLocalBoundsForRange(0, 5).ToString());
+
+  EXPECT_EQ(gfx::Rect(100, 109, 5, 9).ToString(),
+            static_text_accessible->GetLocalBoundsForRange(7, 1).ToString());
+
+  EXPECT_EQ(gfx::Rect(100, 109, 25, 9).ToString(),
+            static_text_accessible->GetLocalBoundsForRange(7, 5).ToString());
+
+  EXPECT_EQ(gfx::Rect(100, 100, 29, 18).ToString(),
+            static_text_accessible->GetLocalBoundsForRange(5, 3).ToString());

[David Tseng, 2013/10/17 21:32:02]

How about passing in lengths beyond the actual text length? Also, should we
ensure nonnegative values are passed?

Did you want to also test the other possible text directions? (i.e. rtl, mixed
directions under a single static text).

[dmazzoni, 2013/10/21 17:25:38]

Sure, I'll add a test for an extreme range.
Good idea. I added a bidirectional test.

Note that the Blink part of the code has more tests that cover different
directions and bidi from the original html. This test is just meant to make sure
that GetLocalBoundsFromRange works correctly assuming the underlying data is
correct.

+}
+
 }  // namespace content