Bug 162081 Wrong letter is underlined as accesskey / mnemonic when widget direction is RTL

p=mozilla-bugzilla@future.shiny.co.il r/sr=bzbarsky

layout/base/nsBidiPresUtils.cpp

@@ -1036,14 +1036,18 @@ nsresult nsBidiPresUtils::GetBidiEngine(nsBidi** aBidiEngine)
   return rv; 
 }
 
-nsresult nsBidiPresUtils::RenderText(PRUnichar*           aText,
+nsresult nsBidiPresUtils::RenderText(const PRUnichar*     aText,
                                      PRInt32              aLength,
                                      nsBidiDirection      aBaseDirection,
                                      nsPresContext*      aPresContext,
                                      nsIRenderingContext& aRenderingContext,
                                      nscoord              aX,
-                                     nscoord              aY)
+                                     nscoord              aY,
+                                     nsBidiPositionResolve* aPosResolve,
+                                     PRInt32              aPosResolveCount)
 {
+  NS_ASSERTION((aPosResolve == nsnull) != (aPosResolveCount > 0), "Incorrect aPosResolve / aPosResolveCount arguments");
+
   PRInt32 runCount;
 
   mBuffer.Assign(aText);
@@ -1056,9 +1060,10 @@ nsresult nsBidiPresUtils::RenderText(PRUnichar*           aText,
   if (NS_FAILED(rv))
     return rv;
 
-  nscoord width, xEndRun;
+  nscoord width, xEndRun, xStartText = aX;
   PRBool isRTL = PR_FALSE;
   PRInt32 i, start, limit, length;
+  PRUint32 visualStart = 0;
   PRUint8 charType;
   PRUint8 prevType = eCharType_LeftToRight;
   nsBidiLevel level;
@@ -1066,6 +1071,12 @@ nsresult nsBidiPresUtils::RenderText(PRUnichar*           aText,
   PRUint32 hints = 0;
   aRenderingContext.GetHints(hints);
   PRBool isBidiSystem = (hints & NS_RENDERING_HINT_BIDI_REORDERING);
+      
+  for(int nPosResolve=0; nPosResolve < aPosResolveCount; ++nPosResolve)
+  {
+    aPosResolve[nPosResolve].visualIndex = kNotFound;
+    aPosResolve[nPosResolve].visualLeftTwips = kNotFound;
+  }
 
   for (i = 0; i < runCount; i++) {
     rv = mBidiEngine->GetVisualRun(i, &start, &length, &aBaseDirection);
@@ -1112,15 +1123,80 @@ nsresult nsBidiPresUtils::RenderText(PRUnichar*           aText,
         isRTL = !isRTL;
         aRenderingContext.SetRightToLeftText(isRTL);
       }
-      FormatUnicodeText(aPresContext, aText + start, subRunLength,
+      
+      nsAutoString runVisualText;
+      runVisualText.Assign(aText + start, subRunLength);
+      FormatUnicodeText(aPresContext, runVisualText.BeginWriting(), subRunLength,
                         (nsCharType)charType, level & 1,
                         isBidiSystem);
 
-      aRenderingContext.GetWidth(aText + start, subRunLength, width, nsnull);
+      aRenderingContext.GetWidth(runVisualText.get(), subRunLength, width, nsnull);
       if (level & 1) {
         aX -= width;
       }
-      aRenderingContext.DrawString(aText + start, subRunLength, aX, aY, width);
+      aRenderingContext.DrawString(runVisualText.get(), subRunLength, aX, aY, width);
+
+      /*
+       * The caller may request to calculate the visual position of one
+       * or more characters.
+       */
+      for(int nPosResolve=0; nPosResolve<aPosResolveCount; ++nPosResolve)
+      {
+        nsBidiPositionResolve* posResolve = &aPosResolve[nPosResolve];
+        /*
+         * Did we already resolve this position's visual metric? If so, skip.
+         */
+        if (posResolve->visualLeftTwips != kNotFound)
+           continue;
+           
+        /*
+         * First find out if the logical position is within this run.
+         */
+        if (start <= posResolve->logicalIndex &&
+            start + subRunLength > posResolve->logicalIndex) {
+          /*
+           * If this run is only one character long, we have an easy case:
+           * the visual position is the x-coord of the start of the run
+           * less the x-coord of the start of the whole text (saved in xStartText).
+           */
+          if (subRunLength == 1) {
+            posResolve->visualIndex = visualStart;
+            posResolve->visualLeftTwips = aX - xStartText;
+          }
+          /*
+           * Otherwise, we need to measure the width of the run's part
+           * which is to the visual left of the index.
+           * In other words, the run is broken in two, around the logical index,
+           * and we measure the part which is visually left.
+           * If the run is right-to-left, this part will span from after the index
+           * up to the end of the run; if it is left-to-right, this part will span
+           * from the start of the run up to (and inclduing) the character before the index.
+           */
+          else {
+            nscoord subWidth;
+            // The position in the text where this run's "left part" begins.
+            const PRUnichar* visualLeftPart;
+            if (level & 1) {
+              // One day, son, this could all be replaced with mBidiEngine.GetVisualIndex ...
+              posResolve->visualIndex = visualStart + (subRunLength - (posResolve->logicalIndex + 1 - start));
+              // Skipping to the "left part".
+              visualLeftPart = aText + posResolve->logicalIndex + 1;
+            }
+            else {
+              posResolve->visualIndex = visualStart + (posResolve->logicalIndex - start);
+              // Skipping to the "left part".
+              visualLeftPart = aText + start;
+            }
+            // The delta between the start of the run and the left part's end.
+            PRInt32 visualLeftLength = posResolve->visualIndex - visualStart;
+            aRenderingContext.GetWidth(visualLeftPart,
+                                       visualLeftLength,
+                                       subWidth, nsnull);
+            posResolve->visualLeftTwips = aX + subWidth - xStartText;
+          }
+        }
+      }
+
       if (!(level & 1)) {
         aX += width;
       }
@@ -1133,6 +1209,8 @@ nsresult nsBidiPresUtils::RenderText(PRUnichar*           aText,
     if (level & 1) {
       aX = xEndRun;
     }
+    
+    visualStart += length;
   } // for
 
   // Restore original reading order

layout/base/nsBidiPresUtils.h

@@ -49,6 +49,23 @@
 #include "nsCOMPtr.h"
 #include "nsDataHashtable.h"
 
+/**
+ * A structure representing a logical position which should be resolved
+ * into its visual position during BiDi processing.
+ */
+struct nsBidiPositionResolve
+{
+  // [in] Logical index within string.
+  PRInt32 logicalIndex;
+  // [out] Visual index within string.
+  // If the logical position was not found, set to kNotFound.
+  PRInt32 visualIndex;
+  // [out] Visual position of the character, from the left (on the X axis), in twips.
+  // Eessentially, this is the X position (relative to the rendering context) where the text was drawn + the font metric of the visual string to the left of the given logical position.
+  // If the logical position was not found, set to kNotFound.
+  PRInt32 visualLeftTwips;
+};
+
 class nsBidiPresUtils {
 public:
   nsBidiPresUtils();
@@ -122,22 +139,27 @@ public:
    * Reorder plain text using the Unicode Bidi algorithm and send it to
    * a rendering context for rendering.
    *
-   * @param aText the string to be rendered
+   * @param[in] aText  the string to be rendered (in logical order)
    * @param aLength the number of characters in the string
    * @param aBaseDirection the base direction of the string
    *  NSBIDI_LTR - left-to-right string
    *  NSBIDI_RTL - right-to-left string
    * @param aPresContext the presentation context
    * @param aRenderingContext the rendering context
-   * @param aTextRect contains the coordinates to render the string
+   * @param aX the x-coordinate to render the string
+   * @param aY the y-coordinate to render the string
+   * @param[in,out] aPosResolve array of logical positions to resolve into visual positions; can be nsnull if this functionality is not required
+   * @param aPosResolveCount number of items in the aPosResolve array
    */
-  nsresult RenderText(PRUnichar*           aText,
+  nsresult RenderText(const PRUnichar*     aText,
                       PRInt32              aLength,
                       nsBidiDirection      aBaseDirection,
                       nsPresContext*      aPresContext,
                       nsIRenderingContext& aRenderingContext,
                       nscoord              aX,
-                      nscoord              aY);
+                      nscoord              aY,
+                      nsBidiPositionResolve* aPosResolve = nsnull,
+                      PRInt32              aPosResolveCount = 0);
 
 private:
   /**

layout/base/public/nsBidiPresUtils.h

@@ -49,6 +49,23 @@
 #include "nsCOMPtr.h"
 #include "nsDataHashtable.h"
 
+/**
+ * A structure representing a logical position which should be resolved
+ * into its visual position during BiDi processing.
+ */
+struct nsBidiPositionResolve
+{
+  // [in] Logical index within string.
+  PRInt32 logicalIndex;
+  // [out] Visual index within string.
+  // If the logical position was not found, set to kNotFound.
+  PRInt32 visualIndex;
+  // [out] Visual position of the character, from the left (on the X axis), in twips.
+  // Eessentially, this is the X position (relative to the rendering context) where the text was drawn + the font metric of the visual string to the left of the given logical position.
+  // If the logical position was not found, set to kNotFound.
+  PRInt32 visualLeftTwips;
+};
+
 class nsBidiPresUtils {
 public:
   nsBidiPresUtils();
@@ -122,22 +139,27 @@ public:
    * Reorder plain text using the Unicode Bidi algorithm and send it to
    * a rendering context for rendering.
    *
-   * @param aText the string to be rendered
+   * @param[in] aText  the string to be rendered (in logical order)
    * @param aLength the number of characters in the string
    * @param aBaseDirection the base direction of the string
    *  NSBIDI_LTR - left-to-right string
    *  NSBIDI_RTL - right-to-left string
    * @param aPresContext the presentation context
    * @param aRenderingContext the rendering context
-   * @param aTextRect contains the coordinates to render the string
+   * @param aX the x-coordinate to render the string
+   * @param aY the y-coordinate to render the string
+   * @param[in,out] aPosResolve array of logical positions to resolve into visual positions; can be nsnull if this functionality is not required
+   * @param aPosResolveCount number of items in the aPosResolve array
    */
-  nsresult RenderText(PRUnichar*           aText,
+  nsresult RenderText(const PRUnichar*     aText,
                       PRInt32              aLength,
                       nsBidiDirection      aBaseDirection,
                       nsPresContext*      aPresContext,
                       nsIRenderingContext& aRenderingContext,
                       nscoord              aX,
-                      nscoord              aY);
+                      nscoord              aY,
+                      nsBidiPositionResolve* aPosResolve = nsnull,
+                      PRInt32              aPosResolveCount = 0);
 
 private:
   /**

layout/base/src/nsBidiPresUtils.cpp

@@ -1036,14 +1036,18 @@ nsresult nsBidiPresUtils::GetBidiEngine(nsBidi** aBidiEngine)
   return rv; 
 }
 
-nsresult nsBidiPresUtils::RenderText(PRUnichar*           aText,
+nsresult nsBidiPresUtils::RenderText(const PRUnichar*     aText,
                                      PRInt32              aLength,
                                      nsBidiDirection      aBaseDirection,
                                      nsPresContext*      aPresContext,
                                      nsIRenderingContext& aRenderingContext,
                                      nscoord              aX,
-                                     nscoord              aY)
+                                     nscoord              aY,
+                                     nsBidiPositionResolve* aPosResolve,
+                                     PRInt32              aPosResolveCount)
 {
+  NS_ASSERTION((aPosResolve == nsnull) != (aPosResolveCount > 0), "Incorrect aPosResolve / aPosResolveCount arguments");
+
   PRInt32 runCount;
 
   mBuffer.Assign(aText);
@@ -1056,9 +1060,10 @@ nsresult nsBidiPresUtils::RenderText(PRUnichar*           aText,
   if (NS_FAILED(rv))
     return rv;
 
-  nscoord width, xEndRun;
+  nscoord width, xEndRun, xStartText = aX;
   PRBool isRTL = PR_FALSE;
   PRInt32 i, start, limit, length;
+  PRUint32 visualStart = 0;
   PRUint8 charType;
   PRUint8 prevType = eCharType_LeftToRight;
   nsBidiLevel level;
@@ -1066,6 +1071,12 @@ nsresult nsBidiPresUtils::RenderText(PRUnichar*           aText,
   PRUint32 hints = 0;
   aRenderingContext.GetHints(hints);
   PRBool isBidiSystem = (hints & NS_RENDERING_HINT_BIDI_REORDERING);
+      
+  for(int nPosResolve=0; nPosResolve < aPosResolveCount; ++nPosResolve)
+  {
+    aPosResolve[nPosResolve].visualIndex = kNotFound;
+    aPosResolve[nPosResolve].visualLeftTwips = kNotFound;
+  }
 
   for (i = 0; i < runCount; i++) {
     rv = mBidiEngine->GetVisualRun(i, &start, &length, &aBaseDirection);
@@ -1112,15 +1123,80 @@ nsresult nsBidiPresUtils::RenderText(PRUnichar*           aText,
         isRTL = !isRTL;
         aRenderingContext.SetRightToLeftText(isRTL);
       }
-      FormatUnicodeText(aPresContext, aText + start, subRunLength,
+      
+      nsAutoString runVisualText;
+      runVisualText.Assign(aText + start, subRunLength);
+      FormatUnicodeText(aPresContext, runVisualText.BeginWriting(), subRunLength,
                         (nsCharType)charType, level & 1,
                         isBidiSystem);
 
-      aRenderingContext.GetWidth(aText + start, subRunLength, width, nsnull);
+      aRenderingContext.GetWidth(runVisualText.get(), subRunLength, width, nsnull);
       if (level & 1) {
         aX -= width;
       }
-      aRenderingContext.DrawString(aText + start, subRunLength, aX, aY, width);
+      aRenderingContext.DrawString(runVisualText.get(), subRunLength, aX, aY, width);
+
+      /*
+       * The caller may request to calculate the visual position of one
+       * or more characters.
+       */
+      for(int nPosResolve=0; nPosResolve<aPosResolveCount; ++nPosResolve)
+      {
+        nsBidiPositionResolve* posResolve = &aPosResolve[nPosResolve];
+        /*
+         * Did we already resolve this position's visual metric? If so, skip.
+         */
+        if (posResolve->visualLeftTwips != kNotFound)
+           continue;
+           
+        /*
+         * First find out if the logical position is within this run.
+         */
+        if (start <= posResolve->logicalIndex &&
+            start + subRunLength > posResolve->logicalIndex) {
+          /*
+           * If this run is only one character long, we have an easy case:
+           * the visual position is the x-coord of the start of the run
+           * less the x-coord of the start of the whole text (saved in xStartText).
+           */
+          if (subRunLength == 1) {
+            posResolve->visualIndex = visualStart;
+            posResolve->visualLeftTwips = aX - xStartText;
+          }
+          /*
+           * Otherwise, we need to measure the width of the run's part
+           * which is to the visual left of the index.
+           * In other words, the run is broken in two, around the logical index,
+           * and we measure the part which is visually left.
+           * If the run is right-to-left, this part will span from after the index
+           * up to the end of the run; if it is left-to-right, this part will span
+           * from the start of the run up to (and inclduing) the character before the index.
+           */
+          else {
+            nscoord subWidth;
+            // The position in the text where this run's "left part" begins.
+            const PRUnichar* visualLeftPart;
+            if (level & 1) {
+              // One day, son, this could all be replaced with mBidiEngine.GetVisualIndex ...
+              posResolve->visualIndex = visualStart + (subRunLength - (posResolve->logicalIndex + 1 - start));
+              // Skipping to the "left part".
+              visualLeftPart = aText + posResolve->logicalIndex + 1;
+            }
+            else {
+              posResolve->visualIndex = visualStart + (posResolve->logicalIndex - start);
+              // Skipping to the "left part".
+              visualLeftPart = aText + start;
+            }
+            // The delta between the start of the run and the left part's end.
+            PRInt32 visualLeftLength = posResolve->visualIndex - visualStart;
+            aRenderingContext.GetWidth(visualLeftPart,
+                                       visualLeftLength,
+                                       subWidth, nsnull);
+            posResolve->visualLeftTwips = aX + subWidth - xStartText;
+          }
+        }
+      }
+
       if (!(level & 1)) {
         aX += width;
       }
@@ -1133,6 +1209,8 @@ nsresult nsBidiPresUtils::RenderText(PRUnichar*           aText,
     if (level & 1) {
       aX = xEndRun;
     }
+    
+    visualStart += length;
   } // for
 
   // Restore original reading order

layout/generic/nsPageFrame.cpp

@@ -540,10 +540,9 @@ nsPageFrame::DrawHeaderFooter(nsPresContext*      aPresContext,
       nsBidiPresUtils* bidiUtils = aPresContext->GetBidiUtils();
       
       if (bidiUtils) {
-        PRUnichar* buffer = str.BeginWriting();
         // Base direction is always LTR for now. If bug 139337 is fixed, 
         // that should change.
-        rv = bidiUtils->RenderText(buffer, str.Length(), NSBIDI_LTR,
+        rv = bidiUtils->RenderText(str.get(), str.Length(), NSBIDI_LTR,
                                    aPresContext, aRenderingContext,
                                    x, y + aAscent);
       }

layout/html/base/src/nsPageFrame.cpp

@@ -540,10 +540,9 @@ nsPageFrame::DrawHeaderFooter(nsPresContext*      aPresContext,
       nsBidiPresUtils* bidiUtils = aPresContext->GetBidiUtils();
       
       if (bidiUtils) {
-        PRUnichar* buffer = str.BeginWriting();
         // Base direction is always LTR for now. If bug 139337 is fixed, 
         // that should change.
-        rv = bidiUtils->RenderText(buffer, str.Length(), NSBIDI_LTR,
+        rv = bidiUtils->RenderText(str.get(), str.Length(), NSBIDI_LTR,
                                    aPresContext, aRenderingContext,
                                    x, y + aAscent);
       }

layout/xul/base/src/nsTextBoxFrame.cpp

@@ -418,22 +418,44 @@ nsTextBoxFrame::PaintTitle(nsPresContext*      aPresContext,
       nsBidiPresUtils* bidiUtils = aPresContext->GetBidiUtils();
 
       if (bidiUtils) {
-        PRUnichar* buffer = ToNewUnicode(mCroppedTitle);
-        if (buffer) {
-          const nsStyleVisibility* vis = GetStyleVisibility();
-          nsBidiDirection direction =
-                                    (NS_STYLE_DIRECTION_RTL == vis->mDirection)
-                                    ? NSBIDI_RTL : NSBIDI_LTR;
-          rv = bidiUtils->RenderText(buffer, mCroppedTitle.Length(), direction,
-                                     aPresContext, aRenderingContext,
-                                     textRect.x, textRect.y + baseline);
-          nsMemory::Free(buffer);
+        const nsStyleVisibility* vis = GetStyleVisibility();
+        nsBidiDirection direction = (NS_STYLE_DIRECTION_RTL == vis->mDirection) ? NSBIDI_RTL : NSBIDI_LTR;
+        if (mAccessKeyInfo && mAccessKeyInfo->mAccesskeyIndex != kNotFound) {
+           // We let the RenderText function calculate the mnemonic's
+           // underline position for us.
+           nsBidiPositionResolve posResolve;
+           posResolve.logicalIndex = mAccessKeyInfo->mAccesskeyIndex;
+           rv = bidiUtils->RenderText(mCroppedTitle.get(), mCroppedTitle.Length(), direction,
+                                      aPresContext, aRenderingContext,
+                                      textRect.x, textRect.y + baseline,
+                                      &posResolve,
+                                      1);
+           mAccessKeyInfo->mBeforeWidth = posResolve.visualLeftTwips;
+        }
+        else
+        {
+           rv = bidiUtils->RenderText(mCroppedTitle.get(), mCroppedTitle.Length(), direction,
+                                      aPresContext, aRenderingContext,
+                                      textRect.x, textRect.y + baseline);
         }
       }
     }
     if (NS_FAILED(rv) )
 #endif // IBMBIDI
-    aRenderingContext.DrawString(mCroppedTitle, textRect.x, textRect.y + baseline);
+    {
+       if (mAccessKeyInfo && mAccessKeyInfo->mAccesskeyIndex != kNotFound) {
+           // In the simple (non-BiDi) case, we calculate the mnemonic's
+           // underline position by getting the text metric.
+           // XXX are attribute values always two byte?
+           if (mAccessKeyInfo->mAccesskeyIndex > 0)
+               aRenderingContext.GetWidth(mCroppedTitle.get(), mAccessKeyInfo->mAccesskeyIndex,
+                                          mAccessKeyInfo->mBeforeWidth);
+           else
+               mAccessKeyInfo->mBeforeWidth = 0;
+       }
+
+       aRenderingContext.DrawString(mCroppedTitle, textRect.x, textRect.y + baseline);
+    }
 
     if (mAccessKeyInfo && mAccessKeyInfo->mAccesskeyIndex != kNotFound) {
         aRenderingContext.FillRect(textRect.x + mAccessKeyInfo->mBeforeWidth,
@@ -468,17 +490,9 @@ void
 nsTextBoxFrame::CalculateUnderline(nsIRenderingContext& aRenderingContext)
 {
     if (mAccessKeyInfo && mAccessKeyInfo->mAccesskeyIndex != kNotFound) {
-         // get all the underline-positioning stuff
-
-         // XXX are attribute values always two byte?
-         const PRUnichar *titleString;
-         titleString = mCroppedTitle.get();
-         if (mAccessKeyInfo->mAccesskeyIndex > 0)
-             aRenderingContext.GetWidth(titleString, mAccessKeyInfo->mAccesskeyIndex,
-                                        mAccessKeyInfo->mBeforeWidth);
-         else
-             mAccessKeyInfo->mBeforeWidth = 0;
-
+         // Calculate all fields of mAccessKeyInfo which
+         // are the same for both BiDi and non-BiDi rames.
+         const PRUnichar *titleString = mCroppedTitle.get();
          aRenderingContext.GetWidth(titleString[mAccessKeyInfo->mAccesskeyIndex],
                                     mAccessKeyInfo->mAccessWidth);
 

layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp

@@ -2920,8 +2920,7 @@ nsTreeBodyFrame::PaintText(PRInt32              aRowIndex,
     nsBidiDirection direction =
       (NS_STYLE_DIRECTION_RTL == vis->mDirection) ?
       NSBIDI_RTL : NSBIDI_LTR;
-    PRUnichar* buffer = text.BeginWriting();
-    rv = bidiUtils->RenderText(buffer, text.Length(), direction,
+    rv = bidiUtils->RenderText(text.get(), text.Length(), direction,
                                aPresContext, aRenderingContext,
                                textRect.x, textRect.y + baseline);
   }