Details

    • Type: Bug
    • Status: Closed
    • Priority: Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.0-beta-1
    • Fix Version/s: 2.1.0-core
    • Component/s: Components
    • Labels:
      None

      Description

      The tr:forEach tag has issues when trying to use component references and trying to keep the component state in sync with the items in a list. It also does not support Maps like the c:forEach tag does.

      I seek to improve the forEach tag in Trinidad, and propose that JSF/JSTL does something similar so that user's can really use the for each tag without issues. Some of the for each problems are described in my blog:
      http://drewdev.blogspot.com/2008/08/cforeach-with-jsf-could-ruin-your-day.html

      I propose to address each of the issues with the JSP tag.

      First, reliable component references:
      <tr:forEach var="item" items="#

      {myBean.items}">
      <tr:panelGroupLayout id="pgl1" layout="horizontal">
      <tr:inputText id="it1" label="Enter value:" value="#{item.text}" autoSubmit="true" />
      <tr:outputText id="ot1" value="Value is: #{item.text}" partialTriggers="it1" />
      </tr:panelGroupLayout>
      </tr:forEach>

      The problem is that the author intended the output text component to partially updated by the sibling input text when it changed value, but that is not the result. Instead, the partial triggers is always "it1", but the generated input text components are "ot1", "ot1j_id_1" and "ot1j_id_2". The result is that the output text components all partially update only when the first input text is changed.

      Another problem is that the indexed value expressions and the var status map that is put into the variable mapper by the for each loop is static. Take the following example:

      <tr:panelGroupLayout id="pgl1" layout="scroll">
      <tr:forEach var="item" items="#{myBean.items}

      " varStatus="vs">
      <tr:outputText id="ot1" value="This is the last item in the list: #

      {vs.last}

      . Item #

      {item.text}

      ." />
      </tr:forEach>
      </tr:panelGroupLayout>

      Say during the first pass, the items in the list are A, B and C and the page would look like this:

      This is the last item in the list: false. Item A.
      This is the last item in the list: false. Item B.
      This is the last item in the list: true. Item C.

      Now, consider what the output would be if someone added an object D to the end of the list during invoke application:

      This is the last item in the list: false. Item A.
      This is the last item in the list: false. Item B.
      This is the last item in the list: true. Item C.
      This is the last item in the list: true. Item D.

      Notice that both C and D think they are the last. The reason is that the UIComponentClassicTagBase will find the components generated for A, B and C during the findComponent call. It will only create a new component for D. When D is created, it will pick up the new variable status map created by the for each tag in its value expressions. Because when three earlier components were build, C was the last item, and the variable status map in their value expressions did not reflect any changes. D gets the correct values since it was just created.

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

      I propose to reduce any work required by page developers to implement the for each loop with re-ordering support (avoid having to use immediate EL in the ID attribute) and fix the issues above.

      The proposal is that Trinidad Tag based components (those components created by UIXComponentELTag) will be able to have key-based IDs automatically generated as a result of being in a for each tag. So consider this example:

      <tr:forEach var="item" items="#

      {bean.items}

      " varStatus="vs">
      <tr:inputText id="it1" value="#

      {item.value}" partialSubmit="true" />
      <tr:outputText id="ot1" value="Value is: #{item.value}

      "
      partialTriggers="it1_$

      {vs.key}

      " />
      </tr:forEach>

      In this code, the IDs of the components would automatically pick up the item key. The item key would be stored on the var status. For List this key would simply be the index, for Map it would be the map key and CollectionModel would use the row key. UIXComponentELTag could check to see if a parent tag desires to alter the component IDs, which in this case, the for each would. With that set, the tags would alter the component IDs to append the key. For example, "_" + key would be appended to each component ID (it1 would become it1_A if the key were "A").

      The for each tag would set the key into the variable mapper so that, instead of indexes, the key would be used to evaluate the EL with the limitation that the key would be the index for List, in which the current behavior would be retained of the component state staying with the index rather than with the object.

      Due to the fact that the JSP does not perform any component reordering, the org.apache.myfaces.trinidad.change.ReorderChildrenComponentChange component change class can be use to alter the component order and also notify the component change manager of the reordering. This would require users that wish to reorder the items in the list to ensure to both re-order the list as well as reorder the components by applying a component change.

      Changes to UIXComponentELTag

      Due to the fact that UIComponentClassicTagBase is a JSF class that we cannot modify, in order to fix the tr:forEach bugs, we need to work with what we can change. We will want to follow this up with proposed changes to the JSF and potentially the JSTL specifications to be able to make this type of functionality standard.

      Since UIXComponentELTag extends UIComponentClassicTagBase, it can call protected methods to alter the ID of the tag. With the altering of the tag ID, the UIComponentClassicTagBase will produce different component IDs. In order to pull this off, there needs to be a defined contract between the for each tag and the Trinidad component tags. This contract could be done with static methods on UIXComponentELTag:

      Proposed methods to be added to UIXComponentELTag
      /**

      • Push a component ID suffix onto the page context to append to component IDs generated by {@link UIXComponentELTag}

        .

      • This will append the suffix on all components up to a and including the text naming container.
      • @param pageContext the JSP page context
      • @param suffix the suffix to append to component IDs
        */
        public static void pushComponentIdSuffix(PageContext pageContext, String suffix)
        {
        ...
        }

      /**

      • Pop the last component ID suffix added to the page context.
      • @see #pushComponentIdSuffix(PageContext, String)
      • @param pageContext the JSP page context
        */
        public static void popComponentIdSuffix(PageContext pageContext)
        {
        ...
        }

      The ForEachTag would then call these methods before and after the body processing respectively. The idea would be that if a UIXComponentELTag created a naming container, it would clear any suffixes for children of the naming container. This is because the IDs would already be unique in the naming container, and there would be no reason to modify them, and it would also stop these changes from affecting any IDs inside of included pages. The push/pop method would be used to support a stack usage. This way, if nested for each tags are present, multiple suffixes would be added to ensure that the IDs in one for each tag would conflict with another's.

      As this would be a documented behavior, users could leverage this API to ensure that their component references (like the partialTriggers attribute) point to the correct component.

      Changes to ForEachTag
      The for each tag, with this proposal, would need to be modified to call the new methods of UIXComponentELTag in doStartTag and doAfterBody functions.

      The tag will need to change to provide dynamic value expressions for the var status and the var so that changes to the items will not break EL. This would involve using the key as a reference instead of an index for non list based items. The var status would need point to a component tree based attribute that could be updated in each request. For example, this would allow new items to be added, and the last attribute to correctly reflect that new state.

      The for each tag would be change to support Map and CollectionModel as well, and adding the key as a valid attribute on the varStatus object.

      Optionally skip this for List and array

      If we did not implement the ID suffixing for List (and arrays), existing applications would not be affected. This is one consideration to have to ensure that backwards compatibility is maintained. Another alternative is to use a web.xml configuration parameter and only introduce the ID suffixing when the web XML parameter has enabled it for lists and arrays.

        Issue Links

          Activity

          Hide
          arobinson74 Andrew Robinson added a comment -

          Okay, I'd like to tweak the design. A problem I found is that CollectionModel produces object row keys, not string, and a Map may not have a ID-friendly string key.

          I'd like to also add a new attribute on the tr:forEach tag – keyPassThru which is a boolean property that defaults to false. If true, the key from the collection model row key or the map would be used in the ID directly, without translation, using the toString() function. If false, the default, the forEach loop would create a token to represent the key (auto incrementing integer).

          As a result, I'd like to add two properties onto the varStatus, key and id. The former, key, would be the raw key from the model or collection model. The latter, id, would be the value used to suffix onto the component IDs. If keyPassThru is set to true, the key and ID would be the same.

          Example:
          <tr:forEach var="item" items="#

          {bean.collectionModel}" keyPassThru="true" varStatus="vs">
          <tr:outputText value="This is true: #{vs.key eq vs.id}" />
          </tr:forEach>

          <tr:forEach var="item" items="#{bean.collectionModel}

          " varStatus="vs">
          <tr:outputText value="This is false: #

          {vs.key eq vs.id}

          " />
          </tr:forEach>

          This also allows the code to use smaller ID suffixes and the keys could be non-ID friendly with this change.

          Show
          arobinson74 Andrew Robinson added a comment - Okay, I'd like to tweak the design. A problem I found is that CollectionModel produces object row keys, not string, and a Map may not have a ID-friendly string key. I'd like to also add a new attribute on the tr:forEach tag – keyPassThru which is a boolean property that defaults to false. If true, the key from the collection model row key or the map would be used in the ID directly, without translation, using the toString() function. If false, the default, the forEach loop would create a token to represent the key (auto incrementing integer). As a result, I'd like to add two properties onto the varStatus, key and id. The former, key, would be the raw key from the model or collection model. The latter, id, would be the value used to suffix onto the component IDs. If keyPassThru is set to true, the key and ID would be the same. Example: <tr:forEach var="item" items="# {bean.collectionModel}" keyPassThru="true" varStatus="vs"> <tr:outputText value="This is true: #{vs.key eq vs.id}" /> </tr:forEach> <tr:forEach var="item" items="#{bean.collectionModel} " varStatus="vs"> <tr:outputText value="This is false: # {vs.key eq vs.id} " /> </tr:forEach> This also allows the code to use smaller ID suffixes and the keys could be non-ID friendly with this change.
          Hide
          arobinson74 Andrew Robinson added a comment -

          I cannot fix this at the Trinidad level after looking into this more. The issue is with the JSF specification and there is no good way to fix this at the component tag level.

          Show
          arobinson74 Andrew Robinson added a comment - I cannot fix this at the Trinidad level after looking into this more. The issue is with the JSF specification and there is no good way to fix this at the component tag level.
          Hide
          arobinson74 Andrew Robinson added a comment -

          Re-opening. The issue I was having appears to be a myfaces-core issue, not a general issue

          Show
          arobinson74 Andrew Robinson added a comment - Re-opening. The issue I was having appears to be a myfaces-core issue, not a general issue
          Hide
          arobinson74 Andrew Robinson added a comment -

          Fixed and provided a demo in trinidad-demo (since it is a JSP only tag at the moment, I could not add it to trinidad-components-showcase). I also improved the tag documentation and mention what is and what is not supported.

          Show
          arobinson74 Andrew Robinson added a comment - Fixed and provided a demo in trinidad-demo (since it is a JSP only tag at the moment, I could not add it to trinidad-components-showcase). I also improved the tag documentation and mention what is and what is not supported.
          Hide
          arobinson74 Andrew Robinson added a comment -

          Found issues with the current code. Re-opening so that I can address them.

          Show
          arobinson74 Andrew Robinson added a comment - Found issues with the current code. Re-opening so that I can address them.

            People

            • Assignee:
              arobinson74 Andrew Robinson
              Reporter:
              arobinson74 Andrew Robinson
            • Votes:
              0 Vote for this issue
              Watchers:
              1 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development