Related: tdf#55160 sw floattable, nested DOCX imp: fix inner tbl at cell start

The bugdoc has an outer table and also two inner tables: the problematic
one is the floating table anchored at the start of B1, which should
float but does not.

This special-casing was added in commit
c1eebcdac9f2b289fd363399130c485ca5ff444c (tdf#79329 DOCX import: fix
missing outer table with floattable at cell start, 2016-11-08), because
there was no easy way to make sure that an inner floating table at cell
start keeps the outer table import working, and having 2 inline tables
is better than having an inner floating table and no outer table at all.
The root of the problem is that in case an SwXParagraph tracks the outer
cell start and we have an inner floating table, then the inner
conversion to text frame will invalidate that SwXParagraph, so the outer
table conversion will fail.

Fix the problem by creating a cursor at cell start, moving it away,
performing the inner table resolution. And in case the cell start
SwXParagraph is now invalid, then move the cursor back and work with
that position.

The original bugdoc has 2 floating tables at cell start, which is still
broken, but this approach allows fixing that in a later commit; while
the old approach simply didn't notice that the 2nd floating table is
also at cell start.

Change-Id: Iedad7b2f023e88dfc5de7875ebc00320c2e6ad65
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/156937
Reviewed-by: Miklos Vajna <vmiklos@collabora.com>
Tested-by: Jenkins

9 files changed, 179 insertions, 48 deletions

diff --git a/writerfilter/CppunitTest_writerfilter_dmapper.mk b/writerfilter/CppunitTest_writerfilter_dmapper.mk
index 21808b1d1ffd..6b7611b41996 100644
--- a/writerfilter/CppunitTest_writerfilter_dmapper.mk
+++ b/writerfilter/CppunitTest_writerfilter_dmapper.mk
@@ -22,6 +22,7 @@ $(eval $(call gb_CppunitTest_add_exception_objects,writerfilter_dmapper, \
     writerfilter/qa/cppunittests/dmapper/DomainMapper \
     writerfilter/qa/cppunittests/dmapper/DomainMapper_Impl \
     writerfilter/qa/cppunittests/dmapper/GraphicImport \
+    writerfilter/qa/cppunittests/dmapper/TableManager \
     writerfilter/qa/cppunittests/dmapper/TextEffectsHandler \
     writerfilter/qa/cppunittests/dmapper/PropertyMap \
     writerfilter/qa/cppunittests/dmapper/SdtHelper \
diff --git a/writerfilter/qa/cppunittests/dmapper/TableManager.cxx b/writerfilter/qa/cppunittests/dmapper/TableManager.cxx
new file mode 100644
index 000000000000..1f20ee1f8432
--- /dev/null
+++ b/writerfilter/qa/cppunittests/dmapper/TableManager.cxx
@@ -0,0 +1,46 @@
+/* -*- 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 <test/unoapi_test.hxx>
+
+#include <com/sun/star/text/XTextFramesSupplier.hpp>
+
+using namespace com::sun::star;
+
+namespace
+{
+/// Tests for writerfilter/source/dmapper/TableManager.cxx.
+class Test : public UnoApiTest
+{
+public:
+    Test()
+        : UnoApiTest("/writerfilter/qa/cppunittests/dmapper/data/")
+    {
+    }
+};
+
+CPPUNIT_TEST_FIXTURE(Test, testFloattableNestedCellStartDOCXImport)
+{
+    // Given a document with a nested floating table at cell start and an other inner floating table:
+    // When importing that document:
+    loadFromURL(u"floattable-nested-cellstart.docx");
+
+    // Then make sure that both inner tables are floating:
+    uno::Reference<text::XTextFramesSupplier> xFramesSupplier(mxComponent, uno::UNO_QUERY);
+    uno::Reference<container::XIndexAccess> xFrames(xFramesSupplier->getTextFrames(),
+                                                    uno::UNO_QUERY);
+    // Without the accompanying fix in place, this test would have failed with:
+    // - Expected: 2
+    // - Actual  : 1
+    // i.e. the first inner table was not floating.
+    CPPUNIT_ASSERT_EQUAL(static_cast<sal_Int32>(2), xFrames->getCount());
+}
+}
+
+/* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/writerfilter/qa/cppunittests/dmapper/data/floattable-nested-cellstart.docx b/writerfilter/qa/cppunittests/dmapper/data/floattable-nested-cellstart.docx
new file mode 100644
index 000000000000..837d539bb984
--- /dev/null
+++ b/writerfilter/qa/cppunittests/dmapper/data/floattable-nested-cellstart.docx
diff --git a/writerfilter/source/dmapper/DomainMapper.cxx b/writerfilter/source/dmapper/DomainMapper.cxx
index 7c830115fb9f..17ad330e5568 100644
--- a/writerfilter/source/dmapper/DomainMapper.cxx
+++ b/writerfilter/source/dmapper/DomainMapper.cxx
@@ -3250,11 +3250,6 @@ void DomainMapper::sprmWithProps( Sprm& rSprm, const PropertyMapPtr& rContext )
     break;
     case NS_ooxml::LN_tblStart:
     {
-        if (m_pImpl->hasTableManager())
-        {
-            bool bTableStartsAtCellStart = m_pImpl->m_nTableDepth > 0 && m_pImpl->m_nTableCellDepth > m_pImpl->m_nLastTableCellParagraphDepth + 1;
-            m_pImpl->getTableManager().setTableStartsAtCellStart(bTableStartsAtCellStart);
-        }
         /*
          * Hack for Importing Section Properties
          * LO is not able to import section properties if first element in the
diff --git a/writerfilter/source/dmapper/DomainMapperTableHandler.cxx b/writerfilter/source/dmapper/DomainMapperTableHandler.cxx
index e2e8f8c54d78..2d1c4f3b44ba 100644
--- a/writerfilter/source/dmapper/DomainMapperTableHandler.cxx
+++ b/writerfilter/source/dmapper/DomainMapperTableHandler.cxx
@@ -1359,7 +1359,7 @@ static void lcl_convertFormulaRanges(const uno::Reference<text::XTextTable> & xT
     }
 }
 
-void DomainMapperTableHandler::endTable(unsigned int nestedTableLevel, bool bTableStartsAtCellStart)
+void DomainMapperTableHandler::endTable(unsigned int nestedTableLevel)
 {
 #ifdef DBG_UTIL
     TagLogger::getInstance().startElement("tablehandler.endTable");
@@ -1596,11 +1596,8 @@ void DomainMapperTableHandler::endTable(unsigned int nestedTableLevel, bool bTab
             m_aTableProperties->getValue(TablePropertyMap::TABLE_WIDTH_TYPE, nTableWidthType);
             // m_xText points to the body text, get the current xText from m_rDMapper_Impl, in case e.g. we would be in a header.
             uno::Reference<text::XTextAppendAndConvert> xTextAppendAndConvert(m_rDMapper_Impl.GetTopTextAppend(), uno::UNO_QUERY);
-            // Don't execute the conversion for nested tables anchored at a cell start: that
-            // currently invalidates the cell start / end references and the outer table conversion
-            // would fail.
             uno::Reference<beans::XPropertySet> xFrameAnchor;
-            if (xTextAppendAndConvert.is() && !(nestedTableLevel >= 2 && bTableStartsAtCellStart))
+            if (xTextAppendAndConvert.is())
             {
                 std::deque<css::uno::Any> aFramedRedlines = m_rDMapper_Impl.m_aStoredRedlines[StoredRedlines::FRAME];
                 std::vector<sal_Int32> redPos, redLen;
diff --git a/writerfilter/source/dmapper/DomainMapperTableHandler.hxx b/writerfilter/source/dmapper/DomainMapperTableHandler.hxx
index d67c4d26bb7c..376a6d366688 100644
--- a/writerfilter/source/dmapper/DomainMapperTableHandler.hxx
+++ b/writerfilter/source/dmapper/DomainMapperTableHandler.hxx
@@ -90,7 +90,7 @@ public:
     void ApplyParagraphPropertiesFromTableStyle(TableParagraph rParaProp, std::vector< PropertyIds > aAllTableProperties, const css::beans::PropertyValues rCellProperties);
 
     /// Handle end of table.
-    void endTable(unsigned int nestedTableLevel, bool bTableStartsAtCellStart);
+    void endTable(unsigned int nestedTableLevel);
     /**
        Handle start of row.
 
diff --git a/writerfilter/source/dmapper/TableData.hxx b/writerfilter/source/dmapper/TableData.hxx
index b151ef9c06d6..e65b9c1c1abc 100644
--- a/writerfilter/source/dmapper/TableData.hxx
+++ b/writerfilter/source/dmapper/TableData.hxx
@@ -101,6 +101,10 @@ public:
 
     sal_uInt32 getGridSpan() const { return m_nGridSpan; }
     void setGridSpan( sal_uInt32 nSpan ) { m_nGridSpan = nSpan; }
+
+    void SetStart(const css::uno::Reference<css::text::XTextRange>& xStart) { mStart = xStart; }
+
+    bool IsValid() const;
 };
 
 /**
@@ -170,6 +174,21 @@ public:
         return mCells.size() > 0 && mCells.back()->isOpen();
     }
 
+    void SetCellStart(const css::uno::Reference<css::text::XTextRange>& xStart)
+    {
+        if (mCells.empty())
+        {
+            return;
+        }
+
+        mCells.back()->SetStart(xStart);
+    }
+
+    bool IsCellValid() const
+    {
+        return !mCells.empty() && mCells.back()->IsValid();
+    }
+
     /**
        Add properties to the row.
 
@@ -343,6 +362,16 @@ public:
         return mpRow->isCellOpen();
     }
 
+    void SetCellStart(const css::uno::Reference<css::text::XTextRange>& xStart)
+    {
+        mpRow->SetCellStart(xStart);
+    }
+
+    bool IsCellValid() const
+    {
+        return mpRow->IsCellValid();
+    }
+
     /**
         Insert properties to the current cell of the current row.
 
diff --git a/writerfilter/source/dmapper/TableManager.cxx b/writerfilter/source/dmapper/TableManager.cxx
index d10834544bf6..d0f90bd20729 100644
--- a/writerfilter/source/dmapper/TableManager.cxx
+++ b/writerfilter/source/dmapper/TableManager.cxx
@@ -26,6 +26,8 @@
 
 #include <comphelper/diagnose_ex.hxx>
 
+using namespace com::sun::star;
+
 namespace writerfilter::dmapper
 {
 void TableManager::clearData() {}
@@ -41,7 +43,7 @@ void TableManager::openCell(const css::uno::Reference<css::text::XTextRange>& rH
 
     if (!mTableDataStack.empty())
     {
-        TableData::Pointer_t pTableData = mTableDataStack.top();
+        TableData::Pointer_t pTableData = mTableDataStack.back();
 
         pTableData->addCell(rHandle, pProps);
     }
@@ -56,19 +58,19 @@ sal_uInt32 TableManager::getGridBefore(sal_uInt32 nRow)
         SAL_WARN("writerfilter", "TableManager::getGridBefore called while not in table");
         return 0;
     }
-    if (nRow >= mTableDataStack.top()->getRowCount())
+    if (nRow >= mTableDataStack.back()->getRowCount())
         return 0;
-    return mTableDataStack.top()->getRow(nRow)->getGridBefore();
+    return mTableDataStack.back()->getRow(nRow)->getGridBefore();
 }
 
 sal_uInt32 TableManager::getCurrentGridBefore()
 {
-    return mTableDataStack.top()->getCurrentRow()->getGridBefore();
+    return mTableDataStack.back()->getCurrentRow()->getGridBefore();
 }
 
 void TableManager::setCurrentGridBefore(sal_uInt32 nSkipGrids)
 {
-    mTableDataStack.top()->getCurrentRow()->setGridBefore(nSkipGrids);
+    mTableDataStack.back()->getCurrentRow()->setGridBefore(nSkipGrids);
 }
 
 sal_uInt32 TableManager::getGridAfter(sal_uInt32 nRow)
@@ -78,33 +80,33 @@ sal_uInt32 TableManager::getGridAfter(sal_uInt32 nRow)
         SAL_WARN("writerfilter", "TableManager::getGridAfter called while not in table");
         return 0;
     }
-    if (nRow >= mTableDataStack.top()->getRowCount())
+    if (nRow >= mTableDataStack.back()->getRowCount())
         return 0;
-    return mTableDataStack.top()->getRow(nRow)->getGridAfter();
+    return mTableDataStack.back()->getRow(nRow)->getGridAfter();
 }
 
 void TableManager::setCurrentGridAfter(sal_uInt32 nSkipGrids)
 {
     assert(isInTable());
-    mTableDataStack.top()->getCurrentRow()->setGridAfter(nSkipGrids);
+    mTableDataStack.back()->getCurrentRow()->setGridAfter(nSkipGrids);
 }
 
 std::vector<sal_uInt32> TableManager::getCurrentGridSpans()
 {
-    return mTableDataStack.top()->getCurrentRow()->getGridSpans();
+    return mTableDataStack.back()->getCurrentRow()->getGridSpans();
 }
 
 void TableManager::setCurrentGridSpan(sal_uInt32 nGridSpan, bool bFirstCell)
 {
-    mTableDataStack.top()->getCurrentRow()->setCurrentGridSpan(nGridSpan, bFirstCell);
+    mTableDataStack.back()->getCurrentRow()->setCurrentGridSpan(nGridSpan, bFirstCell);
 }
 
 sal_uInt32 TableManager::findColumn(const sal_uInt32 nRow, const sal_uInt32 nCell)
 {
-    if (nRow >= mTableDataStack.top()->getRowCount())
+    if (nRow >= mTableDataStack.back()->getRowCount())
         return SAL_MAX_UINT32;
 
-    RowData::Pointer_t pRow = mTableDataStack.top()->getRow(nRow);
+    RowData::Pointer_t pRow = mTableDataStack.back()->getRow(nRow);
     if (!pRow || nCell < pRow->getGridBefore()
         || nCell >= pRow->getCellCount() - pRow->getGridAfter())
     {
@@ -121,10 +123,10 @@ sal_uInt32 TableManager::findColumn(const sal_uInt32 nRow, const sal_uInt32 nCel
 
 sal_uInt32 TableManager::findColumnCell(const sal_uInt32 nRow, const sal_uInt32 nCol)
 {
-    if (nRow >= mTableDataStack.top()->getRowCount())
+    if (nRow >= mTableDataStack.back()->getRowCount())
         return SAL_MAX_UINT32;
 
-    RowData::Pointer_t pRow = mTableDataStack.top()->getRow(nRow);
+    RowData::Pointer_t pRow = mTableDataStack.back()->getRow(nRow);
     if (!pRow || nCol < pRow->getGridBefore())
         return SAL_MAX_UINT32;
 
@@ -288,7 +290,7 @@ void TableManager::closeCell(const css::uno::Reference<css::text::XTextRange>& r
 
     if (!mTableDataStack.empty())
     {
-        TableData::Pointer_t pTableData = mTableDataStack.top();
+        TableData::Pointer_t pTableData = mTableDataStack.back();
 
         pTableData->endCell(rHandle);
 
@@ -305,7 +307,7 @@ void TableManager::ensureOpenCell(const TablePropertyMapPtr& pProps)
 
     if (!mTableDataStack.empty())
     {
-        TableData::Pointer_t pTableData = mTableDataStack.top();
+        TableData::Pointer_t pTableData = mTableDataStack.back();
 
         if (pTableData != nullptr)
         {
@@ -348,7 +350,7 @@ void TableManager::endParagraphGroup()
     if (isRowEnd())
     {
         endOfRowAction();
-        mTableDataStack.top()->endRow(getRowProps());
+        mTableDataStack.back()->endRow(getRowProps());
         mState.resetRowProps();
     }
 
@@ -381,7 +383,7 @@ void TableManager::resolveCurrentTable()
     {
         try
         {
-            TableData::Pointer_t pTableData = mTableDataStack.top();
+            TableData::Pointer_t pTableData = mTableDataStack.back();
 
             unsigned int nRows = pTableData->getRowCount();
 
@@ -406,7 +408,7 @@ void TableManager::resolveCurrentTable()
                 mpTableDataHandler->endRow();
             }
 
-            mpTableDataHandler->endTable(mTableDataStack.size() - 1, m_bTableStartsAtCellStart);
+            mpTableDataHandler->endTable(mTableDataStack.size() - 1);
         }
         catch (css::uno::Exception const&)
         {
@@ -423,27 +425,78 @@ void TableManager::resolveCurrentTable()
 
 void TableManager::endLevel()
 {
+    uno::Reference<text::XTextCursor> xCursor;
     if (mpTableDataHandler != nullptr)
+    {
+        if (mTableDataStack.size() > 1)
+        {
+            // This is an inner table: create a cursor from the outer cell's start position, in case
+            // that would become invalid during the current table resolution.
+            TableData::Pointer_t pUpperTableData = mTableDataStack[mTableDataStack.size() - 2];
+            RowData::Pointer_t pRow = pUpperTableData->getCurrentRow();
+            unsigned int nCells = pRow->getCellCount();
+            if (nCells > 0)
+            {
+                uno::Reference<text::XTextRange> xCellStart = pRow->getCellStart(nCells - 1);
+                if (xCellStart.is())
+                {
+                    try
+                    {
+                        xCursor = xCellStart->getText()->createTextCursorByRange(
+                            xCellStart->getStart());
+                        xCursor->goLeft(1, false);
+                    }
+                    catch (const uno::RuntimeException&)
+                    {
+                        TOOLS_WARN_EXCEPTION(
+                            "writerfilter",
+                            "TableManager::endLevel: createTextCursorByRange() failed");
+                    }
+                }
+            }
+        }
         resolveCurrentTable();
+    }
 
     // Store the unfinished row as it will be used for the next table
     if (mbKeepUnfinishedRow)
-        mpUnfinishedRow = mTableDataStack.top()->getCurrentRow();
+        mpUnfinishedRow = mTableDataStack.back()->getCurrentRow();
     mState.endLevel();
-    mTableDataStack.pop();
+    mTableDataStack.pop_back();
 
-#ifdef DBG_UTIL
     TableData::Pointer_t pTableData;
 
     if (!mTableDataStack.empty())
-        pTableData = mTableDataStack.top();
+        pTableData = mTableDataStack.back();
 
+#ifdef DBG_UTIL
     TagLogger::getInstance().startElement("tablemanager.endLevel");
     TagLogger::getInstance().attribute("level", mTableDataStack.size());
+#endif
 
     if (pTableData != nullptr)
+    {
+#ifdef DBG_UTIL
         TagLogger::getInstance().attribute("openCell", pTableData->isCellOpen() ? "yes" : "no");
+#endif
+        if (pTableData->isCellOpen() && !pTableData->IsCellValid() && xCursor.is())
+        {
+            // The inner table is resolved and we have an outer table, but the currently opened
+            // cell's start position is no longer valid. Try to move the cursor back to where it was
+            // and update the cell start position accordingly.
+            try
+            {
+                xCursor->goRight(1, false);
+                pTableData->SetCellStart(xCursor->getStart());
+            }
+            catch (const uno::RuntimeException&)
+            {
+                TOOLS_WARN_EXCEPTION("writerfilter", "TableManager::endLevel: goRight() failed");
+            }
+        }
+    }
 
+#ifdef DBG_UTIL
     TagLogger::getInstance().endElement();
 #endif
 }
@@ -454,7 +507,7 @@ void TableManager::startLevel()
     TableData::Pointer_t pTableData;
 
     if (!mTableDataStack.empty())
-        pTableData = mTableDataStack.top();
+        pTableData = mTableDataStack.back();
 
     TagLogger::getInstance().startElement("tablemanager.startLevel");
     TagLogger::getInstance().attribute("level", mTableDataStack.size());
@@ -482,7 +535,7 @@ void TableManager::startLevel()
         mpUnfinishedRow.clear();
     }
 
-    mTableDataStack.push(pTableData2);
+    mTableDataStack.push_back(pTableData2);
     mState.startLevel();
 }
 
@@ -490,7 +543,7 @@ bool TableManager::isInTable()
 {
     bool bInTable = false;
     if (!mTableDataStack.empty())
-        bInTable = mTableDataStack.top()->getDepth() > 0;
+        bInTable = mTableDataStack.back()->getDepth() > 0;
     return bInTable;
 }
 
@@ -515,7 +568,7 @@ void TableManager::endRow()
 #ifdef DBG_UTIL
     TagLogger::getInstance().element("tablemanager.endRow");
 #endif
-    TableData::Pointer_t pTableData = mTableDataStack.top();
+    TableData::Pointer_t pTableData = mTableDataStack.back();
 
     // Add borderless w:gridBefore cell(s) to the row
     sal_uInt32 nGridBefore = getCurrentGridBefore();
@@ -589,11 +642,6 @@ void TableManager::cellDepth(sal_uInt32 nDepth)
     mnTableDepthNew = nDepth;
 }
 
-void TableManager::setTableStartsAtCellStart(bool bTableStartsAtCellStart)
-{
-    m_bTableStartsAtCellStart = bTableStartsAtCellStart;
-}
-
 void TableManager::setCellLastParaAfterAutospacing(bool bIsAfterAutospacing)
 {
     m_bCellLastParaAfterAutospacing = bIsAfterAutospacing;
@@ -603,7 +651,6 @@ TableManager::TableManager()
     : mnTableDepthNew(0)
     , mnTableDepth(0)
     , mbKeepUnfinishedRow(false)
-    , m_bTableStartsAtCellStart(false)
 {
     setRowEnd(false);
     setInCell(false);
@@ -612,6 +659,25 @@ TableManager::TableManager()
 }
 
 TableManager::~TableManager() = default;
+
+bool CellData::IsValid() const
+{
+    if (!mStart.is())
+    {
+        return false;
+    }
+
+    try
+    {
+        mStart->getStart();
+    }
+    catch (const uno::RuntimeException&)
+    {
+        return false;
+    }
+
+    return true;
+}
 }
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/writerfilter/source/dmapper/TableManager.hxx b/writerfilter/source/dmapper/TableManager.hxx
index e6600d35793d..f23b371f4a94 100644
--- a/writerfilter/source/dmapper/TableManager.hxx
+++ b/writerfilter/source/dmapper/TableManager.hxx
@@ -298,11 +298,9 @@ private:
 
        for each level of nested tables there is one frame in the stack
      */
-    std::stack<TableData::Pointer_t> mTableDataStack;
+    std::vector<TableData::Pointer_t> mTableDataStack;
     RowData::Pointer_t mpUnfinishedRow;
     bool mbKeepUnfinishedRow;
-    /// If this is a nested table, does it start at cell start?
-    bool m_bTableStartsAtCellStart;
 
     bool m_bCellLastParaAfterAutospacing;
 
@@ -518,7 +516,6 @@ public:
     /// Given a zero-based row/col, return the zero-based cell describing that grid, or SAL_MAX_UINT16 for invalid.
     sal_uInt32 findColumnCell( const sal_uInt32 nRow, const sal_uInt32 nCol );
 
-    void setTableStartsAtCellStart(bool bTableStartsAtCellStart);
     void setCellLastParaAfterAutospacing(bool bIsAfterAutospacing);
     bool isCellLastParaAfterAutospacing() const {return m_bCellLastParaAfterAutospacing;}
 };