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

Regression: Setting innerHTML in Angular template broken #2730

Closed
paulroemer opened this issue Oct 20, 2017 · 17 comments
Closed

Regression: Setting innerHTML in Angular template broken #2730

paulroemer opened this issue Oct 20, 2017 · 17 comments
Assignees
Milestone

Comments

@paulroemer
Copy link
Contributor

Hi all,

We noticed a regression in Angular templates so that using innerHTML does not work anymore:

In Angular template:
<section class="blog-post-view__article-body" itemprop="articleBody" [innerHTML]="post.text"></section>

Rendered in browser:
<section class="blog-post-view__article-body" itemprop="articleBody"></section>

As we are not using innerHTML for Polymer templates we cannot be sure if they are affected, too.

That worked with Flow 0.1.22 but not after upgrading to 1.0.0.alpha5.

@SomeoneToIgnore
Copy link
Contributor

May be caused by: #2375

@paulroemer
Copy link
Contributor Author

paulroemer commented Oct 20, 2017

Did you change the JSoup version during the update or have you used a patched JSoup before?

If it is about changing the version only, I can force a particular one via Maven.

@SomeoneToIgnore
Copy link
Contributor

It's both: we were using custom version before this change and updated to a stock one after it was released with the API we need.

@paulroemer
Copy link
Contributor Author

Ok, so no easy way back.

@SomeoneToIgnore
Copy link
Contributor

First of all, this may not be the case.

At second, we can try out the old version and see if it works (I'll release one for you)
If it does, we will sure roll back on the old one, since we're not supposed to break things for you.

@SomeoneToIgnore SomeoneToIgnore self-assigned this Oct 20, 2017
@paulroemer
Copy link
Contributor Author

Found an interesting affect:

After JSoup.parse() parse the document I can see different handling for innerHTML:

That one is the one that is not working in the end. Note that it is innerhtml now.
<section class="blog-post-view__article-body" itemprop="articleBody" [innerhtml]="post.text"></section>

That one still works.
<p class="blog-post-view__author-description" itemprop="description" [innerHTML]="post.authorDescription"></p>

Check https://preview.vaadin.com/blog/community-spotlight-august-2017

You will notice that the body of the blog is not displayed but the author*s description.

@heruan
Copy link
Member

heruan commented Oct 20, 2017

BTW isn't Angular Template about to be removed?

@paulroemer
Copy link
Contributor Author

08:29:06 vaadin@tuxbook:~/repos/webpage/webpage-frontend> find src/main/resources/com/vaadin/site/view/ -type f|grep View|wc -l
99

Feel free :) We are always happy about PRs!

@SomeoneToIgnore
Copy link
Contributor

BTW isn't Angular Template about to be removed?

Unfortunately, we have to stay with it for a little longer, due to internal dependencies, like the one @paulroemer is talking about :)
Paul, @heruan won't be able to fix those, he's an external contributor :)

Found an interesting affect:

So, did I get it right that the issue is the lower case? Or is it still not displayed?

@paulroemer
Copy link
Contributor Author

@heruan Damn, I forgot that the repo is public nowadays. Sorry for that. But now you know that we have a big cleanup task before we can remove the support.

@SomeoneToIgnore
As the replacing takes place on client side I cannot debug it that easy. But it's likely. The question is why JSoup makes it to lower case once but keeps the case for the other.

@SomeoneToIgnore
Copy link
Contributor

But it's likely.

That sounds promising. I'll prepare a special version for you with the old dependency, but it would be super cool if you try to fix the case thing, since it's the way it's supposed to work officially.
And if you fail or there are way too much changes, we'll just fall back to the original version.

@paulroemer
Copy link
Contributor Author

paulroemer commented Oct 20, 2017

At the moment I have no idea why JSoup is doing it the way it does. Of course, I would prefer fixing it without switching back again.

Since 1.10.1 there is the possibility to keep the tag's case:
https://stackoverflow.com/questions/31400712/how-to-preserve-case-in-jsoup-parsing

I will create a patched template parser routine and test locally if that helps.

@paulroemer
Copy link
Contributor Author

Okay, using the ParserSettings to prevent JSoup from lower casing the tags did the trick.

@SomeoneToIgnore
Copy link
Contributor

So, we can close the issue? :)

@paulroemer
Copy link
Contributor Author

Nope, because I had to patch the angular template parser.

paulroemer added a commit to paulroemer/flow that referenced this issue Oct 20, 2017
@paulroemer
Copy link
Contributor Author

#2734

@SomeoneToIgnore
Copy link
Contributor

Have released a supplementary version.

@torok-peter torok-peter added this to the 1.0.0.alpha7 milestone Oct 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants