Changeset 286801 in webkit

Timestamp:

Dec 9, 2021, 1:28:22 PM (5 years ago)

Author:

Alan Bujtas

Message:

[LFC][IFC] Try not to include non-content type of inline boxes in the visual reordering
​https://bugs.webkit.org/show_bug.cgi?id=234033

Reviewed by Antti Koivisto.

In InlineItemsBuilder::setBidiLevelForOpaqueInlineItems we try to figure out the bidi
level for inline box markers (<span> and </span>) mostly to be able to position
empty inline boxes (specifically with decorations). In some cases though the guessed
bidi level breaks the continuation between the content before and after the marker (e.g. before</span>after)
In this patch we try to limit the number of items associated with this guessed bidi level.

• layout/formattingContexts/inline/InlineDisplayContentBuilder.cpp:

(WebCore::Layout::InlineDisplayContentBuilder::processBidiContent):

• layout/formattingContexts/inline/InlineItem.h:
• layout/formattingContexts/inline/InlineItemsBuilder.cpp:

(WebCore::Layout::InlineItemsBuilder::breakAndComputeBidiLevels):

• layout/formattingContexts/inline/InlineLineBuilder.cpp:

(WebCore::Layout::LineBuilder::layoutInlineContent): we may only submit a subset of runs to the reordering
algorithm. It also means we may end up with gaps between the visual index values and the real run item indexes.
The runIndexOffsetMap ensures that the indexes are always consistent with the content in the lineRuns vector.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• layout/formattingContexts/inline/InlineDisplayContentBuilder.cpp (modified) (2 diffs)
• layout/formattingContexts/inline/InlineItem.h (modified) (1 diff)
• layout/formattingContexts/inline/InlineItemsBuilder.cpp (modified) (1 diff)
• layout/formattingContexts/inline/InlineLineBuilder.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-12-09  Alan Bujtas  <zalan@apple.com>
+
+        [LFC][IFC] Try not to include non-content type of inline boxes in the visual reordering
+        https://bugs.webkit.org/show_bug.cgi?id=234033
+
+        Reviewed by Antti Koivisto.
+
+        In InlineItemsBuilder::setBidiLevelForOpaqueInlineItems we try to figure out the bidi
+        level for inline box markers (<span> and </span>) mostly to be able to position
+        empty inline boxes (specifically with decorations). In some cases though the guessed
+        bidi level breaks the continuation between the content before and after the marker (e.g.  before</span>after)
+        In this patch we try to limit the number of items associated with this guessed bidi level.
+
+        * layout/formattingContexts/inline/InlineDisplayContentBuilder.cpp:
+        (WebCore::Layout::InlineDisplayContentBuilder::processBidiContent):
+        * layout/formattingContexts/inline/InlineItem.h:
+        * layout/formattingContexts/inline/InlineItemsBuilder.cpp:
+        (WebCore::Layout::InlineItemsBuilder::breakAndComputeBidiLevels):
+        * layout/formattingContexts/inline/InlineLineBuilder.cpp:
+        (WebCore::Layout::LineBuilder::layoutInlineContent): we may only submit a subset of runs to the reordering
+        algorithm. It also means we may end up with gaps between the visual index values and the real run item indexes.
+        The runIndexOffsetMap ensures that the indexes are always consistent with the content in the lineRuns vector.
+
 2021-12-08  BJ Burg  <bburg@apple.com>
 

trunk/Source/WebCore/layout/formattingContexts/inline/InlineDisplayContentBuilder.cpp

@@ -461 +461 @@
 void InlineDisplayContentBuilder::processBidiContent(const LineBuilder::LineContent& lineContent, const LineBox& lineBox, const InlineLayoutPoint& lineBoxLogicalTopLeft, DisplayBoxes& boxes)
 {
-    ASSERT(lineContent.visualOrderList.size() == lineContent.runs.size());
+    ASSERT(lineContent.visualOrderList.size() <= lineContent.runs.size());
 
     AncestorStack ancestorStack;
@@ -480 +480 @@
         auto contentRightInVisualOrder = contentStartInVisualOrder;
         auto& runs = lineContent.runs;
-        for (size_t i = 0; i < runs.size(); ++i) {
-            auto visualIndex = lineContent.visualOrderList[i];
-            auto& lineRun = runs[visualIndex];
+        for (auto visualOrder : lineContent.visualOrderList) {
+            ASSERT(runs[visualOrder].bidiLevel() != InlineItem::opaqueBidiLevel);
+
+            auto& lineRun = runs[visualOrder];
             auto& layoutBox = lineRun.layoutBox();
 
-            auto needsDisplayBox = !lineRun.isInlineBoxEnd() && !lineRun.isWordBreakOpportunity();
+            auto needsDisplayBox = !lineRun.isWordBreakOpportunity();
             if (!needsDisplayBox)
                 continue;

trunk/Source/WebCore/layout/formattingContexts/inline/InlineItem.h

@@ -52 +52 @@
 
     Type type() const { return m_type; }
+    static constexpr UBiDiLevel opaqueBidiLevel = 0xff;
     UBiDiLevel bidiLevel() const { return m_bidiLevel; }
     const Box& layoutBox() const { return *m_layoutBox; }

trunk/Source/WebCore/layout/formattingContexts/inline/InlineItemsBuilder.cpp

@@ -334 +334 @@
             }
             if (inlineItems[index].isInlineBoxEnd()) {
-                // Inline box end (e.g. </span>) also uses the content bidi level, but in this case it's the previous content.
-                auto previousBidiLevel = [&]() -> std::optional<UBiDiLevel> {
-                    for (auto i = index; i--;) {
-                        if (inlineItemOffsets[i])
-                            return inlineItems[i].bidiLevel();
-                    }
-                    return { };
-                }();
-                inlineItems[index].setBidiLevel(previousBidiLevel.value_or(rootBidiLevel));
+                // Let's not confuse ubidi with non-content entries. Opaque runs are excluded from the visual list.
+                inlineItems[index].setBidiLevel(InlineItem::opaqueBidiLevel);
                 continue;
             }

trunk/Source/WebCore/layout/formattingContexts/inline/InlineLineBuilder.cpp

@@ -301 +301 @@
             return { };
 
-        Vector<UBiDiLevel> runLevels(lineRuns.size());
-        // FIXME: We may cache these values in Line, if it turns out to be a perf hit.
-        for (size_t i = 0; i < lineRuns.size(); ++i)
-            runLevels[i] = lineRuns[i].bidiLevel();
-
-        Vector<int32_t> visualOrderList(lineRuns.size());
+        Vector<UBiDiLevel> runLevels;
+        runLevels.reserveInitialCapacity(lineRuns.size());
+
+        Vector<size_t> runIndexOffsetMap;
+        runIndexOffsetMap.reserveInitialCapacity(lineRuns.size());
+        auto hasOpaqueRun = false;
+        for (size_t i = 0, accumulatedOffset = 0; i < lineRuns.size(); ++i) {
+            if (lineRuns[i].bidiLevel() == InlineItem::opaqueBidiLevel) {
+                ++accumulatedOffset;
+                hasOpaqueRun = true;
+                continue;
+            }
+            runLevels.append(lineRuns[i].bidiLevel());
+            runIndexOffsetMap.append(accumulatedOffset);
+        }
+
+        Vector<int32_t> visualOrderList(runLevels.size());
         ubidi_reorderVisual(runLevels.data(), runLevels.size(), visualOrderList.data());
+        if (hasOpaqueRun) {
+            ASSERT(visualOrderList.size() == runIndexOffsetMap.size());
+            for (size_t i = 0; i < runIndexOffsetMap.size(); ++i)
+                visualOrderList[i] += runIndexOffsetMap[visualOrderList[i]];
+        }
         return visualOrderList;
     };