ICEfaces
  1. ICEfaces
  2. ICE-5652

UpdateElements.coalesceWithPrevious overwrites JavaScript calls

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.8.2
    • Fix Version/s: 1.8.3, 1.8.2-EE-GA_P02, 2.0.0
    • Component/s: Framework
    • Labels:
      None
    • Environment:
      Linux
      Java 1.6.0_18

      Description

      Our web application receives async events from our external application server that directs the web application to display something new. For example, an event is sent to the web server environment to display a certain object in a page, or display a tab within a page, or focus on a specific field on a page. To implement this, we use Server push to update the page in the browser, and often we use javascript calls (via the JavascriptContext.addJavascriptCall method) to execute this. Typically, we use the renderLater() method on the PersistentFacesContext to initiate the server push.

      We can get multiple async events at once from our server. We've noticed cases where multiple events were being applied to the PersistentFacesState before it had a chance to send the updates to the browser. In these cases, the UpdateElements were combining the changes (as expected).

      What we noticed was if two or more commands were coalesced together, and both commands had different Javascript commands, only the last one was sent to the browser. The coalesce command overwrote any existing Javascript command with the last one in. This is causing us problems, as many of the subsequent events rely on the previous events being sent.
      1. javascript_coalesce.patch
        1 kB
        Ed Hillmann
      2. updatedPatch.txt
        3 kB
        Ed Hillmann

        Activity

        Ed Hillmann created issue -
        Hide
        Ed Hillmann added a comment -

        Patch that changes the UpdateElements so that any JavaScript calls being coalesced will be added to previous javascript calls instead of replacing. Created against 1.8.2 open source code.

        Show
        Ed Hillmann added a comment - Patch that changes the UpdateElements so that any JavaScript calls being coalesced will be added to previous javascript calls instead of replacing. Created against 1.8.2 open source code.
        Ed Hillmann made changes -
        Field Original Value New Value
        Attachment javascript_coalesce.patch [ 12313 ]
        Tyler Johnson made changes -
        Salesforce Case [5007000000C2KVN]
        Ken Fyten made changes -
        Fix Version/s 1.8.2-EE-GA_P02 [ 10226 ]
        Assignee Deryk Sinotte [ deryk.sinotte ]
        Hide
        Deryk Sinotte added a comment -

        Analyze and verify the customer patch for inclusion in the EE-p02 release.

        Show
        Deryk Sinotte added a comment - Analyze and verify the customer patch for inclusion in the EE-p02 release.
        Deryk Sinotte made changes -
        Assignee Deryk Sinotte [ deryk.sinotte ] Mircea Toma [ mircea.toma ]
        Deryk Sinotte made changes -
        Assignee Priority P2
        Hide
        Ted Goddard added a comment -

        If JavaScript calls are accumulated rather than coalesced, a slow client will be required to play back a sequence of all intermediate states of the application rather than being able to catch up to the current state. In extreme cases, the client may not be able to play back the events at the same frequency delivered by the server and may never catch up.

        We would like to discuss the application features in more detail to understand the role of this patch better.

        Show
        Ted Goddard added a comment - If JavaScript calls are accumulated rather than coalesced, a slow client will be required to play back a sequence of all intermediate states of the application rather than being able to catch up to the current state. In extreme cases, the client may not be able to play back the events at the same frequency delivered by the server and may never catch up. We would like to discuss the application features in more detail to understand the role of this patch better.
        Hide
        Ed Hillmann added a comment -

        I'm happy to provide more details. Would you like them included as part of this Issue, or through another mechanism?

        Show
        Ed Hillmann added a comment - I'm happy to provide more details. Would you like them included as part of this Issue, or through another mechanism?
        Hide
        Ted Goddard added a comment -

        Please provide some examples here in preparation for a meeting.

        Show
        Ted Goddard added a comment - Please provide some examples here in preparation for a meeting.
        Hide
        Ted Goddard added a comment -

        We may need to provide both "coalescing" and "concatenating" JavaScript call regions: the examples will help to clarify this.

        Show
        Ted Goddard added a comment - We may need to provide both "coalescing" and "concatenating" JavaScript call regions: the examples will help to clarify this.
        Hide
        Mircea Toma added a comment -

        To properly solve the issue code concatenation we would need to introduce another command type (EvalCode?) that will have its own coalescing rules relative to the itself and the other types of commands that can be sent to the browser.

        Show
        Mircea Toma added a comment - To properly solve the issue code concatenation we would need to introduce another command type (EvalCode?) that will have its own coalescing rules relative to the itself and the other types of commands that can be sent to the browser.
        Hide
        Ed Hillmann added a comment -

        For this particular problem, our ICEfaces application receives asynchronous events from our Server component. The first event instructs the client to assign focus to a specific field (which we implement using the requestFocus() method). The second event returns data that was evaluated asynchronously (we update our model with the value). In both cases, Server Push is used to send the updates to the browser.

        What we were finding when the issue was raised was that the javascript created that assigned focus was lost by the subsequent UpdateElements (which contained Javascript used for manageing modal dialogs and autocomplete components). It seems to be an intermittent problem, but we thought this was more due to inconsistent time spent in the server triggering these events into the client. I've tried to return some relevant data without our patch in ICEfaces, but due to the difficulty in recreating the error, I am having trouble supplying something to look at.

        Whenever we get any updates sent to the browser, we're seeing Javascript for modal dialogs and autocomplete components. I think this is part of the issue, in that both async events trigger updates that contain javascript calls. In the case where the icefaces app receives the Focus async event first, it has the Focus javascript in the UpdateElements. Then, when the Async Deriv event is received, it has Javascript for modal dialogs and autocomplete components. Commands like

        new Ice.Autocompleter('mainForm:field_LayFact2024_08082003_090159-Contact-Type','mainForm:field_LayFact2024_08082003_090159-Contact-Type_div',

        {frequency:0.2}

        ,'iceSelInpTxtRow mandatoryFieldRow','iceSelInpTxtSelRow mandatoryFieldSelRow');

        When these two commands are coalesced, the original Javascript is lost and focus is never assigned to the desired field in the browser.

        However, looking at this again more closely, I'm not really happy with the patch either. We're getting a lot of duplicated calls in the javascript, in regards to the modal and autocomplete calls (one per coalesced command). I've been having a bit of a play, and find this to be a better fit for our app. I've tried to make a patch file that compares the new UpdateElements.java field with the original, unpatched file. Hopefully, it can be applied. It's adding a new private method that attempts to coalesce the Javascript commands (split by semi colons), instead of a blanket replacement. This is working nicely for us locally, as we're preverving all of the javascript events while not duplicating javascript commands that existed in the previous UpdateElements object.

        I hope this is enough to go on.

        Show
        Ed Hillmann added a comment - For this particular problem, our ICEfaces application receives asynchronous events from our Server component. The first event instructs the client to assign focus to a specific field (which we implement using the requestFocus() method). The second event returns data that was evaluated asynchronously (we update our model with the value). In both cases, Server Push is used to send the updates to the browser. What we were finding when the issue was raised was that the javascript created that assigned focus was lost by the subsequent UpdateElements (which contained Javascript used for manageing modal dialogs and autocomplete components). It seems to be an intermittent problem, but we thought this was more due to inconsistent time spent in the server triggering these events into the client. I've tried to return some relevant data without our patch in ICEfaces, but due to the difficulty in recreating the error, I am having trouble supplying something to look at. Whenever we get any updates sent to the browser, we're seeing Javascript for modal dialogs and autocomplete components. I think this is part of the issue, in that both async events trigger updates that contain javascript calls. In the case where the icefaces app receives the Focus async event first, it has the Focus javascript in the UpdateElements. Then, when the Async Deriv event is received, it has Javascript for modal dialogs and autocomplete components. Commands like new Ice.Autocompleter('mainForm:field_LayFact2024_08082003_090159- Contact-Type','mainForm:field_LayFact2024_08082003_090159 -Contact-Type_div', {frequency:0.2} ,'iceSelInpTxtRow mandatoryFieldRow','iceSelInpTxtSelRow mandatoryFieldSelRow'); When these two commands are coalesced, the original Javascript is lost and focus is never assigned to the desired field in the browser. However, looking at this again more closely, I'm not really happy with the patch either. We're getting a lot of duplicated calls in the javascript, in regards to the modal and autocomplete calls (one per coalesced command). I've been having a bit of a play, and find this to be a better fit for our app. I've tried to make a patch file that compares the new UpdateElements.java field with the original, unpatched file. Hopefully, it can be applied. It's adding a new private method that attempts to coalesce the Javascript commands (split by semi colons), instead of a blanket replacement. This is working nicely for us locally, as we're preverving all of the javascript events while not duplicating javascript commands that existed in the previous UpdateElements object. I hope this is enough to go on.
        Hide
        Ed Hillmann added a comment -

        Version 2 of what we've done locally

        Show
        Ed Hillmann added a comment - Version 2 of what we've done locally
        Ed Hillmann made changes -
        Attachment updatedPatch.txt [ 12662 ]
        Ted Goddard made changes -
        Assignee Mircea Toma [ mircea.toma ] Ted Goddard [ ted.goddard ]
        Hide
        Ted Goddard added a comment -

        The root cause in this case appears to be extra JavaScript being generated by modal popups and autocomplete. Assigning to Ken to consider these component bug fixes for P02.

        Show
        Ted Goddard added a comment - The root cause in this case appears to be extra JavaScript being generated by modal popups and autocomplete. Assigning to Ken to consider these component bug fixes for P02.
        Ted Goddard made changes -
        Assignee Ted Goddard [ ted.goddard ] Ken Fyten [ ken.fyten ]
        Ken Fyten made changes -
        Assignee Ken Fyten [ ken.fyten ] Mircea Toma [ mircea.toma ]
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #23101 Thu Nov 11 10:54:12 MST 2010 mircea.toma ICE-5652 Send JS code to controll the popup only when the visibility changes its state.
        Files Changed
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/ext/renderkit/GroupRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/panelpopup/PanelPopupRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/panelpopup/PanelPopup.java
        Hide
        Mircea Toma added a comment -

        Changed popup panel component and renderer to send the JS code controlling the popup in the client only when the visibility is changed. This way the code sent for hiding the popup is send only once after the popup was made invisible, any other actions that won't change the visibility will not trigger JS updates for the popup.

        Show
        Mircea Toma added a comment - Changed popup panel component and renderer to send the JS code controlling the popup in the client only when the visibility is changed. This way the code sent for hiding the popup is send only once after the popup was made invisible, any other actions that won't change the visibility will not trigger JS updates for the popup.
        Ken Fyten made changes -
        Fix Version/s 2.0.0 [ 10230 ]
        Fix Version/s 1.8.3 [ 10211 ]
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #23102 Thu Nov 11 16:58:04 MST 2010 mircea.toma ICE-5652 Send JS initalization code only on page load or component markup update instead of sending it all the time.
        Files Changed
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/selectinputtext/SelectInputTextRenderer.java
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #23103 Thu Nov 11 17:04:24 MST 2010 mircea.toma ICE-5652 Send JS initalization code only on page load or component markup update instead of sending it all the time.
        Files Changed
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/selectinputtext/SelectInputTextRenderer.java
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #23104 Thu Nov 11 17:19:16 MST 2010 mircea.toma ICE-5652 Send JS code to controll the popup only when the visibility changes its state.
        Files Changed
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/panelpopup/PanelPopupRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/panelpopup/PanelPopup.java
        Hide
        Mircea Toma added a comment -

        Changed Auocomplete renderer so that the JS initalization code is sent only on page load or component markup update. The code is rendered in a <script> element along with the rest of the markup instead of using JavascriptContext API.

        Show
        Mircea Toma added a comment - Changed Auocomplete renderer so that the JS initalization code is sent only on page load or component markup update. The code is rendered in a <script> element along with the rest of the markup instead of using JavascriptContext API.
        Mircea Toma made changes -
        Status Open [ 1 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #23111 Fri Nov 12 18:11:30 MST 2010 mark.collette ICE-5652 : UpdateElements.coalesceWithPrevious overwrites JavaScript calls
        Files Changed
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/panelpopup/PanelPopup.java
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #23112 Fri Nov 12 18:24:44 MST 2010 mark.collette ICE-5652 : UpdateElements.coalesceWithPrevious overwrites JavaScript calls
        Files Changed
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/panelpopup/PanelPopup.java
        Hide
        Mark Collette added a comment -

        Tweaked the PanelPopup visibleBefore change to use state saving, so it would pass our SerializedViewTest automated test.

        icefaces trunk
        Subversion 23111

        icefaces2 trunk
        Subversion 23112

        Show
        Mark Collette added a comment - Tweaked the PanelPopup visibleBefore change to use state saving, so it would pass our SerializedViewTest automated test. icefaces trunk Subversion 23111 icefaces2 trunk Subversion 23112
        Hide
        Ken Fyten added a comment -

        It seems this change to panelPopup is causing a new regression failure with the none-functioning of the panelPopup autoCentre and autoPosition attributes:

        New failure: ICE-2368 all browsers (the position panel and autocenter popup Panels are not working)
        This problem is reproducible on component-showcase too

        Show
        Ken Fyten added a comment - It seems this change to panelPopup is causing a new regression failure with the none-functioning of the panelPopup autoCentre and autoPosition attributes: New failure: ICE-2368 all browsers (the position panel and autocenter popup Panels are not working) This problem is reproducible on component-showcase too
        Ken Fyten made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Assignee Priority P2 P1
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #23127 Tue Nov 16 10:59:49 MST 2010 mircea.toma ICE-5652 Render JS code in script element to trigger DOM diff when coordinates are changing. To avoid render interference with visibility state use processDecodes method for saving visibility state.
        Files Changed
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/panelpopup/PanelPopupRenderer.java
        Commit graph MODIFY /icefaces/trunk/icefaces/component/src/com/icesoft/faces/component/panelpopup/PanelPopup.java
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #23131 Tue Nov 16 12:04:28 MST 2010 mircea.toma ICE-5652 Render JS code in script element to trigger DOM diff when coordinates are changing. To avoid render interference with visibility state use processDecodes method for saving visibility state.
        Files Changed
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/panelpopup/PanelPopupRenderer.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/panelpopup/PanelPopup.java
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/core/src/main/javascript/extras/extras.js
        Hide
        Mircea Toma added a comment -

        Changed renderer to output JS code in the markup so that a DOM diff is triggered when popup coordinates changed.

        ICE-2368 test case is changing the visibility and rendering state simultaneously which confuses the logic used for track the changes in visibility state. To fix any future interference between render and visibility state processDecodes method is used for saving visibility state because the method is always called, not only when the component is rendered.

        Show
        Mircea Toma added a comment - Changed renderer to output JS code in the markup so that a DOM diff is triggered when popup coordinates changed. ICE-2368 test case is changing the visibility and rendering state simultaneously which confuses the logic used for track the changes in visibility state. To fix any future interference between render and visibility state processDecodes method is used for saving visibility state because the method is always called, not only when the component is rendered.
        Mircea Toma made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #23132 Tue Nov 16 12:09:37 MST 2010 mircea.toma ICE-5652 Make sure the calls to Ice.modal.stop function are valid since component might invoke the function multiple times.
        Files Changed
        Commit graph MODIFY /icefaces/trunk/icefaces/bridge/lib/extras/style.js
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #23133 Tue Nov 16 12:10:03 MST 2010 mircea.toma ICE-5652 Make sure the calls to Ice.modal.stop function are valid since component might invoke the function multiple times.
        Files Changed
        Commit graph MODIFY /icefaces/trunk/icefaces/bridge/lib/extras/style.js
        Hide
        Krashan Brahmanjara added a comment -

        BTW
        Last behavior of popup panel from Icefaces revision 23102 was unstable. Component showcase draggable panel not always is visible on button enabling. Just try to switch few times between modal and draggable.

        BTW2
        We still use Icefaces from revision 20815(2010.03.01). Similar problem with addJavascriptCall we resolved with much simplier patch. In PanelPopupRenderer at the end of method encodeBegin instead call

        JavascriptContext.addJavascriptCall(facesContext, "Ice.iFrameFix.start('" + clientId + "','" +
        CoreUtils.resolveResourceURL(facesContext, "/xmlhttp/blank") + "');");

        we use only
        if ( (modal == null || !modal.booleanValue() ) )

        { JavascriptContext.addJavascriptCall(facesContext, "Ice.iFrameFix.start('" + clientId + "','" + CoreUtils.resolveResourceURL(facesContext, "/xmlhttp/blank") + "');"); }
        Show
        Krashan Brahmanjara added a comment - BTW Last behavior of popup panel from Icefaces revision 23102 was unstable. Component showcase draggable panel not always is visible on button enabling. Just try to switch few times between modal and draggable. BTW2 We still use Icefaces from revision 20815(2010.03.01). Similar problem with addJavascriptCall we resolved with much simplier patch. In PanelPopupRenderer at the end of method encodeBegin instead call JavascriptContext.addJavascriptCall(facesContext, "Ice.iFrameFix.start('" + clientId + "','" + CoreUtils.resolveResourceURL(facesContext, "/xmlhttp/blank") + "');"); we use only if ( (modal == null || !modal.booleanValue() ) ) { JavascriptContext.addJavascriptCall(facesContext, "Ice.iFrameFix.start('" + clientId + "','" + CoreUtils.resolveResourceURL(facesContext, "/xmlhttp/blank") + "');"); }
        Hide
        Ken Fyten added a comment -

        Krashan, we are unable to reproduce any issues with draggable not always working. Please ensure your browser cache has been cleared and that you have the latest JS, etc.

        Show
        Ken Fyten added a comment - Krashan, we are unable to reproduce any issues with draggable not always working. Please ensure your browser cache has been cleared and that you have the latest JS, etc.
        Hide
        Ken Fyten added a comment -

        The popup panel is not working correctly on icefaces2/trunk compat component showcase as of svn rev# 23167.

        • Modal popup is not modal.
        • Draggable popup is not draggable.
        • Autocentre popup does not autocentre.

        Note that icefaces/trunk popup is working correctly.

        Show
        Ken Fyten added a comment - The popup panel is not working correctly on icefaces2/trunk compat component showcase as of svn rev# 23167. Modal popup is not modal. Draggable popup is not draggable. Autocentre popup does not autocentre. Note that icefaces/trunk popup is working correctly.
        Ken Fyten made changes -
        Resolution Fixed [ 1 ]
        Status Resolved [ 5 ] Reopened [ 4 ]
        Repository Revision Date User Message
        ICEsoft Public SVN Repository #23200 Wed Nov 24 16:16:54 MST 2010 mircea.toma ICE-5652 Render JS code as unescaped text so that browsers interpret the code correctly when updating elements that include scripts.
        Files Changed
        Commit graph MODIFY /icefaces2/trunk/icefaces/compat/components/src/main/java/com/icesoft/faces/component/panelpopup/PanelPopupRenderer.java
        Hide
        Mircea Toma added a comment -

        Changed PanelPopupRenderer in icefaces2.0/compat to render the JS code as unescaped text. This way browsers interpret the code correctly when updating the elements that include scripts.

        Show
        Mircea Toma added a comment - Changed PanelPopupRenderer in icefaces2.0/compat to render the JS code as unescaped text. This way browsers interpret the code correctly when updating the elements that include scripts.
        Mircea Toma made changes -
        Status Reopened [ 4 ] Resolved [ 5 ]
        Resolution Fixed [ 1 ]
        Ken Fyten made changes -
        Status Resolved [ 5 ] Closed [ 6 ]
        Assignee Priority P1
        Hide
        Ed Hillmann added a comment -

        Hi. I've applied 1.8.2 EE Patch 2 to our application, and I get the original error again: attempts to assign focus to a field using Server Push sporadically fail. When I reapply the changes to UpdateCommand as provided in the Patch, the behaviour works consistently (the focus is assigned to the correct field). It still appears that if commands are coalesced, earlier javascript commands are lost if the new command being coalesced contains its own Javascript value.

        I have not tested this with 2.0.

        Show
        Ed Hillmann added a comment - Hi. I've applied 1.8.2 EE Patch 2 to our application, and I get the original error again: attempts to assign focus to a field using Server Push sporadically fail. When I reapply the changes to UpdateCommand as provided in the Patch, the behaviour works consistently (the focus is assigned to the correct field). It still appears that if commands are coalesced, earlier javascript commands are lost if the new command being coalesced contains its own Javascript value. I have not tested this with 2.0.
        Hide
        Mircea Toma added a comment -

        I'll have a look.

        Show
        Mircea Toma added a comment - I'll have a look.

          People

          • Assignee:
            Mircea Toma
            Reporter:
            Ed Hillmann
          • Votes:
            0 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: