Edgewall Software
Modify

Opened 11 years ago

Closed 11 years ago

#7445 closed enhancement (fixed)

Invalid XHTML-1.0-Strict rendering

Reported by: Remy Blank <remy.blank@…> Owned by: remy.blank@…
Priority: low Milestone: 0.11.2
Component: rendering Version: 0.11-stable
Severity: minor Keywords: xhtml patch
Cc: Eli Carter Branch:
Release Notes:
API Changes:

Description

Various pages don't pass XHTML-1.0-Strict validation in the 0.11-stable branch:

  • All pages with an empty ctxtnav: empty <ul>
  • Wiki history, diff: <input> at wrong location
  • Tickets: <form> with name attribute
  • Milestone: empty <table>, <noscript> and <input> at wrong location
  • Milestone edit: <input> at wrong location
  • Admin: missing action attribute in <form>, <form> with method="POST" instead of post
  • About: usage of <tr> in a JavaScript string outside of a CDATA section

I will try to provide patches for all these issues.

Attachments (6)

xhtml-validation.patch (18.4 KB ) - added by Remy Blank <remy.blank@…> 11 years ago.
Patch against 0.11-stable [7352] fixing several XHTML-1.0-Strict validation errors
7445-xhtml-validation-2-r7384.patch (22.9 KB ) - added by Remy Blank <remy.blank@…> 11 years ago.
Same patch, but with ticket form id set to propertyform instead of propform
7445-xhtml-validation-3-r7388.patch (3.1 KB ) - added by Remy Blank <remy.blank@…> 11 years ago.
Patch against 0.11-stable fixing XHTML validation errors
7445-functional-test-validation-r7388.patch (58.5 KB ) - added by Remy Blank <remy.blank@…> 11 years ago.
Patch against 0.11-stable adding XHTML validation to all functional tests
7445-functional-test-validation-r7388.2.patch (58.5 KB ) - added by Remy Blank <remy.blank@…> 11 years ago.
Patch against 0.11-stable adding XHTML validation to all functional tests (fixed normpath → abspath)
7445-functional-test-validation-lxml-r7421.patch (61.8 KB ) - added by Remy Blank <remy.blank@…> 11 years ago.
Patch against 0.11-stable adding XHTML validation to all functional tests using lxml

Download all attachments as: .zip

Change History (28)

by Remy Blank <remy.blank@…>, 11 years ago

Attachment: xhtml-validation.patch added

Patch against 0.11-stable [7352] fixing several XHTML-1.0-Strict validation errors

comment:1 by Remy Blank <remy.blank@…>, 11 years ago

A few explanations about the patch above:

  • <form> tags must have an action attribute.
  • The content of the method attribute of a <form> must be lowercase.
  • JavaScript code containing < or & must be enclosed in a CDATA section or have these characters escaped.
  • <input>, <label> and <select> tags must be in a block.
  • Empty <ul> or <table> elements are not allowed.
  • The content of a <noscript> tag must be a block level element in XHTML-1.0-Strict. As this was the only occurrence of such a tag in Trac, I fixed it by using JavaScript to hide the button instead of enclosing it in a <noscript> tag.
  • <form> tags can only have an id attribute and must not have a name attribute. This required changing some functional tests, as one form had both with different names.

With the patch applied, all unit and functional tests pass (except for ticket.tests.functional.TestAdminMilestoneCompletedFuture, but only between 00:00 and 02:00 at night. No, I'm not kidding. I'm pretty sure this is a bug in the test itself, more precisely in the calculation of the completion date in the future. My timezone happens to be GMT+02:00, and I suspect this is related. I would suggest using a completion time several days in the future).

comment:2 by Remy Blank <remy.blank@…>, 11 years ago

Keywords: patch added

comment:3 by Christian Boos, 11 years ago

Milestone: 0.11.1
Owner: set to remy.blank@…

Testing, looks good so far.

in reply to:  1 comment:4 by osimons, 11 years ago

Replying to Remy Blank <remy.blank@pobox.com>:

