javaserverfaces-spec-public
  1. javaserverfaces-spec-public
  2. JAVASERVERFACES_SPEC_PUBLIC-790

javax.faces.ViewState + PPR does not work out for cross form ppr cases

    Details

    • Type: Bug Bug
    • Status: In Progress
    • Priority: Critical Critical
    • Resolution: Unresolved
    • Affects Version/s: 2.0
    • Fix Version/s: 2.3
    • Component/s: Ajax/JavaScript
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: Macintosh

    • Issuezilla Id:
      790
    • Status Whiteboard:
      Hide

      size_small importance_small

      Show
      size_small importance_small

      Description

      Following problem

      <h:form id="a">
          <h:commandButton action="#{TestBean.action}" value="submit"/>
      </h:form>
      
      <h:form id="b">
          <h:commandLink value="ajax ReRender" 
              onclick="jsf.ajax.request(this,event,{execute:'b a',render:'a'}); return false;"/>
      </h:form> 
      

      Cannot work out because, the viewstate is returned as separate viewstate block
      (in both implementations the <update id="a"> does not pass the viewState in the
      update block).

      Now the specification says:

      If an update element is found in the response with the identifier
      javax.faces.ViewState:

      <update id="javax.faces.ViewState">
         <![CDATA[...]]>
      </update>
      

      locate and update the submitting form's javax.faces.ViewState value with the
      CDATA contents from the response.

      Which means in this special case that the viewState for form a is lost.
      Mojarra has fixed this to some degree by setting the viewstate if a direct form
      render on a happens.

      However if you do following:

      <h:panelGroup id="myGroup">
          <h:form id="a">
              <h:commandButton action="#{TestBean.action}" value="submit"/>
          </h:form>
      </h:panelGroup>
      
      <h:form id="b">
          <h:commandLink value="ajax ReRender" 
              onclick="jsf.ajax.request(this,event,{execute:'b a',render:'myGroup'}); return false;"/>
      </h:form> 
      

      Mojarra also fails.

      The problem here lies clearly with the spec, I am not sure why the viewstate is
      only updated to the issuing form.

      Either all forms must be updated or at least the forms which are processed both
      within the execute and render parts.

      I also opened a discussion on the open mailing list regarding this, since this is
      a usecase which can happen quite often in a typical rich client scenario where a
      lot of detachments can happen to satisfy ie6 and multiple forms are the norm if
      you have floating frames.

      1. 790-js-workaround.txt
        2 kB
        mdirkse
      2. changebundle.txt
        7 kB
        frederickkaempfer
      3. changebundle.txt
        6 kB
        frederickkaempfer
      4. ExtendedViewHandler.java
        5 kB
        sharath.naik

        Issue Links

          Activity

          Hide
          Ed Burns added a comment -

          Agree for inclusion in 2.0 Rev a

          Show
          Ed Burns added a comment - Agree for inclusion in 2.0 Rev a
          Hide
          Ed Burns added a comment -

          take ownership.

          Show
          Ed Burns added a comment - take ownership.
          Hide
          Ed Burns added a comment -

          I agree we should update all forms. Move to 2.1.

          Show
          Ed Burns added a comment - I agree we should update all forms. Move to 2.1.
          Hide
          Ed Burns added a comment -

          triage

          Show
          Ed Burns added a comment - triage
          Hide
          Ed Burns added a comment -

          Change target milestone.

          Show
          Ed Burns added a comment - Change target milestone.
          Hide
          werpu12 added a comment -

          Actually I personally think the only possible solution here is to offload this
          to the server, instead of issuing <update id="javax.faces.ViewState"> we have to
          extend the protocol here so that the client is notified which form and viewstate
          element needs to be updated.
          I am not sure if updating all forms automatically really resolves the issue,
          because in a portlet scenario this does not work out, how about client side
          state saving or even worse if someone introduces multiple viewroots
          programmatically just as portlets do?

          Show
          werpu12 added a comment - Actually I personally think the only possible solution here is to offload this to the server, instead of issuing <update id="javax.faces.ViewState"> we have to extend the protocol here so that the client is notified which form and viewstate element needs to be updated. I am not sure if updating all forms automatically really resolves the issue, because in a portlet scenario this does not work out, how about client side state saving or even worse if someone introduces multiple viewroots programmatically just as portlets do?
          Hide
          rogerk added a comment -

          triage

          Show
          rogerk added a comment - triage
          Hide
          frederickkaempfer added a comment -

          The same problem occurs (in Mojarra 2.1.3) when the entire ViewRoot is replaced via ajax. In that case none of the forms get their view state updated. You need to submit a form at least twice in order to trigger the execution of the full JSF lifecycle. The first time only a new ViewState field is created in the submitted form.

          At least all forms contained in the rendered section need to have their view state field updated. In the current implementation a ViewState field is only created if the render target is it self a form (not a parent of forms).

          Updating all forms on the page is also a possibility though strictly speaking forms not contained in the <update/> section of the Ajax request are not part of the newly generated view state.

          Possible duplicates of this bug I found so far:
          http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1024
          http://java.net/jira/browse/JAVASERVERFACES-2199

          Show
          frederickkaempfer added a comment - The same problem occurs (in Mojarra 2.1.3) when the entire ViewRoot is replaced via ajax. In that case none of the forms get their view state updated. You need to submit a form at least twice in order to trigger the execution of the full JSF lifecycle. The first time only a new ViewState field is created in the submitted form. At least all forms contained in the rendered section need to have their view state field updated. In the current implementation a ViewState field is only created if the render target is it self a form (not a parent of forms). Updating all forms on the page is also a possibility though strictly speaking forms not contained in the <update/> section of the Ajax request are not part of the newly generated view state. Possible duplicates of this bug I found so far: http://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1024 http://java.net/jira/browse/JAVASERVERFACES-2199
          Hide
          frederickkaempfer added a comment -

          I have created a fix for the issue, which updates the ViewState field for forms which are children of the specified render targets.

          Furthermore it handles the case where render="@all" is used. In this case all forms in the document will have their ViewState field updated. Previously this caused forms to loose their ViewState.

          Note that this fix does not update all forms in the document, but only those which have been re-rendered (thus are part of the newly generated view on the server).

          Show
          frederickkaempfer added a comment - I have created a fix for the issue, which updates the ViewState field for forms which are children of the specified render targets. Furthermore it handles the case where render="@all" is used. In this case all forms in the document will have their ViewState field updated. Previously this caused forms to loose their ViewState. Note that this fix does not update all forms in the document, but only those which have been re-rendered (thus are part of the newly generated view on the server).
          Hide
          werpu12 added a comment -

          Actually I have similar fixes for myfaces, I basically update all forms which have render targets in or forms which are part of a render target.
          Additionally to that I added a update all config option which is outside of the spec.
          But nevertheless.

          But that is just a hack in my opinion. The root problem is in the protocol.
          The protocol simply needs to deliver a list of form ids which need a viewstate update.
          So I personally think the only viable long term solution to this problem simply is to update the protocol of jsf 2.2 in this regar ala <update id="javax.faces.ViewState" updateIds="id1 id2 id3">dkghsfkjgh</update> or for backwards compatibility reasons introduce an entirely new tag like <updateViewState id="...">....

          Show
          werpu12 added a comment - Actually I have similar fixes for myfaces, I basically update all forms which have render targets in or forms which are part of a render target. Additionally to that I added a update all config option which is outside of the spec. But nevertheless. But that is just a hack in my opinion. The root problem is in the protocol. The protocol simply needs to deliver a list of form ids which need a viewstate update. So I personally think the only viable long term solution to this problem simply is to update the protocol of jsf 2.2 in this regar ala <update id="javax.faces.ViewState" updateIds="id1 id2 id3">dkghsfkjgh</update> or for backwards compatibility reasons introduce an entirely new tag like <updateViewState id="...">....
          Hide
          rogerk added a comment -

          Your workarounds are fine for now, but clearly this needs to be a spec change for the better.

          Show
          rogerk added a comment - Your workarounds are fine for now, but clearly this needs to be a spec change for the better.
          Hide
          frederickkaempfer added a comment -

          If it was safe to assume that the ViewState does not change during a partial request the logic can be simplified by updating ALL forms in the document (not just render targets).

          In Mojarra's ServerSideStateHelper (line 200) this assumption holds true, but I did not find anything in the spec enforcing this.

          Additionally this would also take care of any Form dynamically added to the view or any render ids added via facesContext.getPartialViewContext().getRenderIds().add(...);

          Right now only the "javax.faces.partial.render" argument is consulted which is indeed suboptimal.

          I can change the workaround to simply update all forms in the page, which would also make it less of a "Hack". What do you think?

          Show
          frederickkaempfer added a comment - If it was safe to assume that the ViewState does not change during a partial request the logic can be simplified by updating ALL forms in the document (not just render targets). In Mojarra's ServerSideStateHelper (line 200) this assumption holds true, but I did not find anything in the spec enforcing this. Additionally this would also take care of any Form dynamically added to the view or any render ids added via facesContext.getPartialViewContext().getRenderIds().add(...); Right now only the "javax.faces.partial.render" argument is consulted which is indeed suboptimal. I can change the workaround to simply update all forms in the page, which would also make it less of a "Hack". What do you think?
          Hide
          rogerk added a comment -

          I was thinking along those same lines..
          In which case we would not need to introduce a new response element as Werner mentioned.
          It's been sometime since we looked at this - it has been discussed before.
          Are there any portlet implications (namespace related) by doing this (that was brought up - but could be for a different issue)..

          Show
          rogerk added a comment - I was thinking along those same lines.. In which case we would not need to introduce a new response element as Werner mentioned. It's been sometime since we looked at this - it has been discussed before. Are there any portlet implications (namespace related) by doing this (that was brought up - but could be for a different issue)..
          Hide
          werpu12 added a comment -

          Actually there are portlet implications, hence i stayed away from updating all forms automatically, but made it a config option you can turn on in myfaces.

          The implication is that different portlets can host different forms under different viewroots and hence have different viewstates.
          Now the main problem is, we dont have any viewroot information. So it comes down to following:
          a) either send the viewstate info with an altered protocol i mentioned

          • safe since every form must have a unique id in the dom no double ids are allowed

          b) add viewroot information and then update all forms under the corresponding viewroot - should be safe as well but is in my opinion not as clean as option a) and probably causes an alteration of the protocol as well (ala introduction of a viewroot element) or on the api side. I am not sure if we have a scenario where we have different viewstates under one viewroot though. I never really bothered to look deeper into this aspect of the jsf spec.

          c) Try to move forward with the usual hacks - worst option of all

          In my personal experience this multi form issue has been a huge issue for the users, I had about 5-10 requests on the myfaces mailinglist where users had problems and thought it was an issue of the implementation.

          Show
          werpu12 added a comment - Actually there are portlet implications, hence i stayed away from updating all forms automatically, but made it a config option you can turn on in myfaces. The implication is that different portlets can host different forms under different viewroots and hence have different viewstates. Now the main problem is, we dont have any viewroot information. So it comes down to following: a) either send the viewstate info with an altered protocol i mentioned safe since every form must have a unique id in the dom no double ids are allowed b) add viewroot information and then update all forms under the corresponding viewroot - should be safe as well but is in my opinion not as clean as option a) and probably causes an alteration of the protocol as well (ala introduction of a viewroot element) or on the api side. I am not sure if we have a scenario where we have different viewstates under one viewroot though. I never really bothered to look deeper into this aspect of the jsf spec. c) Try to move forward with the usual hacks - worst option of all In my personal experience this multi form issue has been a huge issue for the users, I had about 5-10 requests on the myfaces mailinglist where users had problems and thought it was an issue of the implementation.
          Hide
          frederickkaempfer added a comment -

          It's true that updating all forms won't work in any case where there are multiple viewroots involved. Anyhow I was hoping to find a solution that is usable even with the 2.1 and 2.0 spec versions, because this bug is not an enhancement, but currently breaks a couple of basic Ajax features in JSF. To summarize them quickly:

          1. Specifying parents of forms as render targets.
          2. Using the @all render target on any page with multiple forms.
          3. Replacing the entire viewroot, for example via <h:commandLink action="myNavigationOutcome"><f:ajax/></h:commandLink>
          4. Dynamically adding RenderIds that include forms in JSF Lifecycle with facesContext.getPartialViewContext().getRenderIds().add(...)

          The workaround I provided should at least make 1-3 work again. Updating all forms in the document would make 1-4 work, but as you said will break Portlet compatibility.

          There is another option to consider that does not require a protocol change:

          Before the doUpdate function is called for any of the changes read the view state information at the end of the partial response document and save it in a variable in the jsf.ajax.response function. the viewstate can then be passed to any call of doUpdate function, which can then take care of creating view state fields where it is appropriate. This way we would not have to redundantly send the render ids, as they already provided as the <update> ids.

          Show
          frederickkaempfer added a comment - It's true that updating all forms won't work in any case where there are multiple viewroots involved. Anyhow I was hoping to find a solution that is usable even with the 2.1 and 2.0 spec versions, because this bug is not an enhancement, but currently breaks a couple of basic Ajax features in JSF. To summarize them quickly: 1. Specifying parents of forms as render targets. 2. Using the @all render target on any page with multiple forms. 3. Replacing the entire viewroot, for example via <h:commandLink action="myNavigationOutcome"><f:ajax/></h:commandLink> 4. Dynamically adding RenderIds that include forms in JSF Lifecycle with facesContext.getPartialViewContext().getRenderIds().add(...) The workaround I provided should at least make 1-3 work again. Updating all forms in the document would make 1-4 work, but as you said will break Portlet compatibility. There is another option to consider that does not require a protocol change: Before the doUpdate function is called for any of the changes read the view state information at the end of the partial response document and save it in a variable in the jsf.ajax.response function. the viewstate can then be passed to any call of doUpdate function, which can then take care of creating view state fields where it is appropriate. This way we would not have to redundantly send the render ids, as they already provided as the <update> ids.
          Hide
          frederickkaempfer added a comment -

          Here is a changebundle that updates the ViewState for each form element that is replaced during the ajax update.

          I wonder if a Spec change is even necessary because: If a form is processed during an update it means that its ViewState field has to be updated (or created) as well. With this patch it is now done in the doUpdate function. On the other hand, if a form is not re-rendered there is no compelling reason to update its ViewState field, because if it had been changed on the server-side this would not be visible in the browser.

          So following this logic, specifying the "updateIds" in a new protocol element will always replicate the render ids which are already contained in the <update> tags (or any of the forms contained in them, which can also be found using JavaScript, like it is done in the provided patch).

          Do you think this is still too "hack"-ish?

          Show
          frederickkaempfer added a comment - Here is a changebundle that updates the ViewState for each form element that is replaced during the ajax update. I wonder if a Spec change is even necessary because: If a form is processed during an update it means that its ViewState field has to be updated (or created) as well. With this patch it is now done in the doUpdate function. On the other hand, if a form is not re-rendered there is no compelling reason to update its ViewState field, because if it had been changed on the server-side this would not be visible in the browser. So following this logic, specifying the "updateIds" in a new protocol element will always replicate the render ids which are already contained in the <update> tags (or any of the forms contained in them, which can also be found using JavaScript, like it is done in the provided patch). Do you think this is still too "hack"-ish?
          Hide
          werpu12 added a comment -

          Unfortunately things are not that simple. I cannot speak for mojarra, but I assume it is similar, but myfaces has a meta information in the viewstate which also pinpoints to the viewid (and the state history as well) so if you do not update a second form even if you dont have a rerender there, you basically at a submit from that form can get a cannot find viewid once it drops out of the view history.

          So we have two problems
          a) update all forms automatically is prevented due to the fact that it breaks in a portlet environment

          b) not updating not rendered forms might break the state history information of the viewstate not updated

          So a pure javascript fixup will work and might work under normal non portlet circumstances (the simplest one probably is simply to update all forms for a normal webapp) but it will break in corner cases like portlet environments. Thats one of the reasons why I implemted about three fallback modes for this problem in myfaces. So that if even one fallback fails the user can switch to another one which might suite to his environment.

          Show
          werpu12 added a comment - Unfortunately things are not that simple. I cannot speak for mojarra, but I assume it is similar, but myfaces has a meta information in the viewstate which also pinpoints to the viewid (and the state history as well) so if you do not update a second form even if you dont have a rerender there, you basically at a submit from that form can get a cannot find viewid once it drops out of the view history. So we have two problems a) update all forms automatically is prevented due to the fact that it breaks in a portlet environment b) not updating not rendered forms might break the state history information of the viewstate not updated So a pure javascript fixup will work and might work under normal non portlet circumstances (the simplest one probably is simply to update all forms for a normal webapp) but it will break in corner cases like portlet environments. Thats one of the reasons why I implemted about three fallback modes for this problem in myfaces. So that if even one fallback fails the user can switch to another one which might suite to his environment.
          Hide
          frederickkaempfer added a comment -

          I think Mojarra does not even change the ViewState during partial requests.

          Nevertheless I suppose there are two separate issues surfacing here that are remotely related. This patch should work in a Portlet scenario as well as the unpatched version and it fixes the 4 issues I mentioned earlier including the initial bug report. Contrary to updating all forms in the document, it doesn't make it any worse for the portlet scenario. But yes, it is a preliminary hack, if you consider the following change as the right way to handle the situation:

          In your last comment you basically said that on each Ajax request all forms contained in a one ViewRoot have to be updated with new state information - at least in the case of MyFaces (it would work for Mojarra as well because the ViewState doesn't change):

          • If we are in a "normal" (non-portlet) environment with just one ViewRoot, this means updating all forms contained in the entire document.
          • If we are in a portlet environment it would mean updating all forms that are contained in the current ViewRoot.

          So wouldn't the best course of action be your option b): make it possible to specify the view root element's id additionally in the partial response. If it is not specified or is set to a default value, update the entire document, if not only update forms contained in the viewroot element. This would also add the possibility to rerender a complete portlet with the special render target @all, which represents entire ViewRoot element. Sadly I don't know enough about the portlet spec to tell if this is a good idea and if this change is enough to support it properly.

          Providing a list of update ids would then again be redundant, because the forms that need updating depend only on the ViewRoot that is currently - partly or in whole - redisplayed.

          If this sounds like the right way to do it, then applying the changebundle will be counterproductive, because it is unnecessarily complex compared to updating all forms in the current ViewRoot. On the other hand it could even be backported to 2.0 or 2.1, it does not make things worse for portlets and it fixes the 4 critical bugs/problems i mentioned.

          Show
          frederickkaempfer added a comment - I think Mojarra does not even change the ViewState during partial requests. Nevertheless I suppose there are two separate issues surfacing here that are remotely related. This patch should work in a Portlet scenario as well as the unpatched version and it fixes the 4 issues I mentioned earlier including the initial bug report. Contrary to updating all forms in the document, it doesn't make it any worse for the portlet scenario. But yes, it is a preliminary hack, if you consider the following change as the right way to handle the situation: In your last comment you basically said that on each Ajax request all forms contained in a one ViewRoot have to be updated with new state information - at least in the case of MyFaces (it would work for Mojarra as well because the ViewState doesn't change): If we are in a "normal" (non-portlet) environment with just one ViewRoot, this means updating all forms contained in the entire document. If we are in a portlet environment it would mean updating all forms that are contained in the current ViewRoot. So wouldn't the best course of action be your option b): make it possible to specify the view root element's id additionally in the partial response. If it is not specified or is set to a default value, update the entire document, if not only update forms contained in the viewroot element. This would also add the possibility to rerender a complete portlet with the special render target @all, which represents entire ViewRoot element. Sadly I don't know enough about the portlet spec to tell if this is a good idea and if this change is enough to support it properly. Providing a list of update ids would then again be redundant, because the forms that need updating depend only on the ViewRoot that is currently - partly or in whole - redisplayed. If this sounds like the right way to do it, then applying the changebundle will be counterproductive, because it is unnecessarily complex compared to updating all forms in the current ViewRoot. On the other hand it could even be backported to 2.0 or 2.1, it does not make things worse for portlets and it fixes the 4 critical bugs/problems i mentioned.
          Hide
          Ed Burns added a comment -

          Thank you for excellent and clear analysis. Given my desire to include sketches for JAVASERVERFACES_SPEC_PUBLIC-730, JAVASERVERFACES_SPEC_PUBLIC-517, and JAVASERVERFACES_SPEC_PUBLIC-802, in next week's EDR of the spec, I'm going to defer this til after JavaOne.

          Show
          Ed Burns added a comment - Thank you for excellent and clear analysis. Given my desire to include sketches for JAVASERVERFACES_SPEC_PUBLIC-730 , JAVASERVERFACES_SPEC_PUBLIC-517 , and JAVASERVERFACES_SPEC_PUBLIC-802 , in next week's EDR of the spec, I'm going to defer this til after JavaOne.
          Hide
          codylerum added a comment -

          This also is an issue with popup panels which often have their own form that when submitted rerenders content on the main page.

          Since any forms on the main page did not participate in the submit they don't rerender with a viewstate and are unusable.

          Show
          codylerum added a comment - This also is an issue with popup panels which often have their own form that when submitted rerenders content on the main page. Since any forms on the main page did not participate in the submit they don't rerender with a viewstate and are unusable.
          Hide
          mdirkse added a comment -

          Added a javascript workaround that fixes this issue and can be executed via the browser onLoad event handler. There are 2 versions: a plain JS one, and a jQuerified one. Tested and confirmed for Mojarra 2.1.2 (and jQuery 1.7.x).
          Mad propz to frederickkaempfer for the original JS code.

          Show
          mdirkse added a comment - Added a javascript workaround that fixes this issue and can be executed via the browser onLoad event handler. There are 2 versions: a plain JS one, and a jQuerified one. Tested and confirmed for Mojarra 2.1.2 (and jQuery 1.7.x). Mad propz to frederickkaempfer for the original JS code.
          Hide
          werpu12 added a comment - - edited

          Sorry to crush some hopes here, but the posted solution is exactly the one I have in myfaces for the non portlet case (you can turn it on via a config param). And there it works, but it can only work in a non portlet environment, because there you have only one viewroot. So you can update all forms under document.

          In a portlet environment you can have several viewroots under document with independend viewstates and independend forms. There the patch ultimately will break your portlet environment by applying wrong viewroots to other portlets. Thats because the which dom node is the viewroot info is simply missing on the client side and/or the protocol.

          As I said before unless you have the viewroot info or you know which forms you update there is no way to resolve that issue for the portlet and non portlet case at the same time. The problem really is a problem of the protocol not the implementation.

          Show
          werpu12 added a comment - - edited Sorry to crush some hopes here, but the posted solution is exactly the one I have in myfaces for the non portlet case (you can turn it on via a config param). And there it works, but it can only work in a non portlet environment, because there you have only one viewroot. So you can update all forms under document. In a portlet environment you can have several viewroots under document with independend viewstates and independend forms. There the patch ultimately will break your portlet environment by applying wrong viewroots to other portlets. Thats because the which dom node is the viewroot info is simply missing on the client side and/or the protocol. As I said before unless you have the viewroot info or you know which forms you update there is no way to resolve that issue for the portlet and non portlet case at the same time. The problem really is a problem of the protocol not the implementation.
          Hide
          rogerk added a comment -

          I agree with Wevner's earlier assessment - that to handle the portlet case - multiple independent view root (eaxh with one or more forms) - we do need a new "server to client" message protocol that draws the association of which forms the view state should apply.

          Show
          rogerk added a comment - I agree with Wevner's earlier assessment - that to handle the portlet case - multiple independent view root (eaxh with one or more forms) - we do need a new "server to client" message protocol that draws the association of which forms the view state should apply.
          Hide
          mdirkse added a comment -

          werpu12 is right, the workaround I posted only works for the non-portlet case (and is only verified for mojarra 2.1.2)

          Show
          mdirkse added a comment - werpu12 is right, the workaround I posted only works for the non-portlet case (and is only verified for mojarra 2.1.2)
          Hide
          sharath.naik added a comment -

          Updating the MultiViewHandler's [ public void writeState(FacesContext context) throws IOException ] seems to fix the issue. The change made is to add the view state hidden field for forms, even when is a ajax request.

          What is the purpose of not writing the view state if it is a partial request in this method?

          Attaching the custom ViewHandler to override this method. would this be a fix that can be considered to be moved to MultiViewHandler?

          Show
          sharath.naik added a comment - Updating the MultiViewHandler's [ public void writeState(FacesContext context) throws IOException ] seems to fix the issue. The change made is to add the view state hidden field for forms, even when is a ajax request. What is the purpose of not writing the view state if it is a partial request in this method? Attaching the custom ViewHandler to override this method. would this be a fix that can be considered to be moved to MultiViewHandler?
          Hide
          frederickkaempfer added a comment - - edited

          @ sharath.naik: It's probably left out because the jsf ajax response already includes the view state information in an <update> tag, so the field can be created using JavaScript.

          As I said earlier, the minimum a JSF implementation should do is update the view state information for all forms included in the render target list. I don't see how this creates any new problem in the portlet environment and that is what the latest changebundle I submitted does. Currently the algorithm detailed in the spec is not general enough and should be corrected.

          A broader approach is to update all forms under the current view root. For that a protocol extension is necessary which tells the client which DOM element represents the view root. It would also enable scenarios where the whole view root is replaced (like @all and navigation) for the portlet environment. In my opinion for this a new spec enhancement should be issued.

          Show
          frederickkaempfer added a comment - - edited @ sharath.naik: It's probably left out because the jsf ajax response already includes the view state information in an <update> tag, so the field can be created using JavaScript. As I said earlier, the minimum a JSF implementation should do is update the view state information for all forms included in the render target list. I don't see how this creates any new problem in the portlet environment and that is what the latest changebundle I submitted does. Currently the algorithm detailed in the spec is not general enough and should be corrected. A broader approach is to update all forms under the current view root. For that a protocol extension is necessary which tells the client which DOM element represents the view root. It would also enable scenarios where the whole view root is replaced (like @all and navigation) for the portlet environment. In my opinion for this a new spec enhancement should be issued.
          Hide
          codylerum added a comment -

          Is the attached workaround meant to be called from the oncomplete of an ajax component or am I missing another way to trigger it?

          Show
          codylerum added a comment - Is the attached workaround meant to be called from the oncomplete of an ajax component or am I missing another way to trigger it?
          Hide
          frederickkaempfer added a comment -

          @codylerum: it should be enough to execute the script once on a page load, after jsf.js is loaded of course.

          Show
          frederickkaempfer added a comment - @codylerum: it should be enough to execute the script once on a page load, after jsf.js is loaded of course.
          Hide
          Ed Burns added a comment -

          TG> (First of all, javax.faces.ViewState should not be modified for Ajax
          TG> updates using server-side state-saving, but this simple approach
          TG> does not appear to be the current direction.)

          Can you please elaborate more on what you mean by this? The issue
          doesn't mention anything about Ajax specifically. Is this a separate
          concern you are raising, Ted?

          TG> We are a bit unclear on how the current implementation is intended
          TG> to work, but expect that something like the following is necessary:

          TG> When the request is submitted, call
          TG> getElementsByName('javax.faces.ViewState') and store the list of IDs
          TG> for all with equal value to that of the javax.faces.ViewState in the
          TG> submitting form. In the case of future JSP inclusion or current
          TG> Portlets, there will be javax.faces.ViewState fields not associated
          TG> with the current view. They will have different values and are not
          TG> included in the list.

          TG> When the partial response is generated, it may contain rendered
          TG> hidden inputs with name javax.faces.ViewState and unique IDs, but
          TG> also contains the special case <update
          TG> id="javax.faces.ViewState"><![CDATA[...]]></update>. The previously
          TG> stored list of IDs are now all updated to use the new value. Any
          TG> javax.faces.ViewState hidden fields that have been modified by the
          TG> page update itself (potentially now having different IDs) have the
          TG> correct value anyway because they were just updated.

          These two comments are better suited to JAVASERVERFACES_SPEC_PUBLIC-790,
          so I'll copy them there. Ted, can you please take a look at 790 and see
          if you agree that your comments pertain more to that issue than this
          one?

          Show
          Ed Burns added a comment - TG> (First of all, javax.faces.ViewState should not be modified for Ajax TG> updates using server-side state-saving, but this simple approach TG> does not appear to be the current direction.) Can you please elaborate more on what you mean by this? The issue doesn't mention anything about Ajax specifically. Is this a separate concern you are raising, Ted? TG> We are a bit unclear on how the current implementation is intended TG> to work, but expect that something like the following is necessary: TG> When the request is submitted, call TG> getElementsByName('javax.faces.ViewState') and store the list of IDs TG> for all with equal value to that of the javax.faces.ViewState in the TG> submitting form. In the case of future JSP inclusion or current TG> Portlets, there will be javax.faces.ViewState fields not associated TG> with the current view. They will have different values and are not TG> included in the list. TG> When the partial response is generated, it may contain rendered TG> hidden inputs with name javax.faces.ViewState and unique IDs, but TG> also contains the special case <update TG> id="javax.faces.ViewState"><![CDATA [...] ]></update>. The previously TG> stored list of IDs are now all updated to use the new value. Any TG> javax.faces.ViewState hidden fields that have been modified by the TG> page update itself (potentially now having different IDs) have the TG> correct value anyway because they were just updated. These two comments are better suited to JAVASERVERFACES_SPEC_PUBLIC-790 , so I'll copy them there. Ted, can you please take a look at 790 and see if you agree that your comments pertain more to that issue than this one?
          Hide
          tedgoddard added a comment -

          At least in the case of MyFaces, this complicates things considerably:

          "Unfortunately things are not that simple. I cannot speak for mojarra, but I assume it is similar, but myfaces has a meta information in the viewstate which also pinpoints to the viewid (and the state history as well) so if you do not update a second form even if you dont have a rerender there, you basically at a submit from that form can get a cannot find viewid once it drops out of the view history."

          It makes it necessary to update the ViewState key for every request. I was under the impression that a similar strategy was being considered for Mojarra.

          In my above comments, I assume that the form Renderer would be modified to always write out the ViewState (even for partial rendering). Alternatively, the javax.faces.ViewState fields could be updated via two cases:

          • javax.faces.ViewState found in the current HTML document that match the submitting javax.faces.ViewState will be updated after the current response
          • javax.faces.ViewState found in the updated regions will be updated after the current response

          This should handle portlet, servlet inclusion, and multiple form cases.

          Show
          tedgoddard added a comment - At least in the case of MyFaces, this complicates things considerably: "Unfortunately things are not that simple. I cannot speak for mojarra, but I assume it is similar, but myfaces has a meta information in the viewstate which also pinpoints to the viewid (and the state history as well) so if you do not update a second form even if you dont have a rerender there, you basically at a submit from that form can get a cannot find viewid once it drops out of the view history." It makes it necessary to update the ViewState key for every request. I was under the impression that a similar strategy was being considered for Mojarra. In my above comments, I assume that the form Renderer would be modified to always write out the ViewState (even for partial rendering). Alternatively, the javax.faces.ViewState fields could be updated via two cases: javax.faces.ViewState found in the current HTML document that match the submitting javax.faces.ViewState will be updated after the current response javax.faces.ViewState found in the updated regions will be updated after the current response This should handle portlet, servlet inclusion, and multiple form cases.
          Hide
          boogi added a comment -

          Hi,
          It says on fix version "2.2 Sprint 12" but it appears as unresolved on that sprint.
          Do you a new estimation for the solution of this issue?

          Show
          boogi added a comment - Hi, It says on fix version "2.2 Sprint 12" but it appears as unresolved on that sprint. Do you a new estimation for the solution of this issue?
          Hide
          javaone9 added a comment -

          I tried 2.1.14. it did not work.
          I think this issue is urgent for any applications.

          Show
          javaone9 added a comment - I tried 2.1.14. it did not work. I think this issue is urgent for any applications.
          Hide
          frederickkaempfer added a comment - - edited

          Please don't forget this issue for 2.2. It's one of the most annoying and frustrating aspects of the JSF Ajax experience. It would be a shame if the JSF community would have to wait another spec release -possibly another year- before this gets fixed.

          tedgoddard made it very clear how to proceed with this issue in the previous comment.

          Thanks a lot.

          Show
          frederickkaempfer added a comment - - edited Please don't forget this issue for 2.2. It's one of the most annoying and frustrating aspects of the JSF Ajax experience. It would be a shame if the JSF community would have to wait another spec release -possibly another year- before this gets fixed. tedgoddard made it very clear how to proceed with this issue in the previous comment. Thanks a lot.
          Hide
          kfyten added a comment -

          Roger/Ed - Just wanted to make it clear that this issue is currently blocking ICEfaces support for JSF 2.2. From a roadmap perspective it would be very helpful to us if this JIRA could be updated to reflect the current reality in terms of whether/when this issue may be resolved prior to 2.2 final release.

          Thx.

          Show
          kfyten added a comment - Roger/Ed - Just wanted to make it clear that this issue is currently blocking ICEfaces support for JSF 2.2. From a roadmap perspective it would be very helpful to us if this JIRA could be updated to reflect the current reality in terms of whether/when this issue may be resolved prior to 2.2 final release. Thx.
          Hide
          rogerk added a comment -

          Unfortunately I don't believe this made it into 2.2.

          Show
          rogerk added a comment - Unfortunately I don't believe this made it into 2.2.
          Hide
          balusc added a comment -

          This is really unfortunate.

          In the meanwhile, developers can use this script to workaround the issue: http://balusc.blogspot.com/2011/09/communication-in-jsf-20.html#AutomaticallyFixMissingJSFViewStateAfterAjaxRendering

          Show
          balusc added a comment - This is really unfortunate. In the meanwhile, developers can use this script to workaround the issue: http://balusc.blogspot.com/2011/09/communication-in-jsf-20.html#AutomaticallyFixMissingJSFViewStateAfterAjaxRendering
          Hide
          frederickkaempfer added a comment -

          @balusc: I think that the workaround scripts have to be changed for 2.2, because the id of the ViewState field has been changed slightly. I didn't yet have time to look into it.

          Show
          frederickkaempfer added a comment - @balusc: I think that the workaround scripts have to be changed for 2.2, because the id of the ViewState field has been changed slightly. I didn't yet have time to look into it.
          Hide
          arjan tijms added a comment -

          I think that the workaround scripts have to be changed for 2.2, because the id of the ViewState field has been changed slightly. I didn't yet have time to look into it.

          They have changed indeed, see http://jdevelopment.nl/jsf-22/#220 and JAVASERVERFACES_SPEC_PUBLIC-220 for more details about this.

          Show
          arjan tijms added a comment - I think that the workaround scripts have to be changed for 2.2, because the id of the ViewState field has been changed slightly. I didn't yet have time to look into it. They have changed indeed, see http://jdevelopment.nl/jsf-22/#220 and JAVASERVERFACES_SPEC_PUBLIC-220 for more details about this.
          Hide
          codylerum added a comment -

          This is one of the larger pain points for developers utilizing ajax so it is very unfortunate that we are likely looking at years for resolution. Can anything be done at this point to speed that up?

          Currently this issue has almost 2x the votes of the next closest issue.

          Show
          codylerum added a comment - This is one of the larger pain points for developers utilizing ajax so it is very unfortunate that we are likely looking at years for resolution. Can anything be done at this point to speed that up? Currently this issue has almost 2x the votes of the next closest issue.
          Hide
          swathireddy12 added a comment -

          I am making use of JSF 1.2 version of jars (myfaces-api-1.2.9.jar ,myfaces-impl-1.2.9.jar,trinidad-api-1.2.13.jar,trinidad-impl-1.2.13.jar).
          I am trying to retrieve the javax.faces.ViewState using the id attribute in a Javascript which works.

          But i still don't see the id attribute in the loaded page
          <input type="hidden" name="javax.faces.ViewState" value="!-14uywjgjai">

          Could you please tell me if this is an issue with JSF 1.2 version as well?
          Or once the page is rendered the id attribute associated with "javax.faces.ViewState" is not seen anymore.

          Show
          swathireddy12 added a comment - I am making use of JSF 1.2 version of jars (myfaces-api-1.2.9.jar ,myfaces-impl-1.2.9.jar,trinidad-api-1.2.13.jar,trinidad-impl-1.2.13.jar). I am trying to retrieve the javax.faces.ViewState using the id attribute in a Javascript which works. But i still don't see the id attribute in the loaded page <input type="hidden" name="javax.faces.ViewState" value="!-14uywjgjai"> Could you please tell me if this is an issue with JSF 1.2 version as well? Or once the page is rendered the id attribute associated with "javax.faces.ViewState" is not seen anymore.
          Hide
          werpu added a comment -

          No this is a JSF 2.x issue only. This has nothing to do with JSF 1.2.

          Show
          werpu added a comment - No this is a JSF 2.x issue only. This has nothing to do with JSF 1.2.
          Hide
          balusc added a comment -

          This is one of the larger pain points for developers utilizing ajax so it is very unfortunate that we are likely looking at years for resolution. Can anything be done at this point to speed that up?

          For exactly this reason, it has been added to OmniFaces 1.7: http://showcase.omnifaces.org/scripts/FixViewState

          Show
          balusc added a comment - This is one of the larger pain points for developers utilizing ajax so it is very unfortunate that we are likely looking at years for resolution. Can anything be done at this point to speed that up? For exactly this reason, it has been added to OmniFaces 1.7: http://showcase.omnifaces.org/scripts/FixViewState
          Hide
          Ed Burns added a comment -

          Set priority to baseline ahead of JSF 2.3 triage. Priorities will be assigned accurately after this exercise.

          Show
          Ed Burns added a comment - Set priority to baseline ahead of JSF 2.3 triage. Priorities will be assigned accurately after this exercise.
          Hide
          donvip added a comment -

          Is this issue going to be fixed in JSF 2.3?

          Show
          donvip added a comment - Is this issue going to be fixed in JSF 2.3?
          Hide
          arjan tijms added a comment -

          Is this issue going to be fixed in JSF 2.3?

          I think we should definitely address this for 2.3. Thanks for the reminder!

          Show
          arjan tijms added a comment - Is this issue going to be fixed in JSF 2.3? I think we should definitely address this for 2.3. Thanks for the reminder!
          Hide
          balusc added a comment - - edited

          I'm currently looking at this.

          As far as I see there are several solutions being proposed:

          1. Let JS blindly update/add view state of every single form in document - bad, might not work in portlet and might add noise to GET forms and/or non-JSF forms.
          2. Let JS update only missing view state of every single POST form in document - unclear if this works in portlets too.
          3. Let JS update only view states of all POST forms covered in ajax update targets - unclear if this works in portlets too.
          4. Always write view state to response - requires major spec change in ViewHandler#writeState(). I'd rather not do that.
          5. Include client IDs of all POST forms covered in ajax update targets in meta information of partial response.

          I tend to implementing #3. Who can confirm if that will indeed work for portlets? Otherwise we'd better go for #5.

          OmniFaces FixViewState by the way follows #2.

          Show
          balusc added a comment - - edited I'm currently looking at this. As far as I see there are several solutions being proposed: Let JS blindly update/add view state of every single form in document - bad, might not work in portlet and might add noise to GET forms and/or non-JSF forms. Let JS update only missing view state of every single POST form in document - unclear if this works in portlets too. Let JS update only view states of all POST forms covered in ajax update targets - unclear if this works in portlets too. Always write view state to response - requires major spec change in ViewHandler#writeState(). I'd rather not do that. Include client IDs of all POST forms covered in ajax update targets in meta information of partial response. I tend to implementing #3. Who can confirm if that will indeed work for portlets? Otherwise we'd better go for #5. OmniFaces FixViewState by the way follows #2.
          Hide
          werpu added a comment -

          The way I see it the ultimate fix on this issue, is a protocol change for the ajax response, we simply need the meta information there which viewstates in which forms need to be updated.
          Everything else is a hack.

          Show
          werpu added a comment - The way I see it the ultimate fix on this issue, is a protocol change for the ajax response, we simply need the meta information there which viewstates in which forms need to be updated. Everything else is a hack.
          Hide
          balusc added a comment -

          Completely agree that. I updated the previous comment.

          Show
          balusc added a comment - Completely agree that. I updated the previous comment.
          Hide
          balusc added a comment -

          Still no feedback from portlet guys.

          I checked how MyFaces did it. They basically follows #2 with two little differences: it (incorrectly) updates GET forms too, and it skips everything when running in portlet environment.

          Show
          balusc added a comment - Still no feedback from portlet guys. I checked how MyFaces did it. They basically follows #2 with two little differences: it (incorrectly) updates GET forms too, and it skips everything when running in portlet environment.
          Hide
          werpu added a comment -

          For multiform scenarios in no portlet environemnts I also added a config param which basically allows to override the default behavior and updates all forms reachable. This solved the multiform problem for many users since portlets are rather seldomly used.

          Show
          werpu added a comment - For multiform scenarios in no portlet environemnts I also added a config param which basically allows to override the default behavior and updates all forms reachable. This solved the multiform problem for many users since portlets are rather seldomly used.
          Hide
          werpu added a comment -

          #2 simply turned out to be incorrect in multiform non portlet scenarios because the viewstate is kept over more than one form in the standard cases.
          but also updating all forms is a no go for portlets, hence the config hack on my side.

          Show
          werpu added a comment - #2 simply turned out to be incorrect in multiform non portlet scenarios because the viewstate is kept over more than one form in the standard cases. but also updating all forms is a no go for portlets, hence the config hack on my side.
          Hide
          balusc added a comment - - edited

          It's still not clear to me if #3 works for portlets. If it does, then #5 could be specified and implemented following same logic (i.e. update only JSF forms which are covered in ajax update targets).

          Show
          balusc added a comment - - edited It's still not clear to me if #3 works for portlets. If it does, then #5 could be specified and implemented following same logic (i.e. update only JSF forms which are covered in ajax update targets).
          Hide
          donvip added a comment -

          @balusc: thanks for the updates. The status on MyFaces is interesting, I didn't know they had implemented a workaround. Can you please tell me where I can find it? Is there an existing bug report/pull request concerning their incorrect implementation?

          Show
          donvip added a comment - @balusc: thanks for the updates. The status on MyFaces is interesting, I didn't know they had implemented a workaround. Can you please tell me where I can find it? Is there an existing bug report/pull request concerning their incorrect implementation?
          Hide
          Neil Griffin added a comment -

          @balusc, @werpu: Thank you for considering the portlet use-case. I think we can make #3 work in a portlet environment, but need to make sure that the javax.faces.ViewState hidden field values are only updated for forms that are associated with the portlet that invoked the XHR. (In other words, there could be other JSF portlets that are part of the HTML document that should not be touched by jsf.js, even though they share the same jsf.js resource).

          In a portlet environment the UIViewRoot implements NamingContainer, thanks to javax.portlet.faces.component.PortletNamingContainerUIViewRoot

          Because of this, the "id" attribute of forms is namespaced with the portlet id. For example, given this XHTML:

          <h:form id="a">...</h:form>
          <h:form id="b">...</h:form>
          

          The following HTML would be rendered to the response:

          <form id="Pluto_test_portlet_1__24955534_0_:a">...</form>
          <form id="Pluto_test_portlet_1__24955534_0_:b">...</form>
          

          If you consider the following XHR response:

          <partial-response id="Pluto_test_portlet_1__24955534_0_">
            <changes>
              <update id="Pluto_test_portlet_1__24955534_0_:javax.faces.ViewState:0">
                <![CDATA[4617973942428228781:-2797886012162436717]]>
              </update>
            </changes>
          </partial-response>
          

          Then jsf.js could use the Pluto_test_portlet_1_24955534_0 namespace to make sure it only updates javax.faces.ViewState hidden fields in forms that contain that namespace in their "id" attribute.

          As a side note, the portlet namespace is known to Mojarra as "namingContainerId" when it builds up mojarra.ab JavasScript. For more information, see AjaxBehaviorRenderer.java and jsf.js. Mojarra uses it to namespace XHR parameters such as javax.faces.partial.ajax in a portlet environment that requires strict parameter namespacing. The XHR parameter namespacing feature is unique to Mojarra – it has not yet been contributed to MyFaces.

          Show
          Neil Griffin added a comment - @balusc, @werpu: Thank you for considering the portlet use-case. I think we can make #3 work in a portlet environment, but need to make sure that the javax.faces.ViewState hidden field values are only updated for forms that are associated with the portlet that invoked the XHR. (In other words, there could be other JSF portlets that are part of the HTML document that should not be touched by jsf.js, even though they share the same jsf.js resource). In a portlet environment the UIViewRoot implements NamingContainer, thanks to javax.portlet.faces.component.PortletNamingContainerUIViewRoot Because of this, the "id" attribute of forms is namespaced with the portlet id. For example, given this XHTML: <h:form id= "a" > ... </h:form> <h:form id= "b" > ... </h:form> The following HTML would be rendered to the response: <form id= "Pluto_test_portlet_1__24955534_0_:a" > ... </form> <form id= "Pluto_test_portlet_1__24955534_0_:b" > ... </form> If you consider the following XHR response: <partial-response id= "Pluto_test_portlet_1__24955534_0_" > <changes> <update id= "Pluto_test_portlet_1__24955534_0_:javax.faces.ViewState:0" > <![CDATA[4617973942428228781:-2797886012162436717]]> </update> </changes> </partial-response> Then jsf.js could use the Pluto_test_portlet_1_ 24955534_0 namespace to make sure it only updates javax.faces.ViewState hidden fields in forms that contain that namespace in their "id" attribute. As a side note, the portlet namespace is known to Mojarra as "namingContainerId" when it builds up mojarra.ab JavasScript. For more information, see AjaxBehaviorRenderer.java and jsf.js . Mojarra uses it to namespace XHR parameters such as javax.faces.partial.ajax in a portlet environment that requires strict parameter namespacing. The XHR parameter namespacing feature is unique to Mojarra – it has not yet been contributed to MyFaces.
          Hide
          balusc added a comment - - edited

          OK, I will propose the following API changes for JSF 2.3:

          1. PartialResponseWriter#startDocument(): if UIViewRoot is available, then write UIViewRoot#getContainerClientId() as value of the id attribute of the <partial-response> element. Both Mojarra and MyFaces already implement this (although currently unspecified). In Mojarra's jsf.js, this value must replace/substitute the Mojarra-specific namingContainerId variable (which however appears to be already partially prepared for this, given the presence of a partialResponseId variable in doUpdate function which is totally unused). Portlets can then interpret this value as their parameter namespace (this is exactly what the Mojarra-specific namingContainerId did).

          2. Correct an incorrect statement in the javadoc of ViewHandler#writeState() - it currently says that the <update> for javax.faces.ViewState is written into Ajax response during UIViewRoot#encodeEnd(). But this is untrue, in both Mojarra and MyFaces it actually takes place during PartialViewContext#processPartial(). I'll leave the reasoning for this deviation in the middle as this appears to be an oversight. Although I personally find that it makes more sense if it takes place during PartialResponseWriter#endDocument.

          3. Update PartialViewContext#processPartial() to specify that during render response phase, the <update> for javax.faces.ViewState will be written, along with id attribute of exactly UIViewRoot#getContainerClientId() + UINamingContainer#getSeparatorCharacter() + ResponseStateManager#VIEW_STATE_PARAM. Currently, both Mojarra and MyFaces already implement this with one minor difference: an incremental counter identifying the form is being suffixed, this is now unnecessary.

          4. Update the JSF 2.2 section (orange) of jsf.ajax.response jsdoc to remove <UNIQUE_PER_VIEW_NUMBER> altogether as below (changes in bold, strikeout means removal):

          If an <update> element is found in the response with an identifier containing javax.faces.ViewState:

          <update id="<VIEW_ROOT_CONTAINER_CLIENT_ID><SEP>javax.faces.ViewState">
             <![CDATA[...]]>
          </update>
          

          locate and update the submitting form's javax.faces.ViewState value with the CDATA contents from the response. <SEP>: is the currently configured UINamingContainer.getSeparatorChar(). <VIEW_ROOT_CONTAINER_CLIENT_ID> is the return from UIViewRoot.getContainerClientId() on the view from whence this state originated. <UNIQUE_PER_VIEW_NUMBER> is a number that must be unique within this view, but must not be included in the view state. This requirement is simply to satisfy XML correctness in parity with what is done in the corresponding non-partial JSF view. Locate and update the javax.faces.ViewState value for all forms specified covered in the render target list whose ID starts with the same <VIEW_ROOT_CONTAINER_CLIENT_ID> value.

          The <UNIQUE_PER_VIEW_NUMBER> has no value in the API

          @werpu: what do you think?
          @Neil: is this acceptable for portlets?

          Show
          balusc added a comment - - edited OK, I will propose the following API changes for JSF 2.3: 1. PartialResponseWriter#startDocument() : if UIViewRoot is available, then write UIViewRoot#getContainerClientId() as value of the id attribute of the <partial-response> element. Both Mojarra and MyFaces already implement this (although currently unspecified). In Mojarra's jsf.js , this value must replace/substitute the Mojarra-specific namingContainerId variable (which however appears to be already partially prepared for this, given the presence of a partialResponseId variable in doUpdate function which is totally unused). Portlets can then interpret this value as their parameter namespace (this is exactly what the Mojarra-specific namingContainerId did). 2. Correct an incorrect statement in the javadoc of ViewHandler#writeState() - it currently says that the <update> for javax.faces.ViewState is written into Ajax response during UIViewRoot#encodeEnd() . But this is untrue, in both Mojarra and MyFaces it actually takes place during PartialViewContext#processPartial() . I'll leave the reasoning for this deviation in the middle as this appears to be an oversight. Although I personally find that it makes more sense if it takes place during PartialResponseWriter#endDocument . 3. Update PartialViewContext#processPartial() to specify that during render response phase, the <update> for javax.faces.ViewState will be written, along with id attribute of exactly UIViewRoot#getContainerClientId() + UINamingContainer#getSeparatorCharacter() + ResponseStateManager#VIEW_STATE_PARAM . Currently, both Mojarra and MyFaces already implement this with one minor difference: an incremental counter identifying the form is being suffixed, this is now unnecessary. 4. Update the JSF 2.2 section (orange) of jsf.ajax.response jsdoc to remove <UNIQUE_PER_VIEW_NUMBER> altogether as below (changes in bold, strikeout means removal): If an <update> element is found in the response with an identifier containing javax.faces.ViewState: <update id= "<VIEW_ROOT_CONTAINER_CLIENT_ID><SEP>javax.faces.ViewState" > <![CDATA[...]]> </update> locate and update the submitting form's javax.faces.ViewState value with the CDATA contents from the response. <SEP>: is the currently configured UINamingContainer.getSeparatorChar(). <VIEW_ROOT_CONTAINER_CLIENT_ID> is the return from UIViewRoot.getContainerClientId() on the view from whence this state originated. <UNIQUE_PER_VIEW_NUMBER> is a number that must be unique within this view, but must not be included in the view state. This requirement is simply to satisfy XML correctness in parity with what is done in the corresponding non-partial JSF view. Locate and update the javax.faces.ViewState value for all forms specified covered in the render target list whose ID starts with the same <VIEW_ROOT_CONTAINER_CLIENT_ID> value . The <UNIQUE_PER_VIEW_NUMBER> has no value in the API @werpu: what do you think? @Neil: is this acceptable for portlets?
          Hide
          Neil Griffin added a comment - - edited

          @balusc: Thank you for taking the time to propose these changes. Before I comment on your changes, I would like to make you aware of some portlet incompatibilities at are related to javax.faces.ViewState. When you get an opportunity, please read the class-level JavaDoc in ResponseWriterBridgeImpl.java and let me know if it might be appropriate to fix these incompatibilities for JSF 2.3. Thank you.

          Show
          Neil Griffin added a comment - - edited @balusc: Thank you for taking the time to propose these changes. Before I comment on your changes, I would like to make you aware of some portlet incompatibilities at are related to javax.faces.ViewState. When you get an opportunity, please read the class-level JavaDoc in ResponseWriterBridgeImpl.java and let me know if it might be appropriate to fix these incompatibilities for JSF 2.3. Thank you.
          Hide
          balusc added a comment -

          I gather you're talking about limitation #3 in that javadoc? This should already be solved since JSF 2.2 where the javax.faces.ViewState hidden input field ID became namespaced with container client ID.

          Show
          balusc added a comment - I gather you're talking about limitation #3 in that javadoc? This should already be solved since JSF 2.2 where the javax.faces.ViewState hidden input field ID became namespaced with container client ID.
          Hide
          Neil Griffin added a comment -

          @balusc: I'm referring mainly to #2 and #3 in the class-level JavaDoc in ResponseWriterBridgeImpl.java.

          #2 is a workaround for the code in jsf.js that interprets <update id="javax.faces.ViewRoot">...</update> to mean replacing the <body>...</body> of the HTML document. I just created JAVASERVERFACES_SPEC_PUBLIC-1421 in order to see if the JSF EG would consider fixing jsf.js – it might be that the EG decides that this is a bridge concern and that the workaround should become a bridge requirement in JSR 378.

          #3 is a workaround that causes the javax.faces.ViewState hidden field to always be rendered before the closing </form> tag when a partial-response is being written. It works around the code in jsf.js that attempts to add the hidden field to the DOM which fails in a portlet environment. This problem happens when a navigation-rule executes during an XHR. Specifically, the form that was submitted via f:ajax is no longer part of the DOM, since the entire portlet <div>...</div> section was replaced due to navigation to a different viewId. I suppose that the appropriateness of this workaround might be determined by the outcome of JAVASERVERFACES_SPEC_PUBLIC-1421.

          Since this is all somewhat off-topic, I will provide feedback on your proposed API changes in a follow-up comment.

          Show
          Neil Griffin added a comment - @balusc: I'm referring mainly to #2 and #3 in the class-level JavaDoc in ResponseWriterBridgeImpl.java. #2 is a workaround for the code in jsf.js that interprets <update id="javax.faces.ViewRoot">...</update> to mean replacing the <body>...</body> of the HTML document . I just created JAVASERVERFACES_SPEC_PUBLIC-1421 in order to see if the JSF EG would consider fixing jsf.js – it might be that the EG decides that this is a bridge concern and that the workaround should become a bridge requirement in JSR 378. #3 is a workaround that causes the javax.faces.ViewState hidden field to always be rendered before the closing </form> tag when a partial-response is being written . It works around the code in jsf.js that attempts to add the hidden field to the DOM which fails in a portlet environment. This problem happens when a navigation-rule executes during an XHR. Specifically, the form that was submitted via f:ajax is no longer part of the DOM, since the entire portlet <div>...</div> section was replaced due to navigation to a different viewId. I suppose that the appropriateness of this workaround might be determined by the outcome of JAVASERVERFACES_SPEC_PUBLIC-1421 . Since this is all somewhat off-topic , I will provide feedback on your proposed API changes in a follow-up comment.
          Hide
          Neil Griffin added a comment -

          @balusc: Your proposed API changes look good. However, should jsf.js need to add a javax.faces.ViewState hidden field to the DOM on-the-fly, the value of the name attribute should be prefixed by the namingContainerId. Looks like there is a bug in jsf.js in this regard. If you have time in your schedule, it would be great if you could fix the bug as part of your prototype work. Thanks, Neil.

          Show
          Neil Griffin added a comment - @balusc: Your proposed API changes look good. However, should jsf.js need to add a javax.faces.ViewState hidden field to the DOM on-the-fly, the value of the name attribute should be prefixed by the namingContainerId. Looks like there is a bug in jsf.js in this regard. If you have time in your schedule, it would be great if you could fix the bug as part of your prototype work. Thanks, Neil.
          Hide
          balusc added a comment -

          #2 is unrelated to the current issue. It's good you created issue 1421 on this.

          I will keep the field name bug in mind. I only think it should be prefixed at the moment ajax request is to be sent, as done for all other params, not at the line you pointed out.

          Show
          balusc added a comment - #2 is unrelated to the current issue. It's good you created issue 1421 on this. I will keep the field name bug in mind. I only think it should be prefixed at the moment ajax request is to be sent, as done for all other params, not at the line you pointed out.
          Hide
          Neil Griffin added a comment -

          That's fine to prepend the namespace at the time the request is sent. At first glance it looked like a bug to me because the bridge causes the namespace to be prepended when a component is rendered to the response.

          Show
          Neil Griffin added a comment - That's fine to prepend the namespace at the time the request is sent. At first glance it looked like a bug to me because the bridge causes the namespace to be prepended when a component is rendered to the response.
          Hide
          Neil Griffin added a comment -

          @balusc: Regarding the lines I mentioned in jsf.js – after the element is added to the DOM dynamically, then if the form is subsequently submitted without f:ajax (full-page postback) then the lack of a prepended namespace would be a problem.

          Show
          Neil Griffin added a comment - @balusc: Regarding the lines I mentioned in jsf.js – after the element is added to the DOM dynamically, then if the form is subsequently submitted without f:ajax (full-page postback) then the lack of a prepended namespace would be a problem.
          Hide
          balusc added a comment -

          I see, will keep that in mind.

          Show
          balusc added a comment - I see, will keep that in mind.
          Hide
          balusc added a comment - - edited

          Pushed: https://java.net/projects/mojarra/sources/git/revision/ff26c4dccafd50d22757ce8562230e31f9768033

          @Neil: please let me know if that works for portlets as well and you could further reduce current workarounds in portlet side. FYI: the initial Mojarra implementation incorrectly omitted namingcontainer separator character from namespace prefix in ajax specific parameters. I have fixed that as well (and renamed Mojarra-specific RequestParameterMap#getNamingContainerId() to RequestParameterMap#getNamingContainerPrefix(), just in case you were relying on it).

          @werpu: please let me know if the JSF/JS API changes are clear enough for MyFaces. Here's a summary:

          ViewHandler#writeState(FacesContext)

          @@ -737,8 +737,9 @@ public abstract class ViewHandler {
                * <code>Ajax</code> requests, the state is obtained by calling
                * {@link StateManager#getViewState}
                * and then written into the <code>Ajax</code> response during final
          -     * encoding 
          -     * ({@link javax.faces.component.UIViewRoot#encodeEnd}. 
          +     * encoding <span class="changed_modified_2_3">
          +     * ({@link javax.faces.context.PartialViewContext#processPartial(javax.faces.event.PhaseId)})
          +     * </span>.
                * </p>
                *
                * @param context {@link FacesContext} for the current request
          

          PartialViewContext#processPartial(PhaseId)

          @@ -314,6 +317,17 @@ public abstract class PartialViewContext {
                * <code>Collection</code> returned from {@link #getExecuteIds} 
                * and {@link #getRenderIds} will be processed.</p>  
                *
          +     * <p class="changed_added_2_3">When the indicated <code>phaseId</code>
          +     * equals {@link PhaseId#RENDER_RESPONSE}, then obtain the state by calling
          +     * {@link StateManager#getViewState} and write out it as an update element
          +     * with an identifier of
          +     * <code>&lt;VIEW_ROOT_CONTAINER_CLIENT_ID&gt;&lt;SEP&gt;javax.faces.ViewState</code>
          +     * where <code>&lt;VIEW_ROOT_CONTAINER_CLIENT_ID&gt;</code> is the return
          +     * from {@link UIViewRoot#getContainerClientId(FacesContext)} on the view
          +     * from whence this state originated, and <code>&lt;SEP&gt;</code> is the
          +     * currently configured
          +     * {@link UINamingContainer#getSeparatorChar(FacesContext)}.</p>
          +     *
                * @param phaseId the {@link javax.faces.event.PhaseId} that indicates
                * the lifecycle phase the components will be processed in. 
                */ 
          

          PartialResponseWriter#startDocument()

          @@ -113,6 +116,10 @@ public class PartialResponseWriter extends ResponseWriterWrapper {
           
               /**
                * <p class="changed_added_2_0">Write the start of a partial response.</p>
          +     * <p class="changed_added_2_3">If {@link UIViewRoot} is an instance of
          +     * {@link NamingContainer}, then write
          +     * {@link UIViewRoot#getContainerClientId(FacesContext)} as value of the
          +     * <code>id</code> attribute of the root element.</p>
                *
                * @throws IOException if an input/output error occurs
                * @since 2.0
          

          jsf.ajax.request

          @@ -2048,7 +2061,8 @@ if (!((jsf && jsf.specversion && jsf.specversion >= 20000 ) &&
                        * <p><b>Implementation Requirements:</b></p>
                        * This function must:
                        * <ul>
          -             * <li>Be used within the context of a <code>form</code>.</li>
          +             * <li>Be used within the context of a <code>form</code><span class="changed_added_2_3">,
          +             * else throw an error</span>.</li>
                        * <li>Capture the element that triggered this Ajax request
                        * (from the <code>source</code> argument, also known as the
                        * <code>source</code> element.</li>
          
          @@ -2059,6 +2073,16 @@ if (!((jsf && jsf.specversion && jsf.specversion >= 20000 ) &&
                        * <li>If the <code>source</code> argument is a <code>string</code>, find the
                        * DOM element for that <code>string</code> identifier.
                        * <li>If the DOM element could not be determined, throw an error.</li>
          +             * <li class="changed_added_2_3">If the <code>javax.faces.ViewState</code> 
          +             * element could not be found, throw an error.</li>
          +             * <li class="changed_added_2_3">If the <code>javax.faces.ViewState</code> 
          +             * element has a <code>&lt;update id="&lt;VIEW_ROOT_CONTAINER_CLIENT_ID&gt;&lt;SEP&gt;</code>
          +             * prefix, where &lt;SEP&gt; is the currently configured
          +             * <code>UINamingContainer.getSeparatorChar()</code> and
          +             * &lt;VIEW_ROOT_CONTAINER_CLIENT_ID&gt; is the return from
          +             * <code>UIViewRoot.getContainerClientId()</code> on the
          +             * view from whence this state originated, then remember it as <i>namespace prefix</i>.
          +             * This is needed during encoding of the set of post data arguments.</li>
                        * <li>If the <code>onerror</code> and <code>onevent</code> arguments are set,
                        * they must be functions, or throw an error.
                        * <li>Determine the <code>source</code> element's <code>form</code>
          
          @@ -2175,7 +2199,10 @@ if (!((jsf && jsf.specversion && jsf.specversion >= 20000 ) &&
                        * </li>
                        * </ul>
                        * </li>
          -             * <li>Encode the set of post data arguments.</li>
          +             * <li>Encode the set of post data arguments. <span class="changed_added_2_3">
          +             * If the <code>javax.faces.ViewState</code> element has a namespace prefix, then
          +             * make sure that all post data arguments are prefixed with this namespace prefix.
          +             * </span></li>
                        * <li>Join the encoded view state with the encoded set of post data arguments
                        * to form the <code>query string</code> that will be sent to the server.</li>
                        * <li>Create a request <code>context</code> object and set the properties:
          

          jsf.ajax.response

          @@ -2613,8 +2639,9 @@ if (!((jsf && jsf.specversion && jsf.specversion >= 20000 ) &&
                        * correctness in parity with what is done in the
                        * corresponding non-partial JSF view.  Locate and update
                        * the <code>javax.faces.ViewState</code> value for all
          -             * forms specified in the <code>render</code> target
          -             * list.</li>
          +             * POST forms covered in the <code>render</code> target
          +             * list whose ID starts with the same 
          +             * &lt;VIEW_ROOT_CONTAINER_CLIENT_ID&gt; value.</li>
           
                        * <li class="changed_added_2_2">If an
                        * <code>update</code> element is found in the response with
          
          @@ -2639,8 +2666,9 @@ if (!((jsf && jsf.specversion && jsf.specversion >= 20000 ) &&
                        * correctness in parity with what is done in the
                        * corresponding non-partial JSF view.  Locate and update
                        * the <code>javax.faces.ClientWindow</code> value for all
          -             * forms specified in the <code>render</code> target
          -             * list.</li>
          +             * POST forms covered in the <code>render</code> target
          +             * list whose ID starts with the same 
          +             * &lt;VIEW_ROOT_CONTAINER_CLIENT_ID&gt; value.</li>
           
           
                        * <li>If an <code>update</code> element is found in the response with the identifier
          
          Show
          balusc added a comment - - edited Pushed: https://java.net/projects/mojarra/sources/git/revision/ff26c4dccafd50d22757ce8562230e31f9768033 @Neil: please let me know if that works for portlets as well and you could further reduce current workarounds in portlet side. FYI: the initial Mojarra implementation incorrectly omitted namingcontainer separator character from namespace prefix in ajax specific parameters. I have fixed that as well (and renamed Mojarra-specific RequestParameterMap#getNamingContainerId() to RequestParameterMap#getNamingContainerPrefix(), just in case you were relying on it). @werpu: please let me know if the JSF/JS API changes are clear enough for MyFaces. Here's a summary: ViewHandler#writeState(FacesContext) @@ -737,8 +737,9 @@ public abstract class ViewHandler { * <code>Ajax</code> requests, the state is obtained by calling * {@link StateManager#getViewState} * and then written into the <code>Ajax</code> response during final - * encoding - * ({@link javax.faces.component.UIViewRoot#encodeEnd}. + * encoding <span class= "changed_modified_2_3" > + * ({@link javax.faces.context.PartialViewContext#processPartial(javax.faces.event.PhaseId)}) + * </span>. * </p> * * @param context {@link FacesContext} for the current request PartialViewContext#processPartial(PhaseId) @@ -314,6 +317,17 @@ public abstract class PartialViewContext { * <code>Collection</code> returned from {@link #getExecuteIds} * and {@link #getRenderIds} will be processed.</p> * + * <p class= "changed_added_2_3" >When the indicated <code>phaseId</code> + * equals {@link PhaseId#RENDER_RESPONSE}, then obtain the state by calling + * {@link StateManager#getViewState} and write out it as an update element + * with an identifier of + * <code>&lt;VIEW_ROOT_CONTAINER_CLIENT_ID&gt;&lt;SEP&gt;javax.faces.ViewState</code> + * where <code>&lt;VIEW_ROOT_CONTAINER_CLIENT_ID&gt;</code> is the return + * from {@link UIViewRoot#getContainerClientId(FacesContext)} on the view + * from whence this state originated, and <code>&lt;SEP&gt;</code> is the + * currently configured + * {@link UINamingContainer#getSeparatorChar(FacesContext)}.</p> + * * @param phaseId the {@link javax.faces.event.PhaseId} that indicates * the lifecycle phase the components will be processed in. */ PartialResponseWriter#startDocument() @@ -113,6 +116,10 @@ public class PartialResponseWriter extends ResponseWriterWrapper { /** * <p class= "changed_added_2_0" >Write the start of a partial response.</p> + * <p class= "changed_added_2_3" >If {@link UIViewRoot} is an instance of + * {@link NamingContainer}, then write + * {@link UIViewRoot#getContainerClientId(FacesContext)} as value of the + * <code>id</code> attribute of the root element.</p> * * @ throws IOException if an input/output error occurs * @since 2.0 jsf.ajax.request @@ -2048,7 +2061,8 @@ if (!((jsf && jsf.specversion && jsf.specversion >= 20000 ) && * <p><b>Implementation Requirements:</b></p> * This function must: * <ul> - * <li>Be used within the context of a <code>form</code>.</li> + * <li>Be used within the context of a <code>form</code><span class= "changed_added_2_3" >, + * else throw an error</span>.</li> * <li>Capture the element that triggered this Ajax request * (from the <code>source</code> argument, also known as the * <code>source</code> element.</li> @@ -2059,6 +2073,16 @@ if (!((jsf && jsf.specversion && jsf.specversion >= 20000 ) && * <li>If the <code>source</code> argument is a <code>string</code>, find the * DOM element for that <code>string</code> identifier. * <li>If the DOM element could not be determined, throw an error.</li> + * <li class= "changed_added_2_3" >If the <code>javax.faces.ViewState</code> + * element could not be found, throw an error.</li> + * <li class= "changed_added_2_3" >If the <code>javax.faces.ViewState</code> + * element has a <code>&lt;update id="&lt;VIEW_ROOT_CONTAINER_CLIENT_ID&gt;&lt;SEP&gt;</code> + * prefix, where &lt;SEP&gt; is the currently configured + * <code>UINamingContainer.getSeparatorChar()</code> and + * &lt;VIEW_ROOT_CONTAINER_CLIENT_ID&gt; is the return from + * <code>UIViewRoot.getContainerClientId()</code> on the + * view from whence this state originated, then remember it as <i>namespace prefix</i>. + * This is needed during encoding of the set of post data arguments.</li> * <li>If the <code>onerror</code> and <code>onevent</code> arguments are set, * they must be functions, or throw an error. * <li>Determine the <code>source</code> element's <code>form</code> @@ -2175,7 +2199,10 @@ if (!((jsf && jsf.specversion && jsf.specversion >= 20000 ) && * </li> * </ul> * </li> - * <li>Encode the set of post data arguments.</li> + * <li>Encode the set of post data arguments. <span class= "changed_added_2_3" > + * If the <code>javax.faces.ViewState</code> element has a namespace prefix, then + * make sure that all post data arguments are prefixed with this namespace prefix. + * </span></li> * <li>Join the encoded view state with the encoded set of post data arguments * to form the <code>query string</code> that will be sent to the server.</li> * <li>Create a request <code>context</code> object and set the properties: jsf.ajax.response @@ -2613,8 +2639,9 @@ if (!((jsf && jsf.specversion && jsf.specversion >= 20000 ) && * correctness in parity with what is done in the * corresponding non-partial JSF view. Locate and update * the <code>javax.faces.ViewState</code> value for all - * forms specified in the <code>render</code> target - * list.</li> + * POST forms covered in the <code>render</code> target + * list whose ID starts with the same + * &lt;VIEW_ROOT_CONTAINER_CLIENT_ID&gt; value.</li> * <li class= "changed_added_2_2" >If an * <code>update</code> element is found in the response with @@ -2639,8 +2666,9 @@ if (!((jsf && jsf.specversion && jsf.specversion >= 20000 ) && * correctness in parity with what is done in the * corresponding non-partial JSF view. Locate and update * the <code>javax.faces.ClientWindow</code> value for all - * forms specified in the <code>render</code> target - * list.</li> + * POST forms covered in the <code>render</code> target + * list whose ID starts with the same + * &lt;VIEW_ROOT_CONTAINER_CLIENT_ID&gt; value.</li> * <li>If an <code>update</code> element is found in the response with the identifier
          Hide
          Neil Griffin added a comment -

          @balusc: Thank you for making the commit. I hope to have time on Monday June 25 to review the code changes.

          Show
          Neil Griffin added a comment - @balusc: Thank you for making the commit. I hope to have time on Monday June 25 to review the code changes.
          Hide
          Neil Griffin added a comment -

          @balusc: On line 1022 of Util.java, instead of:

          Util.java
          return viewRoot.getContainerClientId(context) + UINamingContainer.getSeparatorChar(context);
          

          Would you instead consider:

          Util.java
          return viewRoot.getContainerClientId(context);
          

          I admit that it is inconsistent to have UIInput name attribute values to be rendered as namespace + ":" + clientId but not include ":" for hidden fields like javax.faces.ViewRoot.

          However, that's the way it was implemented in Mojarra 2.2 and it fits more naturally with the Faces Bridge as well as the method of namespacing parameters encouraged by the Portlet API. Thank you.

          Show
          Neil Griffin added a comment - @balusc: On line 1022 of Util.java , instead of: Util.java return viewRoot.getContainerClientId(context) + UINamingContainer.getSeparatorChar(context); Would you instead consider: Util.java return viewRoot.getContainerClientId(context); I admit that it is inconsistent to have UIInput name attribute values to be rendered as namespace + ":" + clientId but not include ":" for hidden fields like javax.faces.ViewRoot. However, that's the way it was implemented in Mojarra 2.2 and it fits more naturally with the Faces Bridge as well as the method of namespacing parameters encouraged by the Portlet API. Thank you.
          Hide
          balusc added a comment - - edited

          How does that work for regular input fields? They are also namespaced this way. With those JSF API changes, it should theoretically not be necessary anymore to override the request parameter map this way and just rely on standard JSF implementation. Those API changes are meant to further reduce code in Portlet bridge and let JSF do the work.

          Show
          balusc added a comment - - edited How does that work for regular input fields? They are also namespaced this way. With those JSF API changes, it should theoretically not be necessary anymore to override the request parameter map this way and just rely on standard JSF implementation. Those API changes are meant to further reduce code in Portlet bridge and let JSF do the work.
          Hide
          Neil Griffin added a comment -

          The Faces Bridge's implementation of ExternalContext.getRequestParameterMap() decorates a PortletRequest, so it is not possible to use com.sun.faces.context.RequestParameterMap since it decorates a ServletRequest.

          The reason why input fields work is because decode methods automatically include the namespace prefix (and the separator char) by asking for a parameter value with getClientId() as the parameter name, like this:

          FacesContext.getCurrentInstance().getExternalContext().getRequestParameterMap(uiInput.getClientId());
          

          For example, see HtmlBasicRenderer.java

          Lookups for request parameters like javax.faces.ViewState do not automatically include the namespace prefix, which means that the Faces Bridge's implementation of ExternalContext.getRequestParameterMap() must account for this.

          A good example is ResponseStateManagerImpl.java.

          Show
          Neil Griffin added a comment - The Faces Bridge's implementation of ExternalContext.getRequestParameterMap() decorates a PortletRequest, so it is not possible to use com.sun.faces.context.RequestParameterMap since it decorates a ServletRequest . The reason why input fields work is because decode methods automatically include the namespace prefix (and the separator char) by asking for a parameter value with getClientId() as the parameter name, like this: FacesContext.getCurrentInstance().getExternalContext().getRequestParameterMap(uiInput.getClientId()); For example, see HtmlBasicRenderer.java Lookups for request parameters like javax.faces.ViewState do not automatically include the namespace prefix, which means that the Faces Bridge's implementation of ExternalContext.getRequestParameterMap() must account for this. A good example is ResponseStateManagerImpl.java .
          Hide
          balusc added a comment -

          I see, the problem boils down to that JSF impl is internally not prefixing the standard parameters with naming container client ID when the UIViewRoot is an instance of NamingContainer. If that part is fixed, then the whole RequestParameterMap approach becomes unnecessary. And, then the portlet case will also start to work correctly, right?

          Show
          balusc added a comment - I see, the problem boils down to that JSF impl is internally not prefixing the standard parameters with naming container client ID when the UIViewRoot is an instance of NamingContainer. If that part is fixed, then the whole RequestParameterMap approach becomes unnecessary. And, then the portlet case will also start to work correctly, right?
          Hide
          balusc added a comment - - edited

          Neil/Werpu, how about this change on UIViewRoot#encodeChildren()?

               * <p class="changed_added_2_3">If this {@link UIViewRoot} is an instance of {@link NamingContainer}, then the JSF
               * implementation must ensure that all encoded POST request parameter names are prefixed with
               * {@link UIViewRoot#getContainerClientId(FacesContext)} as per rules of {@link UIComponent#getClientId(FacesContext)}.
               * This also covers all predefined POST request parameters such as {@link ResponseStateManager#VIEW_STATE_PARAM},
               * {@link ClientBehaviorContext#BEHAVIOR_SOURCE_PARAM_NAME} and {@link PartialViewContext#PARTIAL_EVENT_PARAM_NAME}.
               * </p>
           

          I implemented this locally on Mojarra and all unit tests have passed. I didn't yet remove Mojarra internal RequestParameterMap just to ensure backwards compatibility.

          Show
          balusc added a comment - - edited Neil/Werpu, how about this change on UIViewRoot#encodeChildren() ? * <p class= "changed_added_2_3" >If this {@link UIViewRoot} is an instance of {@link NamingContainer}, then the JSF * implementation must ensure that all encoded POST request parameter names are prefixed with * {@link UIViewRoot#getContainerClientId(FacesContext)} as per rules of {@link UIComponent#getClientId(FacesContext)}. * This also covers all predefined POST request parameters such as {@link ResponseStateManager#VIEW_STATE_PARAM}, * {@link ClientBehaviorContext#BEHAVIOR_SOURCE_PARAM_NAME} and {@link PartialViewContext#PARTIAL_EVENT_PARAM_NAME}. * </p> I implemented this locally on Mojarra and all unit tests have passed. I didn't yet remove Mojarra internal RequestParameterMap just to ensure backwards compatibility.
          Hide
          balusc added a comment -

          Neil, above proposed implementation changes have been committed as per https://java.net/projects/mojarra/sources/git/revision/3b1d1e2a1330c6789f566f36597b6b9fcb81d509 so you have the opportunity to test it against your Portlet case.

          Only API change (the above Javadoc proposal) has not been committed yet due to lack of confirming feedback.

          Show
          balusc added a comment - Neil, above proposed implementation changes have been committed as per https://java.net/projects/mojarra/sources/git/revision/3b1d1e2a1330c6789f566f36597b6b9fcb81d509 so you have the opportunity to test it against your Portlet case. Only API change (the above Javadoc proposal) has not been committed yet due to lack of confirming feedback.
          Hide
          lu4242 added a comment -

          Other post parameters that could be included are javax.faces.RenderKitId (ResponseStateManager#RENDER_KIT_ID_PARAM) and javax.faces.ClientWindow (ResponseStateManager#javax.faces.ClientWindow).

          Show
          lu4242 added a comment - Other post parameters that could be included are javax.faces.RenderKitId (ResponseStateManager#RENDER_KIT_ID_PARAM) and javax.faces.ClientWindow (ResponseStateManager#javax.faces.ClientWindow).
          Hide
          balusc added a comment - - edited

          Indeed. There are more. Should we specify them all? I have in Mojarra at least collected them as RenderKitUtils.PredefinedPostbackParameter enum. There are so far 9 of those: https://github.com/javaserverfaces/mojarra/blob/3b1d1e2a1330c6789f566f36597b6b9fcb81d509/jsf-ri/src/main/java/com/sun/faces/renderkit/RenderKitUtils.java#L181

          Show
          balusc added a comment - - edited Indeed. There are more. Should we specify them all? I have in Mojarra at least collected them as RenderKitUtils.PredefinedPostbackParameter enum. There are so far 9 of those: https://github.com/javaserverfaces/mojarra/blob/3b1d1e2a1330c6789f566f36597b6b9fcb81d509/jsf-ri/src/main/java/com/sun/faces/renderkit/RenderKitUtils.java#L181
          Hide
          balusc added a comment -

          I've pushed the javadoc changes: https://java.net/projects/mojarra/sources/git/revision/1569145562d8c20cc924f0eb5343b5e931df352d. For now, the issue is technically solved in JSF API and Mojarra impl.

          Before I close off the issue, please let me know if there are any things unclear or missing, particularly with regard to Portlets and MyFaces.

          Show
          balusc added a comment - I've pushed the javadoc changes: https://java.net/projects/mojarra/sources/git/revision/1569145562d8c20cc924f0eb5343b5e931df352d . For now, the issue is technically solved in JSF API and Mojarra impl. Before I close off the issue, please let me know if there are any things unclear or missing, particularly with regard to Portlets and MyFaces.
          Hide
          werpu added a comment -

          Hi sorry for being so silent, fact is I was extremely busy. I will check the changes tonight and will give my comment either tonight or tomorrow morning. So please keep the issue open at least until tomorrow.

          Show
          werpu added a comment - Hi sorry for being so silent, fact is I was extremely busy. I will check the changes tonight and will give my comment either tonight or tomorrow morning. So please keep the issue open at least until tomorrow.
          Hide
          werpu added a comment -

          Ok, again sorry for not commenting on this issue for such a long time, I personally like the ViewRoot prepending to javax.faces.ViewState in the id is a reasonable way to go. I can only comment on the ajax client side however. I personally think this solves the problem for me. The original problem was as far as I remember that in the original specs following was written.

          If an update element is found in the response with the identifier javax.faces.ViewState:
          <update id="javax.faces.ViewState">
          <![CDATA[...]]>
          </update>
          locate and update the submitting form's javax.faces.ViewState value with the CDATA contents from the response.

          Which was clearly wrong, since the viewstate should be the same for all forms under one viewroot. Adding the viewroot id fixes this problem once and for all for all cases.
          Thanks for your efforts on fixing this long outstanding issue.

          Show
          werpu added a comment - Ok, again sorry for not commenting on this issue for such a long time, I personally like the ViewRoot prepending to javax.faces.ViewState in the id is a reasonable way to go. I can only comment on the ajax client side however. I personally think this solves the problem for me. The original problem was as far as I remember that in the original specs following was written. If an update element is found in the response with the identifier javax.faces.ViewState: <update id="javax.faces.ViewState"> <![CDATA [...] ]> </update> locate and update the submitting form's javax.faces.ViewState value with the CDATA contents from the response. Which was clearly wrong, since the viewstate should be the same for all forms under one viewroot. Adding the viewroot id fixes this problem once and for all for all cases. Thanks for your efforts on fixing this long outstanding issue.
          Hide
          balusc added a comment -

          Great, thank you for your feedback! Now yet a confirmation from Portlet guys.

          Show
          balusc added a comment - Great, thank you for your feedback! Now yet a confirmation from Portlet guys.
          Hide
          Neil Griffin added a comment -

          @balusc: I have begun reviewing your recent commits for this issue. Also, I have rebuilt Mojarra 2.3.0-m07 from source and have it running under Pluto 3.0. My goal is to report back tomorrow with more information. Thanks for your patience, Neil.

          Show
          Neil Griffin added a comment - @balusc: I have begun reviewing your recent commits for this issue. Also, I have rebuilt Mojarra 2.3.0-m07 from source and have it running under Pluto 3.0. My goal is to report back tomorrow with more information. Thanks for your patience, Neil.
          Hide
          Neil Griffin added a comment -

          @balusc: I have done preliminary testing and have seen good results so far. I still need to do more testing so I ask that you please keep this issue open. Regarding the namespace prefix, please consider the changes in the following pull request: https://github.com/javaserverfaces/mojarra/pull/3

          The changes in the pull request remove the separator char from the namespace prefix. That's the way it is implemented in Mojarra 2.2 and it fits more naturally with the Faces Bridge as well as the method of namespacing parameters encouraged by the Portlet API. In addition, component suites like PrimeFaces and ICEfaces namespace their own predefined postback parameters without using the separator char. Thank you.

          Show
          Neil Griffin added a comment - @balusc: I have done preliminary testing and have seen good results so far. I still need to do more testing so I ask that you please keep this issue open. Regarding the namespace prefix, please consider the changes in the following pull request: https://github.com/javaserverfaces/mojarra/pull/3 The changes in the pull request remove the separator char from the namespace prefix. That's the way it is implemented in Mojarra 2.2 and it fits more naturally with the Faces Bridge as well as the method of namespacing parameters encouraged by the Portlet API. In addition, component suites like PrimeFaces and ICEfaces namespace their own predefined postback parameters without using the separator char. Thank you.
          Hide
          balusc added a comment -

          Namespacing without separator character does not fit naturally in JSF API. I imagined that doing all the namespacing job in JSF side should reduce the work in the Faces Bridge. What work exactly is left in the Faces Bridge that it still needs its own way of namespacing? Then we can look for ways reducing/removing that work. Perhaps a new JSF API method to obtain a namespaced predefined postback parameter without the need to manually check and prefix it?

          Show
          balusc added a comment - Namespacing without separator character does not fit naturally in JSF API. I imagined that doing all the namespacing job in JSF side should reduce the work in the Faces Bridge. What work exactly is left in the Faces Bridge that it still needs its own way of namespacing? Then we can look for ways reducing/removing that work. Perhaps a new JSF API method to obtain a namespaced predefined postback parameter without the need to manually check and prefix it?
          Hide
          Neil Griffin added a comment -

          @balusc: The convenience of being able to pass a non-namespaced parameter name to the externalContext.getRequestParameterMap().get(String) method has been assumed since JSR 301, and perhaps even by JSF portlet developers over the years. Removing that functionality from the bridge's ExternalContextImpl could be a breaking change for legacy JSF portlets. In fact, there are several places in the Bridge TCK (the TCK that JSR 378 inherited from Oracle) that call externalContext.getRequestParameterMap().get(ResponseStateManager.VIEW_STATE_PARAM) (click here for one example). If you feel that it is important to include the separator char, then we can make an accommodation for that in the bridge's ExternalContextImpl.

          BTW I have completed my testing. I am happy to report that the original use-case reported in the description of this issue is now working for portlets. There is just one small bug in jsf.js that needs to be fixed. Line 495 reads:

          add(document.getElementById(clientIds[i]));
          

          but it needs to be:

          add(document.getElementById(namingContainerPrefix + clientIds[i]));
          

          Thank you, Neil

          Show
          Neil Griffin added a comment - @balusc: The convenience of being able to pass a non -namespaced parameter name to the externalContext.getRequestParameterMap().get(String) method has been assumed since JSR 301, and perhaps even by JSF portlet developers over the years. Removing that functionality from the bridge's ExternalContextImpl could be a breaking change for legacy JSF portlets. In fact, there are several places in the Bridge TCK (the TCK that JSR 378 inherited from Oracle) that call externalContext.getRequestParameterMap().get(ResponseStateManager.VIEW_STATE_PARAM) (click here for one example) . If you feel that it is important to include the separator char, then we can make an accommodation for that in the bridge's ExternalContextImpl. BTW I have completed my testing. I am happy to report that the original use-case reported in the description of this issue is now working for portlets. There is just one small bug in jsf.js that needs to be fixed. Line 495 reads: add(document.getElementById(clientIds[i])); but it needs to be: add(document.getElementById(namingContainerPrefix + clientIds[i])); Thank you, Neil
          Hide
          balusc added a comment - - edited

          The convenience of being able to pass a non-namespaced parameter name to the externalContext.getRequestParameterMap().get(String) method has been assumed since JSR 301, and perhaps even by JSF portlet developers over the years

          As you can see in Mojarra's source code, this should keep working. I've indeed kept it in for sake of backwards compatibility.

          String mapValue = request.getParameter(mapKey);
          if (mapValue == null && !mapKey.startsWith(getNamingContainerPrefix())) {
              // Support cases where enduser manually obtains a request parameter while in a namespaced view.
              mapValue = request.getParameter(getNamingContainerPrefix() + mapKey);
          }
          

          This is however unspecified in API of the request parameter map. Do you think specifying in API is necessary? In that case, we could probably remove the explicit requirement to namespace predefined postback parameters as it will then happen "automatically" in request parameter map.

          Thanks for the jsf.js bug report, I will investigate and fix it.

          Show
          balusc added a comment - - edited The convenience of being able to pass a non-namespaced parameter name to the externalContext.getRequestParameterMap().get(String) method has been assumed since JSR 301, and perhaps even by JSF portlet developers over the years As you can see in Mojarra's source code , this should keep working. I've indeed kept it in for sake of backwards compatibility. String mapValue = request.getParameter(mapKey); if (mapValue == null && !mapKey.startsWith(getNamingContainerPrefix())) { // Support cases where enduser manually obtains a request parameter while in a namespaced view. mapValue = request.getParameter(getNamingContainerPrefix() + mapKey); } This is however unspecified in API of the request parameter map. Do you think specifying in API is necessary? In that case, we could probably remove the explicit requirement to namespace predefined postback parameters as it will then happen "automatically" in request parameter map. Thanks for the jsf.js bug report, I will investigate and fix it.
          Hide
          balusc added a comment - - edited

          As to line 495 of jsf.js, this part is fine. The clientIds[i] or f:ajax render is already namespaced (as it is generated by server). The test case associated with issue 790 would otherwise have failed. Line 2501 of jsf.js contains similar part with ids[i] of f:ajax execute which is also already namespaced. This is only not covered by the unit test yet. I will improve the unit test to make sure it's properly being dealt with.

          Or did you encounter a problem with this in your test application?

          Show
          balusc added a comment - - edited As to line 495 of jsf.js, this part is fine. The clientIds [i] or f:ajax render is already namespaced (as it is generated by server). The test case associated with issue 790 would otherwise have failed. Line 2501 of jsf.js contains similar part with ids [i] of f:ajax execute which is also already namespaced. This is only not covered by the unit test yet. I will improve the unit test to make sure it's properly being dealt with. Or did you encounter a problem with this in your test application?
          Hide
          Neil Griffin added a comment -

          @balusc: In my testing of line 495 using the JS debugger, clientIds[i] is a JavaScript array of length 1 and the value of clientIds[0] is simply 'a' (it is not namespaced). I can provide you with a downloadable Pluto 3 ZIP if you'd like to try it.

          Show
          Neil Griffin added a comment - @balusc: In my testing of line 495 using the JS debugger, clientIds [i] is a JavaScript array of length 1 and the value of clientIds [0] is simply 'a' (it is not namespaced). I can provide you with a downloadable Pluto 3 ZIP if you'd like to try it.
          Hide
          Neil Griffin added a comment -

          Do you think specifying in API is necessary?

          Yes, I think it would be good to add a sentence to ExternalContext#getRequestParameterMap() and ExternalContext#getRequestParameterValuesMap() that says the map's get(String key) method should first try calling getParameter(key) on the underlying request object, but if it returns null, it should try getParameter(namespacePrefix + key). I suppose that ExternalContext. getRequestParameterNames() method should probably return the namespaced version of the key.

          In that case, we could probably remove the explicit requirement to namespace predefined postback parameters as it will then happen "automatically" in request parameter map.

          I think it is good to have the requirement that the namespace prefix be written as part of the name attribute of hidden fields. However, if we added the map requirements I mentioned, then line 204 of RenderKitUtils.java would not need to prepend the namespacePrefix.

          Show
          Neil Griffin added a comment - Do you think specifying in API is necessary? Yes, I think it would be good to add a sentence to ExternalContext#getRequestParameterMap() and ExternalContext#getRequestParameterValuesMap() that says the map's get(String key) method should first try calling getParameter(key) on the underlying request object, but if it returns null, it should try getParameter(namespacePrefix + key) . I suppose that ExternalContext. getRequestParameterNames() method should probably return the namespaced version of the key. In that case, we could probably remove the explicit requirement to namespace predefined postback parameters as it will then happen "automatically" in request parameter map. I think it is good to have the requirement that the namespace prefix be written as part of the name attribute of hidden fields. However, if we added the map requirements I mentioned, then line 204 of RenderKitUtils.java would not need to prepend the namespacePrefix.
          Hide
          balusc added a comment -

          In my testing of line 495 using the JS debugger, clientIds[i] is a JavaScript array of length 1 and the value of clientIds[0] is simply 'a' (it is not namespaced). I can provide you with a downloadable Pluto 3 ZIP if you'd like to try it.

          Is the portlet rendering non-namespaced client IDs? This is confusing. I'd like to see the ZIP and try it, yes.

          I think it is good to have the requirement that the namespace prefix be written as part of the name attribute of hidden fields. However, if we added the map requirements I mentioned, then line 204 of RenderKitUtils.java would not need to prepend the namespacePrefix.

          I'd prefer consistency over convenience. Otherwise things will be too confusing in long term. If the UIViewRoot is an instance of NamingContainer, it's expected that its ID is prefixed over all place.

          Show
          balusc added a comment - In my testing of line 495 using the JS debugger, clientIds [i] is a JavaScript array of length 1 and the value of clientIds [0] is simply 'a' (it is not namespaced). I can provide you with a downloadable Pluto 3 ZIP if you'd like to try it. Is the portlet rendering non-namespaced client IDs? This is confusing. I'd like to see the ZIP and try it, yes. I think it is good to have the requirement that the namespace prefix be written as part of the name attribute of hidden fields. However, if we added the map requirements I mentioned, then line 204 of RenderKitUtils.java would not need to prepend the namespacePrefix. I'd prefer consistency over convenience. Otherwise things will be too confusing in long term. If the UIViewRoot is an instance of NamingContainer, it's expected that its ID is prefixed over all place.
          Hide
          balusc added a comment - - edited

          While working on https://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1372 I think I discovered your problem: invalid client IDs in f:ajax execute/render are not namespaced. I could reproduce it when replacing e.g. render="validClientId" by render="thisDoesNotExist". Can you please reverify?

          Show
          balusc added a comment - - edited While working on https://java.net/jira/browse/JAVASERVERFACES_SPEC_PUBLIC-1372 I think I discovered your problem: invalid client IDs in f:ajax execute/render are not namespaced. I could reproduce it when replacing e.g. render="validClientId" by render="thisDoesNotExist" . Can you please reverify?

            People

            • Assignee:
              balusc
              Reporter:
              werpu12
            • Votes:
              59 Vote for this issue
              Watchers:
              43 Start watching this issue

              Dates

              • Created:
                Updated:

                Time Tracking

                Estimated:
                Original Estimate - 5 days
                5d
                Remaining:
                Remaining Estimate - 5 days
                5d
                Logged:
                Time Spent - Not Specified
                Not Specified