Fixing issue #4122

[abelet]

Fixing the issue descibed here: #4122

[tandraschko]

I would do a small change.

1. rowIsClosed = false per default
2. add rowIsClosed=true in line 146

WDYT?

[abelet]

I guess, if there is no row, just an empty tbody ( when grid.getChildren() is empty ), a closing tr will be placed on the output, which will be invalid again

[abelet]

I hope I understand well, what do you mean.

If I undertand well, the tdody will be always writen out, and the encodeDynamicBody will be always called, if the columns attribute is larger then 0. But it is not the actual number of elements in the PanelGrid component. So if you specify columns=2, and do not put any single component inside the PanelGrid, wrong output is generated, if you do your suggestions.

    public void encodeTableBody(FacesContext context, PanelGrid grid, int columns) throws IOException {
        ResponseWriter writer = context.getResponseWriter();

        writer.startElement("tbody", grid);

        if (columns > 0) {
            encodeDynamicBody(context, grid, grid.getColumns());
        }
        else {
            encodeStaticBody(context, grid);
        }

        writer.endElement("tbody");
    }

[tandraschko]

Not sure, i just don't like the naming probably. It feels not correct as the default value is true (and we don't know yet if the row will be closed really)

[abelet]

Maybe the name isn't perfect, but this variable is for signaling if we should close a row in the end or not. I’m quite sure that the modification you suggested would result a closing tr without an opening one, if there is no item in the panelGrid. I haven't tested it, but seeing the code, I'm quite certain.

[tandraschko]

WDYT about if we count the closing and opening trs?
and then do:

if (opening > closing) {
instead of
if (!rowIsClosed) {

I think it's much easier to read and to understand the actual logic.

[abelet]

Your suggestion seems right to me.

I’d like to ask your opinion on a situation.

The error in the panelGrid is obvious. But I’m even more concerned about the JSF 2.3 implementation, which I think is very dangerous with this incorrectly implemented tag renaming functionality. Besides my case there is a similar one (jboss/mojarra#21), and I’m sure as people starting to move towards JSF 2.3 it is likely that the number problems it causes will rise. I’m worried about the future magical errors it will cause in our systems. I opened an issue about this bug at the JSF project:

eclipse-ee4j/mojarra#4488

The JSF development team closed this issue despite the fact how vulnerable JSF become to component rendering problems, like this one we are about to fix. Do you think the behavior of the JSF implementation should be accepted?

[tandraschko]

I always have 2 opinions in such cases :D

1. as user:
   it would be great if Mojarra would be more safe

2. as framework dev (i'm also working on MyFaces):
   i can understand the Mojarra team and the component libs must produce correct html

I will apply my change now, hope you can retest it then.

[abelet]

The problem with MyFaces does not exists. Unfortunately wildfly uses mojarra.

[tandraschko]

Yes, i'm aware of that. But the goal should be that PrimeFaces always produces correct HTML and no "hacks/failsafe features" should be required in the JSF impls.

[abelet]

Actually there is no need for doing hacks. The HtmlResponseWriter’s endElement method do not receive the componentForElement parameter, and has no information about passthrough attributes, so the elementNames stack away was introduced as a hack, causing troubles. I have several ideas how to modify the current implementation to become safer. But my idea isn’t perfect, and I still hope the JSF team will solve this problem better than I can.
Using the JSF 2.3 implementation, which mixes the HTLM response up, the application developer won't understand what happened, what went wrong. For example @erickdeoliveiraleal
had to switch back to JSF 2.2.