TextEdit: don't skip nested frames or their blocks in updatePaintNode

I don't fully understand the previous logic behind keeping the same
firstDirtyPos while we iterate all the frames. In case a document
contains a mix of tables and text for example, we may iterate to a block
in the root frame that is just beyond the first table, and set
firstDirtyPos accordingly (which I think is meant to say that everything
up to that character position has been rendered, but it's not true at
that moment in time); and then when we look at the frame corresponding
to the first table, rule out rendering the whole frame or any of its
blocks based on the fact that it comes _before_ firstDirtyPos. The fact
that we got past its position in the root frame should not preclude
rendering the nested frame. So now it looks safer to reset firstDirtyPos
before iterating each nested frame.

Anyway the logic from f513e88403b66c4a5efe4c62c160dfce151efb33 assumed
that we wanted to populate the whole document into the scene graph,
and should just avoid redoing work while keeping the nodes up-to-date;
whereas since 9db23e0e04906cf9ea33e23fa41f34955e5e6fe0 we're trying to
avoid populating nodes too far outside the viewport. That's probably the
bigger optimization: if we are avoiding most of the blocks in large
documents most of the time, redoing work to render the ones that are
in the viewport is not such a big deal. And it seems dangerous to
preemptively skip frames or blocks before we've checked whether they
are in the viewport. So now we visit all frames regardless of
firstDirtyPos, but still skip blocks within the frame that are
beyond the bottom of the viewport. And whenever we do render the
first block that is beyond the viewport, we also enlarge our local
copy of the viewport rectangle to encompass that block's bounds,
to ensure that when we visit nested frames, we will also render
other blocks within the enlarged bounds. Effectively this means
if we render the next block in the root frame immediately past a table,
we will also render all cells in the lower part of that table (between
the top of the viewport and the bottom of the table). That might result
in more SG nodes than we strictly need (outside the viewport bounds);
but at the end, the stored QQuickTextEditPrivate::renderedRegion
must include that one block that was past the table, so we want all
intervening blocks too. QQuickTextEditPrivate::transformChanged() will
not call update() if renderedRegion tells it that the coverage is
complete within the current viewport, and nothing more needs to be
rendered. That was the root of this bug.

The autotest just counts the number of blocks rendered at different
scroll positions in a Flickable. Perhaps it will turn out to be
sensitive to font sizes on various platforms; but the number of
blocks rendered was way too small in case of the bug, so we can
afford a tolerance in the comparison, if needed.

Fixes: QTBUG-118636
Pick-to: 6.5 6.6 6.7
Change-Id: Id1f2b393f26e2dd5b198e9d5251e8c33459c3c07
Reviewed-by: Qt CI Bot <qt_ci_bot@qt-project.org>
Reviewed-by: Doris Verria <doris.verria@qt.io>

4 files changed, 127 insertions, 5 deletions

diff --git a/src/quick/items/qquicktextedit.cpp b/src/quick/items/qquicktextedit.cpp
index 66615d9354..22bbd6e05b 100644
--- a/src/quick/items/qquicktextedit.cpp
+++ b/src/quick/items/qquicktextedit.cpp
@@ -2523,14 +2523,17 @@ QSGNode *QQuickTextEdit::updatePaintNode(QSGNode *oldNode, UpdatePaintNodeData *
 
         d->firstBlockInViewport = -1;
         d->firstBlockPastViewport = -1;
+        int frameCount = -1;
         while (!frames.isEmpty()) {
             QTextFrame *textFrame = frames.takeFirst();
+            ++frameCount;
+            if (frameCount > 0)
+                firstDirtyPos = 0;
+            qCDebug(lcVP) << "frame" << frameCount << textFrame
+                          << "from" << positionToRectangle(textFrame->firstPosition()).topLeft()
+                          << "to" << positionToRectangle(textFrame->lastPosition()).bottomRight();
             frames.append(textFrame->childFrames());
             frameDecorationsEngine.addFrameDecorations(d->document, textFrame);
-
-            if (textFrame->lastPosition() < firstDirtyPos
-                    || textFrame->firstPosition() >= firstCleanNode.startPos())
-                continue;
             resetEngine(&engine, d->color, d->selectedTextColor, d->selectionColor);
 
             if (textFrame->firstPosition() > textFrame->lastPosition()
@@ -2613,8 +2616,13 @@ QSGNode *QQuickTextEdit::updatePaintNode(QSGNode *oldNode, UpdatePaintNodeData *
                                 }
                                 break; // skip rest of blocks in this frame
                             }
-                            if (inView && !block.text().isEmpty() && coveredRegion.isValid())
+                            if (inView && !block.text().isEmpty() && coveredRegion.isValid()) {
                                 d->renderedRegion = d->renderedRegion.united(coveredRegion);
+                                // In case we're going to visit more (nested) frames after this, ensure that we
+                                // don't omit any blocks that fit within the region that we claim as fully rendered.
+                                if (!frames.isEmpty())
+                                    viewport = viewport.united(d->renderedRegion);
+                            }
                         }
                         if (inView && d->firstBlockInViewport < 0)
                             d->firstBlockInViewport = block.blockNumber();
@@ -3289,6 +3297,7 @@ void QQuickTextEditPrivate::addCurrentTextNodeToRoot(QQuickTextNodeEngine *engin
     it = textNodeMap.insert(it, TextNode(startPos, node));
     ++it;
     root->appendChildNode(node);
+    ++renderedBlockCount;
 }
 
 QSGInternalTextNode *QQuickTextEditPrivate::createTextNode()
diff --git a/src/quick/items/qquicktextedit_p_p.h b/src/quick/items/qquicktextedit_p_p.h
index c5f0137ea0..da01be47c2 100644
--- a/src/quick/items/qquicktextedit_p_p.h
+++ b/src/quick/items/qquicktextedit_p_p.h
@@ -179,6 +179,7 @@ public:
     int lineCount = 0;
     int firstBlockInViewport = -1;   // can be wrong after scrolling sometimes
     int firstBlockPastViewport = -1; // only for the autotest
+    int renderedBlockCount = -1;     // only for the autotest
     QRectF renderedRegion;
 
     enum UpdateType {
diff --git a/tests/auto/quick/qquicktextedit/data/inFlickable.qml b/tests/auto/quick/qquicktextedit/data/inFlickable.qml
index 7a896db29b..183ddd6701 100644
--- a/tests/auto/quick/qquicktextedit/data/inFlickable.qml
+++ b/tests/auto/quick/qquicktextedit/data/inFlickable.qml
@@ -1,6 +1,7 @@
 import QtQuick 2.0
 
 Flickable {
+    id: flick
     width: 320; height: 120; contentHeight: text.height
     TextEdit {
         id: text
@@ -8,4 +9,10 @@ Flickable {
         font.pixelSize: 20
         text: "several\nlines\nof\ntext\n-\ntry\nto\nflick"
     }
+    Text {
+        color: "red"
+        parent: flick // stay on top
+        anchors.right: parent.right
+        text: flick.contentY.toFixed(1)
+    }
 }
diff --git a/tests/auto/quick/qquicktextedit/tst_qquicktextedit.cpp b/tests/auto/quick/qquicktextedit/tst_qquicktextedit.cpp
index 3a4231f953..cc994fe783 100644
--- a/tests/auto/quick/qquicktextedit/tst_qquicktextedit.cpp
+++ b/tests/auto/quick/qquicktextedit/tst_qquicktextedit.cpp
@@ -8,6 +8,8 @@
 #include <QtQuick/QQuickTextDocument>
 #include <QtQuickTest/QtQuickTest>
 #include <QTextDocument>
+#include <QtGui/qtextobject.h>
+#include <QtGui/QTextTable>
 #include <QtQml/qqmlengine.h>
 #include <QtQml/qqmlcontext.h>
 #include <QtQml/qqmlexpression.h>
@@ -58,6 +60,8 @@ Q_DECLARE_METATYPE(QKeySequence::StandardKey)
 
 Q_LOGGING_CATEGORY(lcTests, "qt.quick.tests")
 
+// #define DEBUG_WRITE_INPUT
+
 static bool isPlatformWayland()
 {
     return !QGuiApplication::platformName().compare(QLatin1String("wayland"), Qt::CaseInsensitive);
@@ -158,6 +162,8 @@ private slots:
     void largeTextObservesViewport();
     void largeTextSelection();
     void renderingAroundSelection();
+    void largeTextTables_data();
+    void largeTextTables();
 
     void signal_editingfinished();
 
@@ -3886,6 +3892,105 @@ void tst_qquicktextedit::renderingAroundSelection()
     QTRY_COMPARE(textItem->sortedLinePositions, sortedLinePositions);
 }
 
+struct OffsetAndExpectedBlocks {
+    int tableIndex;         // which nested frame
+    qreal tableOffset;      // fraction of that frame's height to scroll to
+    int minExpectedBlockCount;
+
+    OffsetAndExpectedBlocks(int i, qreal o, int c)
+        : tableIndex(i), tableOffset(o), minExpectedBlockCount(c) {}
+};
+
+typedef QList<OffsetAndExpectedBlocks> OffsetAndExpectedBlocksList;
+
+void tst_qquicktextedit::largeTextTables_data()
+{
+    QTest::addColumn<int>("tables");
+    QTest::addColumn<int>("tableCols");
+    QTest::addColumn<int>("tableRows");
+    QTest::addColumn<OffsetAndExpectedBlocksList>("steps");
+
+    QTest::newRow("one big table") << 1 << 3 << 70
+                                   << OffsetAndExpectedBlocksList{
+                                      OffsetAndExpectedBlocks(1, 0.75, 150),
+                                      OffsetAndExpectedBlocks(1, 0.5, 150)};
+    QTest::newRow("short tables") << 5 << 3 << 10
+                                  << OffsetAndExpectedBlocksList{
+                                     OffsetAndExpectedBlocks(4, 0.75, 35),
+                                     OffsetAndExpectedBlocks(3, 0.25, 50),
+                                     OffsetAndExpectedBlocks(2, 0.75, 50)};
+}
+
+void tst_qquicktextedit::largeTextTables() // QTBUG-118636
+{
+    QFETCH(int, tables);
+    QFETCH(int, tableCols);
+    QFETCH(int, tableRows);
+    QFETCH(OffsetAndExpectedBlocksList, steps);
+
+    QStringList lines;
+
+    lines << QLatin1String("<h1>") + QTest::currentDataTag() + "</h1>";
+    for (int t = 0; t < tables; ++t) {
+        if (t > 0)
+            lines << QString("<p>table %1</p>").arg(t);
+        lines << "<table border='1'>";
+        for (int r = 0; r < tableRows; ++r) {
+            lines << "  <tr>";
+            for (int c = 0; c < tableCols; ++c)
+                lines << QString("    <td>table %1 cell  %2, %3</td>").arg(t).arg(c).arg(r);
+            lines << "  </tr>";
+        }
+        lines << "</table>";
+    }
+    lines << "<p>here endeth the tables</p>";
+    QString html = lines.join('\n');
+
+#ifdef DEBUG_WRITE_INPUT
+    QFile f(QLatin1String("/tmp/") + QTest::currentDataTag() + ".html");
+    f.open(QFile::WriteOnly);
+    f.write(html.toUtf8());
+    f.close();
+#endif
+
+    QQuickView window;
+    QVERIFY(QQuickTest::showView(window, testFileUrl("inFlickable.qml")));
+    QQuickFlickable *flick = qmlobject_cast<QQuickFlickable *>(window.rootObject());
+    QVERIFY(flick);
+    QQuickTextEdit *te = window.rootObject()->findChild<QQuickTextEdit *>();
+    QVERIFY(te);
+    auto *tePriv = QQuickTextEditPrivate::get(te);
+    auto font = te->font();
+    font.setPixelSize(10);
+    te->setFont(font);
+
+    te->setTextFormat(QQuickTextEdit::RichText);
+    te->setText(html);
+    te->setFlag(QQuickItem::ItemObservesViewport); // this isn't "large text", but test viewporting anyway
+
+    QTextDocument *doc = te->textDocument()->textDocument();
+    QList<QTextFrame *> frames = doc->rootFrame()->childFrames();
+    frames.prepend(doc->rootFrame());
+    qCDebug(lcTests) << "blocks" << doc->blockCount() << "chars" << doc->characterCount() << "frames" << frames;
+
+    for (const OffsetAndExpectedBlocks &oeb : steps) {
+        QCOMPARE_GT(frames.size(), oeb.tableIndex);
+        const QTextFrame *textFrame = frames.at(oeb.tableIndex);
+        const QTextCursor top = textFrame->firstCursorPosition();
+        const qreal yTop = te->positionToRectangle(top.position()).top();
+        const QTextCursor bottom = textFrame->lastCursorPosition();
+        const qreal yBottom = te->positionToRectangle(bottom.position()).bottom();
+        const qreal y = yTop + (yBottom - yTop) * oeb.tableOffset;
+        qCDebug(lcTests) << "frame" << textFrame << "goes from pos" << top.position() << "y" << yTop
+                         << "to pos" << bottom.position() << "y" << yBottom << "; scrolling to" << y
+                         << "which is at" << oeb.tableOffset << "of table height" << (yBottom - yTop);
+        flick->setContentY(y);
+        qCDebug(lcTests) << tePriv->renderedRegion << "rendered blocks" << tePriv->renderedBlockCount << ":"
+                         << tePriv->firstBlockInViewport << "to" << tePriv->firstBlockPastViewport;
+        QTRY_COMPARE_GE(tePriv->renderedBlockCount, oeb.minExpectedBlockCount);
+    }
+}
+
 void tst_qquicktextedit::signal_editingfinished()
 {
     QQuickView window;