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

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.

Observation about the UISeries.getClientID() method:

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

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);
} else

}

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)

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.

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.

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.

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))

}

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

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())

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);
} else

}

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.

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.

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>

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

[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

Linking to parent MyFaces case.

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()

public abstract String getClientId(FacesContext context);

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

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)

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)

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)

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)

return id;
}

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)

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

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().

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

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

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

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