You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@ofbiz.apache.org by jl...@apache.org on 2020/07/31 17:28:04 UTC

[ofbiz-framework] branch trunk updated: Improved: WidgetWorker should not write generated html to Appendable (OFBIZ-11907) (#221)

This is an automated email from the ASF dual-hosted git repository.

jleroux pushed a commit to branch trunk
in repository https://gitbox.apache.org/repos/asf/ofbiz-framework.git


The following commit(s) were added to refs/heads/trunk by this push:
     new 9f3e52e  Improved: WidgetWorker should not write generated html to Appendable (OFBIZ-11907) (#221)
9f3e52e is described below

commit 9f3e52e7ab5f1aa32d30875d5ab9100e008f00bf
Author: Daniel Watford <da...@watfordconsulting.com>
AuthorDate: Fri Jul 31 18:27:52 2020 +0100

    Improved: WidgetWorker should not write generated html to Appendable (OFBIZ-11907) (#221)
    
    * Improved: WidgetWorker should not write generated html to Appendable
    
    (OFBIZ-11907)
    
    Refactoring of WidgetWorker so that it generates URI and JSoup Element
    objects to represent created URLs, hidden forms and anchor tags. This
    replaces the previous approach where WidgetWorker would write string
    representations of the URLS, hidden forms and anchor tags directly to an
    Appendable object passed to it.
    
    Callers to WidgetWorker have been modified to render the new objects
    created by WidgetWorker to their relevant I/O.
    
    * Improved: Handle spaces in link targets when building URIs
    
    (OFBIZ-11907)
    
    Corrected handling of spaces in target values passed to WidgetWorker
    when bulding URIs. Spaces are URL encoded as %20.
---
 .../java/org/apache/ofbiz/widget/WidgetWorker.java | 309 +++++++--------------
 .../org/apache/ofbiz/widget/model/ModelTree.java   |   9 +-
 .../widget/renderer/html/HtmlMenuRenderer.java     |  13 +-
 .../widget/renderer/html/HtmlTreeRenderer.java     |   7 +-
 .../ofbiz/widget/renderer/macro/FtlWriter.java     |  71 +++++
 .../widget/renderer/macro/MacroFormRenderer.java   |  85 +++---
 .../widget/renderer/macro/MacroMenuRenderer.java   |  17 +-
 .../widget/renderer/macro/MacroScreenRenderer.java |  17 +-
 .../widget/renderer/macro/MacroTreeRenderer.java   |  27 +-
 .../org/apache/ofbiz/widget/WidgetWorkerTest.java  |  68 +++++
 .../renderer/macro/MacroFormRendererTest.java      |  63 ++++-
 11 files changed, 387 insertions(+), 299 deletions(-)

diff --git a/framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java b/framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java
index 08de631..7e5ec13 100644
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/WidgetWorker.java
@@ -18,19 +18,17 @@
  *******************************************************************************/
 package org.apache.ofbiz.widget;
 
-import java.io.IOException;
-import java.io.StringWriter;
-import java.net.URLEncoder;
 import java.util.Map;
 
 import javax.servlet.ServletContext;
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+import org.apache.http.client.utils.URIBuilder;
+import org.apache.ofbiz.base.util.Debug;
+import org.apache.ofbiz.base.util.UtilHttp;
 import org.apache.ofbiz.security.CsrfUtil;
-import org.apache.ofbiz.base.util.UtilCodec;
 import org.apache.ofbiz.base.util.UtilGenerics;
-import org.apache.ofbiz.base.util.UtilHttp;
 import org.apache.ofbiz.base.util.UtilValidate;
 import org.apache.ofbiz.entity.Delegator;
 import org.apache.ofbiz.service.LocalDispatcher;
@@ -39,7 +37,16 @@ import org.apache.ofbiz.webapp.control.RequestHandler;
 import org.apache.ofbiz.webapp.taglib.ContentUrlTag;
 import org.apache.ofbiz.widget.model.ModelForm;
 import org.apache.ofbiz.widget.model.ModelFormField;
+import org.jsoup.nodes.Element;
+import org.jsoup.nodes.FormElement;
 import org.jsoup.parser.Parser;
+import org.jsoup.parser.Tag;
+
+import java.net.URI;
+import java.net.URISyntaxException;
+import java.util.HashMap;
+
+import static org.apache.ofbiz.base.util.UtilValidate.isNotEmpty;
 
 public final class WidgetWorker {
 
@@ -47,243 +54,141 @@ public final class WidgetWorker {
 
     private WidgetWorker() { }
 
-    public static void buildHyperlinkUrl(Appendable externalWriter, String target, String targetType, Map<String, String> parameterMap,
-            String prefix, boolean fullPath, boolean secure, boolean encode, HttpServletRequest request, HttpServletResponse response, Map<String, Object> context) throws IOException {
+    public static URI buildHyperlinkUri(String target, String targetType, Map<String, String> parameterMap,
+                                        String prefix, boolean fullPath, boolean secure, boolean encode,
+                                        HttpServletRequest request, HttpServletResponse response) {
         // We may get an encoded request like: &#47;projectmgr&#47;control&#47;EditTaskContents&#63;workEffortId&#61;10003
         // Try to reducing a possibly encoded string down to its simplest form: /projectmgr/control/EditTaskContents?workEffortId=10003
         // This step make sure the following appending externalLoginKey operation to work correctly
         String localRequestName = Parser.unescapeEntities(target, true);
-        localRequestName = UtilHttp.encodeAmpersands(localRequestName);
 
-        Appendable localWriter = new StringWriter();
+        // To handle cases where target contains javascript we need to encode spaces.
+        // Example:  "javascript:set_value('system', 'system', '')" becomes "javascript:set_value('system',%20'system',%20'')
+        localRequestName = UtilHttp.encodeBlanks(localRequestName);
+
+        final URIBuilder uriBuilder;
+        final Map<String, String> additionalParameters = new HashMap<>();
+        final String uriString;
 
         if ("intra-app".equals(targetType)) {
             if (request != null && response != null) {
                 ServletContext servletContext = request.getSession().getServletContext();
                 RequestHandler rh = (RequestHandler) servletContext.getAttribute("_REQUEST_HANDLER_");
-                externalWriter.append(rh.makeLink(request, response, "/" + localRequestName, fullPath, secure, encode));
+
+                uriString = rh.makeLink(request, response, "/" + localRequestName, fullPath, secure, encode);
             } else if (prefix != null) {
-                externalWriter.append(prefix);
-                externalWriter.append(localRequestName);
+                uriString = prefix + localRequestName;
             } else {
-                externalWriter.append(localRequestName);
+                uriString = localRequestName;
             }
         } else if ("inter-app".equals(targetType)) {
-            String fullTarget = localRequestName;
-            localWriter.append(fullTarget);
+            uriString = localRequestName;
             String externalLoginKey = (String) request.getAttribute("externalLoginKey");
-            if (UtilValidate.isNotEmpty(externalLoginKey)) {
-                if (fullTarget.indexOf('?') == -1) {
-                    localWriter.append('?');
-                } else {
-                    localWriter.append("&amp;");
-                }
-                localWriter.append("externalLoginKey=");
-                localWriter.append(externalLoginKey);
-            }
+            additionalParameters.put("externalLoginKey", externalLoginKey);
         } else if ("content".equals(targetType)) {
-            appendContentUrl(localWriter, localRequestName, request);
+            uriString = getContentUrl(localRequestName, request);
         } else {
-            localWriter.append(localRequestName);
+            uriString = localRequestName;
         }
 
-        if (UtilValidate.isNotEmpty(parameterMap)) {
-            String localUrl = localWriter.toString();
-            externalWriter.append(localUrl);
-            boolean needsAmp = true;
-            if (localUrl.indexOf('?') == -1) {
-                externalWriter.append('?');
-                needsAmp = false;
-            }
+        try {
+            uriBuilder = new URIBuilder(uriString);
+        } catch (URISyntaxException e) {
+            final String msg = "Syntax error when parsing URI: " + uriString;
+            Debug.logError(e, msg, MODULE);
+            throw new RuntimeException(msg, e);
+        }
 
-            for (Map.Entry<String, String> parameter: parameterMap.entrySet()) {
-                String parameterValue = parameter.getValue();
+        final String tokenValue = CsrfUtil.generateTokenForNonAjax(request, target);
+        if (isNotEmpty(tokenValue)) {
+            additionalParameters.put(CsrfUtil.getTokenNameNonAjax(), tokenValue);
+        }
 
-                if (needsAmp) {
-                    externalWriter.append("&amp;");
-                } else {
-                    needsAmp = true;
-                }
-                externalWriter.append(parameter.getKey());
-                externalWriter.append('=');
-                UtilCodec.SimpleEncoder simpleEncoder = (UtilCodec.SimpleEncoder) context.get("simpleEncoder");
-                if (simpleEncoder != null && parameterValue != null) {
-                    externalWriter.append(simpleEncoder.encode(URLEncoder.encode(parameterValue, "UTF-8")));
-                } else {
-                    externalWriter.append(parameterValue);
-                }
-            }
-        } else {
-            externalWriter.append(localWriter.toString());
+        if (UtilValidate.isNotEmpty(parameterMap)) {
+            parameterMap.forEach(uriBuilder::addParameter);
         }
 
-        String tokenValue = CsrfUtil.generateTokenForNonAjax(request, target);
-        if (UtilValidate.isNotEmpty(tokenValue)) {
-            String currentString = externalWriter.toString();
-            if (currentString.startsWith("<form")) {
-                currentString = currentString.substring(currentString.lastIndexOf("\"") + 1);
-            }
-            if (currentString.indexOf('?') == -1) {
-                externalWriter.append("?" + CsrfUtil.getTokenNameNonAjax() + "=" + tokenValue);
-            } else {
-                externalWriter.append("&amp;" + CsrfUtil.getTokenNameNonAjax() + "=" + tokenValue);
-            }
+        additionalParameters.forEach(uriBuilder::addParameter);
+
+        try {
+            return uriBuilder.build();
+        } catch (URISyntaxException e) {
+            final String msg = "Syntax error when building URI: " + uriBuilder.toString();
+            Debug.logError(e, msg, MODULE);
+            throw new RuntimeException(msg, e);
         }
     }
 
-    public static void appendContentUrl(Appendable writer, String location, HttpServletRequest request) throws IOException {
+    public static String getContentUrl(final String location, final HttpServletRequest request) {
         StringBuilder buffer = new StringBuilder();
         ContentUrlTag.appendContentPrefix(request, buffer);
-        writer.append(buffer.toString());
-        writer.append(location);
+        buffer.append(location);
+        return buffer.toString();
     }
-    public static void makeHyperlinkByType(Appendable writer, String linkType, String linkStyle, String targetType, String target,
-            Map<String, String> parameterMap, String description, String targetWindow, String confirmation, ModelFormField modelFormField,
-            HttpServletRequest request, HttpServletResponse response, Map<String, Object> context) throws IOException {
-        if (modelFormField == null) {
-            throw new IllegalArgumentException("modelFormField in WidgetWorker.makeHyperlinkByType has turned out to be null");
-        }
-        String realLinkType = WidgetWorker.determineAutoLinkType(linkType, target, targetType, request);
-        if ("hidden-form".equals(realLinkType)) {
-            if ("multi".equals(modelFormField.getModelForm().getType())) {
-                WidgetWorker.makeHiddenFormLinkAnchor(writer, linkStyle, description, confirmation, modelFormField, request, response, context);
-
-                // this is a bit trickier, since we can't do a nested form we'll have to put the link to submit the form in place, but put the actual form def elsewhere, ie after the big form is closed
-                Map<String, Object> wholeFormContext = UtilGenerics.cast(context.get("wholeFormContext"));
-                Appendable postMultiFormWriter = wholeFormContext != null ? (Appendable) wholeFormContext.get("postMultiFormWriter") : null;
-                if (postMultiFormWriter == null) {
-                    postMultiFormWriter = new StringWriter();
-                }
-                WidgetWorker.makeHiddenFormLinkForm(postMultiFormWriter, target, targetType, targetWindow, parameterMap, modelFormField, request, response, context);
-            } else {
-                WidgetWorker.makeHiddenFormLinkForm(writer, target, targetType, targetWindow, parameterMap, modelFormField, request, response, context);
-                WidgetWorker.makeHiddenFormLinkAnchor(writer, linkStyle, description, confirmation, modelFormField, request, response, context);
-            }
-        } else {
-            WidgetWorker.makeHyperlinkString(writer, linkStyle, targetType, target, parameterMap, description, confirmation, modelFormField, request, response, context, targetWindow);
-        }
-
-    }
-    public static void makeHyperlinkString(Appendable writer, String linkStyle, String targetType, String target, Map<String, String> parameterMap,
-            String description, String confirmation, ModelFormField modelFormField, HttpServletRequest request, HttpServletResponse response, Map<String, Object> context, String targetWindow)
-            throws IOException {
-        if (UtilValidate.isNotEmpty(description) || UtilValidate.isNotEmpty(request.getAttribute("image"))) {
-            writer.append("<a");
-
-            if (UtilValidate.isNotEmpty(linkStyle)) {
-                writer.append(" class=\"");
-                writer.append(linkStyle);
-                writer.append("\"");
-            }
-
-            writer.append(" href=\"");
 
-            buildHyperlinkUrl(writer, target, targetType, parameterMap, null, false, false, true, request, response, context);
-
-            writer.append("\"");
-
-            if (UtilValidate.isNotEmpty(targetWindow)) {
-                writer.append(" target=\"");
-                writer.append(targetWindow);
-                writer.append("\"");
-            }
-
-            if (UtilValidate.isNotEmpty(modelFormField.getEvent()) && UtilValidate.isNotEmpty(modelFormField.getAction(context))) {
-                writer.append(" ");
-                writer.append(modelFormField.getEvent());
-                writer.append("=\"");
-                writer.append(modelFormField.getAction(context));
-                writer.append('"');
-            }
-            if (UtilValidate.isNotEmpty(confirmation)) {
-                writer.append(" onclick=\"return confirm('");
-                writer.append(confirmation);
-                writer.append("')\"");
-            }
-            writer.append('>');
+    public static Element makeHiddenFormLinkAnchorElement(String linkStyle, String description, String confirmation,
+                                                          ModelFormField modelFormField, HttpServletRequest request,
+                                                          Map<String, Object> context) {
+        if (isNotEmpty(description) || isNotEmpty(request.getAttribute("image"))) {
+            final Element anchorElement = new Element("a");
 
-            if (UtilValidate.isNotEmpty(request.getAttribute("image"))) {
-                writer.append("<img src=\"");
-                writer.append(request.getAttribute("image").toString());
-                writer.append("\"/>");
+            if (isNotEmpty(linkStyle)) {
+                anchorElement.addClass(linkStyle);
             }
 
-            writer.append(description);
-            writer.append("</a>");
-        }
-    }
+            final String href = "javascript:document." + makeLinkHiddenFormName(context, modelFormField) + ".submit()";
+            anchorElement.attr("href", href);
 
-    public static void makeHiddenFormLinkAnchor(Appendable writer, String linkStyle, String description, String confirmation, ModelFormField modelFormField, HttpServletRequest request, HttpServletResponse response, Map<String, Object> context) throws IOException {
-        if (UtilValidate.isNotEmpty(description) || UtilValidate.isNotEmpty(request.getAttribute("image"))) {
-            writer.append("<a");
 
-            if (UtilValidate.isNotEmpty(linkStyle)) {
-                writer.append(" class=\"");
-                writer.append(linkStyle);
-                writer.append("\"");
+            if (isNotEmpty(modelFormField.getEvent()) && isNotEmpty(modelFormField.getAction(context))) {
+                anchorElement.attr(modelFormField.getEvent(), modelFormField.getAction(context));
             }
 
-            writer.append(" href=\"javascript:document.");
-            writer.append(makeLinkHiddenFormName(context, modelFormField));
-            writer.append(".submit()\"");
-
-            if (UtilValidate.isNotEmpty(modelFormField.getEvent()) && UtilValidate.isNotEmpty(modelFormField.getAction(context))) {
-                writer.append(" ");
-                writer.append(modelFormField.getEvent());
-                writer.append("=\"");
-                writer.append(modelFormField.getAction(context));
-                writer.append('"');
+            if (isNotEmpty(confirmation)) {
+                anchorElement.attr("onclick", "return confirm('" + confirmation + "')");
             }
 
-            if (UtilValidate.isNotEmpty(confirmation)) {
-                writer.append(" onclick=\"return confirm('");
-                writer.append(confirmation);
-                writer.append("')\"");
-            }
+            anchorElement.text(description);
 
-            writer.append('>');
+            if (isNotEmpty(request.getAttribute("image"))) {
+                final Element imageElement = new Element("img");
+                imageElement.attr("src", request.getAttribute("image").toString());
 
-            if (UtilValidate.isNotEmpty(request.getAttribute("image"))) {
-                writer.append("<img src=\"");
-                writer.append(request.getAttribute("image").toString());
-                writer.append("\"/>");
+                anchorElement.appendChild(imageElement);
             }
 
-            writer.append(description);
-            writer.append("</a>");
+            return anchorElement;
+        } else {
+            return null;
         }
     }
 
-    public static void makeHiddenFormLinkForm(Appendable writer, String target, String targetType, String targetWindow, Map<String, String> parameterMap, ModelFormField modelFormField, HttpServletRequest request, HttpServletResponse response, Map<String, Object> context) throws IOException {
-        writer.append("<form method=\"post\"");
-        writer.append(" action=\"");
+    public static Element makeHiddenFormLinkFormElement(String target, String targetType,
+                                              String targetWindow, Map<String, String> parameterMap,
+                                              ModelFormField modelFormField, HttpServletRequest request,
+                                              HttpServletResponse response, Map<String, Object> context) {
+
+        final FormElement formElement = new FormElement(Tag.valueOf("form"), null, null);
+        formElement.attr("method", "post");
+
         // note that this passes null for the parameterList on purpose so they won't be put into the URL
-        WidgetWorker.buildHyperlinkUrl(writer, target, targetType, null, null, false, false, true, request, response, context);
-        writer.append("\"");
+        final URI actionUri = WidgetWorker.buildHyperlinkUri(target, targetType, null, null, false, false, true,
+                request, response);
+        formElement.attr("action", actionUri.toString());
 
-        if (UtilValidate.isNotEmpty(targetWindow)) {
-            writer.append(" target=\"");
-            writer.append(targetWindow);
-            writer.append("\"");
+        if (isNotEmpty(targetWindow)) {
+            formElement.attr("target", targetWindow);
         }
 
-        writer.append(" onsubmit=\"javascript:submitFormDisableSubmits(this)\"");
-
-        writer.append(" name=\"");
-        writer.append(makeLinkHiddenFormName(context, modelFormField));
-        writer.append("\">");
+        formElement.attr("onsubmit", "javascript:submitFormDisableSubmits(this)");
+        formElement.attr("name", makeLinkHiddenFormName(context, modelFormField));
 
-        for (Map.Entry<String, String> parameter: parameterMap.entrySet()) {
-            if (parameter.getValue() != null) {
-                writer.append("<input name=\"");
-                writer.append(parameter.getKey());
-                writer.append("\" value=\"");
-                writer.append(UtilCodec.getEncoder("html").encode(parameter.getValue()));
-                writer.append("\" type=\"hidden\"/>");
-            }
-        }
+        parameterMap.forEach((name, value) -> formElement.appendElement("input")
+                .attr("name", name)
+                .val(value)
+                .attr("type", "hidden"));
 
-        writer.append("</form>");
+        return formElement;
     }
 
     public static String makeLinkHiddenFormName(Map<String, Object> context, ModelFormField modelFormField) {
@@ -302,10 +207,12 @@ public final class WidgetWorker {
             formUniqueId = (String) context.get("formUniqueId");
         }
         if (itemIndex != null) {
-            return formName + modelForm.getItemIndexSeparator() + itemIndex + iterateId + formUniqueId + modelForm.getItemIndexSeparator() + modelFormField.getName();
+            return formName + modelForm.getItemIndexSeparator() + itemIndex + iterateId + formUniqueId
+                    + modelForm.getItemIndexSeparator() + modelFormField.getName();
         }
         return formName + modelForm.getItemIndexSeparator() + modelFormField.getName();
     }
+
     public static String determineAutoLinkType(String linkType, String target, String targetType, HttpServletRequest request) {
         if ("auto".equals(linkType)) {
             if ("intra-app".equals(targetType)) {
@@ -352,9 +259,9 @@ public final class WidgetWorker {
     }
 
     public static int getPaginatorNumber(Map<String, Object> context) {
-        int paginator_number = 0;
+        int paginatorNumber = 0;
         if (context != null) {
-            Integer paginateNumberInt= (Integer) context.get("PAGINATOR_NUMBER");
+            Integer paginateNumberInt = (Integer) context.get("PAGINATOR_NUMBER");
             if (paginateNumberInt == null) {
                 paginateNumberInt = 0;
                 context.put("PAGINATOR_NUMBER", paginateNumberInt);
@@ -363,19 +270,19 @@ public final class WidgetWorker {
                     globalCtx.put("PAGINATOR_NUMBER", paginateNumberInt);
                 }
             }
-            paginator_number = paginateNumberInt;
+            paginatorNumber = paginateNumberInt;
         }
-        return paginator_number;
+        return paginatorNumber;
     }
 
     public static void incrementPaginatorNumber(Map<String, Object> context) {
         Map<String, Object> globalCtx = UtilGenerics.cast(context.get("globalContext"));
         if (globalCtx != null) {
-            Boolean NO_PAGINATOR = (Boolean) globalCtx.get("NO_PAGINATOR");
-            if (UtilValidate.isNotEmpty(NO_PAGINATOR)) {
+            Boolean noPaginator = (Boolean) globalCtx.get("NO_PAGINATOR");
+            if (UtilValidate.isNotEmpty(noPaginator)) {
                 globalCtx.remove("NO_PAGINATOR");
             } else {
-                Integer paginateNumberInt= (Integer) globalCtx.get("PAGINATOR_NUMBER");
+                Integer paginateNumberInt = (Integer) globalCtx.get("PAGINATOR_NUMBER");
                 if (paginateNumberInt == null) {
                     paginateNumberInt = 0;
                 }
@@ -387,12 +294,10 @@ public final class WidgetWorker {
     }
 
     public static LocalDispatcher getDispatcher(Map<String, Object> context) {
-        LocalDispatcher dispatcher = (LocalDispatcher) context.get("dispatcher");
-        return dispatcher;
+        return (LocalDispatcher) context.get("dispatcher");
     }
 
     public static Delegator getDelegator(Map<String, Object> context) {
-        Delegator delegator = (Delegator) context.get("delegator");
-        return delegator;
+        return (Delegator) context.get("delegator");
     }
 }
diff --git a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelTree.java b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelTree.java
index 46f38ae..9408ce2 100644
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelTree.java
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/model/ModelTree.java
@@ -874,13 +874,20 @@ public class ModelTree extends ModelWidget {
 
             // FIXME: Using a widget model in this way is an ugly hack.
             public Link(String style, String target, String text) {
+                this(style, target, text, null);
+            }
+
+            // FIXME: Something to be replaced by a builder class, but allows us to quickly
+            // build Links to represent nodes with parameters in a tree, rather that trying
+            // to encode the parameters early in the link's target.
+            public Link(String style, String target, String text, List<Parameter> parameterList) {
                 this.encode = false;
                 this.fullPath = false;
                 this.idExdr = FlexibleStringExpander.getInstance("");
                 this.image = null;
                 this.linkType = "";
                 this.nameExdr = FlexibleStringExpander.getInstance("");
-                this.parameterList = Collections.emptyList();
+                this.parameterList = parameterList != null ? Collections.unmodifiableList(parameterList) : Collections.emptyList();
                 this.prefixExdr = FlexibleStringExpander.getInstance("");
                 this.secure = false;
                 this.styleExdr = FlexibleStringExpander.getInstance(style);
diff --git a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/html/HtmlMenuRenderer.java b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/html/HtmlMenuRenderer.java
index 4272553..95cf728 100644
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/html/HtmlMenuRenderer.java
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/html/HtmlMenuRenderer.java
@@ -20,6 +20,7 @@ package org.apache.ofbiz.widget.renderer.html;
 
 import java.io.IOException;
 import java.math.BigDecimal;
+import java.net.URI;
 import java.util.List;
 import java.util.Map;
 
@@ -361,8 +362,10 @@ public class HtmlMenuRenderer extends HtmlWidgetRenderer implements MenuStringRe
                 writer.append("<form method=\"post\"");
                 writer.append(" action=\"");
                 // note that this passes null for the parameterList on purpose so they won't be put into the URL
-                WidgetWorker.buildHyperlinkUrl(writer, target, link.getUrlMode(), null, link.getPrefix(context),
-                        link.getFullPath(), link.getSecure(), link.getEncode(), request, response, context);
+                final URI uri = WidgetWorker.buildHyperlinkUri(target, link.getUrlMode(), null,
+                        link.getPrefix(context), link.getFullPath(), link.getSecure(), link.getEncode(),
+                        request, response);
+                writer.append(uri.toString());
                 writer.append("\"");
 
                 if (UtilValidate.isNotEmpty(targetWindow)) {
@@ -425,8 +428,10 @@ public class HtmlMenuRenderer extends HtmlWidgetRenderer implements MenuStringRe
                 writer.append(uniqueItemName);
                 writer.append(".submit()");
             } else {
-                WidgetWorker.buildHyperlinkUrl(writer, target, link.getUrlMode(), link.getParameterMap(context), link.getPrefix(context),
-                        link.getFullPath(), link.getSecure(), link.getEncode(), request, response, context);
+                final URI uri = WidgetWorker.buildHyperlinkUri(target, link.getUrlMode(), link.getParameterMap(context),
+                        link.getPrefix(context), link.getFullPath(), link.getSecure(), link.getEncode(),
+                        request, response);
+                writer.append(uri.toString());
             }
             writer.append("\">");
         }
diff --git a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/html/HtmlTreeRenderer.java b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/html/HtmlTreeRenderer.java
index 1a1e134..a5edc8e 100644
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/html/HtmlTreeRenderer.java
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/html/HtmlTreeRenderer.java
@@ -19,6 +19,7 @@
 package org.apache.ofbiz.widget.renderer.html;
 
 import java.io.IOException;
+import java.net.URI;
 import java.util.List;
 import java.util.Map;
 
@@ -230,8 +231,10 @@ public class HtmlTreeRenderer extends HtmlWidgetRenderer implements TreeStringRe
             HttpServletRequest req = (HttpServletRequest) context.get("request");
             if (urlMode != null && "intra-app".equalsIgnoreCase(urlMode)) {
                 if (req != null && res != null) {
-                    WidgetWorker.buildHyperlinkUrl(writer, target, link.getUrlMode(), link.getParameterMap(context), link.getPrefix(context),
-                        link.getFullPath(), link.getSecure(), link.getEncode(), req, res, context);
+                    final URI uri = WidgetWorker.buildHyperlinkUri(target, link.getUrlMode(),
+                            link.getParameterMap(context), link.getPrefix(context), link.getFullPath(),
+                            link.getSecure(), link.getEncode(), req, res);
+                    writer.append(uri.toString());
                 } else if (prefix != null) {
                     writer.append(prefix).append(target);
                 } else {
diff --git a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/FtlWriter.java b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/FtlWriter.java
new file mode 100644
index 0000000..cb48600
--- /dev/null
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/FtlWriter.java
@@ -0,0 +1,71 @@
+/*******************************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *******************************************************************************/
+package org.apache.ofbiz.widget.renderer.macro;
+
+import freemarker.core.Environment;
+import freemarker.template.Template;
+import freemarker.template.TemplateException;
+import org.apache.ofbiz.base.util.Debug;
+import org.apache.ofbiz.base.util.UtilMisc;
+import org.apache.ofbiz.base.util.template.FreeMarkerWorker;
+import org.apache.ofbiz.widget.renderer.VisualTheme;
+
+import java.io.IOException;
+import java.io.Reader;
+import java.io.StringReader;
+import java.rmi.server.UID;
+import java.util.Map;
+import java.util.WeakHashMap;
+
+public final class FtlWriter {
+    private static final String MODULE = FtlWriter.class.getName();
+
+    private final WeakHashMap<Appendable, Environment> environments = new WeakHashMap<>();
+    private final Template macroLibrary;
+    private final VisualTheme visualTheme;
+
+    public FtlWriter(final String macroLibraryPath, final VisualTheme visualTheme) throws IOException {
+        this.macroLibrary = FreeMarkerWorker.getTemplate(macroLibraryPath);
+        this.visualTheme = visualTheme;
+    }
+
+    public void executeMacro(Appendable writer, String macro) {
+        try {
+            Environment environment = getEnvironment(writer);
+            environment.setVariable("visualTheme", FreeMarkerWorker.autoWrap(visualTheme, environment));
+            environment.setVariable("modelTheme", FreeMarkerWorker.autoWrap(visualTheme.getModelTheme(), environment));
+            Reader templateReader = new StringReader(macro);
+            Template template = new Template(new UID().toString(), templateReader, FreeMarkerWorker.getDefaultOfbizConfig());
+            templateReader.close();
+            environment.include(template);
+        } catch (TemplateException | IOException e) {
+            Debug.logError(e, "Error rendering screen thru ftl, macro: " + macro, MODULE);
+        }
+    }
+
+    private Environment getEnvironment(Appendable writer) throws TemplateException, IOException {
+        Environment environment = environments.get(writer);
+        if (environment == null) {
+            Map<String, Object> input = UtilMisc.toMap("key", null);
+            environment = FreeMarkerWorker.renderTemplate(macroLibrary, input, writer);
+            environments.put(writer, environment);
+        }
+        return environment;
+    }
+}
diff --git a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRenderer.java b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRenderer.java
index 4e07cb9..be7b885 100644
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRenderer.java
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRenderer.java
@@ -19,10 +19,8 @@
 package org.apache.ofbiz.widget.renderer.macro;
 
 import java.io.IOException;
-import java.io.Reader;
-import java.io.StringReader;
 import java.io.StringWriter;
-import java.rmi.server.UID;
+import java.net.URI;
 import java.sql.Timestamp;
 import java.util.HashSet;
 import java.util.Iterator;
@@ -94,6 +92,7 @@ import org.apache.ofbiz.widget.renderer.FormStringRenderer;
 import org.apache.ofbiz.widget.renderer.Paginator;
 import org.apache.ofbiz.widget.renderer.UtilHelpText;
 import org.apache.ofbiz.widget.renderer.VisualTheme;
+import org.jsoup.nodes.Element;
 
 import com.ibm.icu.util.Calendar;
 
@@ -116,10 +115,17 @@ public final class MacroFormRenderer implements FormStringRenderer {
     private final HttpServletResponse response;
     private final boolean javaScriptEnabled;
     private final VisualTheme visualTheme;
+    private final FtlWriter ftlWriter;
     private boolean renderPagination = true;
     private boolean widgetCommentsEnabled = false;
 
-    public MacroFormRenderer(String macroLibraryPath, HttpServletRequest request, HttpServletResponse response) throws TemplateException, IOException {
+    public MacroFormRenderer(String macroLibraryPath, HttpServletRequest request, HttpServletResponse response)
+            throws TemplateException, IOException {
+        this(macroLibraryPath, request, response, null);
+    }
+
+    public MacroFormRenderer(String macroLibraryPath, HttpServletRequest request, HttpServletResponse response,
+                             FtlWriter ftlWriter) throws TemplateException, IOException {
         macroLibrary = FreeMarkerWorker.getTemplate(macroLibraryPath);
         this.request = request;
         this.response = response;
@@ -127,6 +133,7 @@ public final class MacroFormRenderer implements FormStringRenderer {
         this.rh = RequestHandler.from(request);
         this.javaScriptEnabled = UtilHttp.isJavaScriptEnabled(request);
         internalEncoder = UtilCodec.getEncoder("string");
+        this.ftlWriter = ftlWriter != null ? ftlWriter : new FtlWriter(macroLibraryPath, this.visualTheme);
     }
 
     @Deprecated
@@ -143,17 +150,7 @@ public final class MacroFormRenderer implements FormStringRenderer {
     }
 
     private void executeMacro(Appendable writer, String macro) {
-        try {
-            Environment environment = getEnvironment(writer);
-            environment.setVariable("visualTheme", FreeMarkerWorker.autoWrap(visualTheme, environment));
-            environment.setVariable("modelTheme", FreeMarkerWorker.autoWrap(visualTheme.getModelTheme(), environment));
-            Reader templateReader = new StringReader(macro);
-            Template template = new Template(new UID().toString(), templateReader, FreeMarkerWorker.getDefaultOfbizConfig());
-            templateReader.close();
-            environment.include(template);
-        } catch (TemplateException | IOException e) {
-            Debug.logError(e, "Error rendering screen thru ftl, macro: " + macro, MODULE);
-        }
+        ftlWriter.executeMacro(writer, macro);
     }
 
     private Environment getEnvironment(Appendable writer) throws TemplateException, IOException {
@@ -1414,7 +1411,9 @@ public final class MacroFormRenderer implements FormStringRenderer {
         String targ = modelForm.getTarget(context, targetType);
         StringBuilder linkUrl = new StringBuilder();
         if (UtilValidate.isNotEmpty(targ)) {
-            WidgetWorker.buildHyperlinkUrl(linkUrl, targ, targetType, null, null, false, false, true, request, response, context);
+            final URI linkUri = WidgetWorker.buildHyperlinkUri(targ, targetType, null, null, false, false, true,
+                    request, response);
+            linkUrl.append(linkUri.toString());
         }
         String formType = modelForm.getType();
         String targetWindow = modelForm.getTargetWindow(context);
@@ -3213,7 +3212,9 @@ public final class MacroFormRenderer implements FormStringRenderer {
             parameterMap.put(viewIndexField, Integer.toString(viewIndex));
             parameterMap.put(viewSizeField, Integer.toString(viewSize));
             if ("multi".equals(modelForm.getType())) {
-                WidgetWorker.makeHiddenFormLinkAnchor(writer, linkStyle, encodedDescription, confirmation, modelFormField, request, response, context);
+                final Element anchorElement = WidgetWorker.makeHiddenFormLinkAnchorElement(linkStyle,
+                        encodedDescription, confirmation, modelFormField, request, context);
+                writer.append(anchorElement.outerHtml());
                 // this is a bit trickier, since we can't do a nested form we'll have to put the link to submit the form in place, but put the actual form def elsewhere, ie after the big form is closed
                 Map<String, Object> wholeFormContext = UtilGenerics.cast(context.get("wholeFormContext"));
                 Appendable postMultiFormWriter = wholeFormContext != null ? (Appendable) wholeFormContext.get("postMultiFormWriter") : null;
@@ -3221,10 +3222,17 @@ public final class MacroFormRenderer implements FormStringRenderer {
                     postMultiFormWriter = new StringWriter();
                     wholeFormContext.put("postMultiFormWriter", postMultiFormWriter);
                 }
-                WidgetWorker.makeHiddenFormLinkForm(postMultiFormWriter, target, targetType, targetWindow, parameterMap, modelFormField, request, response, context);
+                final Element hiddenFormElement = WidgetWorker.makeHiddenFormLinkFormElement(target, targetType,
+                        targetWindow, parameterMap, modelFormField, request, response, context);
+                postMultiFormWriter.append(hiddenFormElement.outerHtml());
+
             } else {
-                WidgetWorker.makeHiddenFormLinkForm(writer, target, targetType, targetWindow, parameterMap, modelFormField, request, response, context);
-                WidgetWorker.makeHiddenFormLinkAnchor(writer, linkStyle, encodedDescription, confirmation, modelFormField, request, response, context);
+                final Element hiddenFormElement = WidgetWorker.makeHiddenFormLinkFormElement(target, targetType,
+                        targetWindow, parameterMap, modelFormField, request, response, context);
+                writer.append(hiddenFormElement.outerHtml());
+                final Element anchorElement = WidgetWorker.makeHiddenFormLinkAnchorElement(linkStyle,
+                        encodedDescription, confirmation, modelFormField, request, context);
+                writer.append(anchorElement.outerHtml());
             }
         } else {
             if ("layered-modal".equals(realLinkType)) {
@@ -3254,7 +3262,10 @@ public final class MacroFormRenderer implements FormStringRenderer {
             String targetWindow) throws IOException {
         if (description != null || UtilValidate.isNotEmpty(request.getAttribute("image"))) {
             StringBuilder linkUrl = new StringBuilder();
-            WidgetWorker.buildHyperlinkUrl(linkUrl, target, targetType, UtilValidate.isEmpty(request.getAttribute("uniqueItemName"))?parameterMap:null, null, false, false, true, request, response, context);
+            final URI linkUri = WidgetWorker.buildHyperlinkUri(target, targetType,
+                    UtilValidate.isEmpty(request.getAttribute("uniqueItemName")) ? parameterMap : null,
+                    null, false, false, true, request, response);
+            linkUrl.append(linkUri.toString());
             String event = "";
             String action = "";
             String imgSrc = "";
@@ -3385,38 +3396,6 @@ public final class MacroFormRenderer implements FormStringRenderer {
         }
     }
 
-    public void makeHiddenFormLinkForm(Appendable writer, String target, String targetType, String targetWindow, List<CommonWidgetModels.Parameter> parameterList, ModelFormField modelFormField, HttpServletRequest request, HttpServletResponse response, Map<String, Object> context) throws IOException {
-        StringBuilder actionUrl = new StringBuilder();
-        WidgetWorker.buildHyperlinkUrl(actionUrl, target, targetType, null, null, false, false, true, request, response, context);
-        String name = WidgetWorker.makeLinkHiddenFormName(context, modelFormField);
-        StringBuilder parameters = new StringBuilder();
-        parameters.append("[");
-        for (CommonWidgetModels.Parameter parameter : parameterList) {
-            if (parameters.length() > 1) {
-                parameters.append(",");
-            }
-            parameters.append("{'name':'");
-            parameters.append(parameter.getName());
-            parameters.append("'");
-            parameters.append(",'value':'");
-            parameters.append(UtilCodec.getEncoder("html").encode(parameter.getValue(context)));
-            parameters.append("'}");
-        }
-        parameters.append("]");
-        StringWriter sr = new StringWriter();
-        sr.append("<@makeHiddenFormLinkForm ");
-        sr.append("actionUrl=\"");
-        sr.append(actionUrl.toString());
-        sr.append("\" name=\"");
-        sr.append(name);
-        sr.append("\" parameters=");
-        sr.append(parameters.toString());
-        sr.append(" targetWindow=\"");
-        sr.append(targetWindow);
-        sr.append("\" />");
-        executeMacro(writer, sr.toString());
-    }
-
     @Override
     public void renderContainerFindField(Appendable writer, Map<String, Object> context, ContainerField containerField) throws IOException {
         final String id = containerField.getModelFormField().getCurrentContainerId(context);
diff --git a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroMenuRenderer.java b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroMenuRenderer.java
index b99b7bb..38d70f7 100644
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroMenuRenderer.java
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroMenuRenderer.java
@@ -23,6 +23,7 @@ import java.io.Reader;
 import java.io.StringReader;
 import java.io.StringWriter;
 import java.math.BigDecimal;
+import java.net.URI;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
@@ -232,9 +233,11 @@ public class MacroMenuRenderer implements MenuStringRenderer {
         String actionUrl = "";
         StringBuilder targetParameters = new StringBuilder();
         if ("hidden-form".equals(linkType) || "layered-modal".equals(linkType)) {
-            StringBuilder sb = new StringBuilder();
-            WidgetWorker.buildHyperlinkUrl(sb, target, link.getUrlMode(), null, link.getPrefix(context), link.getFullPath(), link.getSecure(), link.getEncode(), request, response, context);
-            actionUrl = sb.toString();
+            final URI actionUri = WidgetWorker.buildHyperlinkUri(target, link.getUrlMode(), null,
+                    link.getPrefix(context), link.getFullPath(), link.getSecure(), link.getEncode(),
+                    request, response);
+            actionUrl = actionUri.toString();
+
             targetParameters.append("[");
             for (Map.Entry<String, String> parameter : link.getParameterMap(context).entrySet()) {
                 if (targetParameters.length() > 1) {
@@ -255,9 +258,11 @@ public class MacroMenuRenderer implements MenuStringRenderer {
         }
         if (UtilValidate.isNotEmpty(target)) {
             if (!"hidden-form".equals(linkType)) {
-                StringBuilder sb = new StringBuilder();
-                WidgetWorker.buildHyperlinkUrl(sb, target, link.getUrlMode(), "layered-modal".equals(linkType)?null:link.getParameterMap(context), link.getPrefix(context), link.getFullPath(), link.getSecure(), link.getEncode(), request, response, context);
-                linkUrl = sb.toString();
+                final URI linkUri = WidgetWorker.buildHyperlinkUri(target, link.getUrlMode(),
+                        "layered-modal".equals(linkType) ? null : link.getParameterMap(context),
+                        link.getPrefix(context), link.getFullPath(), link.getSecure(), link.getEncode(),
+                        request, response);
+                linkUrl = linkUri.toString();
             }
         }
         parameters.put("linkUrl", linkUrl);
diff --git a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroScreenRenderer.java b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroScreenRenderer.java
index 5b5f239..edd8b0b 100644
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroScreenRenderer.java
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroScreenRenderer.java
@@ -23,6 +23,7 @@ import java.io.Reader;
 import java.io.StringReader;
 import java.io.StringWriter;
 import java.math.BigDecimal;
+import java.net.URI;
 import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Locale;
@@ -256,10 +257,10 @@ public class MacroScreenRenderer implements ScreenStringRenderer {
             height = String.valueOf(modelTheme.getLinkDefaultLayeredModalHeight());
         }
         if ("hidden-form".equals(linkType) || "layered-modal".equals(linkType)) {
-            StringBuilder sb = new StringBuilder();
-            WidgetWorker.buildHyperlinkUrl(sb, target, link.getUrlMode(), null, link.getPrefix(context),
-                    link.getFullPath(), link.getSecure(), link.getEncode(), request, response, context);
-            actionUrl = sb.toString();
+            final URI actionUri = WidgetWorker.buildHyperlinkUri(target, link.getUrlMode(), null,
+                    link.getPrefix(context), link.getFullPath(), link.getSecure(), link.getEncode(),
+                    request, response);
+            actionUrl = actionUri.toString();
             parameters.append("[");
             for (Map.Entry<String, String> parameter: link.getParameterMap(context).entrySet()) {
                 if (parameters.length() >1) {
@@ -280,10 +281,10 @@ public class MacroScreenRenderer implements ScreenStringRenderer {
         String text = link.getText(context);
         if (UtilValidate.isNotEmpty(target)) {
             if (!"hidden-form".equals(linkType)) {
-                StringBuilder sb = new StringBuilder();
-                WidgetWorker.buildHyperlinkUrl(sb, target, link.getUrlMode(), link.getParameterMap(context), link.getPrefix(context),
-                        link.getFullPath(), link.getSecure(), link.getEncode(), request, response, context);
-                linkUrl = sb.toString();
+                final URI uri = WidgetWorker.buildHyperlinkUri(target, link.getUrlMode(), link.getParameterMap(context),
+                        link.getPrefix(context), link.getFullPath(), link.getSecure(), link.getEncode(),
+                        request, response);
+                linkUrl = uri.toString();
             }
         }
         String imgStr = "";
diff --git a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroTreeRenderer.java b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroTreeRenderer.java
index b8b0601..fd4359c 100644
--- a/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroTreeRenderer.java
+++ b/framework/widget/src/main/java/org/apache/ofbiz/widget/renderer/macro/MacroTreeRenderer.java
@@ -22,12 +22,14 @@ import java.io.IOException;
 import java.io.Reader;
 import java.io.StringReader;
 import java.io.StringWriter;
+import java.net.URI;
 import java.util.List;
 import java.util.Map;
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
 
+import com.google.common.collect.ImmutableList;
 import org.apache.ofbiz.base.util.Debug;
 import org.apache.ofbiz.base.util.StringUtil;
 import org.apache.ofbiz.base.util.UtilGenerics;
@@ -37,6 +39,7 @@ import org.apache.ofbiz.base.util.template.FreeMarkerWorker;
 import org.apache.ofbiz.webapp.control.RequestHandler;
 import org.apache.ofbiz.webapp.taglib.ContentUrlTag;
 import org.apache.ofbiz.widget.WidgetWorker;
+import org.apache.ofbiz.widget.model.CommonWidgetModels;
 import org.apache.ofbiz.widget.model.ModelTree;
 import org.apache.ofbiz.widget.model.ModelWidget;
 import org.apache.ofbiz.widget.renderer.ScreenRenderer;
@@ -166,13 +169,8 @@ public class MacroTreeRenderer implements TreeStringRenderer {
                     currentNodeTrailPiped = StringUtil.join(currentNodeTrail, "|");
                     StringBuilder target = new StringBuilder(node.getModelTree().getExpandCollapseRequest(context));
                     String trailName = node.getModelTree().getTrailName(context);
-                    if (target.indexOf("?") < 0) {
-                        target.append("?");
-                    } else {
-                        target.append("&");
-                    }
-                    target.append(trailName).append("=").append(currentNodeTrailPiped);
-                    expandCollapseLink = new ModelTree.ModelNode.Link("collapsed", target.toString(), " ");
+                    expandCollapseLink = new ModelTree.ModelNode.Link("collapsed", target.toString(), " ",
+                            ImmutableList.of(new CommonWidgetModels.Parameter(trailName, currentNodeTrailPiped, false)));
                 }
             } else {
                 context.put("processChildren", Boolean.TRUE);
@@ -183,13 +181,8 @@ public class MacroTreeRenderer implements TreeStringRenderer {
                 }
                 StringBuilder target = new StringBuilder(node.getModelTree().getExpandCollapseRequest(context));
                 String trailName = node.getModelTree().getTrailName(context);
-                if (target.indexOf("?") < 0) {
-                    target.append("?");
-                } else {
-                    target.append("&");
-                }
-                target.append(trailName).append("=").append(currentNodeTrailPiped);
-                expandCollapseLink = new ModelTree.ModelNode.Link("expanded", target.toString(), " ");
+                expandCollapseLink = new ModelTree.ModelNode.Link("expanded", target.toString(), " ",
+                        ImmutableList.of(new CommonWidgetModels.Parameter(trailName, currentNodeTrailPiped, false)));
                 // add it so it can be remove in renderNodeEnd
                 currentNodeTrail.add(lastContentId);
             }
@@ -260,8 +253,10 @@ public class MacroTreeRenderer implements TreeStringRenderer {
         HttpServletRequest request = (HttpServletRequest) context.get("request");
 
         if (UtilValidate.isNotEmpty(target)) {
-            WidgetWorker.buildHyperlinkUrl(linkUrl, target, link.getUrlMode(), link.getParameterMap(context), link.getPrefix(context),
-                    link.getFullPath(), link.getSecure(), link.getEncode(), request, response, context);
+            final URI uri = WidgetWorker.buildHyperlinkUri(target, link.getUrlMode(), link.getParameterMap(context),
+                    link.getPrefix(context), link.getFullPath(), link.getSecure(), link.getEncode(),
+                    request, response);
+            linkUrl.append(uri.toString());
         }
 
         String id = link.getId(context);
diff --git a/framework/widget/src/test/java/org/apache/ofbiz/widget/WidgetWorkerTest.java b/framework/widget/src/test/java/org/apache/ofbiz/widget/WidgetWorkerTest.java
new file mode 100644
index 0000000..29e4bba
--- /dev/null
+++ b/framework/widget/src/test/java/org/apache/ofbiz/widget/WidgetWorkerTest.java
@@ -0,0 +1,68 @@
+/*******************************************************************************
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ *******************************************************************************/
+package org.apache.ofbiz.widget;
+
+import mockit.Mock;
+import mockit.MockUp;
+import org.apache.ofbiz.security.CsrfUtil;
+import org.junit.Before;
+import org.junit.Test;
+
+import javax.servlet.http.HttpServletRequest;
+import java.net.URI;
+import java.util.HashMap;
+
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.hamcrest.Matchers.equalTo;
+import static org.hamcrest.Matchers.hasProperty;
+
+public class WidgetWorkerTest {
+
+    @Before
+    public void setupMockups() {
+        new RequestHandlerMockUp();
+    }
+
+    @Test
+    public void buildsHyperlinkUriWithAlreadyUrlEncodedTarget() {
+        final URI alreadyEncodedTargetUri = WidgetWorker.buildHyperlinkUri(
+                "&#47;projectmgr&#47;control&#47;EditTaskContents&#63;workEffortId&#61;10003",
+                "plain", new HashMap<>(), null, false, true, true, null, null);
+
+        assertThat(alreadyEncodedTargetUri, hasProperty("path", equalTo("/projectmgr/control/EditTaskContents")));
+        assertThat(alreadyEncodedTargetUri, hasProperty("query", equalTo("workEffortId=10003")));
+    }
+
+    @Test
+    public void buildsHyperlinkUriWithSpaces() {
+        final URI withEncodedSpaces = WidgetWorker.buildHyperlinkUri(
+                "javascript:set_value('system', 'system', '')",
+                "plain", new HashMap<>(), null, false, true, true, null, null);
+
+        assertThat(withEncodedSpaces, hasProperty("scheme", equalTo("javascript")));
+        assertThat(withEncodedSpaces, hasProperty("schemeSpecificPart", equalTo("set_value('system', 'system', '')")));
+    }
+
+    class RequestHandlerMockUp extends MockUp<CsrfUtil> {
+        @Mock
+        public String generateTokenForNonAjax(HttpServletRequest request, String pathOrRequestUri) {
+            return null;
+        }
+    }
+}
diff --git a/framework/widget/src/test/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRendererTest.java b/framework/widget/src/test/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRendererTest.java
index 3337167..e04695f 100644
--- a/framework/widget/src/test/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRendererTest.java
+++ b/framework/widget/src/test/java/org/apache/ofbiz/widget/renderer/macro/MacroFormRendererTest.java
@@ -34,6 +34,7 @@ import org.apache.ofbiz.base.util.UtilHttp;
 import org.apache.ofbiz.base.util.UtilProperties;
 import org.apache.ofbiz.base.util.template.FreeMarkerWorker;
 import org.apache.ofbiz.entity.Delegator;
+import org.apache.ofbiz.webapp.control.ConfigXMLReader;
 import org.apache.ofbiz.webapp.control.RequestHandler;
 import org.apache.ofbiz.widget.model.ModelForm;
 import org.apache.ofbiz.widget.model.ModelFormField;
@@ -50,6 +51,7 @@ import javax.servlet.http.HttpServletResponse;
 import javax.servlet.http.HttpSession;
 import java.io.IOException;
 import java.io.StringReader;
+import java.io.StringWriter;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
@@ -71,6 +73,9 @@ public class MacroFormRendererTest {
     @Injectable
     private HttpServletResponse response;
 
+    @Injectable
+    private FtlWriter ftlWriter;
+
     @Mocked
     private HttpSession httpSession;
 
@@ -92,11 +97,7 @@ public class MacroFormRendererTest {
     @Mocked
     private ModelFormField modelFormField;
 
-    @Mocked
-    private Appendable appendable;
-
-    @Mocked
-    private StringReader stringReader;
+    private final StringWriter appendable = new StringWriter();
 
     @Injectable
     private String macroLibraryPath = null;
@@ -120,7 +121,7 @@ public class MacroFormRendererTest {
                 label.getText(withNotNull());
                 result = "";
 
-                new StringReader(anyString);
+                ftlWriter.executeMacro(withNotNull(), withNotNull());
                 times = 0;
             }
         };
@@ -164,6 +165,54 @@ public class MacroFormRendererTest {
     }
 
     @Test
+    public void displayEntityFieldMacroRenderedWithLink(@Mocked ModelFormField.DisplayEntityField displayEntityField,
+                                                        @Mocked ModelFormField.SubHyperlink subHyperlink)
+            throws IOException {
+
+        final Map<String, ConfigXMLReader.RequestMap> requestMapMap = new HashMap<>();
+
+        new Expectations() {
+            {
+                displayEntityField.getType();
+                result = "TYPE";
+
+                displayEntityField.getDescription(withNotNull());
+                result = "DESCRIPTION";
+
+                modelFormField.getTooltip(withNotNull());
+                result = "TOOLTIP";
+
+                displayEntityField.getSubHyperlink();
+                result = subHyperlink;
+
+                subHyperlink.getStyle(withNotNull());
+                result = "TestLinkStyle";
+
+                subHyperlink.getUrlMode();
+                result = "url-mode";
+
+                subHyperlink.shouldUse(withNotNull());
+                result = true;
+
+                subHyperlink.getDescription(withNotNull());
+                result = "LinkDescription";
+
+                subHyperlink.getTarget(withNotNull());
+                result = "/link/target/path";
+
+                request.getAttribute("requestMapMap");
+                result = requestMapMap;
+            }
+        };
+
+        Map<String, Object> context = new HashMap<>();
+        macroFormRenderer.renderDisplayField(appendable, context, displayEntityField);
+
+        System.out.println(appendable.toString());
+        assertAndGetMacroString("renderDisplayField", ImmutableMap.of("type", "TYPE"));
+    }
+
+    @Test
     public void textFieldMacroRendered(@Mocked ModelFormField.TextField textField) throws IOException {
         new Expectations() {
             {
@@ -987,7 +1036,7 @@ public class MacroFormRendererTest {
         new Verifications() {
             {
                 List<String> macros = new ArrayList<>();
-                new StringReader(withCapture(macros));
+                ftlWriter.executeMacro(withNotNull(), withCapture(macros));
 
                 assertThat(macros, not(empty()));
                 final String macro = macros.get(0);