Check string and UI direction in GetDisplayStringInLTRDirectionality, etc.

base/i18n/rtl.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "base/i18n/rtl.h"
 
 #include "base/file_path.h"
 #include "base/logging.h"
 #include "base/string_util.h"
 #include "base/utf_string_conversions.h"
@@ skipping 135 matching lines @@
       return RIGHT_TO_LEFT;
     } else if ((property == U_LEFT_TO_RIGHT) ||
                (property == U_LEFT_TO_RIGHT_EMBEDDING) ||
                (property == U_LEFT_TO_RIGHT_OVERRIDE)) {
       return LEFT_TO_RIGHT;
     }
 
     position = next_position;
   }
 
   return LEFT_TO_RIGHT;

[msw, 2012/09/12 20:46:52]

FYI: This is what my tangential RenderTextWin TODO is about.
base::i18n::GetFirstStrongCharacterDirection() returns LTR if there are no
strong characters, but perhaps should return RTL if the UI is RTL? I'm happy to
address this in a separate CL if you have advice.

 }
 
 #if defined(OS_WIN)
 bool AdjustStringForLocaleDirection(string16* text) {
   if (!IsRTL() || text->empty())
     return false;
 
   // Marking the string as LTR if the locale is RTL and the string does not
   // contain strong RTL characters. Otherwise, mark the string as RTL.
   bool has_rtl_chars = StringContainsStrongRTLChars(*text);
@@ skipping 145 matching lines @@
     rtl_safe_path->append(path.value());
 #else  // defined(OS_POSIX) && !defined(OS_MACOSX)
     std::wstring wide_path = base::SysNativeMBToWide(path.value());
     rtl_safe_path->append(WideToUTF16(wide_path));
 #endif
   // Inserting a PDF (Pop Directional Formatting) mark as the last character.
   rtl_safe_path->push_back(kPopDirectionalFormatting);
 }
 
 string16 GetDisplayStringInLTRDirectionality(const string16& text) {
-  if (!IsRTL())
+  // Always wrap the string in RTL UI (it may be appended to RTL string).
+  // Also wrap strings with an RTL first strong character direction in LTR UI.
+  if (!IsRTL() && GetFirstStrongCharacterDirection(text) == LEFT_TO_RIGHT)

[xji, 2012/09/10 18:07:26]

why RTL locale is specially handled (always wrap in LRE/PDF)?

for rtl locale (in other cases), are we still displaying text in RTL
directionality or we are also using first-strong-character's directionality?

[msw, 2012/09/10 18:37:31]

I don't know about Mac/GTK, but Win and CrOs use first-strong-char's dir.
I left the check to not regress additional cases (perhaps in Mac/GTK); also:

In RTL UI, |text| is more likely to be concatenated w/RTL text: "OPENING file".
So we must also wrap "file" if it starts with weak directionality characters.
Otherwise, concatenating "[weak text][ltr text]" to "[RTL TEXT]" would yield:
Logical text: "[RTL TEXT][weak text][ltr text]"
Display text: "[ltr text][weak text][RTL TEXT]" < wrong, fixed by wrapping.

Perhaps we must wrap if the first char isn't strong LTR? Or always wrap?
(just in case LTR UI uses RTL strings somewhere like the example above?)

[xji, 2012/09/10 22:27:57]

In English windows (windows xp), if a user does not install the RTL language
package, LRE/PDF characters render as empty square. So, we do not want always
wrapping.

[msw, 2012/09/10 22:46:13]

Cool, this still avoids adding them in LTR for LTR text.
( I cc'ed myself on that issue http://crbug.com/99300 )
I guess those users see RTL text as empty squares anyway.

[jungshik at Google, 2012/09/12 20:24:35]

Can we match the comment and if-statement? At first, I was confused because the
comment and conditions in if are opposite of each other. 

if (IsRTL() || GetFirst...() == RIGHT_TO_LEFT) {
   string16 text_mutable...
   ....
   return text_mutable;
}

return text;

}

BTW, I wonder about TODO comment in RenderText (from
https://chromiumcodereview.appspot.com/10693061 )

// Use the first strong character direction as the base text direction.
 601   // TODO(msw): Use the application text direction instead of LTR by
default?
Actually, I believe Arial/Times New Roman in XP do have Hebrew (and possibly
Arabic) covered. 

[msw, 2012/09/12 20:46:52]

Done, flipped condition & blocks.
See my comment above about the TODO.

     return text;
   string16 text_mutable(text);
   WrapStringWithLTRFormatting(&text_mutable);
   return text_mutable;
 }
 
 string16 StripWrappingBidiControlCharacters(const string16& text) {
   if (text.empty())
     return text;
   size_t begin_index = 0;
   char16 begin = text[begin_index];
   if (begin == kLeftToRightEmbeddingMark ||
       begin == kRightToLeftEmbeddingMark ||
       begin == kLeftToRightOverride ||
       begin == kRightToLeftOverride)
     ++begin_index;
   size_t end_index = text.length() - 1;
   if (text[end_index] == kPopDirectionalFormatting)
     --end_index;
   return text.substr(begin_index, end_index - begin_index + 1);
 }
 
 }  // namespace i18n
 }  // namespace base