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

ComponentUtils.escapeJQueryId() method escapes too much for JQuery 2.4 #2395

Closed
AndrewShwaika opened this Issue May 31, 2017 · 63 comments

Comments

Projects
None yet
5 participants
@AndrewShwaika

AndrewShwaika commented May 31, 2017

Extended layout with non full page is not working properly

<pe:layout full="false"> has been broken

ComponentUtils.escapeJQueryId() escapes every separator ':' with 4 backslashes '\\\\'
Javascript widget gets parameter with 2 backslashes. As example: #formId\\:clientId

JQuery 2.4 selector $('#formId\\:clientId') can't find an object with such id, but $('#formId\:clientId') works

1) Environment

  • PrimeFaces version: 6.1.1
  • Application server + version: Glassfish 4.1.1, Payara 4.1.1-171
  • Affected browsers: All
  • PrimeFaces Extensions: 6.1.1-SNAPSHOT

2) Expected behavior

pe:layout should be visible

3) Actual behavior

pe:layout is not visible

4) Steps to reproduce

Any page with <pe:layout full="false">

AndrewShwaika pushed a commit to AndrewShwaika/primefaces that referenced this issue May 31, 2017

@tandraschko

This comment has been minimized.

Member

tandraschko commented May 31, 2017

Is there any article about those jquery change available on the web?

@tandraschko tandraschko added the defect label May 31, 2017

@tandraschko tandraschko added this to the 6.2 milestone May 31, 2017

@tandraschko tandraschko self-assigned this May 31, 2017

@AndrewShwaika

This comment has been minimized.

AndrewShwaika commented May 31, 2017

Just debug of not working <pe:layout> after upgrade to Primefaces 6.1.1 and Extensions 6.1.1-SNAPSHOT.
JQuery selector could't find component with escaped string with 2 backslashes.
PrimeFaces.escapeClientId() Javascript method escapes with 1 backslash in result string and it works perfectly.

@tandraschko

This comment has been minimized.

Member

tandraschko commented May 31, 2017

Cant replicate.
I just opnend the showcase on my local machine (which runs with jquery 2.2.4)
http://localhost:8080/showcase/ui/input/inputTextarea.xhtml

and print the following in the console:

$('#j_idt87\\:j_idt89').length -> 1
$('#j_idt87\:j_idt89').length -> 0

@AndrewShwaika

This comment has been minimized.

AndrewShwaika commented May 31, 2017

But when use ComponentUtils.escapeJQueryId() as <pe:layout full="false"> for target.
Query will be like $('#j_idt87\\:j_idt89').length -> 0

@AndrewShwaika

This comment has been minimized.

AndrewShwaika commented May 31, 2017

Issue arise when JQuery identifier formats on server side.
It comes to client side with extra backslashes in string.

@tandraschko

This comment has been minimized.

Member

tandraschko commented May 31, 2017

Then we should check who appenss the additional backslash. IMO on the server side it looks ok.

@AndrewShwaika

This comment has been minimized.

AndrewShwaika commented Jun 1, 2017

Lets count.

Replacement constant string in ComponentUtils.escapeJQueryId() has 8 backslashes.
So internal string will have 4 backslashes.
Javascript for widget creation will have 4 backslashes in constant string in config.
Internal string for configuration parameter gets 2 backslashes!

This string is directly used in JQuery selector (PrimeFaces.widget.ExtLayout Javascript fragment):

this.jq = $(cfg.forTarget);

So jq.length -> 0

Same issue for PrimeFaces.widget.Layout

var dialog = $(this.cfg.center.paneSelector)...

@tandraschko

This comment has been minimized.

Member

tandraschko commented Jun 1, 2017

I see. You confused me with your statement that currently 2 backslashes will be rendered and doesn't work.

@tandraschko

This comment has been minimized.

Member

tandraschko commented Jun 2, 2017

@melloware Do you have time to verify that PE layout is currently broken and this fixes it? :)

@melloware

This comment has been minimized.

Contributor

melloware commented Jun 2, 2017

I will test this out this weekend and get back to you!

@melloware

This comment has been minimized.

Contributor

melloware commented Jun 2, 2017

@tandraschko Tested and everything looks good to me. I don't see any errors.

@tandraschko

This comment has been minimized.

Member

tandraschko commented Jun 2, 2017

So it already works without this PR?

@melloware

This comment has been minimized.

Contributor

melloware commented Jun 2, 2017

No i used this PR and patched my local and tested. I thought that is what you wanted. I can test it without the patch. I misunderstood what you wanted me to test.

@melloware

This comment has been minimized.

Contributor

melloware commented Jun 7, 2017

tandraschko added a commit that referenced this issue Jun 8, 2017

@tandraschko

This comment has been minimized.

Member

tandraschko commented Jun 8, 2017

@mertsincan I applied the PR to fix this issue, the details are here:
primefaces-extensions/primefaces-extensions.github.com#463
Would you merge it to elite to fix it?

@mertsincan

This comment has been minimized.

Member

mertsincan commented Jul 10, 2017

Thanks a lot for the PR ;)

@mertsincan

This comment has been minimized.

Member

mertsincan commented Aug 22, 2017

Hi @tandraschko,
Reverted this PR on 6.1.5 and 6.0.23. Issue; #2653
Our PRO Users have some problems related to this PR after upgrading PrimeFaces elite version. p:autocomplete, p:wizard, p:layout etc are not working after PF6.1.3. When I send a custom jar after reverting this PR, they say our codes work fine. Therefore, I reverted this PR on Elite versions for now. Also, one of our PRO User see the following error on console with 6.1.4;

pe:timePicker (Error: Syntax error, unrecognized expression: unsupported pseudo....)

He says this issue is fixed with your custom jar(without this PR). I wanted a sample code and maven project. When he sends it, I will send it to you.

@tandraschko

This comment has been minimized.

Member

tandraschko commented Aug 22, 2017

@melloware @Rapster we also did something in PFE, right? Could you check that?

@melloware

This comment has been minimized.

Contributor

melloware commented Aug 22, 2017

We ended up not having to fix anything in PFE. It was all fixed by this ComponentUtils fix after the Jquery Upgrade to 2.2.4. I feel like this fix is the right fix and we need to figure out why p:wizard and p:layout are not working with this fix.

@mertsincan

This comment has been minimized.

Member

mertsincan commented Aug 22, 2017

Hi @tandraschko , @melloware and @Rapster

I attached a sample maven project. Could you please try it? An issue is reported by PRO User related to this PR.

primefaces-test.zip

6.2-SNAPSHOT;

  • It doesn't work with this PR
  • it works fine without this PR
@melloware

This comment has been minimized.

Contributor

melloware commented Aug 22, 2017

I have confirmed your findings I get "Uncaught Error: Syntax error, unrecognized expression: unsupported pseudo: j_idt6"

in pe:timepicker it is doing this..

writer.write(",button:'" + org.primefaces.util.ComponentUtils.escapeJQueryId(clientId)
                  + " .pe-timepicker-trigger'");
@Rapster

This comment has been minimized.

Member

Rapster commented Aug 22, 2017

Hey sorry I couldn't help... I'm quite busy integrating Barcelona into our web apps. Will be quite busy for the next months :-/ Glad to hear that issue was fixed, oneof my colleague told me about this problem... Great job everybody! ;-)

@tandraschko

This comment has been minimized.

Member

tandraschko commented Aug 22, 2017

Everthing is ok - you do much more as the most users! ;)

@mertsincan

This comment has been minimized.

Member

mertsincan commented Aug 23, 2017

@tandraschko, I send an email about 64fd1f2

@Rapster

This comment has been minimized.

Member

Rapster commented Aug 23, 2017

@tandraschko you mentionned gtalk but there is also https://gitter.im/ that I think will fit more our need ;-)

@tandraschko

This comment has been minimized.

Member

tandraschko commented Aug 23, 2017

@Rapster We can also do that, no problem. I just used gtalk in the past as gmail is always open in the browser and i usually didn't chat with multiple persions till now.

@tandraschko

This comment has been minimized.

Member

tandraschko commented Aug 23, 2017

I think the fix is also ok when e.g. PFS is used in p:block block attribute as the user must only use 1 slash:
block="@(#j_idt87\:pnl)"

@mertsincan

This comment has been minimized.

Member

mertsincan commented Aug 23, 2017

Thanks, @tandraschko. I'll add the fixes in the DraggableRenderer.java and BaseMenuRenderer.java into elite versions. Also, can we add 1 slash before ":" character in core.expressions.js for PFS?

My test page; http://localhost:8080/showcase/ui/ajax/selector.xhtml
$('.panel :input') works
$('.panel \:input') works
$('.ui-panel \:input\:not(select)') works

@tandraschko

This comment has been minimized.

Member

tandraschko commented Aug 23, 2017

+1 if that works (but really only for PFS - for a normal id, document.getElementId instead of jQuery is used)

@mertsincan

This comment has been minimized.

Member

mertsincan commented Aug 23, 2017

Yes, only for PFS. Also, please see "Special characters in CSS" section in the https://mathiasbynens.be/notes/css-escapes

@melloware

This comment has been minimized.

Contributor

melloware commented Aug 23, 2017

@mertsincan Can you roll all of these changes into Elite 6.1.5 please?

@tandraschko

This comment has been minimized.

Member

tandraschko commented Aug 23, 2017

@mertsincan Whats the important part in your link?
I think we can even do it better:

  • ":" escaping should be done directly in WidgetBuilder#selectorAttr
  • Components like p:block, which uses SEF#resolveClientId should also use #selectorAttr instead of #attr

WDYT?

@melloware

This comment has been minimized.

Contributor

melloware commented Aug 23, 2017

@tandraschko I think you are suggesting similar to how @mertsincan added context.getResponseWriter().write(ComponentUtils.escapeEcmaScriptText(value)); in the attr() function that you would do the Jquery escaping in selectorAttr() method instead of us having to call ComponentUtils before passing into selectorAttr().

I like that.

@tandraschko

This comment has been minimized.

Member

tandraschko commented Aug 23, 2017

exactly yes. This will work fine for our case now.

The second part would be if we can always escape every ":" from the SEF#resolveClientIds, which may also contain PFS.

@mertsincan

This comment has been minimized.

Member

mertsincan commented Aug 24, 2017

@mertsincan Whats the important part in your link?

About my PFS suggestion (https://mathiasbynens.be/notes/css-escapes);

... Theoretically, the : character can be escaped as :, but IE < 8 doesn’t recognize that escape sequence correctly...

I think the solution is good for now. We just need to be careful when we use selectorAttr method.

@mertsincan

This comment has been minimized.

Member

mertsincan commented Aug 24, 2017

@melloware, I'll add this fix to elite versions.

@tandraschko

This comment has been minimized.

Member

tandraschko commented Aug 24, 2017

+1
is think the current behavior with PFS is ok, users must also use a slash when they use jQuery native.

@tandraschko

This comment has been minimized.

Member

tandraschko commented Aug 24, 2017

Maybe we should just think about moving the escape call inside #selectorAttr.

@melloware

This comment has been minimized.

Contributor

melloware commented Aug 24, 2017

@tandraschko +1 for moving the escape inside selectorAttr. I would think the only reason to use this special selectorAttr() call of WidgetBuilder is because you have a Jquery ID. So why count on the caller to escape it first when that is the point of this new method.

@tandraschko

This comment has been minimized.

Member

tandraschko commented Aug 24, 2017

commited something, please have a look at it :)

tandraschko added a commit that referenced this issue Aug 24, 2017

@melloware

This comment has been minimized.

Contributor

melloware commented Aug 24, 2017

I love it. Fixing PFE now!

@melloware

This comment has been minimized.

Contributor

melloware commented Aug 25, 2017

@mertsincan I have tested all of the changes @tandraschko has made for this fix. Can this entire fix be put into Elite 6.1.5 Release please?

@mertsincan

This comment has been minimized.

Member

mertsincan commented Aug 30, 2017

Added to 6.0.23 and 6.1.5. Thanks a lot!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment