incomplete inline markup within table cell generates malformed html

[Jun Omae]

(originally reported at ​th:#13959)

>>> import trac
>>> trac.__version__
'1.4.2'
>>> from trac.mimeview.api import RenderingContext
>>> from trac.resource import Resource
>>> from trac.test import EnvironmentStub
>>> from trac.test import MockRequest
>>> from trac.wiki.formatter import Formatter, format_to_html
>>> resource = Resource('wiki', 'WikiStart')
>>> env = EnvironmentStub()
>>> req = MockRequest(env)
>>> context = RenderingContext(resource, req.href, req.perm)
>>> context.req = req
>>> print(format_to_html(env, context, '|| text^1 ||'))
<table class="wiki">
<tr><td> text<sup>1
</td></tr></table>
</sup>
>>> print(format_to_html(env, context, '|| text^1^ ||'))
<table class="wiki">
<tr><td> text<sup>1</sup>
</td></tr></table>

>>> print(format_to_html(env, context, '|| text**1 ||'))
<table class="wiki">
<tr><td> text<strong>1
</td></tr></table>
</strong>
>>> print(format_to_html(env, context, '|| text**1** ||'))
<table class="wiki">
<tr><td> text<strong>1</strong>
</td></tr></table>

>>>

Complex patterns:

>>> print(format_to_html(env, context, '|| aaa**1 || bbb//2 || ccc^3 ||'))
<table class="wiki">
<tr><td> aaa<strong>1 </td><td> bbb<em>2 </td><td> ccc<sup>3
</td></tr></table>
</sup></em></strong>
>>> print(format_to_html(env, context, '|| aaa**1//2^3,,4~~5 ||'))
<table class="wiki">
<tr><td> aaa<strong>1<em>2<sup>3<sub>4<del>5
</td></tr></table>
</del></sub></sup></em></strong>

[anonymous]

Hello. I've just attached a patch file for this ticket.

Maybe the right way to fix this problem is adding some warning messages to user about those wrong markup tokens. But I'm not sure trac has warning mechanism for this case. So, the method I fix this issue is just resetting formatter's internal states, which means omitting close tags. Of course the formatter can insert close tags, if someone says it's the 'right' way. :)

Please review and give me some feedback.

[Jun Omae]

Thanks for the patch! I haven't test it but it seems that makes sense.

• Did you run unit and functional tests and confirm they pass?
• Could you please try to add unit tests for this issue.

See TracDev/SubmittingPatches and TracDev/UnitTests.

p.s. the attached file name is long a little. I think it would be good to be short and simple, and the suffix be *.diff or *.patch.

[Hong Choi <nik@…>]

Aha! I got it.

• Filename : shorten with .diff suffix
• Testcase : added very basic broken syntax test
• Tests : all passed. 3 were skipped by default.

[Jun Omae]

Thanks. However, I'd like to put </sup> before </td> in this case. Missing </sup> in your suggested changes.

[Hong Choi <nik@…>]

force to close broken wiki text decoration tokens

[Hong Choi <nik@…>]

Sure. If you say so. :)

And, I realize that the same patch should be applied on cell closing. For example, || a^1 || b^2 || should be parsed as a<sup>1</sup> and b<sup>2</sup>, not as a<sup>1 and b</sup>2. I added another test case for that.

[Jun Omae]

trac/wiki/formatter.py

--- trac/wiki/formatter.py (revision 17500)
+++ trac/wiki/formatter.py (working copy)
@@ -1092,7 +1092,10 @@
             attrs += ' style="text-align: %s"' % textalign
         td = '<%s%s>' % (cell, attrs)
         if self.in_table_cell:
-            td = '</%s>' % self.in_table_cell + td
+            unclosed = ''
+            for tag in self._open_tags:
+                    unclosed += self.close_tag(tag)
+            td = unclosed + '</%s>' % self.in_table_cell + td
         self.in_table_cell = cell
         return td
 

• self._open_tags should be used from the last entry.
  • refs. def flush_tags()
• self._open_tags should be clear after closing the tags.
• The indent should be 4 spaces

@@ -1126,6 +1129,8 @@
         if self.in_table_row and (not self.continue_table_row or force):
             self.in_table_row = 0
             if self.in_table_cell:
+                for tag in self._open_tags:
+                    self.out.write(self.close_tag(tag))
                 self.out.write('</%s>' % self.in_table_cell)
                 self.in_table_cell = ''
             self.out.write('</tr>')

• self.flush_tags() may be used here.

trac/wiki/tests/wiki-tests.txt

• Should add test with multiple markups:
  • e.g. || aaa^1**2 || bbb**1//2^3 ||
• Should add test which table cell doesn't end with ||
  • e.g. || aaa^1**2 || bbb**1//2^3

[Hong Choi <nik@…>]

Close tags in reversed way. Testcases for multiple token cases are added

[Hong Choi <nik@…>]

trac/wiki/formatter.py

• self._open_tags should be used from the last entry.
  • Sure!
• self._open_tags should be clear after closing the tags.
  • I don't think so. close_tag() handles everything.
• The indent should be 4 spaces
  • My mistake, corrected.
• self.flush_tags() may be used here.
  • close_tags() will take care of those things.

trac/wiki/tests/wiki-tests.txt

• Should add test with multiple markups
  • Added and passed.
• Should add test which table cell doesn't end with ||
  • Also added and passed.

Please let me know any further details! :)

[Jun Omae]

• self._open_tags should be clear after closing the tags.
  • I don't think so. close_tag() handles everything.

_open_tags shouldn't be changed during iteration of _open_tags (by close_tag() in this case).

• self.flush_tags() may be used here.
  • close_tags() will take care of those things.

We shouldn't put different logic to get the same result by flush_tags().

---

Revised and cleaned up the changes. I'm going to push it. Thanks.

• log:jomae.git@t13355_trunk for trunk
• log:jomae.git@t13355_1.4 for 1.4-stable

[Ryan J Ollos]

The latest revisions by Jun look good. Thank you for the contribution Hong Choi!

One tiny thing, I wonder if this would be simpler and more consistent with other code in module:

trac/wiki/formatter.py

@@ -1098 +1098 @@
         if self.in_table_cell:
             close_tags = ''.join(self._get_close_tag(tag)
                                  for tag in self.pop_tags())
-            td = close_tags + '</%s>' % self.in_table_cell + td
+            td = '%s</%s>%s' % (close_tags, self.in_table_cell, td)
         self.in_table_cell = cell
         return td

[Jun Omae]

Your suggestion makes sense. Thanks.

Pushed the changes with the suggestion in [17510] and merged in [17511].