[JENKINS-5458] Jelly HTML rendering shouldn't render the end tag for BR/HR/IMG etc

[jenkins-infra-bot]

Those close tags that don't exist in HTML can cause browsers to misbehave.

---

Originally reported by kohsuke, imported from: Jelly HTML rendering shouldn't render the end tag for BR/HR/IMG etc

• assignee: mindless
• status: Closed
• priority: Major
• component(s): core
• resolution: Fixed
• resolved: 2010-02-04T18:10:53+00:00
• votes: 0
• watchers: 1
• imported: 2025-11-24

Raw content of original issue

Those close tags that don't exist in HTML can cause browsers to misbehave.

[jenkins-infra-bot]

mindless:

• Original comment link
• Raw content of original comment:

  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).
  

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).

[jenkins-infra-bot]

mindless:

• Original comment link
• Raw content of original comment:

  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
  
  
  

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.javaWed 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

[jenkins-infra-bot]

mindless:

• Original comment link
• Raw content of original comment:

  Patch filed in dom4j project.
  

Patch filed in dom4j project.

[jenkins-infra-bot]

bshine:

• Original comment link
• Raw content of original comment:

  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> 
  

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:



IMHO they should be changed to

which is still valid XHTML
rather than

[jenkins-infra-bot]

mindless:

• Original comment link
• Raw content of original comment:

  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.
  

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.

[jenkins-infra-bot]

mindless:

• Original comment link
• Raw content of original comment:

  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.
  

OK, I have a working setup:

1. dom4j XMLWriter: make its ContentHandler interface methods recognize when a tag is empty.
2. dom4j HTMLWriter: add setEnabled() method.. setting to false makes this just a passthru to XMLWriter. Also removed "P" from default omit-end-tag list.
3. stapler-jelly: use 1.6.1-hudson-* version of dom4j instead of standard 1.6.1
4. stapler-jelly: use servlet 2.4 API (for response.getContentType) instead of 2.3 (Hudson already uses 2.4)
5. stapler-jelly: add HTMLWriterOutput (extends XMLOutput) with a notHTML() method to call setEnabled(false) on the HTMLWriter.
6. stapler-jelly DefaultScriptInvoker: if the response Content-Type has already been set to non-text/html, keep current behavior (XMLWriter).. otherwise use HTMLWriterOutput.
7. 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 stuff in its output, which gets broken by HTMLWriter since in HTML has no end tag.. my testing confirms in this view the HTMLWriter was disabled and stuff came through ok.

[jenkins-infra-bot]

kohsuke:

• Original comment link
• Raw content of original comment:

  dom4j 1.6.1-hudson-3 is deployed with this patch.
  

dom4j 1.6.1-hudson-3 is deployed with this patch.

[jenkins-infra-bot]

scm_issue_link:

• Original comment link
• Raw content of original comment:

  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.
  

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 "
" back to "
" now that they will
be rendered properly.