Bug 961737 - DOMPointToOffset cleaning up, r=tbsaunde

accessible/src/generic/HyperTextAccessible.cpp

@@ -231,18 +231,12 @@ HyperTextAccessible::TextSubstring(int32_t aStartOffset, int32_t aEndOffset,
   endChild->AppendTextTo(aText, 0, endOffset - endChildOffset);
 }
 
-Accessible*
-HyperTextAccessible::DOMPointToHypertextOffset(nsINode* aNode,
-                                               int32_t aNodeOffset,
-                                               int32_t* aHyperTextOffset,
-                                               bool aIsEndOffset) const
+int32_t
+HyperTextAccessible::DOMPointToOffset(nsINode* aNode, int32_t aNodeOffset,
+                                      bool aIsEndOffset) const
 {
-  if (!aHyperTextOffset)
-    return nullptr;
-  *aHyperTextOffset = 0;
-
   if (!aNode)
-    return nullptr;
+    return 0;
 
   uint32_t addTextOffset = 0;
   nsINode* findNode = nullptr;
@@ -254,10 +248,11 @@ HyperTextAccessible::DOMPointToHypertextOffset(nsINode* aNode,
     // For text nodes, aNodeOffset comes in as a character offset
     // Text offset will be added at the end, if we find the offset in this hypertext
     // We want the "skipped" offset into the text (rendered text without the extra whitespace)
-    nsIFrame *frame = aNode->AsContent()->GetPrimaryFrame();
-    NS_ENSURE_TRUE(frame, nullptr);
+    nsIFrame* frame = aNode->AsContent()->GetPrimaryFrame();
+    NS_ENSURE_TRUE(frame, 0);
+
     nsresult rv = ContentToRenderedOffset(frame, aNodeOffset, &addTextOffset);
-    NS_ENSURE_SUCCESS(rv, nullptr);
+    NS_ENSURE_SUCCESS(rv, 0);
     // Get the child node and 
     findNode = aNode;
 
@@ -276,8 +271,7 @@ HyperTextAccessible::DOMPointToHypertextOffset(nsINode* aNode,
         if (aNode == GetNode()) {
           // Case #1: this accessible has no children and thus has empty text,
           // we can only be at hypertext offset 0.
-          *aHyperTextOffset = 0;
-          return nullptr;
+          return 0;
         }
 
         // Case #2: there are no children, we're at this node.
@@ -295,7 +289,7 @@ HyperTextAccessible::DOMPointToHypertextOffset(nsINode* aNode,
 
   // Get accessible for this findNode, or if that node isn't accessible, use the
   // accessible for the next DOM node which has one (based on forward depth first search)
-  Accessible* descendantAcc = nullptr;
+  Accessible* descendant = nullptr;
   if (findNode) {
     nsCOMPtr<nsIContent> findContent(do_QueryInterface(findNode));
     if (findContent && findContent->IsHTML() &&
@@ -305,18 +299,17 @@ HyperTextAccessible::DOMPointToHypertextOffset(nsINode* aNode,
                                  nsGkAtoms::_true,
                                  eIgnoreCase)) {
       // This <br> is the hacky "bogus node" used when there is no text in a control
-      *aHyperTextOffset = 0;
-      return nullptr;
+      return 0;
     }
-    descendantAcc = GetFirstAvailableAccessible(findNode);
+    descendant = GetFirstAvailableAccessible(findNode);
   }
 
   // From the descendant, go up and get the immediate child of this hypertext
-  Accessible* childAccAtOffset = nullptr;
-  while (descendantAcc) {
-    Accessible* parentAcc = descendantAcc->Parent();
-    if (parentAcc == this) {
-      childAccAtOffset = descendantAcc;
+  Accessible* childAtOffset = nullptr;
+  while (descendant) {
+    Accessible* parent = descendant->Parent();
+    if (parent == this) {
+      childAtOffset = descendant;
       break;
     }
 
@@ -328,42 +321,17 @@ HyperTextAccessible::DOMPointToHypertextOffset(nsINode* aNode,
     // is not at 0 offset then the returned offset should be after an embedded
     // character the original point belongs to.
     if (aIsEndOffset)
-      addTextOffset = (addTextOffset > 0 || descendantAcc->IndexInParent() > 0) ? 1 : 0;
+      addTextOffset = (addTextOffset > 0 || descendant->IndexInParent() > 0) ? 1 : 0;
     else
       addTextOffset = 0;
 
-    descendantAcc = parentAcc;
+    descendant = parent;
   }
 
-  // Loop through, adding offsets until we reach childAccessible
-  // If childAccessible is null we will end up adding up the entire length of
-  // the hypertext, which is good -- it just means our offset node
-  // came after the last accessible child's node
-  uint32_t childCount = ChildCount();
-
-  uint32_t childIdx = 0;
-  Accessible* childAcc = nullptr;
-  for (; childIdx < childCount; childIdx++) {
-    childAcc = mChildren[childIdx];
-    if (childAcc == childAccAtOffset)
-      break;
-
-    *aHyperTextOffset += nsAccUtils::TextLength(childAcc);
-  }
-
-  if (childIdx < childCount) {
-    *aHyperTextOffset += addTextOffset;
-    NS_ASSERTION(childAcc == childAccAtOffset,
-                 "These should be equal whenever we exit loop and childAcc != nullptr");
-
-    if (childIdx < childCount - 1 ||
-        addTextOffset < nsAccUtils::TextLength(childAccAtOffset)) {
-      // If not at end of last text node, we will return the accessible we were in
-      return childAccAtOffset;
-    }
-  }
-
-  return nullptr;
+  // If the given DOM point cannot be mapped into offset relative this hypertext
+  // offset then return length as fallback value.
+  return childAtOffset ?
+    GetChildOffset(childAtOffset) + addTextOffset : CharacterCount();
 }
 
 bool
@@ -490,17 +458,14 @@ HyperTextAccessible::FindOffset(int32_t aOffset, nsDirection aDirection,
   if (!pos.mResultContent)
     return -1;
 
-  // Turn the resulting node and offset into a hyperTextOffset
-  // If finalAccessible is nullptr, then DOMPointToHypertextOffset() searched
-  // through the hypertext children without finding the node/offset position.
-  int32_t hyperTextOffset = 0;
-  Accessible* finalAccessible =
-    DOMPointToHypertextOffset(pos.mResultContent, pos.mContentOffset,
-                              &hyperTextOffset, aDirection == eDirNext);
+  // Turn the resulting DOM point into an offset.
+  int32_t hyperTextOffset = DOMPointToOffset(pos.mResultContent,
+                                             pos.mContentOffset,
+                                             aDirection == eDirNext);
 
   // If we reached the end during search, this means we didn't find the DOM point
   // and we're actually at the start of the paragraph
-  if (!finalAccessible && aDirection == eDirPrevious)
+  if (hyperTextOffset == CharacterCount() && aDirection == eDirPrevious)
     return 0;
 
   return hyperTextOffset;
@@ -1173,9 +1138,7 @@ HyperTextAccessible::CaretOffset() const
       return -1;
   }
 
-  int32_t caretOffset = -1;
-  DOMPointToHypertextOffset(focusNode, focusOffset, &caretOffset);
-  return caretOffset;
+  return DOMPointToOffset(focusNode, focusOffset);
 }
 
 int32_t
@@ -1384,13 +1347,8 @@ HyperTextAccessible::SelectionBoundsAt(int32_t aSelectionNum,
     endOffset = tempOffset;
   }
 
-  Accessible* startAccessible =
-    DOMPointToHypertextOffset(startNode, startOffset, aStartOffset);
-  if (!startAccessible) {
-    *aStartOffset = 0; // Could not find start point within this hypertext, so starts before
-  }
-
-  DOMPointToHypertextOffset(endNode, endOffset, aEndOffset, true);
+  *aStartOffset = DOMPointToOffset(startNode, startOffset);
+  *aEndOffset = DOMPointToOffset(endNode, endOffset, true);
   return true;
 }
 
@@ -1781,7 +1739,7 @@ nsresult
 HyperTextAccessible::RangeBoundToHypertextOffset(nsRange* aRange,
                                                  bool aIsStartBound,
                                                  bool aIsStartHTOffset,
-                                                 int32_t* aHTOffset)
+                                                 int32_t* aOffset)
 {
   nsINode* node = nullptr;
   int32_t nodeOffset = 0;
@@ -1794,12 +1752,7 @@ HyperTextAccessible::RangeBoundToHypertextOffset(nsRange* aRange,
     nodeOffset = aRange->EndOffset();
   }
 
-  Accessible* startAcc =
-    DOMPointToHypertextOffset(node, nodeOffset, aHTOffset);
-
-  if (aIsStartHTOffset && !startAcc)
-    *aHTOffset = 0;
-
+  *aOffset = DOMPointToOffset(node, nodeOffset);
   return NS_OK;
 }
 

accessible/src/generic/HyperTextAccessible.h

@@ -99,34 +99,26 @@ public:
   // HyperTextAccessible: DOM point to text offset conversions.
 
   /**
-    * Turn a DOM Node and offset into a character offset into this hypertext.
-    * Will look for closest match when the DOM node does not have an accessible
-    * object associated with it. Will return an offset for the end of
-    * the string if the node is not found.
-    *
-    * @param aNode - the node to look for
-    * @param aNodeOffset - the offset to look for
-    *                      if -1 just look directly for the node
-    *                      if >=0 and aNode is text, this represents a char offset
-    *                      if >=0 and aNode is not text, this represents a child node offset
-    * @param aResultOffset - the character offset into the current
-    *                        HyperTextAccessible
-    * @param aIsEndOffset - if true, then then this offset is not inclusive. The character
-    *                       indicated by the offset returned is at [offset - 1]. This means
-    *                       if the passed-in offset is really in a descendant, then the offset returned
-    *                       will come just after the relevant embedded object characer.
-    *                       If false, then the offset is inclusive. The character indicated
-    *                       by the offset returned is at [offset]. If the passed-in offset in inside a
-    *                       descendant, then the returned offset will be on the relevant embedded object char.
-    *
-    * @return               the accessible child which contained the offset, if
-    *                       it is within the current HyperTextAccessible,
-    *                       otherwise nullptr
-    */
-  Accessible* DOMPointToHypertextOffset(nsINode *aNode,
-                                        int32_t aNodeOffset,
-                                        int32_t* aHypertextOffset,
-                                        bool aIsEndOffset = false) const;
+   * Turn a DOM point (node and offset) into a character offset of this
+   * hypertext. Will look for closest match when the DOM node does not have
+   * an accessible object associated with it. Will return an offset for the end
+   * of the string if the node is not found.
+   *
+   * @param aNode         [in] the node to look for
+   * @param aNodeOffset   [in] the offset to look for
+   *                       if -1 just look directly for the node
+   *                       if >=0 and aNode is text, this represents a char offset
+   *                       if >=0 and aNode is not text, this represents a child node offset
+   * @param aIsEndOffset  [in] if true, then then this offset is not inclusive. The character
+   *                       indicated by the offset returned is at [offset - 1]. This means
+   *                       if the passed-in offset is really in a descendant, then the offset returned
+   *                       will come just after the relevant embedded object characer.
+   *                       If false, then the offset is inclusive. The character indicated
+   *                       by the offset returned is at [offset]. If the passed-in offset in inside a
+   *                       descendant, then the returned offset will be on the relevant embedded object char.
+   */
+  int32_t DOMPointToOffset(nsINode* aNode, int32_t aNodeOffset,
+                           bool aIsEndOffset = false) const;
 
   /**
    * Convert start and end hypertext offsets into DOM range.
@@ -480,7 +472,7 @@ protected:
    * Return hyper text offset for the specified bound of the given DOM range.
    * If the bound is outside of the hyper text then offset value is either
    * 0 or number of characters of hyper text, it depends on type of requested
-   * offset. The method is a wrapper for DOMPointToHypertextOffset.
+   * offset. The method is a wrapper for DOMPointToOffset.
    *
    * @param aRange          [in] the given range
    * @param aIsStartBound   [in] specifies whether the required range bound is

accessible/tests/mochitest/pivot.js

@@ -265,7 +265,9 @@ function setVCTextInvoker(aDocAcc, aPivotMoveMethod, aBoundary, aTextOffsets, aI
 
   this.getID = function setVCPosInvoker_getID()
   {
-    return "Do " + (expectMove ? "" : "no-op ") + aPivotMoveMethod;
+    return "Do " + (expectMove ? "" : "no-op ") + aPivotMoveMethod + " in " +
+      prettyName(aIdOrNameOrAcc) + ", " + boundaryToString(aBoundary) +
+      ", [" + aTextOffsets + "]";
   };
 
   if (expectMove) {

accessible/tests/mochitest/pivot/test_virtualcursor_text.html

@@ -13,6 +13,7 @@
   </script>
 
   <script type="application/javascript" src="../common.js"></script>
+  <script type="application/javascript" src="../text.js"></script>
   <script type="application/javascript" src="../browser.js"></script>
   <script type="application/javascript" src="../events.js"></script>
   <script type="application/javascript" src="../role.js"></script>