-
Bug
-
Resolution: Fixed
-
Major
-
None
-
Powered by SuggestiMate
Those close tags that don't exist in HTML can cause browsers to misbehave.
[JENKINS-5458] Jelly HTML rendering shouldn't render the end tag for BR/HR/IMG etc
Looks like the best option here may be to implement this missing functionality in dom4j.. I see Hudson already uses a patched dom4j, so we can add to that (and hopefully get the patch accepted upstream).
Here's what I have so far:
--- src/java/org/dom4j/io/XMLWriter.java.orig Mon May 16 07:28:42 2005 +++ src/java/org/dom4j/io/XMLWriter.java Wed Feb 3 11:57:30 2010 @@ -17,6 +17,7 @@ import java.util.Iterator; import java.util.List; import java.util.Map; +import java.util.Stack; import java.util.StringTokenizer; import org.dom4j.Attribute; @@ -123,6 +124,9 @@ private char lastChar; + /** Whether any content was written in each tag */ + private Stack/*<Boolean>*/ hadContent = new Stack/*<Boolean>*/(); + /** Whether a flush should occur after writing a document */ private boolean autoFlush; @@ -641,6 +645,7 @@ public void startDocument() throws SAXException { try { + hadContent.clear(); writeDeclaration(); super.startDocument(); } catch (IOException e) { @@ -677,15 +682,20 @@ String qName, Attributes attributes) throws SAXException { try { charsAdded = false; + if (hadContent.size() > 0 && !((Boolean)hadContent.peek()).booleanValue()) { + writer.write('>'); + hadContent.pop(); + hadContent.push(Boolean.TRUE); + } writePrintln(); indent(); - writer.write("<"); + writer.write('<'); writer.write(qName); writeNamespaces(); writeAttributes(attributes); - writer.write(">"); ++indentLevel; + hadContent.push(Boolean.FALSE); lastOutputNodeType = Node.ELEMENT_NODE; lastElementClosed = false; @@ -706,11 +716,10 @@ indent(); } - // XXXX: need to determine this using a stack and checking for - // content / children - boolean hadContent = true; - - if (hadContent) { + if (hadContent.isEmpty()) { + throw new SAXException("Empty stack; endElement without matching startElement?"); + } + if (((Boolean)hadContent.pop()).booleanValue()) { writeClose(qName); } else { writeEmptyElementClose(qName); @@ -732,6 +741,12 @@ } try { + if (!hadContent.isEmpty() && !((Boolean)hadContent.peek()).booleanValue()) { + writer.write('>'); + hadContent.pop(); + hadContent.push(Boolean.TRUE); + } + /* * we can't use the writeString method here because it's possible we * don't receive all characters at once and calling writeString
If I read this bug correctly, it sounds like the dom4j change makes dom4j – and thus Hudson – output invalid XHTML. This is a very bad thing. Valid XHTML is wildly important to making output that is readable on a wide variety of devices, by a wide variety of browsers, and accessible to all.
If the problem is tags of this form:
<br></br>
IMHO they should be changed to <br/>
which is still valid XHTML
rather than <br>
bshine.. agreed, of course. You read about the first thing I tried.. the followup comment ("looks like the best option here..") is exactly to avoid that case of having invalid xhtml.
OK, I have a working setup:
- dom4j XMLWriter: make its ContentHandler interface methods recognize when a tag is empty.
- dom4j HTMLWriter: add setEnabled() method.. setting to false makes this just a passthru to XMLWriter. Also removed "P" from default omit-end-tag list.
- stapler-jelly: use 1.6.1-hudson-* version of dom4j instead of standard 1.6.1
- stapler-jelly: use servlet 2.4 API (for response.getContentType) instead of 2.3 (Hudson already uses 2.4)
- stapler-jelly: add HTMLWriterOutput (extends XMLOutput) with a notHTML() method to call setEnabled(false) on the HTMLWriter.
- stapler-jelly DefaultScriptInvoker: if the response Content-Type has already been set to non-text/html, keep current behavior (XMLWriter).. otherwise use HTMLWriterOutput.
- stapler-jelly ContentTypeTag: if the new content type is not text/html and the given XMLOuput is a HTMLWriterOutput then call notHTML() on it.
Looks good in my testing.. RSS feeds with flavor=rss20 conveniently includes <link>stuff</link> in its output, which gets broken by HTMLWriter since <link> in HTML has no end tag.. my testing confirms in this view the HTMLWriter was disabled and <link>stuff</link> came through ok.
Code changed in hudson
User: : mindless
Path:
trunk/hudson/main/core/pom.xml
trunk/hudson/main/core/src/main/resources/hudson/PluginManager/index.jelly
trunk/hudson/main/core/src/main/resources/hudson/model/ListView/configure-entries.jelly
trunk/hudson/main/core/src/main/resources/hudson/model/View/main.jelly
trunk/www/changelog.html
http://jenkins-ci.org/commit/27015
Log:
[FIXED JENKINS-5458] Update to stapler 1.132 to get better HTML render using "/>"
for elements that should not have a closing tag. Change in stapler:
https://stapler.dev.java.net/source/browse/stapler?view=rev&rev=1363
Now get patched dom4j via stapler dependency, so removed dependency in Hudson
core pom. Also changed a few "<br/>" back to "<br/>" now that they will
be rendered properly.
kohsuke pointed me to the org.dom4j.io.HTMLWriter class which can help with these tags.
I modified stapler's DefaultScriptInvoker.createXMLOutput() to use an HTMLWriter instead of the default XMLWriter.. however, jelly's impl.StaticTag class calls startElement and endElement directly rather than going through writeElement(Element). The former does not detect when the tag is empty and use the writeEmptyElementClose method that HTMLWriter overrides (the latter does). There is a comment in the XMLWriter source about this ("need to determine this using a stack and checking for content/children".. hadContent is hardcoded to true instead).
End result with the change I tried: The tags that should end in "/>" simply end in ">" (ie bad XHTML, but firefox seemed to display it ok).