tdf#155387: fix list restart and opening/closing conditions

1. List restart flag in the node does not yet mean that the restart
would actually happen: it will be ignored, if the next node is more
nested than the previous one.

2. When writing nested list items, we should not write additional
<li><ul> pairs for current level (previously, only level 0 was
skipped); same for closing.

Change-Id: I5f67e8fa9236e7e2e6dba2e0ec5dffbf0d7b7f8b
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/151958
Tested-by: Jenkins
Reviewed-by: Mike Kaganski <mike.kaganski@collabora.com>
Signed-off-by: Xisco Fauli <xiscofauli@libreoffice.org>
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/152010

7 files changed, 142 insertions, 25 deletions

diff --git a/sw/qa/extras/htmlexport/data/sub_li_and_ctd.fodt b/sw/qa/extras/htmlexport/data/sub_li_and_ctd.fodt
new file mode 100644
index 000000000000..b4a3977926c6
--- /dev/null
+++ b/sw/qa/extras/htmlexport/data/sub_li_and_ctd.fodt
@@ -0,0 +1,33 @@
+<?xml version="1.0" encoding="UTF-8"?>
+
+<office:document xmlns:office="urn:oasis:names:tc:opendocument:xmlns:office:1.0" xmlns:text="urn:oasis:names:tc:opendocument:xmlns:text:1.0" office:version="1.3" office:mimetype="application/vnd.oasis.opendocument.text">
+ <office:body>
+  <office:text>
+   <text:list>
+    <text:list-item>
+     <text:p>l1</text:p>
+     <text:p>l1_ctd1</text:p>
+     <text:list>
+      <text:list-item>
+       <text:p>l2</text:p>
+      </text:list-item>
+      <text:list-item>
+       <text:p>l2</text:p>
+      </text:list-item>
+     </text:list>
+     <text:p>l1_ctd2</text:p>
+     <text:list>
+      <text:list-item>
+       <text:list>
+        <text:list-item>
+         <text:p>l3</text:p>
+        </text:list-item>
+       </text:list>
+      </text:list-item>
+     </text:list>
+     <text:p>l1_ctd3</text:p>
+    </text:list-item>
+   </text:list>
+  </office:text>
+ </office:body>
+</office:document>
\ No newline at end of file
diff --git a/sw/qa/extras/htmlexport/htmlexport.cxx b/sw/qa/extras/htmlexport/htmlexport.cxx
index 1663a29f7e9e..d26128315d63 100644
--- a/sw/qa/extras/htmlexport/htmlexport.cxx
+++ b/sw/qa/extras/htmlexport/htmlexport.cxx
@@ -2512,6 +2512,40 @@ CPPUNIT_TEST_FIXTURE(SwHtmlDomExportTest, testReqIfTransparentTifImg)
     CPPUNIT_ASSERT_EQUAL(GraphicFileFormat::TIF, aDescriptor.GetFileFormat());
 }
 
+CPPUNIT_TEST_FIXTURE(SwHtmlDomExportTest, testTdf155387)
+{
+    createSwDoc("sub_li_and_ctd.fodt");
+    ExportToReqif();
+
+    SvMemoryStream aStream;
+    WrapReqifFromTempFile(aStream);
+    xmlDocUniquePtr pDoc = parseXmlStream(&aStream);
+    // Without the fix in place, this would fail
+    CPPUNIT_ASSERT(pDoc);
+
+    // Single top-level list
+    assertXPath(pDoc, "/reqif-xhtml:html/reqif-xhtml:div/reqif-xhtml:ul");
+    // Single top-level item
+    assertXPath(pDoc, "/reqif-xhtml:html/reqif-xhtml:div/reqif-xhtml:ul/reqif-xhtml:li");
+    // 4 top-level paragraphs in the item
+    assertXPath(pDoc,
+                "/reqif-xhtml:html/reqif-xhtml:div/reqif-xhtml:ul/reqif-xhtml:li/reqif-xhtml:p", 4);
+    // 2 sublists in the item
+    assertXPath(
+        pDoc, "/reqif-xhtml:html/reqif-xhtml:div/reqif-xhtml:ul/reqif-xhtml:li/reqif-xhtml:ul", 2);
+    // 2 items in the first sublist
+    assertXPath(pDoc,
+                "/reqif-xhtml:html/reqif-xhtml:div/reqif-xhtml:ul/reqif-xhtml:li/reqif-xhtml:ul[1]/"
+                "reqif-xhtml:li",
+                2);
+    // Check the last (most nested) subitem's text
+    assertXPathContent(
+        pDoc,
+        "/reqif-xhtml:html/reqif-xhtml:div/reqif-xhtml:ul/reqif-xhtml:li/reqif-xhtml:ul[2]/"
+        "reqif-xhtml:li/reqif-xhtml:ul/reqif-xhtml:li/reqif-xhtml:p",
+        "l3");
+}
+
 CPPUNIT_PLUGIN_IMPLEMENT();
 
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sw/source/filter/html/htmlatr.cxx b/sw/source/filter/html/htmlatr.cxx
index dae347aab98e..87b38fc0165f 100644
--- a/sw/source/filter/html/htmlatr.cxx
+++ b/sw/source/filter/html/htmlatr.cxx
@@ -983,7 +983,7 @@ static void OutHTML_SwFormatOff( Writer& rWrt, const SwHTMLTextCollOutputInfo& r
             const SwHTMLNumRuleInfo& rNRInfo = rHWrt.GetNumInfo();
             if( rNextInfo.GetNumRule() != rNRInfo.GetNumRule() ||
                 rNextInfo.GetDepth() != rNRInfo.GetDepth() ||
-                rNextInfo.IsNumbered() || rNextInfo.IsRestart() )
+                rNextInfo.IsNumbered() || rNextInfo.IsRestart(rNRInfo) )
                 rHWrt.ChangeParaToken( HtmlTokenId::NONE );
             OutHTML_NumberBulletListEnd( rHWrt, rNextInfo );
         }
diff --git a/sw/source/filter/html/htmlnum.cxx b/sw/source/filter/html/htmlnum.cxx
index bd8317bb5676..8fa120a630cf 100644
--- a/sw/source/filter/html/htmlnum.cxx
+++ b/sw/source/filter/html/htmlnum.cxx
@@ -44,4 +44,50 @@ void SwHTMLNumRuleInfo::Set(const SwTextNode& rTextNd)
     }
 }
 
+// Restart flag is only effective when this level is not below the previous
+bool SwHTMLNumRuleInfo::IsRestart(const SwHTMLNumRuleInfo& rPrev) const
+{
+    // calling this, when the rules are different, makes no sense
+    assert(rPrev.GetNumRule() == GetNumRule());
+
+    // An example ODF when the restart flag is set, but has no effect:
+    //   <text:list text:style-name="L1">
+    //    <text:list-item>
+    //     <text:p>l1</text:p>
+    //     <text:list>
+    //      <text:list-item>
+    //       <text:p>l2</text:p>
+    //      </text:list-item>
+    //      <text:list-item>
+    //       <text:p>l2</text:p>
+    //      </text:list-item>
+    //     </text:list>
+    //     <text:list>
+    //      <text:list-item>
+    //       <text:list>
+    //        <text:list-item>
+    //         <text:p>l3</text:p>
+    //        </text:list-item>
+    //       </text:list>
+    //      </text:list-item>
+    //     </text:list>
+    //    </text:list-item>
+    //   </text:list>
+    // In this case, "l3" is in a separate sublist than "l2", and so the "l3" node gets the
+    // "list restart" property. But the document rendering would be
+    //   1. l1
+    //        1.1. l2
+    //        1.2. l2
+    //             1.2.1. l3
+    // and the second-level numbering will not actually restart at the "l3" node.
+    //
+    // TODO/LATER: note that restarting may happen at different levels. In the code using this
+    // function, the level is reset to 0 whenever a restart is detected. And also, there is no
+    // code to actually descend to that new level (close corresponding li/ul/ol elements).
+
+    if (rPrev.GetDepth() < GetDepth())
+        return false; // No matter if the restart flag is set, it is not effective for subitems
+    return m_bRestart;
+}
+
 /* vim:set shiftwidth=4 softtabstop=4 expandtab: */
diff --git a/sw/source/filter/html/htmlnum.hxx b/sw/source/filter/html/htmlnum.hxx
index b73fd386e3f3..670f08229df2 100644
--- a/sw/source/filter/html/htmlnum.hxx
+++ b/sw/source/filter/html/htmlnum.hxx
@@ -38,8 +38,8 @@ class SwHTMLNumRuleInfo
     sal_uInt16      m_aNumStarts[MAXLEVEL];
     SwNumRule   *   m_pNumRule;       // current numbering
     sal_uInt16      m_nDeep;          // current numbering depth (1, 2, 3, ...)
-    bool        m_bRestart : 1;   // Export: restart numbering
-    bool        m_bNumbered : 1;  // Export: paragraph is numbered
+    bool        m_bRestart;   // Export: restart numbering
+    bool        m_bNumbered;  // Export: paragraph is numbered
 
 public:
 
@@ -75,7 +75,7 @@ public:
     void DecDepth() { if (m_nDeep!=0) --m_nDeep; }
     inline sal_uInt8 GetLevel() const;
 
-    bool IsRestart() const { return m_bRestart; }
+    bool IsRestart(const SwHTMLNumRuleInfo& rPrev) const;
 
     bool IsNumbered() const { return m_bNumbered; }
 
diff --git a/sw/source/filter/html/htmlnumwriter.cxx b/sw/source/filter/html/htmlnumwriter.cxx
index 2244cf00d1ac..365864810551 100644
--- a/sw/source/filter/html/htmlnumwriter.cxx
+++ b/sw/source/filter/html/htmlnumwriter.cxx
@@ -53,7 +53,7 @@ void SwHTMLWriter::FillNextNumInfo()
             // numbering level during import.
             if( bTable &&
                 m_pNextNumRuleInfo->GetNumRule()==GetNumInfo().GetNumRule() &&
-                !m_pNextNumRuleInfo->IsRestart() )
+                !m_pNextNumRuleInfo->IsRestart(GetNumInfo()) )
             {
                 m_pNextNumRuleInfo->SetDepth( GetNumInfo().GetDepth() );
             }
@@ -90,7 +90,7 @@ Writer& OutHTML_NumberBulletListStart( SwHTMLWriter& rWrt,
     SwHTMLNumRuleInfo& rPrevInfo = rWrt.GetNumInfo();
     bool bSameRule = rPrevInfo.GetNumRule() == rInfo.GetNumRule();
     if( bSameRule && rPrevInfo.GetDepth() >= rInfo.GetDepth() &&
-        !rInfo.IsRestart() )
+        !rInfo.IsRestart(rPrevInfo) )
     {
         return rWrt;
     }
