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.

Sign up for GitHub

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

Jump to bottom

expressions with html not evaluated #744

Closed
simoami opened this issue May 19, 2015 · 12 comments
Closed

expressions with html not evaluated #744

simoami opened this issue May 19, 2015 · 12 comments
Assignees
@aMarCruz
Labels
enhancement feature request question

Comments

@simoami
Copy link

simoami commented May 19, 2015

An expression like this does not get evaluated:

{ [address1,address2].join('<br>') }

{ address2 ? '<br>' + address2 : '' }

The presence of html content, even though quoted, causes the expression to be bypassed.
@rsbondi
Copy link
Contributor

rsbondi commented May 20, 2015

I have verified, this is a bug

@samuelmesq
Copy link
Contributor

whenever I see examples like this, I think the best thing to do is not to put the logic in the view.

@cognitom
Copy link
Member

I've thought this is a spec. The document says:

Riot expressions can only render text values without HTML formatting
https://muut.com/riotjs/guide/#render-unescaped-html

Even if it works, you need to avoid sanitising.

@simoami
Copy link
Author

simoami commented May 31, 2015

Thanks for the update.

I'm very much aware that this type of processing can be delegated to a member function. Just like every starting project, one gets to start somewhere and then improve code during a second iteration of refactoring phase. However, can we admit that this is indeed a limitation of the parsing engine? You can limit your thoughts to the example I gave, but I'm sure someone else will present a more valid use case.

@simoami
Copy link
Author

simoami commented May 31, 2015

@cognitom
Thanks for the link. Since this is already mentioned in the guide, this would be a feature enhancement request. It's not so obvious that html tags would cause expressions to be rendered as static text. It took me time to find out what could be wrong with the expression, until I found the root cause.

The feeling I got from using Riot is "It just works!". I'd like for html support in expressions to follow the same trend :)

@aMarCruz
Copy link
Contributor

By the way, from the expression parser standpoint
in the second case { address2 ? '<br>' + address2 : '' }, the <br> doesn't matter, is the ternary :.
The RegExp used by the parser confuses these as a object key name { address2 : '' }, and don't fix address2.
@cognitom, posible workaround is { address2 ? '<br>' + this.address2 : '' }, but I don't test it.

Fixing requires change the logic, and about twice code.

@aMarCruz
Copy link
Contributor

aMarCruz commented Jul 8, 2015

@GianlucaGuarini ,
I'm tracing the sample of jsfiddle with the new version of tmpl.js, this issue is related to the parser in the compiler, I think.
tmpl() receives { [address1,address2].join(' only, and wraps this part as expected, but obviously a syntax error generated.
I can't enything about this, 'cause don't know how the compiler works.

aMarCruz added a commit that referenced this issue Jul 9, 2015
@aMarCruz
Update tmpl.js, many changes.
1d361fb 
@tipiirai, @GianlucaGuarini, @cognitom, @rsbondi

This days I spent some time cleaning and testing tmpl.js, and reviewing its logic.
This is the result.

I wanted above all: backward compatibility, correct details that have generated bugs & issues,
provide flexibility in the implementation, and facilities to understand the code through multiple comments; all without the code grew ...too
(the minified size of this version is XXX bytes vs XXX bytes of the one in the master branch).

But I have not had time to study other parts of riot, and how they interact with tmpl,
so perhaps tmpl caller filters out characteristics that this code implements, and
therefore has grown unnecessarily.
EDIT: In testing with some issues, I think there are details in the compiler that cause these issues. The string received from tmpl is filtered, new characteristics are not so usefull, and in other cases, the output of tmpl is ignored.

This file passed all riot tests and, in my opinion, is in beta phase, ready to test for
performance and in real environments, so please, break the code now.
I'm test it in latest IE, FireFox, and Chrome. I will do some mobile tests later.

Code is heavily commented 'cause today, we know what we doing, tomorrow... God knows?

#784 is fixed
#744 is related to the compiler

Thanks.
aMarCruz added a commit that referenced this issue Jul 9, 2015
@aMarCruz
tmpl.js complete rewriting
40153c4 
@tipiirai @GianlucaGuarini @cognitom @rsbondi , hi

Last days I spent some time cleaning and testing tmpl.js, and reviewing its logic.
This is the result.

I wanted above all: backward compatibility, correct details that have generated bugs & issues,
provide flexibility in the implementation, and facilities to understand the code through multiple comments; all without the code grew ...too
(the minified size of this version is XXX bytes vs XXX bytes of the one in the master branch).

But I have not had time to study other parts of riot, and how they interact with tmpl,
so perhaps tmpl caller filters out characteristics that this code implements, and
therefore has grown unnecessarily.
EDIT: In testing with some issues, I think there are details in the compiler that cause these issues. The string received from tmpl is filtered, new characteristics are not so usefull, and in other cases, the output of tmpl is ignored.

This file passed all riot tests and, in my opinion, is in beta phase and ready for testing on
performance and in real environments, so please, break the code now.
I'm test it in latest IE, FireFox, and Chrome. I will do some mobile tests later.

Code is heavily commented 'cause today, we know what we doing, tomorrow... God knows?
(Corrections are welcomed, the English is not my friend)

#784 is fixed
#744 is related to the compiler

Thanks.
@aMarCruz aMarCruz mentioned this issue Jul 9, 2015
Closed
@GianlucaGuarini GianlucaGuarini added this to the 2.3.0 milestone Aug 5, 2015
@cognitom cognitom mentioned this issue Aug 19, 2015
Closed
@maturanomx maturanomx mentioned this issue Aug 19, 2015
Closed
@GianlucaGuarini GianlucaGuarini removed this from the 2.3.0 milestone Nov 5, 2015
@aMarCruz
Copy link
Contributor

This is fixed in 2.3.x, only restriction is you can't use unquoted '>' in expressions.

@aMarCruz
Copy link
Contributor

Sorry, I was wrong with this issue, it is not fixed. We are working in a workaround yet. It is in the roadmap to riot 2.4.0 #1322

@aMarCruz aMarCruz reopened this Nov 16, 2015
@aMarCruz aMarCruz mentioned this issue Dec 2, 2015
Closed
@aMarCruz
Copy link
Contributor

aMarCruz commented Dec 2, 2015

I will close this issue. Raw HTML is WIP now. See tmpl#2 for details.

@aMarCruz aMarCruz closed this as completed Dec 2, 2015
@MartinMuzatko
Copy link
Contributor

This issue popped up for a few times, happy you decided to make it possible.

@simoami
Copy link
Author

simoami commented Dec 3, 2015

@aMarCruz Sounds good. Thanks for the update.

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

@aMarCruz aMarCruz

Labels
enhancement feature request question
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@cognitom @samuelmesq @GianlucaGuarini @simoami @MartinMuzatko @rsbondi @aMarCruz