Use fixHtmlCmdFunc to ensure event-handlers are installed after the DOM is updated

[andreak]

Use fixHtmlCmdFunc to ensure event-handlers are installed after the DOM is updated

[andreak]

Fixes #1801

[Shadowfiend]

So, I'm actually not sure if this will work… In the delta bit, I think it's fine. But, in the case of the JqAppend and friends, these are JsExps. That means it can stand in wherever a value is expected, except using fixHtmlCmdFunc would emit JS that wouldn't allow that, I think? That's why there's a distinction between fixHtmlFunc and fixHtmlCmdFunc to begin with—fixHtmlFunc wraps everything in a JS IIFE with a final value of the original JsExp.

I wonder if the right way to fix this issue isn't to rewrite fixHtmlFunc to do this:

((function() { var result = <original JsExp>; <extracted commands>; return result; })())

I think this is incidentally also more in line with what would be the expected behavior of invoking this JS with an HTML block that contained a script block—executing it before the block has been acted on could yield some pretty funky results, including exactly the one we ran into here where the JS expects the content to already be in the DOM.

To be clear, I think the conversion of the things inside new JsCmd to use fixHtmlCmdFunc was correct no matter what we do with the rest.

[Shadowfiend]

Actually I'm still not sure if that'll work either… Because these are designed to be chained with ~>, so the result of the overall expression would go after a ., I think. If that's the case, then we would end up with something like $('#id').((function() { ... })()), which isn't valid JS.

The solution in this PR is valid only as long as you do a single method call—e.g. $('#id').append('one thing')—but the API is designed to take multiple—e.g., $('#id').append('one thing').append('another thing'). This approach would fall down because we'd get:

$('#id')
  .append('updated thing'); extractedJS;
  .append('another thing'); extractedJS;

Once again, invalid JS... Oof, this is a tough one 😞

Could be that fixHtmlFunc just calls appendJs. Basically runs through, extracts the JS from the content, runs S.appendJs with that JS, and returns the updated content. No IIFE, no extra JsCmds, etc.

EDIT: The downside, of course, is that this makes all of these stateful, which feels dirty and unnecessary. Another way to do it is to maybe give JsExp two methods, baseJs and extractedJs, and use fixHtmlAndJs to assign each of those in the subclasses. Then we'd have a single toJsCmd that joins the final baseJs and then adds extractedJs to the end. We'd need to rework the way ~>, +, and friends work, as well, to do this correctly.

[farmdawgnation]

Are there extra changes in this PR that don't deal with the core bug? I'm a bit confused at how all of this relates to the bug as described in the original issue.

That said, revisiting the original issue:

installing eventhandlers on elements which has not yet been inserted into the DOM

Am I crazy or does this sound like more of an ordering issue than a JsExp or JsCmd issue?

[Shadowfiend]

This is an ordering issue, but it's an ordering issue that gets at the fundamental underlying way that JsExp and JsCmd are designed to differ—JsExp as a vehicle for pieces of a JS expression, and JsCmd as a vehicle for a whole JS expression. Both can potentially reference HTML, and the HTML can in turn potentially contain event handlers, which need to be separated out for execution as separate commands. So the ordering issue blossoms in complexity because we are worried about two orderings: one, making sure event handlers are installed after an expression's HTML attaches to the DOM, and two, making sure that event handler attachment happens after a JsExp's chained form has been put into a command, i.e., making sure they are appended as JS after whatever final containing JsCmd is built from the JsExp.

We can also manage this on the client via a lift.nextTick or direct call to setTimeout. (EDIT: details here, use a similar approach to fixHtmlFunc, but append the event handlers inside a setTimeout function so they run after the current script has finished injecting whatever. Basically appendJs on the client heh.)

Lastly, no matter what we do, we'll need unit tests for this so we're not tripped up by it in the future.

[joescii]

@Shadowfiend Will those unit tests need to run JS in a browser environment?

[Shadowfiend]

No, that would pretty much be an integration test by definition. We just need to basically confirm that the JS looks like what we expect it to. (EDIT: Once we decide what we expect it to look like, of course ;) )

[joescii]

Ok, understood. I asked because in my vdom branch, @blmeadows and I have used HtmlUnit to accomplish this. I agree that it's integration testing at that point, but I rarely take time to split those hairs. :)

[Shadowfiend]

Ok, just looked at this again. I was wrong about this:

these are designed to be chained with ~>, so the result of the overall expression would go after a .

If we look at the invocations in the code base, we see patterns like this:

  case class JqAppend(content: NodeSeq) extends JsExp with JsMember {
    override val toJsCmd = 
      "append("+fixHtmlFunc("inline", content){a => a}+")"      
  }

Here, we apply fixHtmlFunc to the passed content, but then we pass the result itself to the append function, so that doesn't go after append.

However! This brings up a very interesting note: if there were ever a script element in content that referred to other stuff in that same NodeSeq, the above approach, which is the current one, would not work! This is because we wouldn't actually run the append until after the IIFE is invoked, therefore after the script that expected the element in question to already be in the DOM ran.

Net-net, my concern with the initial suggestion for fixing fixHtmlFunc is really maintaining “bug-parity” with the current Lift 2.6… Except the framework now triggers that bug consistently heh. I think for Lift 3.0.0, I'm going to rewrite fixHtmlFunc to use setTimeout (rather than add to the public API of lift.js) and then we can see if there's a better way after we get the initial release out the door.

[Shadowfiend]

@andreak can you try the branch shadowfiend-changes-to-fixhtmlfunc and tell me if that fixes your issue? And if not, any chance you can put up a sample project that exhibits the problem? I'd like to get this knocked out soon.

[andreak]

First; I think this is a serious issue that needs to be fixed before 3.0 goes final.

Second; We need to make this work with potential script-elements in content, and have them evaluated before a potential chained call to another JqSomething. If not it will require at least us (Visena) to still have our custom version of these JqAppend-like classes.

I really don't like the approach with setTimeout, I'm sorry but it is a code-smell and it seems hacky. We should find a better way, although I'm not sure what that is. I'm sure we can figure this out though.

I see that my proposed solution using fixHtmlCmdFunc does not work with chaining so that needs to be fixed as well.

I'm in Bulgaria with my 9-year old daughter this week (training camp) so I'm not able to be too creative here, but will try to give it some thoughts.

[Shadowfiend]

Hmmm… When you get back from vacation, can you lay out why the JS needs to be executed pre-DOM-append? The problem for me is that having that JS evaluated before a chained call is an actively broken model, IMO. The simplest example of this is the following construction:

Jq("#my-element") ~>
  JqAppend(
    Group(
      <div id="new-element" style="display: none;">Show me!</div>
      <script type="text/javascript">$('#new-element').show()</script>
    )
  )

It is incorrect to execute that script until after the new element has been appended (and won't work otherwise anyway). Now, when it's laid out as above it's easy to fix. But imagine you had a snippet that included JS like the above that could be rendered in a regular snippet or could be rendered as a JQuery append expression. The execution of that script is no longer the same in the two cases.

---

Btw, I do agree that using setTimeout is in general dirty and a smell. However, we're at this point deep in the RC cycle for Lift 3, and Lift 3's been on the docket for a while. Moreover, using setTimeout is an internal implementation detail (i.e., not part of public API).

I think the correct solution to this is to have JsExp track its internal JS and the HTML-extracted JS independently and only combine them when it is converted into a JsCmd—at which point no further chaining is possible. This also allows a custom JsExp at the end of the chain to change the way/order that the extracted JS is executed. But, this is an invasive change, and I suggest that we could implement this more correct version in Lift 3.1, transparently to most framework users.

[Shadowfiend]

Another option is really to just provide a flag to disable event extraction, or disable event extraction automatically when inline scripts are allowed in the SecurityRules ContentSecurityPolicy.

[andreak]

Note that I'm not advocating that the embeded JS needs to be executed pre-DOM-append, but before a potential chained call.

Take this:

JqId(JE.Str(uid)) ~> JqAppend(content) ~> JqSlideDown(600)

What I mean is that if content contains script-elements, they must be evaluated before the call to JqSlideDown()

[andreak]

So, all JS (both the event-extraction code and in script-elements) from content must be evaluated before the chained call to JqSlideDown. My point is that we'll be having a hard time doing that with the setTimeout-approach, unless I'm missing something?

[Shadowfiend]

You're right… Unfortunately I think we have to choose either before inserting the content, or after any chained calls. Each case has its own problems, and neither is strictly correct. Only the latter works for extracted DOM event handlers.

I don't think we can execute any JS after the current chained function actually executes, but before the next one does… Will keep thinking on it though.

[Shadowfiend]

@andreak any updated thoughts on this? Thinking of opening a PR to allow for disabling event extraction and perhaps disable it by default in Lift 3…

EDIT: Or disable it by default if your security rules allow inline JS.

[farmdawgnation]

How should we proceed on this PR given that we've shipped @Shadowfiend's fix?

[Shadowfiend]

I think we'll need to decide which path is the “correct” one—probably best done on the ML. The ultimate solution will have to be different than what's here, as it will be more core—would be down for closing this PR. While we wait on what @andreak thinks, I'll drop the milestone on the PR.

[farmdawgnation]

@Shadowfiend @andreak Ping, this has been here for awhile now.

[Shadowfiend]

Posted this ML thread to pick up the question of what to do. Closing this PR as the code here can produce invalid JS; we'll pick it back up in issue #1801 once we see what happens on the ML.