@@ -205,7 +205,7 @@ Writer& OutHTML_NumberBulletListStart( SwHTMLWriter& rWrt,
     OSL_ENSURE( rWrt.m_nLastParaToken == HtmlTokenId::NONE,
                 "<PRE> was not closed before <OL>." );
     sal_uInt16 nPrevDepth =
-        (bSameRule && !rInfo.IsRestart()) ? rPrevInfo.GetDepth() : 0;
+        (bSameRule && !rInfo.IsRestart(rPrevInfo)) ? rPrevInfo.GetDepth() : 0;
 
     for( sal_uInt16 i=nPrevDepth; i<rInfo.GetDepth(); i++ )
     {
@@ -213,8 +213,9 @@ Writer& OutHTML_NumberBulletListStart( SwHTMLWriter& rWrt,
 
         rWrt.m_aBulletGrfs[i].clear();
         OString sOut = "<" + rWrt.GetNamespace();
-        if (rWrt.mbXHTML && (nPrevDepth != 0 || i != 0))
+        if (rWrt.mbXHTML && i != nPrevDepth)
         {
+            // for all skipped sublevels, add a li
             sOut += OOO_STRING_SVTOOLS_HTML_li "><" + rWrt.GetNamespace();
         }
         const SwNumFormat& rNumFormat = rInfo.GetNumRule()->Get( i );
@@ -321,7 +322,8 @@ Writer& OutHTML_NumberBulletListEnd( SwHTMLWriter& rWrt,
 {
     SwHTMLNumRuleInfo& rInfo = rWrt.GetNumInfo();
     bool bSameRule = rNextInfo.GetNumRule() == rInfo.GetNumRule();
-    bool bListEnd = !bSameRule || rNextInfo.GetDepth() < rInfo.GetDepth() || rNextInfo.IsRestart();
+    bool bListEnd = !bSameRule || rNextInfo.GetDepth() < rInfo.GetDepth() || rNextInfo.IsRestart(rInfo);
+    bool bNextIsSubitem = !bListEnd && rNextInfo.GetDepth() > rInfo.GetDepth();
 
     std::optional<bool> oAtLeastOneNumbered;
     if (!rInfo.IsNumbered())
@@ -354,15 +356,18 @@ Writer& OutHTML_NumberBulletListEnd( SwHTMLWriter& rWrt,
         }
     }
 
-    // The list is numbered if the previous text node is numbered or any other previous text
-    // node is numbered.
-    bool bPrevIsNumbered = rInfo.IsNumbered() || *oAtLeastOneNumbered;
-    // XHTML </li> for the list item content, if there is an open <li>.
-    if ((bListEnd && bPrevIsNumbered) || (!bListEnd && rNextInfo.IsNumbered()))
+    if (rWrt.mbXHTML)
     {
-        HTMLOutFuncs::Out_AsciiTag(
-            rWrt.Strm(), Concat2View(rWrt.GetNamespace() + OOO_STRING_SVTOOLS_HTML_li),
-            false);
+        // The list is numbered if the previous text node is numbered or any other previous text
+        // node is numbered.
+        bool bPrevIsNumbered = rInfo.IsNumbered() || *oAtLeastOneNumbered;
+        // XHTML </li> for the list item content, if there is an open <li>.
+        if ((bListEnd && bPrevIsNumbered) || (!bListEnd && !bNextIsSubitem && rNextInfo.IsNumbered()))
+        {
+            HTMLOutFuncs::Out_AsciiTag(
+                rWrt.Strm(), Concat2View(rWrt.GetNamespace() + OOO_STRING_SVTOOLS_HTML_li),
+                false);
+        }
     }
 
     if (!bListEnd)
@@ -382,7 +387,7 @@ Writer& OutHTML_NumberBulletListEnd( SwHTMLWriter& rWrt,
     OSL_ENSURE( rWrt.m_nLastParaToken == HtmlTokenId::NONE,
                 "<PRE> was not closed before </OL>." );
     sal_uInt16 nNextDepth =
-        (bSameRule && !rNextInfo.IsRestart()) ? rNextInfo.GetDepth() : 0;
+        (bSameRule && !rNextInfo.IsRestart(rInfo)) ? rNextInfo.GetDepth() : 0;
 
     // MIB 23.7.97: We must loop backwards, to get the right order of </OL>/</UL>
     for( sal_uInt16 i=rInfo.GetDepth(); i>nNextDepth; i-- )
@@ -399,8 +404,9 @@ Writer& OutHTML_NumberBulletListEnd( SwHTMLWriter& rWrt,
         else
             aTag = OOO_STRING_SVTOOLS_HTML_orderlist;
         HTMLOutFuncs::Out_AsciiTag( rWrt.Strm(), Concat2View(rWrt.GetNamespace() + aTag), false );
-        if (rWrt.mbXHTML && (nNextDepth != 0 || i != 1))
+        if (rWrt.mbXHTML && i != nNextDepth + 1)
         {
+            // for all skipped sublevels, close a li
             HTMLOutFuncs::Out_AsciiTag(
                 rWrt.Strm(), Concat2View(rWrt.GetNamespace() + OOO_STRING_SVTOOLS_HTML_li),
                 /*bOn=*/false);
diff --git a/sw/source/filter/html/htmltabw.cxx b/sw/source/filter/html/htmltabw.cxx
index ab611724474d..538a509db53c 100644
--- a/sw/source/filter/html/htmltabw.cxx
+++ b/sw/source/filter/html/htmltabw.cxx
@@ -1036,9 +1036,8 @@ Writer& OutHTML_SwTableNode( Writer& rWrt, SwTableNode & rNode,
         if( aLRItem.GetLeft() > 0 && rHTMLWrt.m_nDefListMargin > 0 &&
             ( !rHTMLWrt.GetNumInfo().GetNumRule() ||
               ( rHTMLWrt.GetNextNumInfo() &&
-                (rHTMLWrt.GetNextNumInfo()->IsRestart() ||
-                 rHTMLWrt.GetNumInfo().GetNumRule() !=
-                    rHTMLWrt.GetNextNumInfo()->GetNumRule()) ) ) )
+                (rHTMLWrt.GetNumInfo().GetNumRule() != rHTMLWrt.GetNextNumInfo()->GetNumRule() ||
+                 rHTMLWrt.GetNextNumInfo()->IsRestart(rHTMLWrt.GetNumInfo())) ) ) )
         {
             // If the paragraph before the table is not numbered or the
             // paragraph after the table starts with a new numbering or with
@@ -1186,9 +1185,8 @@ Writer& OutHTML_SwTableNode( Writer& rWrt, SwTableNode & rNode,
     rHTMLWrt.m_bOutTable = false;
 
     if( rHTMLWrt.GetNextNumInfo() &&
-        !rHTMLWrt.GetNextNumInfo()->IsRestart() &&
-        rHTMLWrt.GetNextNumInfo()->GetNumRule() ==
-            rHTMLWrt.GetNumInfo().GetNumRule() )
+        rHTMLWrt.GetNextNumInfo()->GetNumRule() == rHTMLWrt.GetNumInfo().GetNumRule() &&
+        !rHTMLWrt.GetNextNumInfo()->IsRestart(rHTMLWrt.GetNumInfo()) )
     {
         // If the paragraph after the table is numbered with the same rule as the
         // one before, then the NumInfo of the next paragraph holds the level of