Some of our tags, such as <optionalBlock> and <dropdownList> involves grouping...

Some of our tags, such as <optionalBlock> and <dropdownList> involves grouping of sibling table rows, and controlling visibility of them. So when such tags nest to each other, the visibility updates need to be done carefully, or else the visibility could get out of sync with the model (imagine outer group is made visible while inner group is not visible --- if all the rows are simply enumerated and visibility changed, we end up making the inner group visible.)

The rowVisibilityGroup object in hudson-behavior.js is used to coordinate this activity, and this test ensures that it's working.

changelog.html

@@ -58,6 +58,8 @@ Upcoming changes</a>
   <li class=bug>
     Allow blank rootDN in LDAPSecurityRealm.
     (<a href="http://jenkins.361315.n4.nabble.com/LDAP-and-empty-root-DN-values-td2216124.html">thread</a>)
+  <li class=bug>
+    Fixed the UI rendering problem when certain controls are nested together.
   <li class=rfe>
     Add active configurations in remote API for matrix projects.
     (<a href="http://issues.jenkins-ci.org/browse/JENKINS-9248">issue 9248</a>)

core/src/main/resources/lib/form/dropdownListBlock.jelly

@@ -50,9 +50,9 @@ THE SOFTWARE.
     </j:when>
     <j:when test="${dropdownListMode=='generateEntries'}">
       <!-- sandwich them by markers so that we know what to show/hide -->
-      <tr class="dropdownList-start" style="display:none">
+      <tr class="dropdownList-start rowvg-start">
         <j:if test="${!empty(attrs.staplerClass)}">
-          <td><input type="hidden" name="stapler-class" value="${attrs.staplerClass}"/></td>
+          <td style="display:none"><input type="hidden" name="stapler-class" value="${attrs.staplerClass}"/></td>
         </j:if>
       </tr>
       <j:choose>
@@ -65,7 +65,7 @@ THE SOFTWARE.
           <d:invokeBody />
         </j:otherwise>
       </j:choose>
-      <tr class="dropdownList-end" style="display:none" />
+      <tr class="dropdownList-end rowvg-end" />
     </j:when>
   </j:choose>
 </j:jelly>

core/src/main/resources/lib/form/optionalBlock.jelly

@@ -80,9 +80,10 @@ THE SOFTWARE.
       <j:if test="${attrs.help!=null}">
         <f:helpArea />
       </j:if>
+      <tr class="rowvg-start" />
       <d:invokeBody />
       <!-- end marker -->
-      <tr class="${attrs.inline?'':'row-set-end'} optional-block-end" />
+      <tr class="${attrs.inline?'':'row-set-end'} rowvg-end optional-block-end" />
     </j:when>
 
     <j:otherwise>

test/src/test/java/lib/form/RowVisibilityGroupTest.java

