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

Do not parse bindings inside script tags #3872

Closed

Conversation

TimvdLippe
Copy link
Contributor

Scripts are now ignored when parsing bindings. This makes it impossible to craft weird functions like returnStringWithBindingInScript.

Reference Issue

Fixes #3726

@TimvdLippe TimvdLippe force-pushed the fix-script-annotation-parsing branch from 9fdadd0 to 9cb224b Compare August 22, 2016 16:55
@TimvdLippe
Copy link
Contributor Author

The failure on Firefox is a result of <script> not being executed inside a <template> when stamped. See webcomponents/webcomponentsjs#470

Therefore you can craft the incorrect script behavior in Chrome, but you can not in Firefox.

@tjsavage tjsavage added the 1.x label Sep 8, 2016
@TimvdLippe
Copy link
Contributor Author

You can see the bug live here btw: https://timvdlippe.github.io/blog/#/b1-carbon-route
It shows <app-location route="&#123;&#123;route&#125;&#125;"></app-location> instead of <app-location route="{{route}}"></app-location>

@jfrazzano
Copy link

As a side note, this free-style parsing also becomes an issue with string literals as they are layered and parsed en masse at browser discretion; issues include ui collapse in favor of traditional don-insertions—elements placed on the dom at a previous time, removed, placed in a mobile ui, and then called (textarea was the villain) fall back to their old pointer location, and wipe current screen, solid garbage collection fixes this issue. On the “in script tag issue”, soon we will all be “stringing” things together, and the location and timing of the parse will be a challenge at best to identify… I sort of have an inkling their is a larger order of operations issue that $${maybe solved by slot assignment???.italics()}… Don’t know if this helps or is a valid concern. Just an issue I have experienced, when carelessly mixing traditional dom placement and parsing with on the fly es6 versions of same. Items and elements get lost. I have been mapping for myself and it helps a ton.

Also, Tim, on the library work: There are some simple element related parsing patterns with ternaries that make api building way easier: tabs, numbered sets of questions, all lists, list like columns, and /poorman’s tables/ fall under a one three element set, columned numbered lined animated text, drop down menus, toggle buttons, animated inputs with dropdown menus, dropdown menu cells that can animate to full pages—tested at sets of 50 dropdown items in 2500 inputs sharing the same dropdown, but filling it with different data, fall into the second.

The ternaries can get dicey if dealing with poor data sets on the way in, but if regularized in a professional setting, they could ad a dynamism, universality, polymorphism, and standards adherence (they endlessly destroy themselves and recreate themselves) that the more declarative, and state aware— i do miss that awareness, but that’s the opposite of the standard—approach offered.

Again, you guys can have anything I have done, and I only want to offer some help, if its possible.

Thanks again.

No response needed.

Keep up the awesome work.

Sincerely,

Jason

On Nov 13, 2016, at 6:22 PM, Tim van der Lippe [email protected] wrote:

You can see the bug live here btw: https://timvdlippe.github.io/blog/#/b1-carbon-route https://timvdlippe.github.io/blog/#/b1-carbon-route
It shows instead of


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub #3872 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AHxnvpA4PPnqyjUeMnZX4ZapGpm8-qWJks5q95vMgaJpZM4Joacl.

@tomalec
Copy link
Contributor

tomalec commented Nov 17, 2016

@TimvdLippe Please note that, unfortunately, my workaround for

The failure on Firefox is a result of <script> not being executed inside a <template> when stamped.

May result in another bug
webcomponents/webcomponentsjs#470 (comment)

@TimvdLippe
Copy link
Contributor Author

@tomalec I am sorry but I do not understand what you mean. Could you maybe elaborate on what you want to say?

@tomalec
Copy link
Contributor

tomalec commented Nov 17, 2016

I have found a workaround for the fact that "a result of <script> is not being executed inside a <template> when stamped". It's described at webcomponents/webcomponentsjs#470 (comment)

Thanks to that, I believe the fix you provided is even more useful. Even though it fails in Firefox, authors would be able to run it there with this workaround.

Then, what i tried to say in the comment above, is that today, I found a buggy side-effect of my workaround, which is described at webcomponents/webcomponentsjs#470 (comment) :(.

@TimvdLippe
Copy link
Contributor Author

@tomalec Ah I understand now. Yes that sounds like a good fix, hopefully you can find a solution for the side-effect so we can get this merged in 🎉

@TimvdLippe
Copy link
Contributor Author

@tomalec What is the status of your PR on the polyfill? Since the v1 polyfill is now being actively developed, will the v0 polyfill still be updated to work around this bug?

@tomalec
Copy link
Contributor

tomalec commented Jan 11, 2017

I haven't made a PR for that, yet. Also, I haven't tested it with v1, as back then I found more bugs in wc.js stopping me to get there. I would try to take another look somewhere in the middle of February.

What's more the ongoing redesign of HTML Imports polyfill behavior webcomponents/html-imports#1 may change the story drastically. Especially, that imported document may no longer be... er... a Document ;) https://github.com/webcomponents/html-imports/issues/3

@TimvdLippe
Copy link
Contributor Author

I will update this PR to both ignore styles and scripts, as that is really unintended. I will update the tests as well to work on non-Chrome browsers.

@TimvdLippe
Copy link
Contributor Author

Superseded by #4841

@TimvdLippe TimvdLippe closed this Sep 15, 2017
@TimvdLippe TimvdLippe deleted the fix-script-annotation-parsing branch September 15, 2017 23:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants