Defect #32971: New line between list items break a list - Redmine

Custom queries

Documentation issues
Open defects
Open features
Patch queue
Plugin issues
Translation patches

Actions

Copy link

Defect #32971

closed

New line between list items break a list

Added by Jihyeon Gim about 4 years ago. Updated about 4 years ago.

Status:

Closed

Priority:

Normal

Assignee:

Go MAEDA

Category:

Text formatting

Target version:

4.1.1

Start date:

Due date:

% Done:

Estimated time:

Resolution:

Fixed

Affected version:

4.1.0

Description

New line between list items break a list when I try to write ordered/unordered list with textile.

Example

* list1

  * list2
  * list3

# list4

  # list5
  # list6

In 3.2.1

In 4.1.0

Files

Download all files

3.2.1.png (20.4 KB) 3.2.1.png		Jihyeon Gim, 2020-02-08 15:02
4.1.0.png (13.6 KB) 4.1.0.png		Jihyeon Gim, 2020-02-08 15:02
32971-fixed.patch (824 Bytes) 32971-fixed.patch		Yuichi HARADA, 2020-02-17 07:58
32971-fixed-v3.patch (827 Bytes) 32971-fixed-v3.patch		Yuichi HARADA, 2020-02-19 07:17
32971-fixed-v4.patch (1.69 KB) 32971-fixed-v4.patch		Yuichi HARADA, 2020-02-20 08:58

Related issues

Related to Redmine - Defect #29756: \f or \v character in Textile markup may cause RegexpError exception

Closed

Go MAEDA

Actions

Issue # Delay: days Cancel

History
Notes
Property changes
Associated revisions

Actions

Copy link

Updated by Yuichi HARADA about 4 years ago

File 32971-fixed.patch 32971-fixed.patch added

I think the behavior has probably changed by applying r17603 ( Defect !#29756: \f or \v character in Textile markup may cause RegexpError exception ).

https://www.redmine.org/projects/redmine/repository/revisions/17603/diff/trunk/lib/redmine/wiki_formatting/textile/redcloth3.rb

It could be resolved with the following patch.

diff --git a/lib/redmine/wiki_formatting/textile/redcloth3.rb b/lib/redmine/wiki_formatting/textile/redcloth3.rb
index 80e0a3626..ff4687f7e 100644
--- a/lib/redmine/wiki_formatting/textile/redcloth3.rb
+++ b/lib/redmine/wiki_formatting/textile/redcloth3.rb
@@ -1020,10 +1020,13 @@ class RedCloth3 < String
     end

     def flush_left( text )
-        indt = 0
-        if text =~ /^ /
+        if /(?![\r\n\t ])[[:cntrl:]]/.match?(text)
+            text.gsub!(/(?![\r\n\t ])[[:cntrl:]]/, '')
+        end
+        if /^ /.match?(text) && text.length > 1
+            indt = 0
             unless text.empty?
-                indt += 1 while text !~ /^ {#{indt}}[^ ]/
+                indt += 1 while text !~ /^ {#{indt}}\S/
             end
             if indt.nonzero?
                 text.gsub!( /^ {#{indt}}/, '' )

Actions

Copy link

Updated by Jihyeon Gim about 4 years ago

Thanks for your help Yuichi! :)

This patch works gracefully well!

Thank you again for patching it :)

Actions

Copy link

Updated by Go MAEDA about 4 years ago

Related to Defect #29756: \f or \v character in Textile markup may cause RegexpError exception added

Actions

Copy link

Updated by Go MAEDA about 4 years ago

Target version set to Candidate for next minor release

Actions

Copy link

Updated by Yuichi HARADA about 4 years ago

File 32971-fixed-v2.patch added

Yuichi HARADA wrote:

I think the behavior has probably changed by applying r17603 ( Defect !#29756: \f or \v character in Textile markup may cause RegexpError exception ).

https://www.redmine.org/projects/redmine/repository/revisions/17603/diff/trunk/lib/redmine/wiki_formatting/textile/redcloth3.rb

It could be resolved with the following patch.

[...]

Sorry, I found that String#lstrip! could do the same.

https://www.rubydoc.info/stdlib/core/String#lstrip!-instance_method

diff --git a/lib/redmine/wiki_formatting/textile/redcloth3.rb b/lib/redmine/wiki_formatting/textile/redcloth3.rb
index 80e0a3626..815f860e6 100644
--- a/lib/redmine/wiki_formatting/textile/redcloth3.rb
+++ b/lib/redmine/wiki_formatting/textile/redcloth3.rb
@@ -1020,15 +1020,7 @@ class RedCloth3 < String
     end

     def flush_left( text )
-        indt = 0
-        if text =~ /^ /
-            unless text.empty?
-                indt += 1 while text !~ /^ {#{indt}}[^ ]/
-            end
-            if indt.nonzero?
-                text.gsub!( /^ {#{indt}}/, '' )
-            end
-        end
+        text&.lstrip!
     end

     def footnote_ref( text )

Actions

Copy link

Updated by Yuichi HARADA about 4 years ago

Yuichi HARADA wrote:

Yuichi HARADA wrote:

I think the behavior has probably changed by applying r17603 ( Defect !#29756: \f or \v character in Textile markup may cause RegexpError exception ).

https://www.redmine.org/projects/redmine/repository/revisions/17603/diff/trunk/lib/redmine/wiki_formatting/textile/redcloth3.rb

It could be resolved with the following patch.

[...]

Sorry, I found that String#lstrip! could do the same.

https://www.rubydoc.info/stdlib/core/String#lstrip!-instance_method

[...]

It did not work well with attachment:32971-fixed-v2.patch . The same problem occurred as in the previous situation.
Please delete attachment:32971-fixed-v2.patch .

Actions

Copy link

Updated by Go MAEDA about 4 years ago

File deleted (~~32971-fixed-v2.patch~~)

Actions

Copy link

Updated by Yuichi HARADA about 4 years ago

File 32971-fixed-v3.patch 32971-fixed-v3.patch added

I fixed the 32971-fixed.patch because sometimes the flush_left method did not end.

diff --git a/lib/redmine/wiki_formatting/textile/redcloth3.rb b/lib/redmine/wiki_formatting/textile/redcloth3.rb
index 80e0a3626..c9bdd0aeb 100644
--- a/lib/redmine/wiki_formatting/textile/redcloth3.rb
+++ b/lib/redmine/wiki_formatting/textile/redcloth3.rb
@@ -1020,11 +1020,12 @@ class RedCloth3 < String
     end

     def flush_left( text )
-        indt = 0
-        if text =~ /^ /
-            unless text.empty?
-                indt += 1 while text !~ /^ {#{indt}}[^ ]/
-            end
+        if /(?![\r\n\t ])[[:cntrl:]]/.match?(text)
+            text.gsub!(/(?![\r\n\t ])[[:cntrl:]]/, '')
+        end
+        if /^ +\S/.match?(text)
+            indt = 0
+            indt += 1 while !/^ {#{indt}}\S/.match?(text)
             if indt.nonzero?
                 text.gsub!( /^ {#{indt}}/, '' )
             end

Actions

Copy link

Updated by Kevin Fischer about 4 years ago

How about adding a unit test to prevent future regressions of this conversion?

Actions

Copy link

#10

Updated by Yuichi HARADA about 4 years ago

Ji-Hyeon Gim wrote:

New line between list items break a list when I try to write ordered/unordered list with textile.
Example
* list1

  * list2
  * list3

# list4

  # list5
  # list6
In 3.2.1

I noticed when looking at source:trunk/test/unit/lib/redmine/wiki_formatting/textile_formatter_test.rb#L148 that this nested list is probably wrong for Textile.

  def test_nested_lists
    raw = <<~RAW
      # Item 1
      # Item 2
      ** Item 2a
      ** Item 2b
      # Item 3
      ** Item 3a
    RAW
    expected = <<~EXPECTED
      <ol>
        <li>Item 1</li>
        <li>Item 2
          <ul>
            <li>Item 2a</li>
            <li>Item 2b</li>
          </ul>
        </li>
        <li>Item 3
          <ul>
            <li>Item 3a</li>
          </ul>
        </li>
      </ol>
    EXPECTED
    assert_equal expected.gsub(%r{\s+}, ''), to_html(raw).gsub(%r{\s+}, '')
  end

Actions

Copy link

#11

Updated by Yuichi HARADA about 4 years ago

File 32971-fixed-v4.patch 32971-fixed-v4.patch added

Kevin Fischer wrote:

How about adding a unit test to prevent future regressions of this conversion?

Thank you for pointing it out.
I have added the following test to 32971-fixed-v3.patch .

diff --git a/test/unit/lib/redmine/wiki_formatting/textile_formatter_test.rb b/test/unit/lib/redmine/wiki_formatting/textile_formatter_test.rb
index 19128524e..2358ded58 100644
--- a/test/unit/lib/redmine/wiki_formatting/textile_formatter_test.rb
+++ b/test/unit/lib/redmine/wiki_formatting/textile_formatter_test.rb
@@ -171,6 +171,24 @@ class Redmine::WikiFormatting::TextileFormatterTest < ActionView::TestCase
       </ol>
     EXPECTED
     assert_equal expected.gsub(%r{\s+}, ''), to_html(raw).gsub(%r{\s+}, '')
+
+    raw = <<~RAW
+      * Item-1
+
+        * Item-1a
+        * Item-1b
+    RAW
+    expected = <<~EXPECTED
+      <ul>
+        <li>Item-1
+          <ul>
+            <li>Item-1a</li>
+            <li>Item-1b</li>
+          </ul>
+        </li>
+      </ul>
+    EXPECTED
+    assert_equal expected.gsub(%r{\s+}, ''), to_html(raw).gsub(%r{\s+}, '')
   end

   def test_escaping

Actions

Copy link

#12