Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-5458

Jelly HTML rendering shouldn't render the end tag for BR/HR/IMG etc

    • Icon: Bug Bug
    • Resolution: Fixed
    • Icon: Major Major
    • core
    • None

      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

          Alan Harder added a 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).

          Alan Harder added a 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).

          Alan Harder added a 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
          

          Alan Harder added a 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

          Alan Harder added a comment -

          Patch filed in dom4j project.

          Alan Harder added a comment - Patch filed in dom4j project.

          bshine added a 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>

          bshine added a 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>

          Alan Harder added a 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.

          Alan Harder added a 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.

          Alan Harder added a comment -

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

          Alan Harder added a 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.

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

          Kohsuke Kawaguchi added a comment - dom4j 1.6.1-hudson-3 is deployed with this patch.

          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.

          SCM/JIRA link daemon added a 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.

            mindless Alan Harder
            kohsuke Kohsuke Kawaguchi
            Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

              Created:
              Updated:
              Resolved: