Bug 961737 - DOMPointToOffset cleaning up, r=tbsaunde
authorAlexander Surkov <surkov.alexander@gmail.com>
Mon, 03 Feb 2014 19:56:45 -0500
changeset 166696 a70d1fe8d5100f5d45014c70365e557965ed9f2d
parent 166695 e0db5f09d041b521822c833061d7500e35e0a3f9
child 166697 f3d031c9648c06854af5b9731e7e2eab62a23050
push id26148
push userryanvm@gmail.com
push dateTue, 04 Feb 2014 19:23:26 +0000
treeherdermozilla-central@2e5880940469 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstbsaunde
bugs961737
milestone30.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 961737 - DOMPointToOffset cleaning up, r=tbsaunde
accessible/src/generic/HyperTextAccessible.cpp
accessible/src/generic/HyperTextAccessible.h
accessible/tests/mochitest/pivot.js
accessible/tests/mochitest/pivot/test_virtualcursor_text.html
--- a/accessible/src/generic/HyperTextAccessible.cpp
+++ b/accessible/src/generic/HyperTextAccessible.cpp
@@ -226,43 +226,38 @@ HyperTextAccessible::TextSubstring(int32
   int32_t endChildOffset =  GetChildOffset(endChildIdx);
   if (endChildOffset == -1)
     return;
 
   Accessible* endChild = GetChildAt(endChildIdx);
   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;
 
   if (aNodeOffset == -1) {
     findNode = aNode;
 
   } else if (aNode->IsNodeOfType(nsINode::eTEXT)) {
     // 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;
 
   } else {
     // findNode could be null if aNodeOffset == # of child nodes, which means
     // one of two things:
     // 1) there are no children, and the passed-in node is not mContent -- use
     //    parentContent for the node to find
@@ -271,18 +266,17 @@ HyperTextAccessible::DOMPointToHypertext
     // 3) there are children and we're at the end of the children
 
     findNode = aNode->GetChildAt(aNodeOffset);
     if (!findNode) {
       if (aNodeOffset == 0) {
         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.
         findNode = aNode;
       } else if (aNodeOffset == aNode->GetChildCount()) {
         // Case #3: we're after the last child, get next node to this one.
         for (nsINode* tmpNode = aNode;
              !findNode && tmpNode && tmpNode != mContent;
@@ -290,85 +284,59 @@ HyperTextAccessible::DOMPointToHypertext
           findNode = tmpNode->GetNextSibling();
         }
       }
     }
   }
 
   // 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() &&
         findContent->NodeInfo()->Equals(nsGkAtoms::br) &&
         findContent->AttrValueIs(kNameSpaceID_None,
                                  nsGkAtoms::mozeditorbogusnode,
                                  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;
     }
 
     // This offset no longer applies because the passed-in text object is not
     // a child of the hypertext. This happens when there are nested hypertexts,
     // e.g. <div>abc<h1>def</h1>ghi</div>. Thus we need to adjust the offset
     // to make it relative the hypertext.
     // If the end offset is not supposed to be inclusive and the original point
     // 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
 HyperTextAccessible::OffsetsToDOMRange(int32_t aStartOffset, int32_t aEndOffset,
                                        nsRange* aRange)
 {
   DOMPoint startPoint = OffsetToDOMPoint(aStartOffset);
   if (!startPoint.node)
@@ -485,27 +453,24 @@ HyperTextAccessible::FindOffset(int32_t 
   // PeekOffset fails on last/first lines of the text in certain cases.
   if (NS_FAILED(rv) && aAmount == eSelectLine) {
     pos.mAmount = (aDirection == eDirNext) ? eSelectEndLine : eSelectBeginLine;
     frameAtOffset->PeekOffset(&pos);
   }
   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;
 }
 
 int32_t
 HyperTextAccessible::FindLineBoundary(int32_t aOffset,
                                       EWhichLineBoundary aWhichLineBoundary)
@@ -1168,19 +1133,17 @@ HyperTextAccessible::CaretOffset() const
       nsCoreUtils::GetDOMNodeFromDOMPoint(focusNode, focusOffset);
 
     nsINode* thisNode = GetNode();
     if (resultNode != thisNode &&
         !nsCoreUtils::IsAncestorOf(thisNode, resultNode))
       return -1;
   }
 
-  int32_t caretOffset = -1;
-  DOMPointToHypertextOffset(focusNode, focusOffset, &caretOffset);
-  return caretOffset;
+  return DOMPointToOffset(focusNode, focusOffset);
 }
 
 int32_t
 HyperTextAccessible::CaretLineNumber()
 {
   // Provide the line number for the caret, relative to the
   // currently focused node. Use a 1-based index
   nsRefPtr<nsFrameSelection> frameSelection = FrameSelection();
@@ -1379,23 +1342,18 @@ HyperTextAccessible::SelectionBoundsAt(i
     nsINode* tempNode = startNode;
     startNode = endNode;
     endNode = tempNode;
     int32_t tempOffset = startOffset;
     startOffset = endOffset;
     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;
 }
 
 bool
 HyperTextAccessible::SetSelectionBoundsAt(int32_t aSelectionNum,
                                           int32_t aStartOffset,
                                           int32_t aEndOffset)
 {
@@ -1776,35 +1734,30 @@ HyperTextAccessible::GetDOMPointByFrameO
   return NS_OK;
 }
 
 // HyperTextAccessible
 nsresult
 HyperTextAccessible::RangeBoundToHypertextOffset(nsRange* aRange,
                                                  bool aIsStartBound,
                                                  bool aIsStartHTOffset,
-                                                 int32_t* aHTOffset)
+                                                 int32_t* aOffset)
 {
   nsINode* node = nullptr;
   int32_t nodeOffset = 0;
 
   if (aIsStartBound) {
     node = aRange->GetStartParent();
     nodeOffset = aRange->StartOffset();
   } else {
     node = aRange->GetEndParent();
     nodeOffset = aRange->EndOffset();
   }
 
-  Accessible* startAcc =
-    DOMPointToHypertextOffset(node, nodeOffset, aHTOffset);
-
-  if (aIsStartHTOffset && !startAcc)
-    *aHTOffset = 0;
-
+  *aOffset = DOMPointToOffset(node, nodeOffset);
   return NS_OK;
 }
 
 // HyperTextAccessible
 nsresult
 HyperTextAccessible::GetSpellTextAttribute(nsINode* aNode,
                                            int32_t aNodeOffset,
                                            int32_t* aHTStartOffset,
--- a/accessible/src/generic/HyperTextAccessible.h
+++ b/accessible/src/generic/HyperTextAccessible.h
@@ -94,44 +94,36 @@ public:
     Accessible* child = GetChildAtOffset(aOffset);
     return child ? LinkIndexOf(child) : -1;
   }
 
   //////////////////////////////////////////////////////////////////////////////
   // 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.
    *
    * @param  aStartOffset  [in] the given start hypertext offset
    * @param  aEndOffset    [in] the given end hypertext offset
    * @param  aRange        [in, out] the range whose bounds to set
    * @return true   if conversion was successful
@@ -475,17 +467,17 @@ protected:
                                     Accessible* aAccessible,
                                     mozilla::a11y::DOMPoint* aPoint);
 
 
   /**
    * 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
    *                        start bound
    * @param aIsStartOffset  [in] the offset type, used when the range bound is
    *                        outside of hyper text
    * @param aHTOffset       [out] the result offset
    */
--- a/accessible/tests/mochitest/pivot.js
+++ b/accessible/tests/mochitest/pivot.js
@@ -260,17 +260,19 @@ function setVCTextInvoker(aDocAcc, aPivo
     var moved = aDocAcc.virtualCursor[aPivotMoveMethod](aBoundary);
     SimpleTest.is(!!moved, !!expectMove,
                   "moved pivot by text with " + aPivotMoveMethod +
                   " to " + aIdOrNameOrAcc);
   };
 
   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) {
     this.eventSeq = [
       new VCChangedChecker(aDocAcc, aIdOrNameOrAcc, aTextOffsets, aPivotMoveMethod)
     ];
   } else {
     this.eventSeq = [];
--- a/accessible/tests/mochitest/pivot/test_virtualcursor_text.html
+++ b/accessible/tests/mochitest/pivot/test_virtualcursor_text.html
@@ -8,16 +8,17 @@
   <script type="application/javascript"
           src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js">
   </script>
   <script type="application/javascript"
           src="chrome://mochikit/content/chrome-harness.js">
   </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>
   <script type="application/javascript" src="../states.js"></script>
   <script type="application/javascript" src="../pivot.js"></script>
   <script type="application/javascript" src="../layout.js"></script>
 
   <script type="application/javascript">