With the patch applied, all unit and functional tests pass (except for ticket.tests.functional.TestAdminMilestoneCompletedFuture, but only between 00:00 and 02:00 at night. No, I'm not kidding. I'm pretty sure this is a bug in the test itself, more precisely in the calculation of the completion date in the future. My timezone happens to be GMT+02:00, and I suspect this is related. I would suggest using a completion time several days in the future).

Hehe. At 00:30 CET I ran into that test failure as well. Fixed in [7370:7371], and decided to stick with 1 day and just account for local timezone. Always useful to have borderline-calculations in unit tests…

Carry on with the actual topic of the ticket, please :-)

in reply to:  1 ; comment:5 by Christian Boos, 11 years ago

Replying to Remy Blank <remy.blank@pobox.com>:

  • <form> tags can only have an id attribute and must not have a name attribute. This required changing some functional tests, as one form had both with different names.

Last minute hesitation: was the change regarding the id attribute really needed? i.e. can't we just drop the name attribute and keep the id as it is? (some people may have used #propertyform in their custom style sheets).

in reply to:  5 comment:6 by Remy Blank <remy.blank@…>, 11 years ago

Replying to cboos:

Last minute hesitation: was the change regarding the id attribute really needed? i.e. can't we just drop the name attribute and keep the id as it is? (some people may have used #propertyform in their custom style sheets).

Well, I had to select one of the two names. Both are referenced in the functional tests, and there were more propform references than propertyform, so I selected the former.

But your argument is valid. Give me a few minutes and I'll drop the name="propform" attribute and leave id unchanged instead.

by Remy Blank <remy.blank@…>, 11 years ago

Same patch, but with ticket form id set to propertyform instead of propform

comment:7 by Remy Blank <remy.blank@…>, 11 years ago

The second patch keeps the propertyform id and just removes the propform name. I have also rebased the patch to [7384]. All tests pass here.

comment:8 by Christian Boos, 11 years ago

Resolution: fixed
Status: newclosed

Second patch committed in r7388. Thanks!

comment:9 by Remy Blank <remy.blank@…>, 11 years ago

Resolution: fixed
Status: closedreopened

I found a few more, one empty <ul>, a few empty <tbody> tags in special cases (empty query result list, empty file list, empty revision list), and an empty <optgroup> (milestone selection in tickets).

by Remy Blank <remy.blank@…>, 11 years ago

Patch against 0.11-stable fixing XHTML validation errors

comment:10 by Remy Blank <remy.blank@…>, 11 years ago

The empty <tbody> tags occurred in the following cases:

  • An empty ticket query result list
  • An empty file list
  • An empty revision list

The two latter typically occur when the Subversion repository is empty. In each case, I added a single line saying "No {items} found" with {items} = tickets, files, revisions. I find this also looks better compared to an empty table.

by Remy Blank <remy.blank@…>, 11 years ago

Patch against 0.11-stable adding XHTML validation to all functional tests

comment:11 by Remy Blank <remy.blank@…>, 11 years ago

The patch above is how I found these validation errors. It adds XHTML validation to all functional tests by hooking into Twill and validating every parsed page. Validation is done using PyXML and is optional, i.e. it is skipped if PyXML is not installed.

The systematic validation adds about 15% to the execution time of the complete test suite. The size of the patch is due to its containing the XHTML DTD files.

I would have added Eli Carter to Cc, as he seems to be the most active tester, but it seems that I don't have the permissions to do that.

comment:12 by Eli Carter, 11 years ago

Cc: Eli Carter added

by Remy Blank <remy.blank@…>, 11 years ago

Patch against 0.11-stable adding XHTML validation to all functional tests (fixed normpath → abspath)

comment:13 by Christian Boos, 11 years ago

