MyFaces Core
  1. MyFaces Core
  2. MYFACES-2858

pointless oamsubmit inline rendering (patch available)

    Details

    • Type: Improvement Improvement
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.2-SNAPSHOT
    • Fix Version/s: 2.0.2
    • Component/s: None
    • Labels:
      None

      Description

      We have had several functions rendered inline for ages, namely appendHiddenInput oamSubmit, the autoscroll stuff etc...
      I personally think the rendering of those functions as inline scripts is pointless, blows up the browsers tremendously and
      prevents that the affected scripts can be browser cached.
      A quick look at the code revealed that there is basically nothing which would prevent to externalize the scripts. My main problem is where to we handle the auto append code.
      My personal guess is we probably simply should add it as a resource definitions to the commandLink, Button etc.. renderers, any ideas regarding this?

        Activity

        Hide
        Jan-Kees van Andel added a comment -

        No opinion regarding externalizing the scripts. I think it's a good idea.

        What I did notice a while ago, was that those scripts are very annoying when doing AJAX. I didn't look at it in detail, but it looked like they messed up the event handling.

        Show
        Jan-Kees van Andel added a comment - No opinion regarding externalizing the scripts. I think it's a good idea. What I did notice a while ago, was that those scripts are very annoying when doing AJAX. I didn't look at it in detail, but it looked like they messed up the event handling.
        Hide
        Werner Punz added a comment -

        I have isolated most of the scripts, from what I can see is there is no direct event handling messed up, but what could be is that the autoscroll feature does something in that case, do you have a bugreport on the issue?
        But there is one section from what I can gather which never worked, that was that one
        //this never worked in our code because version never was defined, I will drop it for now, since no one uses
        //it anyway
        //if (agentString.indexOf('msie') != -1) {
        // if (!(agentString.indexOf('ppc') != -1 && agentString.indexOf('windows ce') != -1 && version >= 4.0) && window.external && window.external.AutoCompleteSaveForm)

        { // window.external.AutoCompleteSaveForm(form); // }

        //}

        (I commented that out) it sort of was a special context param which when enabled triggered autosave in older ie versions on input params, probably absolutely no one has used it, otherwise they would have stepped on following error:

        gentString.indexOf('windows ce') != -1 && version >= 4.0)

        the version var never was defined and probably would have caused an error. But the good thing is this code only was activated via a context param.

        Nevertheless externalizing all this should be prio #1 it reduces the page size by about 8kbyte on every request!!! and also makes the code more maintainable.

        Show
        Werner Punz added a comment - I have isolated most of the scripts, from what I can see is there is no direct event handling messed up, but what could be is that the autoscroll feature does something in that case, do you have a bugreport on the issue? But there is one section from what I can gather which never worked, that was that one //this never worked in our code because version never was defined, I will drop it for now, since no one uses //it anyway //if (agentString.indexOf('msie') != -1) { // if (!(agentString.indexOf('ppc') != -1 && agentString.indexOf('windows ce') != -1 && version >= 4.0) && window.external && window.external.AutoCompleteSaveForm) { // window.external.AutoCompleteSaveForm(form); // } //} (I commented that out) it sort of was a special context param which when enabled triggered autosave in older ie versions on input params, probably absolutely no one has used it, otherwise they would have stepped on following error: gentString.indexOf('windows ce') != -1 && version >= 4.0) the version var never was defined and probably would have caused an error. But the good thing is this code only was activated via a context param. Nevertheless externalizing all this should be prio #1 it reduces the page size by about 8kbyte on every request!!! and also makes the code more maintainable.
        Hide
        Werner Punz added a comment -

        Ok I just ran a test since I got the first basics working, the actual savings is 2-3 kbyte per page, which is not too shabby I guess.

        Show
        Werner Punz added a comment - Ok I just ran a test since I got the first basics working, the actual savings is 2-3 kbyte per page, which is not too shabby I guess.
        Hide
        Jan-Kees van Andel added a comment -

        Never mind. I'll report my thingy in a separate issue. It's not related to your externalization.

        Sorry for the unclear comment.

        Show
        Jan-Kees van Andel added a comment - Never mind. I'll report my thingy in a separate issue. It's not related to your externalization. Sorry for the unclear comment.
        Hide
        Werner Punz added a comment -

        Ok the isolation so far works here, but I also want a compressed uncompressed version like we have for jsf.js, it is a 60% filesize difference between
        those two.
        The second issue is, I have the same problem as with jsf.js if inline rendered already and included in a ppr case we do not habe any suppression
        on the server side hence the script gets aloaded anew in auto eval cases (mozilla)

        Show
        Werner Punz added a comment - Ok the isolation so far works here, but I also want a compressed uncompressed version like we have for jsf.js, it is a 60% filesize difference between those two. The second issue is, I have the same problem as with jsf.js if inline rendered already and included in a ppr case we do not habe any suppression on the server side hence the script gets aloaded anew in auto eval cases (mozilla)
        Hide
        Werner Punz added a comment -

        Ok this issue also fixes the double include of jsf.js and externalizes the oamSubmit functions.
        Leonardo and Jakob, could you please have a look at the patch. I am not directly committing it yet
        because it changes too many things in the core, and I do not want to break it.

        Show
        Werner Punz added a comment - Ok this issue also fixes the double include of jsf.js and externalizes the oamSubmit functions. Leonardo and Jakob, could you please have a look at the patch. I am not directly committing it yet because it changes too many things in the core, and I do not want to break it.
        Hide
        Jakob Korherr added a comment -

        committed a slightly modified version of Werner's patch.

        Show
        Jakob Korherr added a comment - committed a slightly modified version of Werner's patch.
        Hide
        Leonardo Uribe added a comment -

        We have to review first what happen if tomahawk + mojarra is used. Note in this case oamSubmit.js is not on the classpath.

        I think if myfaces is used, we could include oamSubmit.js inside our jsf.js, but if mojarra is used, include it on a separate file as part of tomahawk and add it inline as shown.

        Show
        Leonardo Uribe added a comment - We have to review first what happen if tomahawk + mojarra is used. Note in this case oamSubmit.js is not on the classpath. I think if myfaces is used, we could include oamSubmit.js inside our jsf.js, but if mojarra is used, include it on a separate file as part of tomahawk and add it inline as shown.
        Hide
        Leonardo Uribe added a comment -

        Note that on myfaces core there are two copies of oamSubmit.js: one on api and the other one on impl.

        The functions on oamSubmit.js:

        function oamSetHiddenInput(formname, name, value)
        function oamClearHiddenInput(formname, name, value)
        function oamSubmitForm(formName, linkId, target, params)

        Shouldn't we just put them on jsf.js, but change its names to something like <someothernamespace>.oamSetHiddenInput ? In theory, those methods are myfaces specific. If by some reason someone needs them defined, why don't just put a resource with the functions calling the ones with namespace like this:

        function oamSetHiddenInput(formname, name, value)

        { <someothernamespace>.oamSetHiddenInput(formname, name, value) }

        Or why don't create a variable that allows choose between render it inline or use the namespaced functions (default one)?

        Show
        Leonardo Uribe added a comment - Note that on myfaces core there are two copies of oamSubmit.js: one on api and the other one on impl. The functions on oamSubmit.js: function oamSetHiddenInput(formname, name, value) function oamClearHiddenInput(formname, name, value) function oamSubmitForm(formName, linkId, target, params) Shouldn't we just put them on jsf.js, but change its names to something like <someothernamespace>.oamSetHiddenInput ? In theory, those methods are myfaces specific. If by some reason someone needs them defined, why don't just put a resource with the functions calling the ones with namespace like this: function oamSetHiddenInput(formname, name, value) { <someothernamespace>.oamSetHiddenInput(formname, name, value) } Or why don't create a variable that allows choose between render it inline or use the namespaced functions (default one)?
        Hide
        Jakob Korherr added a comment -

        Acutally that's not true. It first was on api (in Werner's patch) and I moved it to impl, because it is an implementation detail.

        However you're right. I did not think about using tomahawk standalone with Mojarra. This will currently not work. Thus putting it on shared may solve the problem, but I have to try this first.

        If you put them on jsf.js, the scenario tomahawk + mojarra also won't work. I will try some scenarios and provide a patch afterwards!

        Show
        Jakob Korherr added a comment - Acutally that's not true. It first was on api (in Werner's patch) and I moved it to impl, because it is an implementation detail. However you're right. I did not think about using tomahawk standalone with Mojarra. This will currently not work. Thus putting it on shared may solve the problem, but I have to try this first. If you put them on jsf.js, the scenario tomahawk + mojarra also won't work. I will try some scenarios and provide a patch afterwards!
        Hide
        Jakob Korherr added a comment -

        As with MYFACESTEST-24, we can remove the setUpFactories() workaround for the PartialViewContextFactory. I will also do a snapshot deploy of MyFaces test to verify that this is updated in the repo too and that no-one has build problems because of that.

        Show
        Jakob Korherr added a comment - As with MYFACESTEST-24 , we can remove the setUpFactories() workaround for the PartialViewContextFactory. I will also do a snapshot deploy of MyFaces test to verify that this is updated in the repo too and that no-one has build problems because of that.
        Hide
        Jakob Korherr added a comment -

        This patch moves oamSubmit.js from impl to shared, thus it will be included in myfaces-impl and in tomahawk20 at build time (a few pom-changes were necessary to make this work, actually).

        The patch works in all possible scenarios:

        • MyFaces standalone (oamSubmit.js taken from myfaces-impl)
        • MyFaces and Tomahawk (oamSubmit.js taken from myfaces-impl)
        • Mojarra and Tomahawk (oamSubmit.js taken from tomahawk)

        The only difference is that when Mojarra is used with Tomahawk, oamSubmit.js will always be served as a compressed javascript file (regardless of the project stage), because tomahawk has no control over the resource-loading here. However I wouldn't consider this a problem. If someone thinks we should address this issue, then we just have to move the uncompressed file from internal-resources to resources and make ResourceUtils aware of the project stage.

        If there are no objections, I will commit this patch soon.

        Show
        Jakob Korherr added a comment - This patch moves oamSubmit.js from impl to shared, thus it will be included in myfaces-impl and in tomahawk20 at build time (a few pom-changes were necessary to make this work, actually). The patch works in all possible scenarios: MyFaces standalone (oamSubmit.js taken from myfaces-impl) MyFaces and Tomahawk (oamSubmit.js taken from myfaces-impl) Mojarra and Tomahawk (oamSubmit.js taken from tomahawk) The only difference is that when Mojarra is used with Tomahawk, oamSubmit.js will always be served as a compressed javascript file (regardless of the project stage), because tomahawk has no control over the resource-loading here. However I wouldn't consider this a problem. If someone thinks we should address this issue, then we just have to move the uncompressed file from internal-resources to resources and make ResourceUtils aware of the project stage. If there are no objections, I will commit this patch soon.
        Hide
        Jakob Korherr added a comment -

        Patch applied, this should work now in every possible scenario.

        Show
        Jakob Korherr added a comment - Patch applied, this should work now in every possible scenario.

          People

          • Assignee:
            Jakob Korherr
            Reporter:
            Werner Punz
          • Votes:
            1 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development