sw floattable: avoid layout loop for negative vert offset in MakePos()

The bugdoc has 2 paragraphs and a floating table between them. The
floating table has a large enough negative vertical offset so it "pushes
down" the paragraph that is before the table in the doc model. Our
layout didn't push the first paragraph down, the second paragraph
overlapped with the table and the whole layout process was just stopped
by the layout loop control.

What happened is that we realized that we have to push down the first
paragraph, so an SwFlyPortion was created for it. Then we made an
UnlockPosition() in SwTextFrame::MakePos(), so it could be calculated
again. This lead to an oscillation, and the calculated vertical position
of the floating table's fly changed between 964 (good) and 2990 (bad)
twips.

Fix the problem by limiting when SwTextFrame::MakePos() calls
UnlockPosition(). The general strategy is that flys are only positioned
once during a layout pass. The exception from this is when the fly moved
to a new page and it's not yet positioned. So if we unlock when the fly
only has the page frame top left position, then we keep the original
use-case working, but fix the layout loop.

Regression from commit 12a9009a1c19ee26c65fb44fc90f3432c88ab6a5 (sw
floattable: fix bad position of follow fly if anchor is positioned late,
2023-03-23), where I didn't realize how dangerous UnlockPosition() is.

(cherry picked from commit 01eff4a68b05dd4eeee94bc4b3b018059efa60d4)

Change-Id: I0aa0a52db712a464105e8040497fd429e0604309
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/153315
Tested-by: Jenkins CollaboraOffice <jenkinscollaboraoffice@gmail.com>
Reviewed-by: Caolán McNamara <caolan.mcnamara@collabora.com>

4 files changed, 72 insertions, 1 deletions

diff --git a/sw/CppunitTest_sw_core_text.mk b/sw/CppunitTest_sw_core_text.mk
index 1d8db3e6eeb1..79ac4a0320bf 100644
--- a/sw/CppunitTest_sw_core_text.mk
+++ b/sw/CppunitTest_sw_core_text.mk
@@ -14,6 +14,7 @@ $(eval $(call gb_CppunitTest_CppunitTest,sw_core_text))
 $(eval $(call gb_CppunitTest_use_common_precompiled_header,sw_core_text))
 
 $(eval $(call gb_CppunitTest_add_exception_objects,sw_core_text, \
+    sw/qa/core/text/frmform \
     sw/qa/core/text/text \
 ))
 
diff --git a/sw/qa/core/text/data/floattable-negative-vert-offset.docx b/sw/qa/core/text/data/floattable-negative-vert-offset.docx
new file mode 100644
index 000000000000..688b5b8c1188
--- /dev/null
+++ b/sw/qa/core/text/data/floattable-negative-vert-offset.docx
diff --git a/sw/qa/core/text/frmform.cxx b/sw/qa/core/text/frmform.cxx
new file mode 100644
index 000000000000..3c1a16a99444
--- /dev/null
+++ b/sw/qa/core/text/frmform.cxx
@@ -0,0 +1,64 @@
+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
+/*
+ * This file is part of the LibreOffice project.
+ *
+ * This Source Code Form is subject to the terms of the Mozilla Public
+ * 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 <swmodeltestbase.hxx>
+
+#include <memory>
+
+#include <IDocumentLayoutAccess.hxx>
+#include <rootfrm.hxx>
+#include <sortedobjs.hxx>
+#include <anchoredobject.hxx>
+#include <pagefrm.hxx>
+
+namespace
+{
+/// Covers sw/source/core/text/frmform.cxx fixes.
+class Test : public SwModelTestBase
+{
+public:
+    Test()
+        : SwModelTestBase("/sw/qa/core/text/data/")
+    {
+    }
+};
+
+CPPUNIT_TEST_FIXTURE(Test, testFloattableNegativeVertOffset)
+{
+    // Given a document with 2 paragraphs, floating table is between the two (so anchored to the
+    // 2nd) and with a negative vertical offset:
+    createSwDoc("floattable-negative-vert-offset.docx");
+
+    // When laying out that document:
+    calcLayout();
+
+    // Then make sure that the negative vertical offset shifts both paragraphs down:
+    SwDoc* pDoc = getSwDoc();
+    SwRootFrame* pLayout = pDoc->getIDocumentLayoutAccess().GetCurrentLayout();
+    auto pPage = dynamic_cast<SwPageFrame*>(pLayout->Lower());
+    CPPUNIT_ASSERT(pPage);
+    CPPUNIT_ASSERT(pPage->GetSortedObjs());
+    const SwSortedObjs& rPageObjs = *pPage->GetSortedObjs();
+    CPPUNIT_ASSERT_EQUAL(static_cast<size_t>(1), rPageObjs.size());
+    SwAnchoredObject* pPageObj = rPageObjs[0];
+    const SwRect& rFlyRect = pPageObj->GetObjRectWithSpaces();
+    SwFrame* pBody = pPage->FindBodyCont();
+    SwFrame* pPara1 = pBody->GetLower();
+    SwFrame* pPara2 = pPara1->GetNext();
+    // Without the accompanying fix in place, this test would have failed with:
+    // - Expected less than: 993
+    // - Actual  : 2709
+    // i.e. the expectation that the fly doesn't overlap with the 2nd paragraph was not true.
+    // Instead we got a layout loop, aborted by the loop control, and the fly overlapped with the
+    // 2nd paragraph.
+    CPPUNIT_ASSERT_LESS(pPara2->getFrameArea().Top(), rFlyRect.Bottom());
+}
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sw/source/core/text/frmform.cxx b/sw/source/core/text/frmform.cxx
index 7d1e686db748..50b3ca91b20c 100644
--- a/sw/source/core/text/frmform.cxx
+++ b/sw/source/core/text/frmform.cxx
@@ -362,7 +362,13 @@ void SwTextFrame::MakePos()
             }
             // Possibly this fly was positioned relative to us, invalidate its position now that our
             // position is changed.
-            pFly->UnlockPosition();
+            SwPageFrame* pPageFrame = pFly->FindPageFrame();
+            if (pPageFrame && pFly->getFrameArea().Pos() == pPageFrame->getFrameArea().Pos())
+            {
+                // The position was just adjusted to be be inside the page frame, so not really
+                // positioned, unlock the position once to allow a recalc.
+                pFly->UnlockPosition();
+            }
             pFly->InvalidatePos();
         }
     }