+package lib.form;
+
+import com.gargoylesoftware.htmlunit.html.HtmlElement;
+import com.gargoylesoftware.htmlunit.html.HtmlInput;
+import com.gargoylesoftware.htmlunit.html.HtmlOption;
+import com.gargoylesoftware.htmlunit.html.HtmlPage;
+import com.gargoylesoftware.htmlunit.html.HtmlSelect;
+import hudson.model.AbstractDescribableImpl;
+import hudson.model.Describable;
+import hudson.model.Descriptor;
+import net.sf.json.JSONObject;
+import org.jvnet.hudson.test.HudsonTestCase;
+import org.jvnet.hudson.test.TestExtension;
+import org.kohsuke.stapler.DataBoundConstructor;
+import org.kohsuke.stapler.StaplerRequest;
+
+import java.util.List;
+
+/**
+ * Tests the 'rowvg-start' and 'rowvg-end' CSS attributes and their effects.
+ *
+ * <p>
+ * Some of our tags, such as &lt;optionalBlock> and &lt;dropdownList> involves grouping of sibling table rows,
+ * and controlling visibility of them. So when such tags nest to each other, the visibility updates need to be
+ * done carefully, or else the visibility could get out of sync with the model (imagine outer group is made visible
+ * while inner group is not visible --- if all the rows are simply enumerated and visibility changed, we end up
+ * making the inner group visible.)
+ *
+ * <p>
+ * The rowVisibilityGroup object in hudson-behavior.js is used to coordinate this activity, and this test
+ * ensures that it's working.
+ *
+ * @author Kohsuke Kawaguchi
+ */
+public class RowVisibilityGroupTest extends HudsonTestCase implements Describable<RowVisibilityGroupTest> {
+    public Drink drink;
+    private Beer beer;
+
+    /**
+     * Nested optional blocks
+     */
+    public void test1() throws Exception {
+        HtmlPage p = createWebClient().goTo("self/test1");
+
+        HtmlElement outer = (HtmlElement)p.selectSingleNode("//INPUT[@name='outer']");
+        HtmlElement inner = (HtmlElement)p.selectSingleNode("//INPUT[@name='inner']");
+        HtmlInput field = (HtmlInput)p.selectSingleNode("//INPUT[@type='text'][@name='_.field']");
+
+        // outer gets unfolded, but inner should be still folded
+        outer.click();
+        assertFalse(field.isDisplayed());
+        // now click inner, to reveal the field
+        inner.click();
+        assertTrue(field.isDisplayed());
+
+        // folding outer should hide everything
+        outer.click();
+        assertFalse(field.isDisplayed());
+        // but if we unfold outer, everything should be revealed because inner is already checked.
+        outer.click();
+        assertTrue(field.isDisplayed());
+    }
+
+    /**
+     * optional block inside the dropdownDescriptorSelector
+     */
+    public void test2() throws Exception {
+        HtmlPage p = createWebClient().goTo("self/test2");
+
+        HtmlSelect s = (HtmlSelect)p.selectSingleNode("//SELECT");
+        List<HtmlOption> opts = s.getOptions();
+
+        // those first selections will load additional HTMLs
+        s.setSelectedAttribute(opts.get(0),true);
+        s.setSelectedAttribute(opts.get(1),true);
+
+        // now select back what's already loaded, to cause the existing elements to be displayed
+        s.setSelectedAttribute(opts.get(0),true);
+
+        // make sure that the inner control is still hidden
+        List<HtmlInput> textboxes = p.selectNodes("//INPUT[@name='_.textbox2']");
+        assertEquals(2,textboxes.size());
+        for (HtmlInput e : textboxes)
+            assertTrue(!e.isDisplayed());
+
+        // reveal the text box
+        List<HtmlInput> checkboxes = p.selectNodes("//INPUT[@name='inner']");
+        assertEquals(2,checkboxes.size());
+        checkboxes.get(0).click();
+        assertTrue(textboxes.get(0).isDisplayed());
+        textboxes.get(0).type("Budweiser");
+
+        // toggle the selection again
+        s.setSelectedAttribute(opts.get(1),true);
+        s.setSelectedAttribute(opts.get(0),true);
+
+        // make sure it's still displayed this time
+        assertTrue(checkboxes.get(0).isChecked());
+        assertTrue(textboxes.get(0).isDisplayed());
+
+        // make sure we get what we expect
+        submit(p.getFormByName("config"));
+        assertEqualDataBoundBeans(beer,new Beer("",new Nested("Budweiser")));
+    }
+
+    public void doSubmitTest2(StaplerRequest req) throws Exception {
+        JSONObject json = req.getSubmittedForm();
+        System.out.println(json);
+        beer = (Beer)req.bindJSON(Drink.class,json.getJSONObject("drink"));
+    }
+
+    public DescriptorImpl getDescriptor() {
+        return hudson.getDescriptorByType(DescriptorImpl.class);
+    }
+
+    @TestExtension
+    public static final class DescriptorImpl extends Descriptor<RowVisibilityGroupTest> {
+        public String getDisplayName() {
+            return null;
+        }
+    }
+
+    public static class Nested {
+        public String textbox2;
+
+        @DataBoundConstructor
+        public Nested(String textbox2) {
+            this.textbox2 = textbox2;
+        }
+    }
+
+    public static abstract class Drink extends AbstractDescribableImpl<Drink> {
+        public String textbox1;
+        public Nested inner;
+
+        @DataBoundConstructor
+        protected Drink(String textbox1, Nested inner) {
+            this.textbox1 = textbox1;
+            this.inner = inner;
+        }
+    }
+
+    public static class Beer extends Drink {
+        @DataBoundConstructor
+        public Beer(String textbox1, Nested inner) {
+            super(textbox1, inner);
+        }
+
+        @TestExtension
+        public static class DescriptorImpl extends Descriptor<Drink> {
+            @Override
+            public String getDisplayName() {
+                return "Beer";
+            }
+        }
+    }
+
+    public static class Coke extends Drink {
+        @DataBoundConstructor
+        public Coke(String textbox1, Nested inner) {
+            super(textbox1, inner);
+        }
+
+        @TestExtension
+        public static class DescriptorImpl extends Descriptor<Drink> {
+            @Override
+            public String getDisplayName() {
+                return "Coke";
+            }
+        }
+    }
+}

