Add BiDirectional Text formatting utilites to the i18n library.

lib/i18n/bidi_formatter.dart

Index: lib/i18n/bidi_formatter.dart
===================================================================
--- lib/i18n/bidi_formatter.dart	(revision 0)
+++ lib/i18n/bidi_formatter.dart	(revision 0)
@@ -0,0 +1,222 @@
+// Copyright (c) 2012, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+#library('bidi_formatter');
+
+#import('bidi_utils.dart');
+
+/**
+ * Bidi stands for Bi-directional text.
+ * According to http://en.wikipedia.org/wiki/Bi-directional_text:
+ * Bi-directional text is text containing text in both text directionalities,
+ * both right-to-left (RTL) and left-to-right (LTR). It generally involves text
+ * containing different types of alphabets, but may also refer to boustrophedon,
+ * which is changing text directionality in each row.
+ * Go to https://sites.google.com/a/google.com/bidi-howto/ to learn more about

[Alan Knight, 2012/06/22 00:16:53]

This site isn't accessible outside Google. Shouldn't things listed as resources
here be publicly accessible?

[Alan Knight, 2012/06/25 23:29:56]

Do we do anything about this?

[Emily Fortuna, 2012/06/26 01:04:29]

You should look at the new version of these files that I uploaded with my last
email. I just took the url out. The URL is technically available to anyone on
the internet, they just don't all have permission to view it (you get a google
docs error).

+ * dealing with BiDi correctly.
+ *
+ * Utility class for formatting display text in a potentially
+ * opposite-directionality context without garbling layout issues.
+ * Mostly a very "slimmed-down" and dart-ified port of the Closure Birectional
+ * formatting libary. If there is a utility in the Closure library (or ICU, or
+ * elsewhere) that you would like this formatter to make available, please
+ * contact the Dart team.
+ *
+ * Provides the following functionality:
+ *
+ * 1. *BiDi Wrapping*
+ * When text in one language is mixed into a document in another, opposite-
+ * directionality language, e.g. when an English business name is embedded in a
+ * Hebrew web page, both the inserted string and the text following it may be
+ * displayed incorrectly unless the inserted string is explicitly separated
+ * from the surrounding text in a "wrapper" that declares its directionality at
+ * the start and then resets it back at the end. This wrapping can be done in
+ * HTML mark-up (e.g. a 'span dir=rtl' tag) or - only in contexts where mark-up
+ * can not be used - in Unicode BiDi formatting codes (LRE|RLE and PDF).
+ * Providing such wrapping services is the basic purpose of the BiDi formatter.
+ *
+ * 2. *Directionality estimation*
+ * How does one know whether a string about to be inserted into surrounding
+ * text has the same directionality? Well, in many cases, one knows that this
+ * must be the case when writing the code doing the insertion, e.g. when a
+ * localized message is inserted into a localized page. In such cases there is
+ * no need to involve the BiDi formatter at all. In the remaining cases, e.g.
+ * when the string is user-entered or comes from a database, the language of
+ * the string (and thus its directionality) is not known a priori, and must be
+ * estimated at run-time. The BiDi formatter does this automatically.
+ *
+ * 3. *Escaping*
+ * When wrapping plain text - i.e. text that is not already HTML or HTML-
+ * escaped - in HTML mark-up, the text must first be HTML-escaped to prevent XSS
+ * attacks and other nasty business. This of course is always true, but the
+ * escaping cannot be done after the string has already been wrapped in
+ * mark-up, so the BiDi formatter also serves as a last chance and includes
+ * escaping services.
+ *
+ * Thus, in a single call, the formatter will escape the input string as
+ * specified, determine its directionality, and wrap it as necessary. It is
+ * then up to the caller to insert the return value in the output.
+ */
+
+#import('dart:web');
+
+class BidiFormatter {
+
+  /** The direction of the surrounding text (the context). */
+  TextDirection contextDirection;
+
+  /**
+   * Indicates if we should always wrap the formatted text in a <span<,.
+   */
+  bool _alwaysSpan;
+
+  /**
+   * Create a formatting object with a direction. If [alwaysSpan] is true we
+   * should always use a `span' tag, even when the input directionality is
+   * neutral or matches the context, so that the DOM structure of the output
+   * does not depend on the combination of directionalities.
+   */
+  BidiFormatter.LTR([alwaysSpan=false]) : contextDirection = TextDirection.LTR,
+      _alwaysSpan = alwaysSpan;
+  BidiFormatter.RTL([alwaysSpan=false]) : contextDirection = TextDirection.RTL,
+      _alwaysSpan = alwaysSpan;
+  BidiFormatter.UNKNOWN([alwaysSpan=false]) : 
+      contextDirection = TextDirection.UNKNOWN, _alwaysSpan = alwaysSpan;
+
+  /** Is true if the context direction for this formatter is RTL. */
+  bool get isRTL() => contextDirection == TextDirection.RTL;
+
+  /**
+   * Formats a string of a given (or estimated, if not provided)
+   * [direction] for use in HTML output of the context directionality, so
+   * an opposite-directionality string is neither garbled nor garbles what
+   * follows it.
+   * If the input string's directionality doesn't match the context
+   * directionality, we wrap it with a `span' tag and adds a `dir' attribute

[Alan Knight, 2012/06/22 00:16:53]

Grammar nitpicking on comments, but "we" and "adds" don't agree. Also the quotes
around span and dir have directionality - that is, the leading one is a
backquote, which looks odd, at least in this font, not sure if we have a
convention for whether to use word-processor style quotes or programming style.

[Emily Fortuna, 2012/06/25 20:25:35]

The backticks are for marking code sections in markdown:
http://daringfireball.net/projects/markdown/syntax#code

[Alan Knight, 2012/06/25 23:29:56]

Ah, ok.

+   * (either "dir=rtl" or "dir=ltr").
+   * If alwaysSpan was true when constructing the formatter, the input is always
+   * wrapped with `span' tag, skipping the dir attribute when it's not needed.
+   *
+   * If [resetDir] is true and the overall directionality or the exit
+   * directionality of [text] is opposite to the context directionality,
+   * a trailing unicode BiDi mark matching the context directionality is
+   * appended (LRM or RLM). If [isHtml] is false, we HTML-escape the [text].

[Alan Knight, 2012/06/22 00:16:53]

The comment says that the unicode bidi characters should be used only where
markup cannot be used, but we have an option on this to always append one at the
end. That seems very odd.

[Emily Fortuna, 2012/06/25 20:25:35]

This is what the closure and python libraries do. We can get rid of it, but not
knowing all the use cases for this I'm not certain if that would be problematic
for applications using our library, particularly if it's expected to behave the
same way as in the libraries in other languages.

[Alan Knight, 2012/06/25 23:29:56]

OK.

+   */
+  String spanWrap(String text, [bool isHtml=false, bool resetDir=true,
+                  TextDirection direction]) {
+    if (direction == null) direction = estimateDirection(text, isHtml);
+    var result = '';

[Alan Knight, 2012/06/22 00:16:53]

Is there a point initializing this to a value that we will only ever throw away?

[Emily Fortuna, 2012/06/25 20:25:35]

Result has to be in scope to return it at line 121. It's not in scope if you
define it at lines 117 and 119.

[Alan Knight, 2012/06/25 23:29:56]

Yes, it needs to be declared there, but there seems little point in initializing
it rather than leaving it null. There's no scenario where that initial value can
escape, or where it would be a valid return value. Not exactly a major point,
though.

[Emily Fortuna, 2012/06/26 01:04:29]

Done.

+    if (!isHtml) text = htmlEscape(text);
+    var directionChange = _requiresDirectionChange(direction);
+    if (_alwaysSpan || directionChange) {
+      var spanDirection = '';
+      if (directionChange) {
+        spanDirection = ' dir=${direction.spanText}';
+      }
+      result= '<span$spanDirection>$text</span>';
+    } else {
+      result = text;
+    }
+    return result.concat(_resetDirIfNeeded(text, direction, isHtml, resetDir));
+  }
+
+  /**
+   * Format [text] of a known (if specified) or estimated [direction] for use 
+   * in *plain-text* output of the context directionality, so an
+   * opposite-directionality text is neither garbled nor garbles what follows
+   * it. Unlike spanWrap, this makes use of unicode BiDi formatting characters
+   * instead of spans for wrapping. The returned string would be
+   * RLE+text+PDF for RTL text, or LRE+text+PDF for LTR text.
+   *
+   * If [resetDir] is true, and if the overall directionality or the exit
+   * directionality of text are opposite to the context directionality,
+   * a trailing unicode BiDi mark matching the context directionality is
+   * appended (LRM or RLM).
+   *
+   * In HTML, the *only* valid use of this function is inside of elements that
+   * do not allow markup, e.g. an 'option' tag.
+   * This function does *not* do HTML-escaping regardless of the value of
+   * [isHtml].

[Alan Knight, 2012/06/22 00:16:53]

What's the point of having a parameter that's ignored? Is there something that
expects to call these very symmetrically? I could see maybe if you were calling
it reflectively you want the same arguments, but we don't even have that yet.

[Emily Fortuna, 2012/06/25 20:25:35]

isHtml is needed in the call to estimateDirection, so the parameter is not
actually ignored. This replicates the closure library's behavior.

[Alan Knight, 2012/06/25 23:29:56]

But it sounds like this is only applicable to plain text, so the value of isHtml
can be replaced with a literal false (or left out) on the call to
estimateDirection. All it does there is control whether the input is stripped of
html, which should never apply if called from here. Unless there's a use case in
which you put in HTML and return plain text, which seems very odd.

[Emily Fortuna, 2012/06/26 01:04:29]

There's the (admittedly) obscure case the comments mention in lines 137-138 on
this particular version of the file with option tags, but we could take that out
and see if people complained about it missing if you'd like...?

+   */
+  String unicodeWrap(String text, [bool isHtml=false, bool resetDir=true,
+                     TextDirection direction]) {

[Alan Knight, 2012/06/22 00:16:53]

I found the name unicodeWrap initially confusing. It sort of sounds like
wrapping the string in Unicode, whatever that would mean. It might make it
clearer, as well as making the two methods a bit more clearly symmetric if they
were something like wrapWithSpan and wrapWithFormatCharacters? Or some other
name for the second one, I'm not quite sure what to call those things.  But if
the current names are consistent with other libraries people might have already
used, that might outweigh this. So I'm waffling, basically.

[Emily Fortuna, 2012/06/25 20:25:35]

These names were taken directly from closure. I've changed them now, though.

[Alan Knight, 2012/06/25 23:29:56]

OK.

+    if (direction == null) direction = estimateDirection(text, isHtml);
+    var result = text;
+    if (_requiresDirectionChange(direction)) {
+      result = '''${direction == TextDirection.RTL ?
+                      BidiUtils.RLE :
+                      BidiUtils.LRE}$text${BidiUtils.PDF}''';
+    }
+    return result.concat(_resetDirIfNeeded(text, direction, isHtml, resetDir));

[Alan Knight, 2012/06/22 00:16:53]

If we're always ending with PDF, wouldn't do that do the reset automatically? Or
maybe I don't understand what PDF does, but if it doesn't reset the direction,
what does it do?

[Emily Fortuna, 2012/06/25 20:25:35]

It's needed to mark the end of the text that is being set in a particular
direction. See http://www.w3.org/TR/html4/struct/dirlang.html#h-8.2.3 I suspect
they're explicitly marking the change in direction to be very clear with
formatting and not leave layout to chance in the world of many browsers. This
replicates the behavior in Closure. 

[Alan Knight, 2012/06/25 23:29:56]

OK, I think I understand that. It's ending the text, and the reset is adding
another explicit override to a particular direction. Seems very messy, but
that's probably part of why they recommend the HTML form.

+  }
+
+  /**
+   * Returns a Unicode BiDi mark matching the context directionality
+   * (LRM or RLM) if either the directionality or the exit directionality of
+   * [text] is opposite to the context directionality. Otherwise returns
+   * the empty string. [isHtml] is true if the text is HTML or HTML-escaped.
+   */
+  String markAfter(String text, [bool isHtml=false]) {
+    text = BidiUtils.stripHtmlIfNeeded(text, isHtml);
+    return _resetDirIfNeeded(text, estimateDirection(text), false, true);
+  }
+
+  /**
+   * Returns the Unicode BiDi mark matching the context directionality.
+   */

[Alan Knight, 2012/06/22 00:16:53]

Maybe it's just my ignorance of the usage of these things, but it would be nice
if the comments gave an indication of what things might be used for. The wrap
calls are pretty obvious. This one seems less clear - why do I want the marker
for the direction that I'm already going?

[Emily Fortuna, 2012/06/25 20:25:35]

This one, like the others, I took from the library this is based off of. We can
certainly take it out. I was erring on the side of providing functions that I
was less sure about the usage of since I don't have a lot of experience with
writing products that regularly use bidi myself.

[Alan Knight, 2012/06/25 23:29:56]

Yeah, I worry about us just reproducing a bunch of messy stuff that we don't
understand. I think I'm inclined to err on the side of leaving things out and
letting people complain about their absence, at which point we should be able to
get a use case for why they want it.

The triple value logic is particularly odd. It doesn't actually return the
Unicode Bidi mark, it returns either that or an empty string. This seems to be
treating a null context direction as meaning UNKNOWN, even though we have a
representation for that and probably ought to initialize it that way rather than
having the possibility for it to be null.

And if we had the methods on TextDirection to return the markers, then this
becomes close to a no-op.  return contextDirection.unicodeMark;  One more line
if we want to handle the null case.

[Emily Fortuna, 2012/06/26 01:04:29]

*Please* look at the updated version of the file I uploaded when I sent out the
email again. I already deleted these methods.

+  String mark() {
+    if (contextDirection == TextDirection.LTR) {
+      return BidiUtils.LRM;
+    } else if (contextDirection == TextDirection.RTL) {
+      return BidiUtils.RLM;
+    } else {
+      return '';
+    }
+  }
+
+  /**
+   * Estimates the directionality of [text] using the best known
+   * general-purpose method (using relative word counts). A 
+   * TextDirection.UNKNOWN return value indicates completely neutral input.
+   * [isHtml] is true if [text] HTML or HTML-escaped.
+   */
+  TextDirection estimateDirection(String text, [bool isHtml=false]) {
+    return BidiUtils.estimateDirection(text, isHtml);
+  }
+
+  /**
+   * Returns a unicode BiDi mark matching the context [direction].

[Alan Knight, 2012/06/22 00:16:53]

When you say "the context" that makes me think you mean something in the
environment. Wouldn't it be better here to just say "matching [direction]" ?
Except I think that's not really what it means.

[Emily Fortuna, 2012/06/25 20:25:35]

rephrased.

+   * If [resetDir] is true, and if the overall directionality or the exit
+   * directionality of [text] is opposite the context directionality, it will
+   * return LRM or RLM according to the directionality. Otherwise return the
+   * empty string. [isHtml] is true if [text] is HTML or HTML-escaped.

[Alan Knight, 2012/06/22 00:16:53]

I find the name resetDirIfNeeded much clearer than the explanation.

[Emily Fortuna, 2012/06/25 20:25:35]

Good thing I changed the name differently from the original closure version
then.

+   */
+  String _resetDirIfNeeded(final String text, TextDirection direction,

[Alan Knight, 2012/06/22 00:16:53]

Why is the text final here, but nowhere else? What do we get by declaring it
that way?

[Emily Fortuna, 2012/06/25 20:25:35]

That was for an optimization that is no longer needed. removed.

+                           bool isHtml, bool resetDir) {
+    // endsWithRtl and endsWithLtr are called only if needed (short-circuit).
+    if (resetDir && 

[Alan Knight, 2012/06/22 00:16:53]

We've defined the class TextDirection. Couldn't we simplify a bunch of this if
we defined methods it that would return the Unicode Bidi indicator characters
for the corresponding and the opposite directions, and maybe equality. e.g.
  if (contextDirection != direction) return contextDirection.unicodeOpposite();

[Emily Fortuna, 2012/06/25 20:25:35]

This function requires knowledge of the last strongly directional text in
"text", so although we could subsitute isDirectionChange in for the
contextDirection and direction checks, we'd still have to add back in the one
check with context Direction to make sure that the context is LTR but the text
ends with RTL. Adding unicode characters to the TextDirection class feels like
we're muddying the distinctions between the classes for a class that's just
represents  text direction, not expose characters to help you do formatting.

[Alan Knight, 2012/06/25 23:29:56]

So we can go from an 8 clause if statement to 3 (and 2 if we also factor out the
obvious guard clause)

    if (!resetDir) return '';
    if ((contextDirection != textDirection) || 
      (contextDirection != endDirection(text,isHTML))
        return contextDirection.unicodeOpposite();
      } else { 
        return contextDirection.unicodeMarker();
     }

That seems like a significant improvement.  As far as putting unicode characters
onto text direction, it already has HTML constants, so that doesn't seem like a
stretch. Or you could store the constants elsewhere and just use functions to
ask for
   unicodeOpposite(contextDirection)
though I think I'd rather have them accessible from the object. 

[Emily Fortuna, 2012/06/26 01:04:29]

You can't get rewrite this and preserve the "short-circuit" properties that this
expression has, which you had mentioned is good because we don't want to be
calling both endsWithRtl AND endsWithLtr. Rewriting it would look like: 

  if (contextDirection.isOpposite(direction) && 
        contextDirection.isOpposite(
        BidiUtils.endDirection(text, isHtml, contextDirection))) {
      contextDirection.unicodeMarker();
    }

And then the endDirection method implemented would be like:


static TextDirection endDirection(String text, [isHtml=false,
      expectedDirection=TextDirection.UNKNOWN]) {
    if (expectedDirection == TextDirection.LTR) {
      if(endsWithRtl(text, isHtml) {...}
    } else if(etc...)
  }

In which case we're rearranging the deck chairs on the Titanic -- we're still
doing the same comparison of explicit LTR and RTL, just in a different location
in the file. So either, we don't have the short circuit property, which might be
slow, but it looks clean or we leave it as is and it's faster but is a little
complex in this if statement.

+        ((contextDirection == TextDirection.LTR &&
+          (direction == TextDirection.RTL ||
+           BidiUtils.endsWithRtl(text, isHtml))) ||
+         (contextDirection == TextDirection.RTL &&
+          (direction == TextDirection.LTR ||
+           BidiUtils.endsWithLtr(text, isHtml))))) {
+      if (contextDirection == TextDirection.LTR) {
+        return BidiUtils.LRM;
+      } else {
+        return BidiUtils.RLM;
+      }
+    } else {
+      return '';
+    }
+  }
+
+  /**
+   * Returns true if the new text [direction] requires a direction change, given
+   * the surrounding contextDirection.
+   */
+  bool _requiresDirectionChange(TextDirection direction) {

[Alan Knight, 2012/06/22 00:16:53]

This seems like it could be a method on TextDirection. e.g.
contextDirection.requiresChangeTo(direction)

[Emily Fortuna, 2012/06/25 20:25:35]

Done.

+    return direction != TextDirection.UNKNOWN && direction != contextDirection;
+  }
+}