Bug 1160847 part 3 - Restore virtual bidi control characters for reordering. r=jfkthame

This patch mainly consists of two parts, one for resolving and the other
for reordering.

In the resolving part, the added code stores the lowest embedding level
of all bidi formatting characters precede a frame to the bidi data of
that frame when necessary.

In the reordering part, virtual frame is restored from the information
stored above before asking the bidi engine to reorder frames

Collapsing a run of continuous virtual formatting characters into one
virtual character with the lowest embedding level among them should work
because a character with a higher embedding level than either of its
neighbors should not affect the reordering result of any other part of
the sequence. (No formal proof of this theorem, though)

MozReview-Commit-ID: LQjRu0mWsZP

--HG--
extra : source : 5d0cf1cbd270e9963d848a23b37528ed503ed6a0

layout/base/nsBidi.cpp

@@ -10,6 +10,9 @@
 
 using namespace mozilla::unicode;
 
+static_assert(mozilla::kBidiLevelNone > NSBIDI_MAX_EXPLICIT_LEVEL + 1,
+              "The pseudo embedding level should be out-of-range");
+
 // These are #defined in <sys/regset.h> under Solaris 10 x86
 #undef CS
 #undef ES

layout/base/nsBidi.h

@@ -410,11 +410,20 @@ struct LevState {
 };
 
 namespace mozilla {
+
+// Pseudo bidi embedding level indicating nonexistence.
+static const nsBidiLevel kBidiLevelNone = 0xff;
+
 struct FrameBidiData
 {
   nsBidiLevel baseLevel;
   nsBidiLevel embeddingLevel;
+  // The embedding level of virtual bidi formatting character before
+  // this frame if any. kBidiLevelNone is used to indicate nonexistence
+  // or unnecessity of such virtual character.
+  nsBidiLevel precedingControl;
 };
+
 } // namespace mozilla
 
 /**

layout/base/nsBidiPresUtils.cpp

@@ -3,6 +3,8 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
+#include "mozilla/IntegerRange.h"
+
 #include "nsAutoPtr.h"
 #include "nsBidiPresUtils.h"
 #include "nsFontMetrics.h"
@@ -371,23 +373,65 @@ struct BidiLineData {
 
     bool isReordered = false;
     bool hasRTLFrames = false;
+    bool hasVirtualControls = false;
 
-    for (nsIFrame* frame = aFirstFrameOnLine;
-         frame && aNumFramesOnLine--;
-         frame = frame->GetNextSibling()) {
-      AppendFrame(frame);
-      nsBidiLevel level = nsBidiPresUtils::GetFrameEmbeddingLevel(frame);
+    auto appendFrame = [&](nsIFrame* frame, nsBidiLevel level) {
+      mLogicalFrames.AppendElement(frame);
       mLevels.AppendElement(level);
       mIndexMap.AppendElement(0);
       if (IS_LEVEL_RTL(level)) {
         hasRTLFrames = true;
       }
+    };
+
+    bool firstFrame = true;
+    for (nsIFrame* frame = aFirstFrameOnLine;
+         frame && aNumFramesOnLine--;
+         frame = frame->GetNextSibling()) {
+      FrameBidiData bidiData = nsBidiPresUtils::GetFrameBidiData(frame);
+      // Ignore virtual control before the first frame. Doing so should
+      // not affect the visual result, but could avoid running into the
+      // stripping code below for many cases.
+      if (!firstFrame && bidiData.precedingControl != kBidiLevelNone) {
+        appendFrame(NS_BIDI_CONTROL_FRAME, bidiData.precedingControl);
+        hasVirtualControls = true;
+      }
+      appendFrame(frame, bidiData.embeddingLevel);
+      firstFrame = false;
     }
 
     // Reorder the line
     nsBidi::ReorderVisual(mLevels.Elements(), FrameCount(),
                           mIndexMap.Elements());
 
+    // Strip virtual frames
+    if (hasVirtualControls) {
+      auto originalCount = mLogicalFrames.Length();
+      nsTArray<int32_t> realFrameMap(originalCount);
+      size_t count = 0;
+      for (auto i : MakeRange(originalCount)) {
+        if (mLogicalFrames[i] == NS_BIDI_CONTROL_FRAME) {
+          realFrameMap.AppendElement(-1);
+        } else {
+          mLogicalFrames[count] = mLogicalFrames[i];
+          mLevels[count] = mLevels[i];
+          realFrameMap.AppendElement(count);
+          count++;
+        }
+      }
+      // Only keep index map for real frames.
+      for (size_t i = 0, j = 0; i < originalCount; ++i) {
+        auto newIndex = realFrameMap[mIndexMap[i]];
+        if (newIndex != -1) {
+          mIndexMap[j] = newIndex;
+          j++;
+        }
+      }
+      mLogicalFrames.TruncateLength(count);
+      mLevels.TruncateLength(count);
+      mIndexMap.TruncateLength(count);
+    }
+
     for (int32_t i = 0; i < FrameCount(); i++) {
       mVisualFrames.AppendElement(LogicalFrameAt(mIndexMap[i]));
       if (i != mIndexMap[i]) {
@@ -399,11 +443,6 @@ struct BidiLineData {
     mIsReordered = isReordered || hasRTLFrames;
   }
 
-  void AppendFrame(nsIFrame* aFrame)
-  {
-    mLogicalFrames.AppendElement(aFrame); 
-  }
-
   int32_t FrameCount(){ return mLogicalFrames.Length(); }
 
   nsIFrame* LogicalFrameAt(int32_t aIndex){ return mLogicalFrames[aIndex]; }
@@ -737,6 +776,27 @@ nsBidiPresUtils::ResolveParagraph(nsBlockFrame* aBlockFrame,
   }
 
   nsIFrame* lastRealFrame = nullptr;
+  nsBidiLevel lastEmbedingLevel = kBidiLevelNone;
+  nsBidiLevel precedingControl = kBidiLevelNone;
+
+  auto storeBidiDataToFrame = [&]() {
+    FrameBidiData bidiData;
+    bidiData.embeddingLevel = embeddingLevel;
+    bidiData.baseLevel = aBpd->GetParaLevel();
+    // If a control character doesn't have a lower embedding level than
+    // both the preceding and the following frame, it isn't something
+    // needed for getting the correct result. This optimization should
+    // remove almost all of embeds and overrides, and some of isolates.
+    if (precedingControl >= embeddingLevel ||
+        precedingControl >= lastEmbedingLevel) {
+      bidiData.precedingControl = kBidiLevelNone;
+    } else {
+      bidiData.precedingControl = precedingControl;
+    }
+    precedingControl = kBidiLevelNone;
+    lastEmbedingLevel = embeddingLevel;
+    propTable->Set(frame, nsBidi::BidiDataProperty(), bidiData);
+  };
 
   for (; ;) {
     if (fragmentLength <= 0) {
@@ -765,10 +825,7 @@ nsBidiPresUtils::ResolveParagraph(nsBlockFrame* aBlockFrame,
           frame->AdjustOffsetsForBidi(0, 0);
           // Set the base level and embedding level of the current run even
           // on an empty frame. Otherwise frame reordering will not be correct.
-          FrameBidiData bidiData;
-          bidiData.embeddingLevel = embeddingLevel;
-          bidiData.baseLevel = aBpd->GetParaLevel();
-          propTable->Set(frame, nsBidi::BidiDataProperty(), bidiData);
+          storeBidiDataToFrame();
           continue;
         }
         int32_t start, end;
@@ -795,13 +852,16 @@ nsBidiPresUtils::ResolveParagraph(nsBlockFrame* aBlockFrame,
     } // if (runLength <= 0)
 
     if (frame == NS_BIDI_CONTROL_FRAME) {
+      // In theory, we only need to do this for isolates. However, it is
+      // easier to do this for all here because we do not maintain the
+      // index to get corresponding character from buffer. Since we do
+      // have proper embedding level for all those characters, including
+      // them wouldn't affect the final result.
+      precedingControl = std::min(precedingControl, embeddingLevel);
       ++lineOffset;
     }
     else {
-      FrameBidiData bidiData;
-      bidiData.embeddingLevel = embeddingLevel;
-      bidiData.baseLevel = aBpd->GetParaLevel();
-      propTable->Set(frame, nsBidi::BidiDataProperty(), bidiData);
+      storeBidiDataToFrame();
       if (isTextFrame) {
         if ( (runLength > 0) && (runLength < fragmentLength) ) {
           /*
@@ -1216,6 +1276,12 @@ nsBidiPresUtils::GetFirstLeaf(nsIFrame* aFrame)
   return firstLeaf;
 }
 
+FrameBidiData
+nsBidiPresUtils::GetFrameBidiData(nsIFrame* aFrame)
+{
+  return nsBidi::GetBidiData(GetFirstLeaf(aFrame));
+}
+
 nsBidiLevel
 nsBidiPresUtils::GetFrameEmbeddingLevel(nsIFrame* aFrame)
 {
@@ -1698,6 +1764,7 @@ nsBidiPresUtils::RemoveBidiContinuation(BidiParagraphData *aBpd,
                                         int32_t&        aOffset)
 {
   FrameBidiData bidiData = nsBidi::GetBidiData(aFrame);
+  bidiData.precedingControl = kBidiLevelNone;
   for (int32_t index = aFirstIndex + 1; index <= aLastIndex; index++) {
     nsIFrame* frame = aBpd->FrameAt(index);
     if (frame == NS_BIDI_CONTROL_FRAME) {

layout/base/nsBidiPresUtils.h

@@ -280,6 +280,11 @@ public:
 
   static nsIFrame* GetFirstLeaf(nsIFrame* aFrame);
     
+  /**
+   * Get the bidi data of the given (inline) frame.
+   */
+  static mozilla::FrameBidiData GetFrameBidiData(nsIFrame* aFrame);
+
   /**
    * Get the bidi embedding level of the given (inline) frame.
    */

layout/generic/nsTextFrame.cpp

@@ -1609,7 +1609,8 @@ BuildTextRunsScanner::ContinueTextRunAcrossFrames(nsTextFrame* aFrame1, nsTextFr
   if (mBidiEnabled) {
     FrameBidiData data1 = nsBidi::GetBidiData(aFrame1);
     FrameBidiData data2 = nsBidi::GetBidiData(aFrame2);
-    if (data1.embeddingLevel != data2.embeddingLevel) {
+    if (data1.embeddingLevel != data2.embeddingLevel ||
+        data2.precedingControl != kBidiLevelNone) {
       return false;
     }
   }
@@ -4154,6 +4155,7 @@ nsContinuingTextFrame::Init(nsIContent*       aContent,
   }
   if (aPrevInFlow->GetStateBits() & NS_FRAME_IS_BIDI) {
     FrameBidiData bidiData = nsBidi::GetBidiData(aPrevInFlow);
+    bidiData.precedingControl = kBidiLevelNone;
     Properties().Set(nsBidi::BidiDataProperty(), bidiData);
 
     if (nextContinuation) {
@@ -4167,6 +4169,9 @@ nsContinuingTextFrame::Init(nsIContent*       aContent,
         NS_ASSERTION(bidiData.embeddingLevel == nextBidiData.embeddingLevel &&
                      bidiData.baseLevel == nextBidiData.baseLevel,
                      "stealing text from different type of BIDI continuation");
+        MOZ_ASSERT(nextBidiData.precedingControl == kBidiLevelNone,
+                   "There shouldn't be any virtual bidi formatting character "
+                   "between continuations");
 #endif
         nextContinuation->mContentOffset = mContentOffset;
         nextContinuation = static_cast<nsTextFrame*>(nextContinuation->GetNextContinuation());

layout/reftests/bidi/reftest.list

@@ -36,8 +36,8 @@ random-if(cocoaWidget) == mirroring-02.html mirroring-02-ref.html
 == mixedChartype-03-j.html mixedChartype-03-ref.html
 == unicode-bidi-anonymous-001.html unicode-bidi-anonymous-001-ref.html
 == unicode-bidi-anonymous-002.html unicode-bidi-anonymous-002-ref.html
-fails == unicode-bidi-isolate-basic.html unicode-bidi-isolate-basic-ref.html # bug 712600
-fails == unicode-bidi-isolate-aharon.html unicode-bidi-isolate-aharon-ref.html # bug 712600
+== unicode-bidi-isolate-basic.html unicode-bidi-isolate-basic-ref.html
+== unicode-bidi-isolate-aharon.html unicode-bidi-isolate-aharon-ref.html
 fuzzy-if(/^Windows\x20NT\x206\.1/.test(http.oscpu)&&!layersGPUAccelerated&&!azureSkia,73,1) fuzzy-if(skiaContent,104,32) == unicode-bidi-plaintext.html unicode-bidi-plaintext-ref.html
 == unicode-bidi-plaintext-textarea-1.html unicode-bidi-plaintext-textarea-ref.html
 == unicode-bidi-plaintext-textarea-2.html unicode-bidi-plaintext-textarea-ref.html

testing/web-platform/meta/html/dom/elements/requirements-relating-to-bidirectional-algorithm-formatting-characters/dir-isolation-003a.html.ini

@@ -1,3 +0,0 @@
-[dir-isolation-003a.html]
-  type: reftest
-  expected: FAIL

testing/web-platform/meta/html/dom/elements/requirements-relating-to-bidirectional-algorithm-formatting-characters/dir-isolation-003b.html.ini

@@ -1,3 +0,0 @@
-[dir-isolation-003b.html]
-  type: reftest
-  expected: FAIL

testing/web-platform/meta/html/dom/elements/requirements-relating-to-bidirectional-algorithm-formatting-characters/dir-isolation-003c.html.ini

@@ -1,3 +0,0 @@
-[dir-isolation-003c.html]
-  type: reftest
-  expected: FAIL

testing/web-platform/meta/html/dom/elements/requirements-relating-to-bidirectional-algorithm-formatting-characters/dir-isolation-007a.html.ini

@@ -1,3 +0,0 @@
-[dir-isolation-007a.html]
-  type: reftest
-  expected: FAIL

testing/web-platform/meta/html/dom/elements/requirements-relating-to-bidirectional-algorithm-formatting-characters/dir-isolation-007b.html.ini

@@ -1,3 +0,0 @@
-[dir-isolation-007b.html]
-  type: reftest
-  expected: FAIL

testing/web-platform/meta/html/dom/elements/requirements-relating-to-bidirectional-algorithm-formatting-characters/dir-isolation-007c.html.ini

@@ -1,3 +0,0 @@
-[dir-isolation-007c.html]
-  type: reftest
-  expected: FAIL

testing/web-platform/meta/html/semantics/text-level-semantics/the-bdi-element/bdi-neutral-to-another-bdi-2.html.ini

@@ -1,3 +0,0 @@
-[bdi-neutral-to-another-bdi-2.html]
-  type: reftest
-  expected: FAIL

testing/web-platform/meta/html/semantics/text-level-semantics/the-bdi-element/bdi-neutral-to-letter-following-2.html.ini

@@ -1,3 +0,0 @@
-[bdi-neutral-to-letter-following-2.html]
-  type: reftest
-  expected: FAIL

testing/web-platform/meta/html/semantics/text-level-semantics/the-bdi-element/bdi-neutral-to-letter-preceding-2.html.ini

@@ -1,3 +0,0 @@
-[bdi-neutral-to-letter-preceding-2.html]
-  type: reftest
-  expected: FAIL

testing/web-platform/meta/html/semantics/text-level-semantics/the-bdi-element/bdi-neutral-to-number-following-2.html.ini

@@ -1,3 +0,0 @@
-[bdi-neutral-to-number-following-2.html]
-  type: reftest
-  expected: FAIL