test/src/test/resources/lib/form/RowVisibilityGroupTest/Drink/config.jelly

+<!--
+The MIT License
+
+Copyright (c) 2011, CloudBees, Inc.
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in
+all copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+THE SOFTWARE.
+-->
+
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
+  <f:entry title="Textbox1" field="textbox1">
+    <f:textbox/>
+  </f:entry>
+  <f:optionalBlock title="Inner" name="inner">
+    <f:entry title="Textbox2" field="textbox2">
+      <f:textbox />
+    </f:entry>
+  </f:optionalBlock>
+</j:jelly>

test/src/test/resources/lib/form/RowVisibilityGroupTest/test1.jelly

+<!--
+The MIT License
+
+Copyright (c) 2011, CloudBees, Inc.
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in
+all copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+THE SOFTWARE.
+-->
+
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
+<l:layout title="Test collision between optionalBlocks">
+  <l:main-panel>
+    <f:form method="post" name="config" action="test1submit">
+      <f:optionalBlock title="Outer" name="outer">
+        <f:optionalBlock title="Inner" name="inner">
+          <f:entry title="Field" field="field">
+            <f:textbox />
+          </f:entry>
+        </f:optionalBlock>
+      </f:optionalBlock>
+    </f:form>
+  </l:main-panel>
+</l:layout>
+</j:jelly>

test/src/test/resources/lib/form/RowVisibilityGroupTest/test2.jelly

+<!--
+The MIT License
+
+Copyright (c) 2011, CloudBees, Inc.
+
+Permission is hereby granted, free of charge, to any person obtaining a copy
+of this software and associated documentation files (the "Software"), to deal
+in the Software without restriction, including without limitation the rights
+to use, copy, modify, merge, publish, distribute, sublicense, and/or sell
+copies of the Software, and to permit persons to whom the Software is
+furnished to do so, subject to the following conditions:
+
+The above copyright notice and this permission notice shall be included in
+all copies or substantial portions of the Software.
+
+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN
+THE SOFTWARE.
+-->
+
+<?jelly escape-by-default='true'?>
+<j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form">
+<l:layout title="Test collision between optionalBlocks">
+  <l:main-panel>
+    <j:set var="instance" value="${it}"/>
+    <j:set var="descriptor" value="${it.descriptor}"/>
+    <f:form method="post" name="config" action="submitTest2">
+      <f:dropdownDescriptorSelector field="drink" title="Drink" />
+      <f:block>
+        <f:submit value="submit"/>
+      </f:block>
+    </f:form>
+  </l:main-panel>
+</l:layout>
+</j:jelly>

war/src/main/webapp/scripts/behavior.js

@@ -54,12 +54,30 @@ var Behaviour = {
         this.applySubtree(document);
     },
 
+    /**
+     * Applies behaviour rules to a subtree/subforest.
+     *
+     * @param {HTMLElement|HTMLElement[]} startNode
+     *      Subtree/forest to apply rules.
+     *
+     *      Within a single subtree, rules are the outer loop and the nodes in the tree are the inner loop,
+     *      and sometimes the behaviour rules rely on this ordering to work correctly. When you pass a forest,
+     *      this semantics is preserved.
+     */
     applySubtree : function(startNode,includeSelf) {
         Behaviour.list._each(function(sheet) {
             for (var selector in sheet){
-                var list = findElementsBySelector(startNode,selector,includeSelf);
-                if (list.length>0)  // just to simplify setting of a breakpoint.
-                    list._each(sheet[selector]);
+                function apply(n) {
+                    var list = findElementsBySelector(n,selector,includeSelf);
+                    if (list.length>0)  // just to simplify setting of a breakpoint.
+                        list._each(sheet[selector]);
+                }
+
+                if (startNode instanceof Array) {
+                    startNode._each(apply)
+                } else {
+                    apply(startNode);
+                }
             }
         });
     },

war/src/main/webapp/scripts/hudson-behavior.js

@@ -240,7 +240,7 @@ function findFollowingTR(input, className) {
     // then next TR that matches the CSS
     do {
         tr = tr.nextSibling;
-    } while (tr != null && (tr.tagName != "TR" || tr.className != className));
+    } while (tr != null && (tr.tagName != "TR" || !Element.hasClassName(tr,className)));
 
     return tr;
 }
@@ -453,8 +453,7 @@ function renderOnDemand(e,callback,noBehaviour) {
         }
         Element.remove(e);
 
-        t.responseText.evalScripts();
-        elements.each(function(n) { Behaviour.applySubtree(n,true); });
+        Behaviour.applySubtree(elements,true);
 
         if (callback)   callback(t);
     });
@@ -805,6 +804,81 @@ var hudsonRules = {
         e.setAttribute("ref", checkbox.id = "cb"+(iota++));
     },
 
+    // see RowVisibilityGroupTest
+    "TR.rowvg-start" : function(e) {
+        // figure out the corresponding end marker
+        function findEnd(e) {
+            for( var depth=0; ; e=e.nextSibling) {
+                if(Element.hasClassName(e,"rowvg-start"))    depth++;
+                if(Element.hasClassName(e,"rowvg-end"))      depth--;
+                if(depth==0)    return e;
+            }
+        }
+
+        e.rowVisibilityGroup = {
+            outerVisible: true,
+            innerVisible: true,
+            /**
+             * TR that marks the beginning of this visibility group.
+             */
+            start: e,
+            /**
+             * TR that marks the end of this visibility group.
+             */
+            end: findEnd(e),
+
+            /**
+             * Considers the visibility of the row group from the point of view of outside.
+             * If you think of a row group like a logical DOM node, this is akin to its .style.display.
+             */
+            makeOuterVisisble : function(b) {
+                this.outerVisible = b;
+                this.updateVisibility();
+            },
+
+            /**
+             * Considers the visibility of the rows in this row group. Since all the rows in a rowvg
+             * shares the single visibility, this just needs to be one boolean, as opposed to many.
+             *
+             * If you think of a row group like a logical DOM node, this is akin to its children's .style.display.
+             */
+            makeInnerVisisble : function(b) {
+                this.innerVisible = b;
+                this.updateVisibility();
+            },
+
+            /**
+             * Based on innerVisible and outerVisible, update the relevant rows' actual CSS display attribute.
+             */
+            updateVisibility : function() {
+                var display = (this.outerVisible && this.innerVisible) ? "" : "none";
+                for (var e=this.start; e!=this.end; e=e.nextSibling) {
+                    if (e.rowVisibilityGroup && e!=this.start) {
+                        e.rowVisibilityGroup.makeOuterVisisble(this.innerVisible);
+                        e = e.rowVisibilityGroup.end; // the above call updates visibility up to e.rowVisibilityGroup.end inclusive
+                    } else {
+                        e.style.display = display;
+                    }
+                }
+            },
+
+            /**
+             * Enumerate each row and pass that to the given function.
+             *
+             * @param {boolean} recursive
+             *      If true, this visits all the rows from nested visibility groups.
+             */
+            eachRow : function(recursive,f) {
+                if (recursive) {
+                    for (var e=this.start; e!=this.end; e=e.nextSibling)
+                        f(e);
+                } else {
+                    throw "not implemented yet";
+                }
+            }
+        };
+    },
+
     "TR.row-set-end": function(e) { // see rowSet.jelly and optionalBlock.jelly
         // figure out the corresponding start block
         var end = e;
@@ -946,20 +1020,39 @@ var hudsonRules = {
     "SELECT.dropdownList" : function(e) {
         if(isInsideRemovable(e))    return;
 
-        e.subForms = [];
+        var subForms = [];
         var start = findFollowingTR(e, 'dropdownList-container').firstChild.nextSibling, end;
         do { start = start.firstChild; } while (start && start.tagName != 'TR');
-        if (start && start.className != 'dropdownList-start')
+
+        if (start && !Element.hasClassName(start,'dropdownList-start'))
             start = findFollowingTR(start, 'dropdownList-start');
         while (start != null) {
-            end = findFollowingTR(start, 'dropdownList-end');
-            e.subForms.push({ 'start': start, 'end': end });
-            start = findFollowingTR(end, 'dropdownList-start');
+            subForms.push(start);
+            start = findFollowingTR(start, 'dropdownList-start');
+        }
+
+        // control visibility
+        function updateDropDownList() {
+            for (var i = 0; i < subForms.length; i++) {
+                var show = e.selectedIndex == i;
+                var f = subForms[i];
+
+                if (show)   renderOnDemand(f.nextSibling);
+                f.rowVisibilityGroup.makeInnerVisisble(show);
+
+                // TODO: this is actually incorrect in the general case if nested vg uses field-disabled
+                // so far dropdownList doesn't create such a situation.
+                f.rowVisibilityGroup.eachRow(true, show?function(e) {
+                    e.removeAttribute("field-disabled");
+                } : function(e) {
+                    e.setAttribute("field-disabled","true");
+                });
+            }
         }
 
-        e.onchange = function () { updateDropDownList(e); };
+        e.onchange = updateDropDownList;
 
-        updateDropDownList(e);
+        updateDropDownList();
     },
 
     // select.jelly
@@ -1117,6 +1210,7 @@ function applyNameRef(s,e,id) {
     }
 }
 
+
 // used by optionalBlock.jelly to update the form status
 //   @param c     checkbox element
 function updateOptionalBlock(c,scroll) {
@@ -1124,39 +1218,21 @@ function updateOptionalBlock(c,scroll) {
     var s = c;
     while(!Element.hasClassName(s, "optional-block-start"))
         s = s.parentNode;
-    var tbl = s.parentNode;
-    var i = false;
-    var o = false;
-
-    var checked = xor(c.checked,Element.hasClassName(c,"negative"));
-    var lastRow = null;
-
-    for (var j = 0; tbl.rows[j]; j++) {
-        var n = tbl.rows[j];
 
-        if (i && Element.hasClassName(n, "optional-block-end"))
-            o = true;
+    // find the beginning of the rowvg
+    var vg = s;
+    while (!Element.hasClassName(vg,"rowvg-start"))
+        vg = vg.nextSibling;
 
-        if (i && !o) {
-            if (checked) {
-                n.style.display = "";
-                lastRow = n;
-            } else
-                n.style.display = "none";
-        }
+    var checked = xor(c.checked,Element.hasClassName(c,"negative"));
 
-        if (n==s) {
-            if (n.getAttribute('hasHelp') == 'true')
-                j++;
-            i = true;
-        }
-    }
+    vg.rowVisibilityGroup.makeInnerVisisble(checked);
 
     if(checked && scroll) {
         var D = YAHOO.util.Dom;
 
         var r = D.getRegion(s);
-        if(lastRow!=null)   r = r.union(D.getRegion(lastRow));
+        r = r.union(D.getRegion(vg.rowVisibilityGroup.end));
         scrollIntoView(r);
     }
 
@@ -1361,26 +1437,6 @@ Form.findMatchingInput = function(base, name) {
     return null;        // not found
 }
 
-// used witih <dropdownList> and <dropdownListBlock> to control visibility
-function updateDropDownList(sel) {
-    for (var i = 0; i < sel.subForms.length; i++) {
-        var show = sel.selectedIndex == i;
-        var f = sel.subForms[i];
-        var tr = f.start;
-        while (true) {
-            tr.style.display = (show ? "" : "none");
-            if(show) {
-                tr.removeAttribute("field-disabled");
-                renderOnDemand(tr);
-            } else    // buildFormData uses this attribute and ignores the contents
-                tr.setAttribute("field-disabled","true");
-            if (tr == f.end) break;
-            tr = tr.nextSibling;
-        }
-    }
-}
-
-
 // code for supporting repeatable.jelly
 var repeatableSupport = {
     // set by the inherited instance to the insertion point DIV