Bidi-Isolate inlines break layout with collapsed whitespace

Source/core/rendering/InlineIterator.h

 /*
  * Copyright (C) 2000 Lars Knoll (knoll@kde.org)
  * Copyright (C) 2003, 2004, 2006, 2007, 2008, 2009, 2010 Apple Inc. All right reserved.
  * Copyright (C) 2010 Google Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
  * License as published by the Free Software Foundation; either
  * version 2 of the License, or (at your option) any later version.
  *
@@ skipping 456 matching lines @@
 // FIXME: This belongs on InlineBidiResolver, except it's a template specialization
 // of BidiResolver which knows nothing about RenderObjects.
 static inline void addPlaceholderRunForIsolatedInline(InlineBidiResolver& resolver, RenderObject* obj, unsigned pos)
 {
     ASSERT(obj);
     BidiRun* isolatedRun = new BidiRun(pos, 0, obj, resolver.context(), resolver.dir());
     resolver.runs().addRun(isolatedRun);
     // FIXME: isolatedRuns() could be a hash of object->run and then we could cheaply
     // ASSERT here that we didn't create multiple objects for the same inline.
     resolver.isolatedRuns().append(isolatedRun);
+    LineMidpointState& lineMidpointState = resolver.midpointState();
+    resolver.setMidpointStateForIsolatedRun(isolatedRun, lineMidpointState);
 }
 
 class IsolateTracker {
 public:
     explicit IsolateTracker(unsigned nestedIsolateCount)
         : m_nestedIsolateCount(nestedIsolateCount)
         , m_haveAddedFakeRunForRootIsolate(false)
     {
     }
 
     void enterIsolate() { m_nestedIsolateCount++; }
     void exitIsolate()
     {
         ASSERT(m_nestedIsolateCount >= 1);
         m_nestedIsolateCount--;
         if (!inIsolate())
             m_haveAddedFakeRunForRootIsolate = false;
     }
     bool inIsolate() const { return m_nestedIsolateCount; }
 
     // We don't care if we encounter bidi directional overrides.
     void embed(WTF::Unicode::Direction, BidiEmbeddingSource) { }
     void commitExplicitEmbedding() { }
 
-    void addFakeRunIfNecessary(RenderObject* obj, unsigned pos, InlineBidiResolver& resolver)
+    void adjustMidpointForIsolatedRun(RenderObject* obj, unsigned start, unsigned end, InlineBidiResolver& resolver)

[leviw_travelin_and_unemployed, 2013/10/07 21:58:42]

This seems really similar to RenderBlock::appendRunsForObject, which makes me
sad the logic is duplicated. It's also hard for me to wrap my head fully around
how this solves the problem.

+    {
+        if (start > end)
+            return;
+
+        LineMidpointState& lineMidpointState = resolver.midpointState();
+        bool haveNextMidpoint = (lineMidpointState.currentMidpoint < lineMidpointState.numMidpoints);
+        InlineIterator nextMidpoint;
+        if (haveNextMidpoint)
+            nextMidpoint = lineMidpointState.midpoints[lineMidpointState.currentMidpoint];
+        if (lineMidpointState.betweenMidpoints) {
+            if (!(haveNextMidpoint && nextMidpoint.m_obj == obj))
+                return;
+            lineMidpointState.betweenMidpoints = false;
+            lineMidpointState.currentMidpoint++;
+            if (start < end)
+                return adjustMidpointForIsolatedRun(obj, start, end, resolver);
+        } else {
+            if (!haveNextMidpoint || (obj != nextMidpoint.m_obj))
+                return;
+            if (static_cast<int>(nextMidpoint.m_pos + 1) <= end) {
+                lineMidpointState.betweenMidpoints = true;
+                lineMidpointState.currentMidpoint++;
+                if (nextMidpoint.m_pos != UINT_MAX)
+                    return adjustMidpointForIsolatedRun(obj, nextMidpoint.m_pos + 1, end,  resolver);
+            }
+        }
+    }
+
+    void addFakeRunIfNecessary(RenderObject* obj, unsigned pos, unsigned end, InlineBidiResolver& resolver)
     {
         // We only need to add a fake run for a given isolated span once during each call to createBidiRunsForLine.
         // We'll be called for every span inside the isolated span so we just ignore subsequent calls.
         // We also avoid creating a fake run until we hit a child that warrants one, e.g. we skip floats.
-        if (m_haveAddedFakeRunForRootIsolate || RenderBlock::shouldSkipCreatingRunsForObject(obj))
-            return;
-        m_haveAddedFakeRunForRootIsolate = true;
+        if (!(m_haveAddedFakeRunForRootIsolate || RenderBlock::shouldSkipCreatingRunsForObject(obj))) {
+            addPlaceholderRunForIsolatedInline(resolver, obj, pos);
+            m_haveAddedFakeRunForRootIsolate = true;
+        }
+        adjustMidpointForIsolatedRun(obj, pos, end, resolver);
+
         // obj and pos together denote a single position in the inline, from which the parsing of the isolate will start.
         // We don't need to mark the end of the run because this is implicit: it is either endOfLine or the end of the
         // isolate, when we call createBidiRunsForLine it will stop at whichever comes first.
-        addPlaceholderRunForIsolatedInline(resolver, obj, pos);
-        // FIXME: Inline isolates don't work properly with collapsing whitespace, see webkit.org/b/109624
-        // For now, if we enter an isolate between midpoints, we increment our current midpoint or else
-        // we'll leave the isolate and ignore the content that follows.
-        MidpointState<InlineIterator>& midpointState = resolver.midpointState();
-        if (midpointState.betweenMidpoints && midpointState.midpoints[midpointState.currentMidpoint].object() == obj) {
-            midpointState.betweenMidpoints = false;
-            ++midpointState.currentMidpoint;
-        }
     }
 
 private:
     unsigned m_nestedIsolateCount;
     bool m_haveAddedFakeRunForRootIsolate;
 };
 
 template <>
 inline void InlineBidiResolver::appendRun()
 {
     if (!m_emptyRun && !m_eor.atEnd() && !m_reachedEndOfLine) {
         // Keep track of when we enter/leave "unicode-bidi: isolate" inlines.
         // Initialize our state depending on if we're starting in the middle of such an inline.
         // FIXME: Could this initialize from this->inIsolate() instead of walking up the render tree?
         IsolateTracker isolateTracker(numberOfIsolateAncestors(m_sor));
         int start = m_sor.m_pos;
         RenderObject* obj = m_sor.m_obj;
         while (obj && obj != m_eor.m_obj && obj != endOfLine.m_obj) {
             if (isolateTracker.inIsolate())
-                isolateTracker.addFakeRunIfNecessary(obj, start, *this);
+                isolateTracker.addFakeRunIfNecessary(obj, start, obj->length(), *this);
             else
                 RenderBlock::appendRunsForObject(m_runs, start, obj->length(), obj, *this);
             // FIXME: start/obj should be an InlineIterator instead of two separate variables.
             start = 0;
             obj = bidiNextSkippingEmptyInlines(m_sor.root(), obj, &isolateTracker);
         }
         if (obj) {
             unsigned pos = obj == m_eor.m_obj ? m_eor.m_pos : INT_MAX;
             if (obj == endOfLine.m_obj && endOfLine.m_pos <= pos) {
                 m_reachedEndOfLine = true;
                 pos = endOfLine.m_pos;
             }
             // It's OK to add runs for zero-length RenderObjects, just don't make the run larger than it should be
             int end = obj->length() ? pos + 1 : 0;
             if (isolateTracker.inIsolate())
-                isolateTracker.addFakeRunIfNecessary(obj, start, *this);
+                isolateTracker.addFakeRunIfNecessary(obj, start, end, *this);
             else
                 RenderBlock::appendRunsForObject(m_runs, start, end, obj, *this);
         }
 
         m_eor.increment();
         m_sor = m_eor;
     }
 
     m_direction = WTF::Unicode::OtherNeutral;
     m_status.eor = WTF::Unicode::OtherNeutral;
 }
 
 }
 
 #endif // InlineIterator_h