ICEfaces
  1. ICEfaces
  2. ICE-7244

MyFaces 2: component ids with datatables include row index twice

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 2.0.2
    • Fix Version/s: 3.0.RC2, 3.0
    • Component/s: Framework, ICE-Components
    • Labels:
      None
    • Environment:
      MyFaces 2 ICEfaces 2 datatables
    • Assignee Priority:
      P1
    • Affects:
      Compatibility/Configuration

      Description

      When running under MyFaces, components within datatables are being assigned ids where the row index appears to be appended twice. So for example, in Mojarra where an element within a datatable might be identified as;

      <span id="iceform:datTbl:2:number" class="iceOutTxt">1</span>

      in MyFaces, the same element is identified as:

      <span id="iceform:datTbl:2:2:number" class="iceOutTxt">1</span>

      While it doesn't appear to strictly break any application functionality that I can identify, it does cause distress to many of our Selenium tests where we attempt to find elements within a datatable.
      1. ICE-7244.patch
        10 kB
        Deryk Sinotte
      1. datatable-ids.png
        501 kB

        Issue Links

          Activity

          Hide
          Deryk Sinotte added a comment - - edited

          Added some logging to the OutputTextRender for a small table. It clearly shows that the call to getClientId() from the component is returning the row index twice:

          MyFaces logging:

          OutputTextRenderer.encodeBegin: iceform:datTbl:0:0:number from com.icesoft.faces.component.ext.HtmlOutputText@f57e027
          OutputTextRenderer.encodeBegin: iceform:datTbl:1:1:number from com.icesoft.faces.component.ext.HtmlOutputText@f57e027
          OutputTextRenderer.encodeBegin: iceform:datTbl:2:2:number from com.icesoft.faces.component.ext.HtmlOutputText@f57e027

          Mojarra logging:

          OutputTextRenderer.encodeBegin: iceform:datTbl:0:number from com.icesoft.faces.component.ext.HtmlOutputText@3154bfa8
          OutputTextRenderer.encodeBegin: iceform:datTbl:1:number from com.icesoft.faces.component.ext.HtmlOutputText@3154bfa8
          OutputTextRenderer.encodeBegin: iceform:datTbl:2:number from com.icesoft.faces.component.ext.HtmlOutputText@3154bfa8

          Show
          Deryk Sinotte added a comment - - edited Added some logging to the OutputTextRender for a small table. It clearly shows that the call to getClientId() from the component is returning the row index twice: MyFaces logging: OutputTextRenderer.encodeBegin: iceform:datTbl:0:0:number from com.icesoft.faces.component.ext.HtmlOutputText@f57e027 OutputTextRenderer.encodeBegin: iceform:datTbl:1:1:number from com.icesoft.faces.component.ext.HtmlOutputText@f57e027 OutputTextRenderer.encodeBegin: iceform:datTbl:2:2:number from com.icesoft.faces.component.ext.HtmlOutputText@f57e027 Mojarra logging: OutputTextRenderer.encodeBegin: iceform:datTbl:0:number from com.icesoft.faces.component.ext.HtmlOutputText@3154bfa8 OutputTextRenderer.encodeBegin: iceform:datTbl:1:number from com.icesoft.faces.component.ext.HtmlOutputText@3154bfa8 OutputTextRenderer.encodeBegin: iceform:datTbl:2:number from com.icesoft.faces.component.ext.HtmlOutputText@3154bfa8
          Hide
          Deryk Sinotte added a comment -

          Attached a screenshot showing side-by-side Firefox windows. On the left is the MyFaces page and the id of the element in the datatable that has the double row index. On the right is Mojarra where there is only the single row index value.

          Show
          Deryk Sinotte added a comment - Attached a screenshot showing side-by-side Firefox windows. On the left is the MyFaces page and the id of the element in the datatable that has the double row index. On the right is Mojarra where there is only the single row index value.
          Hide
          Deryk Sinotte added a comment -

          Observation about the UISeries.getClientID() method:

          public String getClientId(FacesContext context) {
          if (context == null)

          { throw new NullPointerException(); }

          String baseClientId = super.getClientId(context);
          if (getRowIndex() >= 0) {
          //this extra if is to produce the same ids among myfaces and sunri
          //myfaces uses the getRowIndex() and SunRI directly using the rowIndex
          //variable inside its getClientId()
          if (!baseClientId.endsWith(
          "" + NamingContainer.SEPARATOR_CHAR + getRowIndex()))

          { return (baseClientId + NamingContainer.SEPARATOR_CHAR + getRowIndex()); }

          return (baseClientId);
          } else

          { return (baseClientId); }

          }

          However, it appears that this code is being hit in both MyFaces and Mojarra. So further debugging showed the following method in the MyFaces implementation of javax.faces.component.UIData:

          @Override
          public String getContainerClientId(FacesContext context)
          {
          //MYFACES-2744 UIData.getClientId() should not append rowIndex, instead use UIData.getContainerClientId()
          String clientId = super.getContainerClientId(context);

          int rowIndex = getRowIndex();
          if (rowIndex == -1)

          { return clientId; }

          StringBuilder bld = __getSharedStringBuilder(context);
          return bld.append(clientId).append(UINamingContainer.getSeparatorChar(context)).append(rowIndex).toString();
          }

          The comment points to this case where the issue was originally discussed: https://issues.apache.org/jira/browse/MYFACES-2744.

          Show
          Deryk Sinotte added a comment - Observation about the UISeries.getClientID() method: public String getClientId(FacesContext context) { if (context == null) { throw new NullPointerException(); } String baseClientId = super.getClientId(context); if (getRowIndex() >= 0) { //this extra if is to produce the same ids among myfaces and sunri //myfaces uses the getRowIndex() and SunRI directly using the rowIndex //variable inside its getClientId() if (!baseClientId.endsWith( "" + NamingContainer.SEPARATOR_CHAR + getRowIndex())) { return (baseClientId + NamingContainer.SEPARATOR_CHAR + getRowIndex()); } return (baseClientId); } else { return (baseClientId); } } However, it appears that this code is being hit in both MyFaces and Mojarra. So further debugging showed the following method in the MyFaces implementation of javax.faces.component.UIData: @Override public String getContainerClientId(FacesContext context) { //MYFACES-2744 UIData.getClientId() should not append rowIndex, instead use UIData.getContainerClientId() String clientId = super.getContainerClientId(context); int rowIndex = getRowIndex(); if (rowIndex == -1) { return clientId; } StringBuilder bld = __getSharedStringBuilder(context); return bld.append(clientId).append(UINamingContainer.getSeparatorChar(context)).append(rowIndex).toString(); } The comment points to this case where the issue was originally discussed: https://issues.apache.org/jira/browse/MYFACES-2744 .
          Hide
          Deryk Sinotte added a comment -

          So it appears that by fixing https://issues.apache.org/jira/browse/MYFACES-2744, we no longer need the workaround that is included in the UISeries.getClientID() method. I removed that method altogether to prevent it overriding the normal processing and it appears to work in both MyFaces and Mojarra.

          Not sure of the entire risk around this. It looks like it was originally put in to handle a MyFaces shortcoming that has now been fixed so hopefully there are no undue side effects.

          Show
          Deryk Sinotte added a comment - So it appears that by fixing https://issues.apache.org/jira/browse/MYFACES-2744 , we no longer need the workaround that is included in the UISeries.getClientID() method. I removed that method altogether to prevent it overriding the normal processing and it appears to work in both MyFaces and Mojarra. Not sure of the entire risk around this. It looks like it was originally put in to handle a MyFaces shortcoming that has now been fixed so hopefully there are no undue side effects.
          Hide
          Deryk Sinotte added a comment -

          So removing that method fixed the problem with the components in the table but now it looks like the ids for the tr elements is missing the row index value. Investigating further.

          Show
          Deryk Sinotte added a comment - So removing that method fixed the problem with the components in the table but now it looks like the ids for the tr elements is missing the row index value. Investigating further.
          Hide
          Deryk Sinotte added a comment -

          Looks like the logic that was used in the UISeries.getClientId() method was interfering with the id generation for components within the datatable. It was originally there to work around a difference between MyFaces and Mojarra. However, it appears that the id generation for 'tr' elements with row selectors might have been relying on that extra work. So I copied the gist of the code into the com.icesoft.faces.component.ext.renderkit.TableRenderer.encodeChildren() method to ensure that the row index is added when needed:

          if (rowIndex >= 0) {
          if (!id.endsWith("" + NamingContainer.SEPARATOR_CHAR + rowIndex))

          { id = id + NamingContainer.SEPARATOR_CHAR + rowIndex; }

          }

          Tested against the ICE-1445 nightly regression test and MyFaces and Mojarra both show the same behaviour now.

          Show
          Deryk Sinotte added a comment - Looks like the logic that was used in the UISeries.getClientId() method was interfering with the id generation for components within the datatable. It was originally there to work around a difference between MyFaces and Mojarra. However, it appears that the id generation for 'tr' elements with row selectors might have been relying on that extra work. So I copied the gist of the code into the com.icesoft.faces.component.ext.renderkit.TableRenderer.encodeChildren() method to ensure that the row index is added when needed: if (rowIndex >= 0) { if (!id.endsWith("" + NamingContainer.SEPARATOR_CHAR + rowIndex)) { id = id + NamingContainer.SEPARATOR_CHAR + rowIndex; } } Tested against the ICE-1445 nightly regression test and MyFaces and Mojarra both show the same behaviour now.
          Hide
          Deryk Sinotte added a comment -

          Seems like some of the compat components are relying on the UISeries adding the row index so I tried re-instating the method but simply putting a check for MyFaces and bypassing the logic if we are running under MyFaces:

          /**

          • @see javax.faces.component.UIData#getClientId(FacesContext)
            */
            public String getClientId(FacesContext context) {
            if (context == null) { throw new NullPointerException(); }

          //MyFaces no longer requires the special row index logic in this method but it seems
          //some compat components still rely on it.
          if(EnvUtils.isMyFaces())

          { return super.getClientId(context); }

          String baseClientId = super.getClientId(context);
          if (getRowIndex() >= 0) {
          //this extra if is to produce the same ids among myfaces and sunri
          //myfaces uses the getRowIndex() and SunRI directly using the rowIndex
          //variable inside its getClientId()
          if (!baseClientId.endsWith(
          "" + NamingContainer.SEPARATOR_CHAR + getRowIndex()))

          { return (baseClientId + NamingContainer.SEPARATOR_CHAR + getRowIndex()); }

          return (baseClientId);
          } else

          { return (baseClientId); }

          }

          While this is fine for some of the issues, there is still a problem with UIColumns (at least). When you look at the generated ids for each implementation:

          MyFaces:
          <span id="iceForm:dataTbl:iceColumns:00:0:j_id1623796847_60c92cbf" class="iceOutTxt">1</span>

          Mojarra:
          <span id="iceForm:dataTbl:iceColumns:0:00:j_idt8" class="iceOutTxt">1</span>

          Note the inverted order of the column identifier.

          Show
          Deryk Sinotte added a comment - Seems like some of the compat components are relying on the UISeries adding the row index so I tried re-instating the method but simply putting a check for MyFaces and bypassing the logic if we are running under MyFaces: /** @see javax.faces.component.UIData#getClientId(FacesContext) */ public String getClientId(FacesContext context) { if (context == null) { throw new NullPointerException(); } //MyFaces no longer requires the special row index logic in this method but it seems //some compat components still rely on it. if(EnvUtils.isMyFaces()) { return super.getClientId(context); } String baseClientId = super.getClientId(context); if (getRowIndex() >= 0) { //this extra if is to produce the same ids among myfaces and sunri //myfaces uses the getRowIndex() and SunRI directly using the rowIndex //variable inside its getClientId() if (!baseClientId.endsWith( "" + NamingContainer.SEPARATOR_CHAR + getRowIndex())) { return (baseClientId + NamingContainer.SEPARATOR_CHAR + getRowIndex()); } return (baseClientId); } else { return (baseClientId); } } While this is fine for some of the issues, there is still a problem with UIColumns (at least). When you look at the generated ids for each implementation: MyFaces: <span id="iceForm:dataTbl:iceColumns:00:0:j_id1623796847_60c92cbf" class="iceOutTxt">1</span> Mojarra: <span id="iceForm:dataTbl:iceColumns:0:00:j_idt8" class="iceOutTxt">1</span> Note the inverted order of the column identifier.
          Hide
          Deryk Sinotte added a comment -

          This logic is also not working for tab set:

          MyFaces:
          <td style="vertical-align:top;" id="iceform:dynamicTabSetht0" class="icePnlTb">

          Mojarra:
          <td style="vertical-align:top;" id="iceform:dynamicTabSet:0ht0" class="icePnlTb">

          MyFaces is missing the ":0" near the end.

          It's possible that applications running with these different identifier schemes, it may produce problems with certain updates and definitely causes problems with Selenium test scripts.

          Show
          Deryk Sinotte added a comment - This logic is also not working for tab set: MyFaces: <td style="vertical-align:top;" id="iceform:dynamicTabSetht0" class="icePnlTb"> Mojarra: <td style="vertical-align:top;" id="iceform:dynamicTabSet:0ht0" class="icePnlTb"> MyFaces is missing the ":0" near the end. It's possible that applications running with these different identifier schemes, it may produce problems with certain updates and definitely causes problems with Selenium test scripts.
          Hide
          Deryk Sinotte added a comment - - edited

          I'm apparently confusing myself thoroughly here. Looks like there is a difference between the id generation for the table itself and the ids of the components inside the table. If I put the processing back to the way it was before I started fiddling with it, there is some additional detail that I can provide.

          Running the row selection example of component showcase, I can see the following:

          With MyFaces, the "tr" element has the correct id but the actual component that is spitting out the value does not (double row index).

          <tr id="iceform:employeeList:0"...
          <span id="iceform:employeeList:0:0:Number" class="iceOutTxt">140</span>

          With Mojarra, the element is correctly identified:

          <tr id="iceform:employeeList:0"...
          <span id="iceform:employeeList:0:Number" class="iceOutTxt">140</span>

          If I put the "fix" back in and disable the extra row index processing, then with MyFaces the "tr" element is incorrectly id'ed but the component is fine.

          <tr id="iceform:employeeList"...
          <span id="iceform:employeeList:0:Number" class="iceOutTxt">140</span>

          Mojarra is fine in this case as it doesn't rely on the logic either way.

          <tr id="iceform:employeeList:0"...
          <span id="iceform:employeeList:0:Number" class="iceOutTxt">140</span>

          Other id anomalies related to the two different JSF implementations occur regardless of whether or not the extra rowIndex processing is done. These include (but may not be limited to) the following.

          Looking at the dynamic panel tab set:

          MyFaces is missing the row index:
          <a id="iceform:icePnlTbSet.1"...

          By Mojarra is fine:
          <a id="iceform:icePnlTbSet:1.1"...

          And tables with UIColumns have the values reversed:

          MyFaces
          <div id="iceform:dataTbl:columnDataModel:00:0:j_id643716342_a15a6a5"
          <span id="iceform:dataTbl:columnDataModel:00:0:cellValue" class="iceOutTxt">A</span></div>

          Mojarra has the values reversed (:0:00 rather than :00:0)

          <div id="iceform:dataTbl:columnDataModel:0:00:j_idt278"
          <span id="iceform:dataTbl:columnDataModel:0:00:cellValue" class="iceOutTxt">A</span></div>

          Show
          Deryk Sinotte added a comment - - edited I'm apparently confusing myself thoroughly here. Looks like there is a difference between the id generation for the table itself and the ids of the components inside the table. If I put the processing back to the way it was before I started fiddling with it, there is some additional detail that I can provide. Running the row selection example of component showcase, I can see the following: With MyFaces, the "tr" element has the correct id but the actual component that is spitting out the value does not (double row index). <tr id="iceform:employeeList:0"... <span id="iceform:employeeList:0:0:Number" class="iceOutTxt">140</span> With Mojarra, the element is correctly identified: <tr id="iceform:employeeList:0"... <span id="iceform:employeeList:0:Number" class="iceOutTxt">140</span> If I put the "fix" back in and disable the extra row index processing, then with MyFaces the "tr" element is incorrectly id'ed but the component is fine. <tr id="iceform:employeeList"... <span id="iceform:employeeList:0:Number" class="iceOutTxt">140</span> Mojarra is fine in this case as it doesn't rely on the logic either way. <tr id="iceform:employeeList:0"... <span id="iceform:employeeList:0:Number" class="iceOutTxt">140</span> Other id anomalies related to the two different JSF implementations occur regardless of whether or not the extra rowIndex processing is done. These include (but may not be limited to) the following. Looking at the dynamic panel tab set: MyFaces is missing the row index: <a id="iceform:icePnlTbSet.1"... By Mojarra is fine: <a id="iceform:icePnlTbSet:1.1"... And tables with UIColumns have the values reversed: MyFaces <div id="iceform:dataTbl:columnDataModel:00:0:j_id643716342_a15a6a5" <span id="iceform:dataTbl:columnDataModel:00:0:cellValue" class="iceOutTxt">A</span></div> Mojarra has the values reversed (:0:00 rather than :00:0) <div id="iceform:dataTbl:columnDataModel:0:00:j_idt278" <span id="iceform:dataTbl:columnDataModel:0:00:cellValue" class="iceOutTxt">A</span></div>
          Hide
          Deryk Sinotte added a comment -

          Have to revert the fix where MyFaces skipped the extra processing as it breaks worse than not having it in.

          Show
          Deryk Sinotte added a comment - Have to revert the fix where MyFaces skipped the extra processing as it breaks worse than not having it in.
          Hide
          Ken Fyten added a comment -

          [17:05:55] - svn commit -m "ICE-7244 / ICE-7291 - Revert change to ExtendedExceptionHandler that causes View cannot be restored error to appear when session expires." 1 item(s)
          [17:05:55] - Sending /Users/Ken/Code/svn/grimlock/icefaces/core/src/main/java/org/icefaces/impl/application/ExtendedExceptionHandler.java
          [17:05:55] - Transmitting file data.
          [17:05:56] - Refreshing files state...
          [17:05:56] - Refresh done
          [17:05:56] - Committed revision 25788

          Show
          Ken Fyten added a comment - [17:05:55] - svn commit -m " ICE-7244 / ICE-7291 - Revert change to ExtendedExceptionHandler that causes View cannot be restored error to appear when session expires." 1 item(s) [17:05:55] - Sending /Users/Ken/Code/svn/grimlock/icefaces/core/src/main/java/org/icefaces/impl/application/ExtendedExceptionHandler.java [17:05:55] - Transmitting file data. [17:05:56] - Refreshing files state... [17:05:56] - Refresh done [17:05:56] - Committed revision 25788
          Hide
          Deryk Sinotte added a comment -

          Linking to parent MyFaces case.

          Show
          Deryk Sinotte added a comment - Linking to parent MyFaces case.
          Hide
          Deryk Sinotte added a comment -

          As per Mark's request, I did a quick and dirty analysis of the essential code delta between Mojarra and MyFaces - particularly in how they implement getClientId(), getContainerClientId() in UIComponent / UIComponentBase / UIData. In short, in UIData and UIRepeat the implementations diverge on overriding and using getClientId (Mojarra) and getContainerClientId (MyFaces) to hold the logic for determining row indexes and such.

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

          UIComponent.getClientId and getContainerClientId

          Mojarra/MyFaces do the same basic things with these methods:

          public String getClientId()

          { return getClientId(getFacesContext()); }

          public abstract String getClientId(FacesContext context);

          public String getContainerClientId(FacesContext context) {
          if (context == null)

          { throw new NullPointerException(); }

          return this.getClientId(context);
          }

          UIComponentBase.getClientId

          Mojarra/MyFaces basic client id determination work is done:

          • return client id if already known, otherwise calculate it
          • they both rely on finding UniqueIdVendor and the parent NamingContainer to get this done but the way they go about it is different
          • it's not obvious to me that the differences would cause the problem we are seeing

          UIComponentBase.getContainerClientId

          Mojarra/MyFaces defer to UIComponent implementation.

          UIData.getClientId

          Mojarra: overrides UIComponentBase

          This is a lengthy method which builds up the client id by taking into account the row index and whether or not the component is nested withing another UIData component.

          MyFaces: defers to UIComponentBase however it used to override it:

          /*
          @Override
          public String getClientId(FacesContext context)
          {
          String clientId = super.getClientId(context);
          int rowIndex = getRowIndex();
          if (rowIndex == -1)

          { return clientId; }

          StringBuilder bld = __getSharedStringBuilder();
          return bld.append(clientId).append(UINamingContainer.getSeparatorChar(context)).append(rowIndex).toString();
          }*/

          but has now been commented out - the responsibility is now with the getContainerClientId method (see next section)

          UIData.getContainerClientId

          Mojarra: defers to UIComponent

          MyFaces: overrides UIComponent

          The method is short and has a potentially relevant comment:

          @Override
          public String getContainerClientId(FacesContext context)
          {
          //MYFACES-2744 UIData.getClientId() should not append rowIndex, instead use UIData.getContainerClientId()
          String clientId = super.getContainerClientId(context);

          int rowIndex = getRowIndex();
          if (rowIndex == -1)

          { return clientId; }

          StringBuilder bld = __getSharedStringBuilder(context);
          return bld.append(clientId).append(UINamingContainer.getSeparatorChar(context)).append(rowIndex).toString();
          }

          UIRepeat.getClientId

          Mojarra: overrides UIComponentBase

          public String getClientId(FacesContext faces) {
          String id = super.getClientId(faces);
          if (this.index >= 0)

          { id = this.getBuffer().append(id).append( getSeparatorChar(faces)).append(this.index) .toString(); }

          return id;
          }

          MyFaces: defers to UIComponentBase

          UIRepeat.getContainerClientId

          Mojarra: defers to UIComponent

          MyFaces: overrides UIComponent

          @Override
          public String getContainerClientId(FacesContext faces)
          {
          String id = super.getContainerClientId(faces);
          if (_index >= 0)

          { id = _getBuffer().append(id).append(UINamingContainer.getSeparatorChar(faces)).append(_index).toString(); }

          return id;
          }

          Show
          Deryk Sinotte added a comment - As per Mark's request, I did a quick and dirty analysis of the essential code delta between Mojarra and MyFaces - particularly in how they implement getClientId(), getContainerClientId() in UIComponent / UIComponentBase / UIData. In short, in UIData and UIRepeat the implementations diverge on overriding and using getClientId (Mojarra) and getContainerClientId (MyFaces) to hold the logic for determining row indexes and such. --------------------------------------------------------------------------------------------------------------------------------------------------- UIComponent.getClientId and getContainerClientId Mojarra/MyFaces do the same basic things with these methods: public String getClientId() { return getClientId(getFacesContext()); } public abstract String getClientId(FacesContext context); public String getContainerClientId(FacesContext context) { if (context == null) { throw new NullPointerException(); } return this.getClientId(context); } UIComponentBase.getClientId Mojarra/MyFaces basic client id determination work is done: return client id if already known, otherwise calculate it they both rely on finding UniqueIdVendor and the parent NamingContainer to get this done but the way they go about it is different it's not obvious to me that the differences would cause the problem we are seeing UIComponentBase.getContainerClientId Mojarra/MyFaces defer to UIComponent implementation. UIData.getClientId Mojarra: overrides UIComponentBase This is a lengthy method which builds up the client id by taking into account the row index and whether or not the component is nested withing another UIData component. MyFaces: defers to UIComponentBase however it used to override it: /* @Override public String getClientId(FacesContext context) { String clientId = super.getClientId(context); int rowIndex = getRowIndex(); if (rowIndex == -1) { return clientId; } StringBuilder bld = __getSharedStringBuilder(); return bld.append(clientId).append(UINamingContainer.getSeparatorChar(context)).append(rowIndex).toString(); }*/ but has now been commented out - the responsibility is now with the getContainerClientId method (see next section) UIData.getContainerClientId Mojarra: defers to UIComponent MyFaces: overrides UIComponent The method is short and has a potentially relevant comment: @Override public String getContainerClientId(FacesContext context) { //MYFACES-2744 UIData.getClientId() should not append rowIndex, instead use UIData.getContainerClientId() String clientId = super.getContainerClientId(context); int rowIndex = getRowIndex(); if (rowIndex == -1) { return clientId; } StringBuilder bld = __getSharedStringBuilder(context); return bld.append(clientId).append(UINamingContainer.getSeparatorChar(context)).append(rowIndex).toString(); } UIRepeat.getClientId Mojarra: overrides UIComponentBase public String getClientId(FacesContext faces) { String id = super.getClientId(faces); if (this.index >= 0) { id = this.getBuffer().append(id).append( getSeparatorChar(faces)).append(this.index) .toString(); } return id; } MyFaces: defers to UIComponentBase UIRepeat.getContainerClientId Mojarra: defers to UIComponent MyFaces: overrides UIComponent @Override public String getContainerClientId(FacesContext faces) { String id = super.getContainerClientId(faces); if (_index >= 0) { id = _getBuffer().append(id).append(UINamingContainer.getSeparatorChar(faces)).append(_index).toString(); } return id; }
          Hide
          Deryk Sinotte added a comment -

          Mark responded:

          "From the Mojarra mailing list, I believe they're migrating to the same approach as MyFaces, and moving that indexing logic into getContainerClientId(). So this might just fix itself in the next Mojarra release, without us needing to tweak out code to compensate for it.

          If we were to try to fix anything still, it should probably be to adopt the MyFaces and soon-to-be Mojarra solution, by overriding both getContainerClientId() and getClientId(), in UISeries, and have them do things from the ground up, so that they're not affected by their super-class' implementations. That would basically entail putting the UIComponentBase.getClientId() logic into UISeries.getClientId(), and making getContainerClientId() look like this:

          @Override
          public String getContainerClientId(FacesContext context)
          {
          // We can't use and super methods, since they vary on implementation
          String clientId = getClientId(context);

          int rowIndex = getRowIndex();
          if (rowIndex == -1)

          { return clientId; }

          StringBuilder bld = __getSharedStringBuilder(context);
          return bld.append(clientId).append(UINamingContainer.getSeparatorChar(context)).append(rowIndex).toString();
          }
          "

          Show
          Deryk Sinotte added a comment - Mark responded: "From the Mojarra mailing list, I believe they're migrating to the same approach as MyFaces, and moving that indexing logic into getContainerClientId(). So this might just fix itself in the next Mojarra release, without us needing to tweak out code to compensate for it. If we were to try to fix anything still, it should probably be to adopt the MyFaces and soon-to-be Mojarra solution, by overriding both getContainerClientId() and getClientId(), in UISeries, and have them do things from the ground up, so that they're not affected by their super-class' implementations. That would basically entail putting the UIComponentBase.getClientId() logic into UISeries.getClientId(), and making getContainerClientId() look like this: @Override public String getContainerClientId(FacesContext context) { // We can't use and super methods, since they vary on implementation String clientId = getClientId(context); int rowIndex = getRowIndex(); if (rowIndex == -1) { return clientId; } StringBuilder bld = __getSharedStringBuilder(context); return bld.append(clientId).append(UINamingContainer.getSeparatorChar(context)).append(rowIndex).toString(); } "
          Hide
          Deryk Sinotte added a comment -

          As per Mark's suggestion, I copied a bunch of MyFaces code into UISeries as outlined, then ran the Row Selection example in Component Showcase. The logging shows that both UISeries.getContainerClientId and UISeries.getClientId are being exercised and the resulting ids appear to be consistent between MyFaces and Mojarra. However, it looks to be missing one aspect of the id in the table row. Currently a row renders out as:

          <tr id="iceform:employeeList" ...
          <span id="iceform:employeeList:0:Number">...

          whereas before this change, Mojarra (which I'm using as the benchmark for this) used to do:

          <tr id="iceform:employeeList:0"...
          <span id="iceform:employeeList:0:Number"...

          So it appears that the row is simply missing the index at this point. Our latest theory is that there is a bunch of code in various components and renderers related to UISeries that need to be changed to use the getContainerClientId() method instead of getClientId().

          Show
          Deryk Sinotte added a comment - As per Mark's suggestion, I copied a bunch of MyFaces code into UISeries as outlined, then ran the Row Selection example in Component Showcase. The logging shows that both UISeries.getContainerClientId and UISeries.getClientId are being exercised and the resulting ids appear to be consistent between MyFaces and Mojarra. However, it looks to be missing one aspect of the id in the table row. Currently a row renders out as: <tr id="iceform:employeeList" ... <span id="iceform:employeeList:0:Number">... whereas before this change, Mojarra (which I'm using as the benchmark for this) used to do: <tr id="iceform:employeeList:0"... <span id="iceform:employeeList:0:Number"... So it appears that the row is simply missing the index at this point. Our latest theory is that there is a bunch of code in various components and renderers related to UISeries that need to be changed to use the getContainerClientId() method instead of getClientId().
          Hide
          Deryk Sinotte added a comment -

          After discussing with Ken and Mark, I'm re-assigning to Mark as this is something that the component team may need to tackle.

          Show
          Deryk Sinotte added a comment - After discussing with Ken and Mark, I'm re-assigning to Mark as this is something that the component team may need to tackle.
          Hide
          Deryk Sinotte added a comment -

          Attaching a patch of the changes I made to UISeries class to incorporate Mark's suggestions.

          Show
          Deryk Sinotte added a comment - Attaching a patch of the changes I made to UISeries class to incorporate Mark's suggestions.
          Hide
          Mark Collette added a comment -

          Sub-classes of UISeries would need to change, in certain places, their use of getClientId() to instead use getContainerClientId(), for this patch to work.

          Show
          Mark Collette added a comment - Sub-classes of UISeries would need to change, in certain places, their use of getClientId() to instead use getContainerClientId(), for this patch to work.
          Hide
          Mark Collette added a comment -

          The main change now is that a UIData's getClientId() should just follow the same behaviour as a non-UIData, and not include the rowIndex into the clientId anymore. While getContainerClientId() will take on that responsibility. This requires that sub-classes' rendering code make use of getContainerClientId() instead of getClientId() when rendering row specific information, and getClientId() when rendering stuff for the whole component.

          I took Deryk's patch, and adapted it a bit. Worked through the sub-classes of UISeries, searching for calls to getClientId(), modifying them as appropriate. Compat dataTable had a problem interoperating with compat columns, which columns was trying to work around by incorporating the dataTable's rowIndex into its own clientId calculations. Fixed that, so that only dataTable would have to be responsible for its rowIndex and not columns. And both would follow the new technique from UISeries.

          I tried fixing some little nit picking detail where panelTabSet in a non-UIData (not using value) scenario would still append ":0" to itself. But that ended up breaking a bunch of subtle things, so I ended up reverting all that.

          icefaces3
          Subversion 26886

          A regression was found where the RowSelector was broken, which I hadn't noticed since the showcase uses pre-styling. It turned out that there was code which expected clientId to have rowIndex in it, and was stripping that out, so when I made getClientId() to not have the rowIndex, then the stripping code removed other info. This code could be called from iterating and non-iterating code, so it just needed the stripping code to be removed, and to always use getClientId() instead of getContainerClientId() sometimes.

          It also highlighted how the search for getClientId() use had maybe not been thorough enough, since I had searched for UISeriesBase.getClientId() and in some cases it was being used as UIComponent.getClientId(), so I searched for a broader usage.

          icefaces3
          Subversion 26929

          Show
          Mark Collette added a comment - The main change now is that a UIData's getClientId() should just follow the same behaviour as a non-UIData, and not include the rowIndex into the clientId anymore. While getContainerClientId() will take on that responsibility. This requires that sub-classes' rendering code make use of getContainerClientId() instead of getClientId() when rendering row specific information, and getClientId() when rendering stuff for the whole component. I took Deryk's patch, and adapted it a bit. Worked through the sub-classes of UISeries, searching for calls to getClientId(), modifying them as appropriate. Compat dataTable had a problem interoperating with compat columns, which columns was trying to work around by incorporating the dataTable's rowIndex into its own clientId calculations. Fixed that, so that only dataTable would have to be responsible for its rowIndex and not columns. And both would follow the new technique from UISeries. I tried fixing some little nit picking detail where panelTabSet in a non-UIData (not using value) scenario would still append ":0" to itself. But that ended up breaking a bunch of subtle things, so I ended up reverting all that. icefaces3 Subversion 26886 A regression was found where the RowSelector was broken, which I hadn't noticed since the showcase uses pre-styling. It turned out that there was code which expected clientId to have rowIndex in it, and was stripping that out, so when I made getClientId() to not have the rowIndex, then the stripping code removed other info. This code could be called from iterating and non-iterating code, so it just needed the stripping code to be removed, and to always use getClientId() instead of getContainerClientId() sometimes. It also highlighted how the search for getClientId() use had maybe not been thorough enough, since I had searched for UISeriesBase.getClientId() and in some cases it was being used as UIComponent.getClientId(), so I searched for a broader usage. icefaces3 Subversion 26929

            People

            • Assignee:
              Mark Collette
              Reporter:
              Deryk Sinotte
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: