Wicket
  1. Wicket
  2. WICKET-4252 Ajax refactoring
  3. WICKET-3367

Rewrite all JavaScript inline event handlers to be proper attached event handlers

    Details

    • Type: Sub-task Sub-task
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 6.0.0-beta1
    • Component/s: None
    • Labels:
      None

      Description

      Currently events added inside onclick, onsubmit attributes are problematic because they cannot be cancelled by other event handlers that are attached to events via javascript's addEventListener() and friends. We should extract all such inlined handlers and attach them via our Wicket.Event api.

      1. WICKET-3367.patch
        310 kB
        Martin Grigorov
      2. WICKET-3367.patch
        300 kB
        Martin Grigorov
      3. WICKET-3367.patch
        219 kB
        Martin Grigorov
      4. WICKET-3367.patch
        3 kB
        Martin Grigorov

        Issue Links

          Activity

          Hide
          Martin Grigorov added a comment -

          IAjaxCallDecorator is removed. The replacement is IAjaxCallListener.

          Show
          Martin Grigorov added a comment - IAjaxCallDecorator is removed. The replacement is IAjaxCallListener.
          Hide
          Martin Grigorov added a comment -

          getEventHandler() is no needed anymore and is removed with ae8a88aab539cf701855c97cf7b312a19b41bbe2.
          See http://markmail.org/thread/azqvfogw3l6gwohb

          Show
          Martin Grigorov added a comment - getEventHandler() is no needed anymore and is removed with ae8a88aab539cf701855c97cf7b312a19b41bbe2. See http://markmail.org/thread/azqvfogw3l6gwohb
          Hide
          Emond Papegaaij added a comment -

          It seems there are still some problems left. If you search the wicket code for Wicket.Ajax.post and Wicket.Ajax.get, you'll find a few calls to the wicket AJAX api that still use the old format, which no longer works. Some of them are in the deprecated methods in AbstractDefaultAjaxBehavior, but for example AjaxFormComponentUpdatingBehavior.getEventHandler also uses the old API (with a string). Perhaps these methods should simply be removed? Users who changed the generated AJAX scripts probably have no problems upgrading to the new API.

          One more thing: AjaxEventBehavior.renderHead checks if the target is AJAX. That check is not needed anymore. The IHeaderResponse implementation for AJAX requests already renders onDomReady scripts as appended JS.

          Show
          Emond Papegaaij added a comment - It seems there are still some problems left. If you search the wicket code for Wicket.Ajax.post and Wicket.Ajax.get, you'll find a few calls to the wicket AJAX api that still use the old format, which no longer works. Some of them are in the deprecated methods in AbstractDefaultAjaxBehavior, but for example AjaxFormComponentUpdatingBehavior.getEventHandler also uses the old API (with a string). Perhaps these methods should simply be removed? Users who changed the generated AJAX scripts probably have no problems upgrading to the new API. One more thing: AjaxEventBehavior.renderHead checks if the target is AJAX. That check is not needed anymore. The IHeaderResponse implementation for AJAX requests already renders onDomReady scripts as appended JS.
          Hide
          Martin Grigorov added a comment - - edited

          The patch is committed to current trunk.

          There are few known problems:

          • ModalWindow example doesn't work completely (FIXED)
          • AjaxEditable**Label doesn't support the special behavior for ENTER/ESCAPE keys (FIXED)
          • the AjaxCheckBox in Ajax Todo List example doesn't mark itself as selected (FIXED)
          • KittenCaptcha example doesn't select the kittens (FIXED)
          • Ajax call throttling should be integrated somehow with the event handlers (FIXED)

          These problems will be debugged soon.

          Show
          Martin Grigorov added a comment - - edited The patch is committed to current trunk. There are few known problems: ModalWindow example doesn't work completely (FIXED) AjaxEditable**Label doesn't support the special behavior for ENTER/ESCAPE keys (FIXED) the AjaxCheckBox in Ajax Todo List example doesn't mark itself as selected (FIXED) KittenCaptcha example doesn't select the kittens (FIXED) Ajax call throttling should be integrated somehow with the event handlers (FIXED) These problems will be debugged soon.
          Hide
          Robert McGuinness added a comment -

          Martin,

          I'll familiarize myself with with 6.0 javascript layer and see if I can help.

          Show
          Robert McGuinness added a comment - Martin, I'll familiarize myself with with 6.0 javascript layer and see if I can help.
          Hide
          Martin Grigorov added a comment -

          Robert,

          I'll be thankful for some help on this.
          I also saw something similar described at https://plus.google.com/u/0/115060278409766341143/posts/ViaVbBMpSVG but I don't feel that I'm that deep in JavaScript

          Show
          Martin Grigorov added a comment - Robert, I'll be thankful for some help on this. I also saw something similar described at https://plus.google.com/u/0/115060278409766341143/posts/ViaVbBMpSVG but I don't feel that I'm that deep in JavaScript
          Hide
          Robert McGuinness added a comment -

          some food for thought that maybe Wicket can incorporate into it's javascript layer (pardon if this is already handled)

          "In some cases your scripts will load after the user has clicked on something that requires a JavaScript function to handle the click. It’s possible you have a pure HTML version, but if the user has JavaScript enabled then we want to use it, even if the JavaScript hasn’t loaded yet. There needs to be a way of handling events before all of the assets have finished loading."

          http://dailyjs.com/2011/11/28/flickr-async/

          Show
          Robert McGuinness added a comment - some food for thought that maybe Wicket can incorporate into it's javascript layer (pardon if this is already handled) "In some cases your scripts will load after the user has clicked on something that requires a JavaScript function to handle the click. It’s possible you have a pure HTML version, but if the user has JavaScript enabled then we want to use it, even if the JavaScript hasn’t loaded yet. There needs to be a way of handling events before all of the assets have finished loading." http://dailyjs.com/2011/11/28/flickr-async/
          Hide
          Martin Grigorov added a comment -

          Because initially I thought they are not common, like the other attributes are. But now I see the event is also needed for the timer behaviors which don't extend AjaxEventBehavior and I'm thinking to move the in ARA.

          My current problem is that jQuery Ajax doesn't support multipart form submit... And I don't want to use additional jQuery plugin for that, so I'm working on reusing our previous code for that. But now it doesn't look so nice anymore :-/

          Show
          Martin Grigorov added a comment - Because initially I thought they are not common, like the other attributes are. But now I see the event is also needed for the timer behaviors which don't extend AjaxEventBehavior and I'm thinking to move the in ARA. My current problem is that jQuery Ajax doesn't support multipart form submit... And I don't want to use additional jQuery plugin for that, so I'm working on reusing our previous code for that. But now it doesn't look so nice anymore :-/
          Hide
          Sven Meier added a comment -

          Nice work.

          Question:
          Any reason why "e" and "f" are not members of AjaxRequestAttributes (e.g. no AjaxRequestAttributes#form), but added directly to the json object?

          Show
          Sven Meier added a comment - Nice work. Question: Any reason why "e" and "f" are not members of AjaxRequestAttributes (e.g. no AjaxRequestAttributes#form), but added directly to the json object?
          Hide
          Martin Grigorov added a comment -

          Not sure how to respond to a comment in Jira, so I copied it below...

          > allowdefault - should we rename to stop-propagation which is a more commonly used term?

          A: preventDefault and stopPropagation are different thingies. Now I think allowDefault should be removed. Clicking on Ajax link or submitting a form with Ajax should not allow the default behavior (non-ajax click/submit). Probably I'll drop it.

          > one of the biggest pains right now is being able to pass extra parameters back to the server - as in i want to override ajax event behavior to append some parameters to the callback url. i think the easiest way to do this would be to add a callback parameters attribute that takes a list of functions to which the user can add and we evaluate in order, so:

          A: there is a way to add static parameters. I'll see how to make it easy to add dynamically calculated ones

          -------------------------------

          > i think using JSON objects to get started is great. but, i think when we are done designing and we are optimizing we should use something of our own so that we dont include any attributes that dont have values. for example, if there is no indicator then the call to .do() should not have i:null, etc. this will make js as short as possible.

          A: the delivered JSON object contains only the entries with values.

          > also, maybe we can add iheaderresponse.appendDoCall(attributes). this will allow us to further trim the js by removing multiple "Wicket.AJAX.do(" strings, and instead have one "Wicket.Ajax.do(" that takes a list of maps

          A: this will be better solved with HeaderResponseDecorator that collects all onDomReady contributions and delivers them in one <script> element. This is part of the patch that Emond and Hielke work on.

          Show
          Martin Grigorov added a comment - Not sure how to respond to a comment in Jira, so I copied it below... > allowdefault - should we rename to stop-propagation which is a more commonly used term? A: preventDefault and stopPropagation are different thingies. Now I think allowDefault should be removed. Clicking on Ajax link or submitting a form with Ajax should not allow the default behavior (non-ajax click/submit). Probably I'll drop it. > one of the biggest pains right now is being able to pass extra parameters back to the server - as in i want to override ajax event behavior to append some parameters to the callback url. i think the easiest way to do this would be to add a callback parameters attribute that takes a list of functions to which the user can add and we evaluate in order, so: A: there is a way to add static parameters. I'll see how to make it easy to add dynamically calculated ones ------------------------------- > i think using JSON objects to get started is great. but, i think when we are done designing and we are optimizing we should use something of our own so that we dont include any attributes that dont have values. for example, if there is no indicator then the call to .do() should not have i:null, etc. this will make js as short as possible. A: the delivered JSON object contains only the entries with values. > also, maybe we can add iheaderresponse.appendDoCall(attributes). this will allow us to further trim the js by removing multiple "Wicket.AJAX.do(" strings, and instead have one "Wicket.Ajax.do(" that takes a list of maps A: this will be better solved with HeaderResponseDecorator that collects all onDomReady contributions and delivers them in one <script> element. This is part of the patch that Emond and Hielke work on.
          Hide
          Igor Vaynberg added a comment -

          looks good. a couple of comments:

          allowdefault - should we rename to stop-propagation which is a more commonly used term?

          -------------------------------

          one of the biggest pains right now is being able to pass extra parameters back to the server - as in i want to override ajax event behavior to append some parameters to the callback url. i think the easiest way to do this would be to add a callback parameters attribute that takes a list of functions to which the user can add and we evaluate in order, so:

          u: 'editable-label?6-1.IBehaviorListener.0-text1-label',
          cbp: [], <-- thats the default - no extra params, or leave it out to make json shorter

          if we want to pass extra parameters i can say:

          getattributes().addcallbackparameterfunction("function(p)

          { p.val=Wicket.$('#foo').value }");

          which outputs

          cbp: [function(p) { p.val=Wicket.$('#foo').value }

          ]

          when we are building the callback url for a GET or parameter map for the POST we loop through these and let them build an extra map

          -------------------------------

          i think using JSON objects to get started is great. but, i think when we are done designing and we are optimizing we should use something of our own so that we dont include any attributes that dont have values. for example, if there is no indicator then the call to .do() should not have i:null, etc. this will make js as short as possible.

          also, maybe we can add iheaderresponse.appendDoCall(attributes). this will allow us to further trim the js by removing multiple "Wicket.AJAX.do(" strings, and instead have one "Wicket.Ajax.do(" that takes a list of maps

          but these are all nice to haves in the future...

          Show
          Igor Vaynberg added a comment - looks good. a couple of comments: allowdefault - should we rename to stop-propagation which is a more commonly used term? ------------------------------- one of the biggest pains right now is being able to pass extra parameters back to the server - as in i want to override ajax event behavior to append some parameters to the callback url. i think the easiest way to do this would be to add a callback parameters attribute that takes a list of functions to which the user can add and we evaluate in order, so: u: 'editable-label?6-1.IBehaviorListener.0-text1-label', cbp: [], <-- thats the default - no extra params, or leave it out to make json shorter if we want to pass extra parameters i can say: getattributes().addcallbackparameterfunction("function(p) { p.val=Wicket.$('#foo').value }"); which outputs cbp: [function(p) { p.val=Wicket.$('#foo').value } ] when we are building the callback url for a GET or parameter map for the POST we loop through these and let them build an extra map ------------------------------- i think using JSON objects to get started is great. but, i think when we are done designing and we are optimizing we should use something of our own so that we dont include any attributes that dont have values. for example, if there is no indicator then the call to .do() should not have i:null, etc. this will make js as short as possible. also, maybe we can add iheaderresponse.appendDoCall(attributes). this will allow us to further trim the js by removing multiple "Wicket.AJAX.do(" strings, and instead have one "Wicket.Ajax.do(" that takes a list of maps but these are all nice to haves in the future...
          Hide
          Martin Grigorov added a comment -

          Here are few real examples of the generated javascript for Links Example:

          <script type="text/javascript" id="event-click-c1_link1">
          /<![CDATA[/
          Wicket.Ajax.do(

          {"u":"links?0-1.IBehaviorListener.0-c1~link","e":"click","c":"c1_link1"}

          );
          /]]>/
          </script>

          <script type="text/javascript" id="event-click-c2_link2">
          /<![CDATA[/
          Wicket.Ajax.do(

          {"u":"links?0-1.IBehaviorListener.0-c2~link","e":"click","c":"c2_link2"}

          );
          /]]>/
          </script>
          <script type="text/javascript" id="event-click-c3_link3">
          /<![CDATA[/
          Wicket.Ajax.do(

          {"u":"links?0-1.IBehaviorListener.1-c3~link","e":"click","c":"c3_link3","i":"c3_link3--ajax-indicator"}

          );
          /]]>/
          </script>
          <script type="text/javascript" id="event-click-success_link4">
          /<![CDATA[/
          Wicket.Ajax.do(

          {"u":"links?0-1.IBehaviorListener.0-success~link","e":"click","c":"success_link4","pre":["return false;"],"fh":["alert('Failure');"],"bh":["alert('Before ajax call');"],"sh":["alert('Success');"]}

          );
          /]]>/
          </script>
          <script type="text/javascript" id="event-click-failure_link5">
          /<![CDATA[/
          Wicket.Ajax.do(

          {"u":"links?0-1.IBehaviorListener.0-failure~link","e":"click","c":"failure_link5","fh":["alert('Failure');"],"bh":["alert('Before ajax call');"],"sh":["alert('Success');"]}

          );
          /]]>/

          Show
          Martin Grigorov added a comment - Here are few real examples of the generated javascript for Links Example: <script type="text/javascript" id="event-click-c1_link1"> / <![CDATA[ / Wicket.Ajax.do( {"u":"links?0-1.IBehaviorListener.0-c1~link","e":"click","c":"c1_link1"} ); / ]]> / </script> <script type="text/javascript" id="event-click-c2_link2"> / <![CDATA[ / Wicket.Ajax.do( {"u":"links?0-1.IBehaviorListener.0-c2~link","e":"click","c":"c2_link2"} ); / ]]> / </script> <script type="text/javascript" id="event-click-c3_link3"> / <![CDATA[ / Wicket.Ajax.do( {"u":"links?0-1.IBehaviorListener.1-c3~link","e":"click","c":"c3_link3","i":"c3_link3--ajax-indicator"} ); / ]]> / </script> <script type="text/javascript" id="event-click-success_link4"> / <![CDATA[ / Wicket.Ajax.do( {"u":"links?0-1.IBehaviorListener.0-success~link","e":"click","c":"success_link4","pre":["return false;"],"fh":["alert('Failure');"],"bh":["alert('Before ajax call');"],"sh":["alert('Success');"]} ); / ]]> / </script> <script type="text/javascript" id="event-click-failure_link5"> / <![CDATA[ / Wicket.Ajax.do( {"u":"links?0-1.IBehaviorListener.0-failure~link","e":"click","c":"failure_link5","fh":["alert('Failure');"],"bh":["alert('Before ajax call');"],"sh":["alert('Success');"]} ); / ]]> /
          Hide
          Martin Grigorov added a comment - - edited

          A patch with AjaxAttributes.

          This is the current state. It is not yet finished!

          The idea is:

          • AjaxEventBehavior produces something like: Wicket.Ajax.do(attrs), where attrs is a JSON object like:
            {
            u: 'editable-label?6-1.IBehaviorListener.0-text1-label', // url
            m: 'POST', // method name. Default: 'GET'
            c: 'label7', // component id (String) or window for page
            e: 'click', // event name
            sh: [], // list of success handlers
            fh: [], // list of failure handlers
            pre: [], // list of preconditions. If empty set default : Wicket.$(settings {c}

            ) !== null
            ep: {}, // extra parameters
            async: true|false, // asynchronous XHR or not
            ch: 'someName|d', // AjaxChannel
            i: 'indicatorId', // indicator component id
            ad: true, // allow default
            }

          All old Ajax-related methods like #getChannel(), #getPrecondition(), #getSuccessScript(), #getFailureScript(), ... now donate to the AjaxAttributes and are marked as deprecated. Still around for backward compatibility.
          See LinksPage example to see how the old methods should be in 6.0.

          Most of the wicket-examples work.
          AjaxFallback*** are broken at the moment.
          Pooling of XMLHttpRequest objects is not preserved because I didn't find the reason we do this in the old code.

          www.json.org/java classes are used to generate the JSON but we can remove them if needed. Our needs are really simple.

          TODO:

          • fix all wicket-examples
          • use channels
          • the old decorateScript(Component, CharSeq) is now just BeforeHandler, i.e. there is no way to add script at the end of the original. decorateSuccess/Failure can be used to execute something after the Ajax call. Do we need handler that is executed immediately after the start of the Ajax call?
          • regenerate the unit tests' expectations
          • remove the old Ajax code in wicket-ajax.js
          Show
          Martin Grigorov added a comment - - edited A patch with AjaxAttributes. This is the current state. It is not yet finished! The idea is: AjaxEventBehavior produces something like: Wicket.Ajax.do(attrs), where attrs is a JSON object like: { u: 'editable-label?6-1.IBehaviorListener.0-text1-label', // url m: 'POST', // method name. Default: 'GET' c: 'label7', // component id (String) or window for page e: 'click', // event name sh: [], // list of success handlers fh: [], // list of failure handlers pre: [], // list of preconditions. If empty set default : Wicket.$(settings {c} ) !== null ep: {}, // extra parameters async: true|false, // asynchronous XHR or not ch: 'someName|d', // AjaxChannel i: 'indicatorId', // indicator component id ad: true, // allow default } All old Ajax-related methods like #getChannel(), #getPrecondition(), #getSuccessScript(), #getFailureScript(), ... now donate to the AjaxAttributes and are marked as deprecated. Still around for backward compatibility. See LinksPage example to see how the old methods should be in 6.0. Most of the wicket-examples work. AjaxFallback*** are broken at the moment. Pooling of XMLHttpRequest objects is not preserved because I didn't find the reason we do this in the old code. www.json.org/java classes are used to generate the JSON but we can remove them if needed. Our needs are really simple. TODO: fix all wicket-examples use channels the old decorateScript(Component, CharSeq) is now just BeforeHandler, i.e. there is no way to add script at the end of the original. decorateSuccess/Failure can be used to execute something after the Ajax call. Do we need handler that is executed immediately after the start of the Ajax call? regenerate the unit tests' expectations remove the old Ajax code in wicket-ajax.js
          Hide
          Igor Vaynberg added a comment -

          yeah, missed that it was only doing that for hrefs, cool

          Show
          Igor Vaynberg added a comment - yeah, missed that it was only doing that for hrefs, cool
          Hide
          Martin Grigorov added a comment -

          Issuing several Wicket.Event.add(targetX, 'click', fn1), Wicket.Event.add(targetX, 'click', fn2), Wicket.Event.add(targetX, 'click', fn3), ... will execute all functions, not just the last one.
          I see WicketTester as a problem here because it assumes that only one behavior per event type can exists. I'll create a task ticket for this functionality to make sure it is properly handled.

          OK. I'm going to look in introducing the AjaxAttributes.

          Show
          Martin Grigorov added a comment - Issuing several Wicket.Event.add(targetX, 'click', fn1), Wicket.Event.add(targetX, 'click', fn2), Wicket.Event.add(targetX, 'click', fn3), ... will execute all functions, not just the last one. I see WicketTester as a problem here because it assumes that only one behavior per event type can exists. I'll create a task ticket for this functionality to make sure it is properly handled. OK. I'm going to look in introducing the AjaxAttributes.
          Hide
          Igor Vaynberg added a comment -

          doing it this way wont solve the problem of not being able to attach multiple behaviors to the same event. we should probably write out the entire handler in wicket.event.add() instead of writing it into the attribute.

          also, i think adding AjaxAttributes now would be a Good Thing. they stay out of the way unless you want to touch them. and, if you are touching the callback url in the behavior chances are you will want to rewrite your code using attributes because without them it is very fragile. you have to do a sql-injeciton-like attack on the url string to even get your own attributes into it...so the current way is broken anyways.

          Show
          Igor Vaynberg added a comment - doing it this way wont solve the problem of not being able to attach multiple behaviors to the same event. we should probably write out the entire handler in wicket.event.add() instead of writing it into the attribute. also, i think adding AjaxAttributes now would be a Good Thing. they stay out of the way unless you want to touch them. and, if you are touching the callback url in the behavior chances are you will want to rewrite your code using attributes because without them it is very fragile. you have to do a sql-injeciton-like attack on the url string to even get your own attributes into it...so the current way is broken anyways.
          Hide
          Martin Grigorov added a comment -

          How big will be the refactoring here ?
          Matej did some major refactoring at http://svn.apache.org/repos/asf/wicket/sandbox/knopp/experimental few years ago. See https://cwiki.apache.org/WICKET/wicket-ajax-rewriting.html for more info. While I like the approach with o.a.w.ajax.AjaxRequestAttributes.java there I prefer to avoid of making bigger changes. Knowing how much Ajax I used in my previous applications at dailyjob I see it will be quite a work to migrate.

          Show
          Martin Grigorov added a comment - How big will be the refactoring here ? Matej did some major refactoring at http://svn.apache.org/repos/asf/wicket/sandbox/knopp/experimental few years ago. See https://cwiki.apache.org/WICKET/wicket-ajax-rewriting.html for more info. While I like the approach with o.a.w.ajax.AjaxRequestAttributes.java there I prefer to avoid of making bigger changes. Knowing how much Ajax I used in my previous applications at dailyjob I see it will be quite a work to migrate.
          Hide
          Martin Grigorov added a comment -

          We need to improve HeaderResponse to not create a new <script> for each event binding. It would be much better if there is just one <script> with bigger body.

          Show
          Martin Grigorov added a comment - We need to improve HeaderResponse to not create a new <script> for each event binding. It would be much better if there is just one <script> with bigger body.
          Hide
          Martin Grigorov added a comment -

          He is a possible change to accomplish this.

          Show
          Martin Grigorov added a comment - He is a possible change to accomplish this.

            People

            • Assignee:
              Martin Grigorov
              Reporter:
              Igor Vaynberg
            • Votes:
              1 Vote for this issue
              Watchers:
              6 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development