Edgewall Software
Modify

Opened 7 years ago

Closed 6 years ago

#5647 closed defect (fixed)

Wiki page with macro is not valid XHTML.

Reported by: Blackhex Owned by: ebray
Priority: normal Milestone: 0.11.2
Component: wiki system Version: devel
Severity: minor Keywords:
Cc: blackhex@…
Release Notes:
API Changes:

Description

Trac: trunk (r5790)

This should be probably reported as separated bugs but they are connected and very minor.

In attached file check.html is result of XHTML validation of file page.html. First error is probably cause by missing DTD inclusion. Second is caused becase [[ViewTopic]] macro returns block content and wiki formater formats returned content of IWikiMacroProvider implementator inside <P> element. Is it possible to not generate that macro content inside <P> element?

And the last and most disturbing problem for me is that in Firefox or Konqueror page.html is not displayed properly. Page rendering is stopped right after <TEXTAREA/> tag. Try just open it in Firefox to see what I mean. When I change manually <TEXTAREA/> tag to <TEXTAREA><TEXTAREA/> tags problem don't occurs. Maybe it's kind of bug in Firefox and Konqueror, maybe it's bug in my code but I don't see any obvious problem. If it isn't, is it possible to change formatter to return complete tags instead of short ones as it used to be in 0.10?

Attachments (4)

check (7.9 KB) - added by Blackhex 7 years ago.
Result of XHTML validation.
page.html (11.4 KB) - added by Blackhex 7 years ago.
Not validated page.
ticket-5647-r7499.patch (1.2 KB) - added by ebray 6 years ago.
Here's a simple patch that adds a slightly more flexible check for div or table (that isn't a code block).
ticket-5647-r7601.patch (1.2 KB) - added by ebray 6 years ago.
Updated patch for latest trunk with slightly more flexible RE. Also removed a seemingly superfulous unicode() call.

Download all attachments as: .zip

Change History (16)

Changed 7 years ago by Blackhex

Result of XHTML validation.

Changed 7 years ago by Blackhex

Not validated page.

comment:1 Changed 6 years ago by ebray

I was just going to make a MailingList post about this very issue, but it looks like there's a ticket. Has anyone thought of a way to render a macro containing block-level elements outside the containing <p>? I'd argue there is no good way outside of some ugly hack using ITemplateStreamFilter. Perhaps the macro API could be expanded somewhat to allow specifying whether a macro will be rendered as a block-level or inline element.

comment:2 Changed 6 years ago by ebray

Nevermind—I realize now that the wiki formatter *should* close <p> tags before rendering a <div> or <table>. However, there is at least one macro in Trac (namely, TicketQuery with format=table) where this is not handled correctly, since the formatter is checking for an Element object, and in this case gets a Stream.

Changed 6 years ago by ebray

Here's a simple patch that adds a slightly more flexible check for div or table (that isn't a code block).

comment:3 Changed 6 years ago by ebray

One more validation issue with the [[TicketQuery(format=table)]] macro. The generated html starts out like this: <div xmlns="http://www.w3.org/1999/xhtml"> Odd, considering that when the same template is included in query.html, the xmlns attribute is stripped, but not when used in the macro. Is this a known bug in Genshi?

comment:4 Changed 6 years ago by martin@…

When macros are always put in <p> tags then the following convention could be using:

macros which return block content produce a leading closing </p> and a trailing opening <p>.

Future implementation of the wiki formatter could then check if the returned text starts with </p> and avoid the <p>'s altogether: not printing own tags and stripping the surrounding ones of the macro.

comment:5 Changed 6 years ago by cboos

  • Milestone set to 0.11.2
  • Owner changed from cboos to ebray

Ouch no, the macros should certainly not return invalid XHTML. As ebray said, the macro API could be expanded somewhat to allow specifying whether a macro will be rendered as a block-level or inline element. In the meantime, we could cope with this issue like suggested by attachment:ticket-5647-r7499.patch.

Feedback on that patch from the original reporter would be welcomed.

comment:6 Changed 6 years ago by ebray

The one problem I've found with that patch (but haven't fixed yet) is that neither _code_block_re or _block_elem_re account for the possibility of leading whitespace. This can occur if, for example, the macro's template is for whatever reason a fully formed XHTML document. For example:

<html xmlns="..." xmlns:py="..." py:strip="">
  <body py:strip="">
    <div>
      ...
    </div>
  </body>
</html>

In this case the macro will be rendered with the leading whitespace before the first <div>, and thus won't be matched by either RE. Shouldn't need anything more than a \s* after the ^ in each RE though.

comment:7 Changed 6 years ago by cboos

I think that _code_block_re was anyway for a very narrow case (when the <div>...</div> is in one single line), so probably only the new _block_elem_re should be smarter about that, something like:

    _block_elem_re = re.compile(r'^\s*<(div|table)(?:\s+[^>]+)?>', 
                                re.MULTILINE)

comment:8 Changed 6 years ago by ebray

Ah, yes. Multiline too. And perhaps even IGNORECASE (if it's properly formed that shouldn't be necessary, but it can't hurt).

comment:9 Changed 6 years ago by cboos

  • Summary changed from Wiki page with macro is not walid XHTML. to Wiki page with macro is not valid XHTML.

Any chance to get an updated patch?

comment:10 Changed 6 years ago by ebray

Odd. I meant to submit one this morning, but apparently I just left the tab open before actually clicking 'Add attachment'. Must have gotten distracted by something.

Changed 6 years ago by ebray

Updated patch for latest trunk with slightly more flexible RE. Also removed a seemingly superfulous unicode() call.

comment:11 Changed 6 years ago by cboos

Patch looks fine, I'll test it.

However I just read again the ticket, and regarding comment:2, I think we should do better in that case, i.e. look into the Stream.

comment:12 Changed 6 years ago by cboos

  • Resolution set to fixed
  • Status changed from new to closed

Patch committed, part of [7618:7620].

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain ebray.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from ebray to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.