Changeset 287866 in webkit

Timestamp:

Jan 10, 2022, 6:17:09 PM (5 years ago)

Author:

Wenson Hsieh

Message:

Followup to r287863 - adjust line wrapping behavior in image overlays
​https://bugs.webkit.org/show_bug.cgi?id=235035
rdar://85139146

Reviewed by Tim Horton.

Source/WebCore:

The previous change I landed in r287863 was written under the assumption that -shouldWrap describes whether or
not a recognized line of text should wrap to the next line. However, after trying it out, it seems to indicate
whether or not the previous line of text should wrap to the current line. As a result, the current adoption of
this property is wrong, since the line wrapping is off-by-one.

Fix this by renaming the shouldWrap boolean flag in TextRecognitionLineData to the much less ambiguous name
hasTrailingNewline, and set hasTrailingNewline based on the shouldWrap property of the *next*
VKWKLineInfo.

• dom/ImageOverlay.cpp:

(WebCore::ImageOverlay::updateSubtree):

Avoid a potentially even-more-confusing != !! here by using static_cast<> to check the nullity of the
RefPtr.

• platform/TextRecognitionResult.h:

(WebCore::TextRecognitionLineData::TextRecognitionLineData):
(WebCore::TextRecognitionLineData::encode const):
(WebCore::TextRecognitionLineData::decode):

Make sure we also flip the default from false to true, since hasTrailingNewline is intended to have the
opposite effect as the extant shouldWrap.

• testing/Internals.cpp:

(WebCore::makeDataForLine):

• testing/Internals.h:
• testing/Internals.idl:

Source/WebKit:

Use the -shouldWrap property to compute hasTrailingNewline. See WebCore/ChangeLog for more details.

• Platform/cocoa/TextRecognitionUtilities.mm:

(WebKit::makeTextRecognitionResult):

LayoutTests:

Make a slight adjustment to an existing layout test (i.e. rename shouldWrap to hasTrailingNewline). See
WebCore/ChangeLog for more details.

• fast/images/text-recognition/image-overlay-line-wrapping.html:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/images/text-recognition/image-overlay-line-wrapping.html (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/dom/ImageOverlay.cpp (modified) (2 diffs)
• Source/WebCore/platform/TextRecognitionResult.h (modified) (4 diffs)
• Source/WebCore/testing/Internals.cpp (modified) (1 diff)
• Source/WebCore/testing/Internals.h (modified) (1 diff)
• Source/WebCore/testing/Internals.idl (modified) (1 diff)
• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/Platform/cocoa/TextRecognitionUtilities.mm (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2022-01-10  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Followup to r287863 - adjust line wrapping behavior in image overlays
+        https://bugs.webkit.org/show_bug.cgi?id=235035
+        rdar://85139146
+
+        Reviewed by Tim Horton.
+
+        Make a slight adjustment to an existing layout test (i.e. rename `shouldWrap` to `hasTrailingNewline`). See
+        WebCore/ChangeLog for more details.
+
+        * fast/images/text-recognition/image-overlay-line-wrapping.html:
+
 2022-01-10  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/LayoutTests/fast/images/text-recognition/image-overlay-line-wrapping.html

@@ -26 +26 @@
             bottomRight : new DOMPointReadOnly(1, 0.3),
             bottomLeft : new DOMPointReadOnly(0, 0.3),
-            shouldWrap: true,
+            hasTrailingNewline: false,
             children: [
                 {

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2022-01-10  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Followup to r287863 - adjust line wrapping behavior in image overlays
+        https://bugs.webkit.org/show_bug.cgi?id=235035
+        rdar://85139146
+
+        Reviewed by Tim Horton.
+
+        The previous change I landed in r287863 was written under the assumption that `-shouldWrap` describes whether or
+        not a recognized line of text should wrap to the next line. However, after trying it out, it seems to indicate
+        whether or not the previous line of text should wrap to the current line. As a result, the current adoption of
+        this property is wrong, since the line wrapping is off-by-one.
+
+        Fix this by renaming the `shouldWrap` boolean flag in TextRecognitionLineData to the much less ambiguous name
+        `hasTrailingNewline`, and set `hasTrailingNewline` based on the `shouldWrap` property of the *next*
+        VKWKLineInfo.
+
+        * dom/ImageOverlay.cpp:
+        (WebCore::ImageOverlay::updateSubtree):
+
+        Avoid a potentially even-more-confusing `!= !!` here by using `static_cast<>` to check the nullity of the
+        `RefPtr`.
+
+        * platform/TextRecognitionResult.h:
+        (WebCore::TextRecognitionLineData::TextRecognitionLineData):
+        (WebCore::TextRecognitionLineData::encode const):
+        (WebCore::TextRecognitionLineData::decode):
+
+        Make sure we also flip the default from `false` to `true`, since `hasTrailingNewline` is intended to have the
+        opposite effect as the extant `shouldWrap`.
+
+        * testing/Internals.cpp:
+        (WebCore::makeDataForLine):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
 2022-01-10  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Source/WebCore/dom/ImageOverlay.cpp

@@ -277 +277 @@
                 auto& lineElements = elements.lines[lineIndex];
                 auto& childTextElements = lineElements.children;
-                if (lineResult.shouldWrap != !lineElements.lineBreak)
+                if (lineResult.hasTrailingNewline != static_cast<bool>(lineElements.lineBreak))
                     return false;
 
@@ -336 +336 @@
             }
 
-            if (!line.shouldWrap) {
+            if (line.hasTrailingNewline) {
                 lineElements.lineBreak = HTMLBRElement::create(document.get());
                 lineContainer->appendChild(*lineElements.lineBreak);

trunk/Source/WebCore/platform/TextRecognitionResult.h

@@ -82 +82 @@
 
 struct TextRecognitionLineData {
-    TextRecognitionLineData(FloatQuad&& quad, Vector<TextRecognitionWordData>&& theChildren, bool wrap)
+    TextRecognitionLineData(FloatQuad&& quad, Vector<TextRecognitionWordData>&& theChildren, bool newline)
         : normalizedQuad(WTFMove(quad))
         , children(WTFMove(theChildren))
-        , shouldWrap(wrap)
+        , hasTrailingNewline(newline)
     {
     }
@@ -91 +91 @@
     FloatQuad normalizedQuad;
     Vector<TextRecognitionWordData> children;
-    bool shouldWrap { false };
+    bool hasTrailingNewline { true };
 
     template<class Encoder> void encode(Encoder&) const;
@@ -117 +117 @@
     encoder << normalizedQuad;
     encoder << children;
-    encoder << shouldWrap;
+    encoder << hasTrailingNewline;
 }
 
@@ -132 +132 @@
         return std::nullopt;
 
-    std::optional<bool> shouldWrap;
-    decoder >> shouldWrap;
-    if (!shouldWrap)
-        return std::nullopt;
-
-    return { { WTFMove(*normalizedQuad), WTFMove(*children), *shouldWrap } };
+    std::optional<bool> hasTrailingNewline;
+    decoder >> hasTrailingNewline;
+    if (!hasTrailingNewline)
+        return std::nullopt;
+
+    return { { WTFMove(*normalizedQuad), WTFMove(*children), *hasTrailingNewline } };
 }
 

trunk/Source/WebCore/testing/Internals.cpp

@@ -5771 +5771 @@
             return { textChild.text, getQuad<Internals::ImageOverlayText>(textChild), textChild.hasLeadingWhitespace };
         }),
-        line.shouldWrap
+        line.hasTrailingNewline
     };
 }

trunk/Source/WebCore/testing/Internals.h

@@ -923 +923 @@
         RefPtr<DOMPointReadOnly> bottomLeft;
         Vector<ImageOverlayText> children;
-        bool shouldWrap { false };
+        bool hasTrailingNewline { true };
 
         ~ImageOverlayLine();

trunk/Source/WebCore/testing/Internals.idl

@@ -293 +293 @@
     required DOMPointReadOnly bottomLeft;
     sequence<ImageOverlayText> children;
-    boolean shouldWrap = false;
+    boolean hasTrailingNewline = true;
 };
 

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2022-01-10  Wenson Hsieh  <wenson_hsieh@apple.com>
+
+        Followup to r287863 - adjust line wrapping behavior in image overlays
+        https://bugs.webkit.org/show_bug.cgi?id=235035
+        rdar://85139146
+
+        Reviewed by Tim Horton.
+
+        Use the `-shouldWrap` property to compute `hasTrailingNewline`. See WebCore/ChangeLog for more details.
+
+        * Platform/cocoa/TextRecognitionUtilities.mm:
+        (WebKit::makeTextRecognitionResult):
+
 2022-01-10  Wenson Hsieh  <wenson_hsieh@apple.com>
 

trunk/Source/WebKit/Platform/cocoa/TextRecognitionUtilities.mm

@@ -79 +79 @@
 TextRecognitionResult makeTextRecognitionResult(VKImageAnalysis *analysis)
 {
-    NSArray<VKWKTextInfo *> *allLines = analysis.allLines;
+    NSArray<VKWKLineInfo *> *allLines = analysis.allLines;
     TextRecognitionResult result;
     result.lines.reserveInitialCapacity(allLines.count);
 
     bool isFirstLine = true;
+    size_t nextLineIndex = 1;
     for (VKWKLineInfo *line in allLines) {
         Vector<TextRecognitionWordData> children;
@@ -115 +116 @@
             children.uncheckedAppend({ WTFMove(childText), floatQuad(child.quad), hasLeadingWhitespace });
         }
-        result.lines.uncheckedAppend({
-            floatQuad(line.quad),
-            WTFMove(children),
-            [line respondsToSelector:@selector(shouldWrap)] && [line shouldWrap]
-        });
+        VKWKLineInfo *nextLine = nextLineIndex < allLines.count ? allLines[nextLineIndex] : nil;
+        // The `shouldWrap` property indicates whether or not a line should wrap, relative to the previous line.
+        bool hasTrailingNewline = nextLine && (![nextLine respondsToSelector:@selector(shouldWrap)] || ![nextLine shouldWrap]);
+        result.lines.uncheckedAppend({ floatQuad(line.quad), WTFMove(children), hasTrailingNewline });
         isFirstLine = false;
+        nextLineIndex++;
     }