Looks like PyXML is not supported anymore (that's what the PyXML download page on sourceforge says). Wouldn't it be better to use lxml for validation?

comment:14 by Remy Blank <remy.blank@…>, 11 years ago

Argh… Before starting, I researched what validating XML parsers were available in Python, and I narrowed the choice down to PyXML and lxml. I had both installed on my machine, and lxml seemed to require an additional library (libxml2), so I went with the former. I hadn't seen the note on the SourceForge download page.

Looking some more, I see that PyXML actually also requires an additional library (expat), so this point is moot anyway.

Let me try with lxml. It looks very interesting, so this will be a good exercise.

by Remy Blank <remy.blank@…>, 11 years ago

Patch against 0.11-stable adding XHTML validation to all functional tests using lxml

comment:15 by Remy Blank <remy.blank@…>, 11 years ago

The patch above validates with lxml instead of PyXML. The lxml documentation is a bit obscure, and it took me a while to figure out how to load the DTD from disk instead of from the network.

The patch also contains the fixes for the remaining validation errors.

in reply to:  14 comment:16 by anonymous, 11 years ago

Replying to Remy Blank <remy.blank@…>:

Argh… Before starting, I researched what validating XML parsers were available in Python, and I narrowed the choice down to PyXML and lxml. I had both installed on my machine, and lxml seemed to require an additional library (libxml2), so I went with the former. I hadn't seen the note on the SourceForge download page.

PyXML development has stalled so it is not a viable option.

comment:17 by Jonas Borgström, 11 years ago

Milestone: 0.11.10.11.2

I'm afraid I can't seem to get this patch to work right now. For some reason the custom lxml Resolver does not kick in so lxml tries to download xhtml1-strict.dtd from w3.org instead of using the bundled copy. I'm using lxml(2.1.1), libxml2(2.6.32) and libxslt(1.1.24) on osx leopard.

So I'm afraid this patch will have to wait until 0.11.2 since I'm hoping to release 0.11.1 really soon now.

in reply to:  17 comment:18 by Remy Blank <remy.blank@…>, 11 years ago

Replying to jonas:

I'm using lxml(2.1.1), libxml2(2.6.32) and libxslt(1.1.24) on osx leopard.

I'm using lxml-2.0.5, libxml2-2.6.31 and libxslt-1.1.24 on Gentoo. Most probably something has changed in lxml. I'll see if I can package lxml-2.1.1 for Gentoo (it's not available yet) and try to reproduce the problem.

So I'm afraid this patch will have to wait until 0.11.2 since I'm hoping to release 0.11.1 really soon now.

It's low priority anyway, so this is fine. You could have pulled only the changes that fix validation errors from the patch, but this can certainly wait for 0.11.2 as well.

Thanks for trying the patch!

comment:19 by Jonas Borgström, 11 years ago

Milestone: 0.11.30.11.2

comment:20 by Remy Blank <remy.blank@…>, 11 years ago

Strange. I have upgraded to lxml-2.1.1 and libxml2-2.6.32, and the patch still works well here. Maybe this is OSX-specific? I unfortunately don't have access to an OSX machine.

You may want to print the values of system_url, public_id, the first parameter passed to self.resolve_filename() and its result in better_twill._Resolver.resolve() to get an idea of what's going wrong.

I'll try to get access to an OSX Leopard machine somehow.

comment:21 by osimons, 11 years ago

Noticed the added note in 4:ticket:7503:

When the patch in #7445 will be applied, the table will have a single line containing "No files found" for an empty repository.

This has quite a few similarities with #5549, although that concerns just a message for diff rendering: 'no files' can occur for a number of reasons… I see this patch adds a number of 'no <something>' content, and we need to make sure that the message makes sense for all/most common reasons (typically permissions).

comment:22 by Jonas Borgström, 11 years ago

Resolution: fixed
Status: reopenedclosed

I finally managed to track down the problem I was seeing. For some reason lxml didn't work if it was imported after twill.

After a lot of digging I found that the mac specific module "ic" (used by urllib.getproxies) breaks lxml for some unknown reason.

In [7458] I applied a modified version of the patch which should work on OSX as well.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain remy.blank@….
The resolution will be deleted. Next status will be 'reopened'.
to as closed The owner will be changed from remy.blank@… 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.