Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pe:layout not rendered with PF 6.1.2 #463

Closed
mmariotti opened this issue Jun 7, 2017 · 10 comments
Closed

pe:layout not rendered with PF 6.1.2 #463

mmariotti opened this issue Jun 7, 2017 · 10 comments
Assignees
Labels
Milestone

Comments

@mmariotti
Copy link

Hello,

try this simple snippet:

<h:form id="form">
    <pe:layout id="layout1" widgetVar="layout1" fullPage="false" stateCookie="false" style="height: 100px">
        <pe:layoutPane position="center">
            center
        </pe:layoutPane>
        <pe:layoutPane position="east" size="50%">
            east
        </pe:layoutPane>
    </pe:layout>
<h:form>

and nothing is displayed.

That's because this version of PF ships changes in WidgetBuilder:

public WidgetBuilder attr(String name, String value) throws IOException {
	if (value != null) {
		context.getResponseWriter().write(",");
		context.getResponseWriter().write(name);
		context.getResponseWriter().write(":\"");
		context.getResponseWriter().write(ComponentUtils.escapeEcmaScriptText(value)); // THIS LINE HAS BEEN CHANGED
		context.getResponseWriter().write("\"");
	}

	return this;
}

and now it escapes attribute values.

Inside LayoutRenderer:

protected void encodeScript(final FacesContext fc, final Layout layout) throws IOException {
	final ResponseWriter writer = fc.getResponseWriter();
	final String clientId = layout.getClientId(fc);
	final WidgetBuilder wb = getWidgetBuilder(fc);
	wb.initWithDomReady("ExtLayout", layout.resolveWidgetVar(), clientId);
	wb.attr("clientState", layout.isStateCookie());
	if (layout.isFullPage()) {
		wb.attr("forTarget", "body");
	} else {
		wb.attr("forTarget", ComponentUtils.escapeJQueryId(clientId)); // NOTE THIS LINE
	}
	
	...

there's another escape, resulting in cfg.forTarget: "#form\\:layout1" (note the double backslash), which is unresolved by jQuery.

The solution is very simple, just modify LayoutRenderer with:

if (layout.isFullPage()) {
		wb.attr("forTarget", "body");
	} else {
// 		wb.attr("forTarget", ComponentUtils.escapeJQueryId(clientId)); // REMOVED
		wb.nativeAttr("forTarget", ComponentUtils.escapeJQueryId(clientId)); // ADDED
	}
@melloware
Copy link
Member

I think it is related to this PF bug?

primefaces/primefaces#2395

@mmariotti
Copy link
Author

Yes, It is exactly the same thing.

@melloware
Copy link
Member

So looking at your proposed fix I think it will work with people using PF 6.1.0 Public Release and the new Elite 6.1.2 which uses Jquery 2 and has this escape in it.

@melloware
Copy link
Member

I will fix this and I plan on doing a PFE 6.1.1 Release very very soon as I have fixed a few issues discovered since the 6.1.0 Release and I don't want to hold onto them until the 6.2.0 release.

@tandraschko
Copy link
Member

-1 to fix this in PFE
this will then incompatible with the community 6.1
we should just fix it in PF and hopefully mert will merge to 6.1.3.

@mmariotti
Copy link
Author

Nope, it will not work on PF 6.1.0, since there's no WidgetBuilder.nativeAttr method. However, there exist more than one backward compatible solution, or:

  • refactor the renderer to use the ResponseWriter directly, avoiding WidgetBuilder
  • refactor ExtLayout JS widget init function to check cfg.forTarget

If you want I can code both solutions tomorrow

@mmariotti
Copy link
Author

Well, I just got my hands on a random PC, so here you are two different solutions:

  • LayoutRenderer solution:
core/src/main/java/org/primefaces/extensions/component/layout/LayoutRenderer.java

    protected void encodeScript(final FacesContext fc, final Layout layout) throws IOException
    {
        final ResponseWriter writer = fc.getResponseWriter();
        final String clientId = layout.getClientId(fc);
        final WidgetBuilder wb = getWidgetBuilder(fc);
        wb.initWithDomReady("ExtLayout", layout.resolveWidgetVar(), clientId);
        wb.attr("clientState", layout.isStateCookie());

//   REMOVED
//        if(layout.isFullPage())
//        {
//            wb.attr("forTarget", "body");
//        }
//        else
//        {
//            wb.attr("forTarget", ComponentUtils.escapeJQueryId(clientId));
//        }
        
//  ADDED
        String forTarget = layout.isFullPage() ? "body" : ComponentUtils.escapeJQueryId(clientId);
        wb.append(",forTarget:\"" + forTarget + "\"");
  • JS solution:
core/src/main/resources/META-INF/resources/primefaces-extensions/layout/1-layout.js

PrimeFaces.widget.ExtLayout = PrimeFaces.widget.DeferredWidget.extend({
    /**
     * Initializes the widget.
     * 
     * @param {object}
     *        cfg The widget configuration.
     */
    init : function(cfg) {
        this._super(cfg);
        this.cfg = cfg;
        this.id = cfg.id;

        // REMOVED
        //this.jq = $(cfg.forTarget);
        
        // ADDED
	if(cfg.forTarget == 'body') {
		this.jq = $(document.body);
	} else {
		this.jq = $(PrimeFaces.escapeClientId(cfg.id));
	}

Nevertheless, even if backward compatible, they are just workarounds.
IMHO changing the behavior of WidgetBuilder in a minor release has been a very bad choiche.
Expect more bugs like this because WidgetBuilder.attr(String, String) is referenced (PF + PE) 167 times!!

Keep up the good work!

@tandraschko
Copy link
Member

fixed in PF

@melloware
Copy link
Member

OK I am going to close this issue since it currently works with 6.1. Public Release and will be fixed in PF Elite 6.1.3.

@melloware
Copy link
Member

We have since modified this fix to use the new WidgetBuilder.selectorAttr which does the proper Jquery escaping. #476

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants