eclipse-ee4j / mojarra

Mojarra, a Jakarta Faces implementation
Other
160 stars 109 forks source link

ClientWindow: Custom PrimeClientWindowFactory not being respected #5316

Closed melloware closed 1 year ago

melloware commented 1 year ago

Description

Mojarra Version: 4.0.4

Original Issue reported for 2.3.21: https://github.com/eclipse-ee4j/mojarra/issues/5297

PrimeFaces has its own PrimeClientWindow and by overriding the client window factory like this:

<client-window-factory>org.primefaces.clientwindow.PrimeClientWindowFactory</client-window-factory>

This works fine in MyFaces but in Mojarra it does not pick up. To turn it on we have to enable in web.xml

    <context-param>
        <param-name>jakarta.faces.CLIENT_WINDOW_MODE</param-name>
        <param-value>url</param-value>
    </context-param>

Reproducer: Attached is a reproducer simply test it yourself with. pf-client-window-factory.zip

mvn clean jetty:run -Pmojarra40 FAILS mvn clean jetty:run -Pmyfaces40 WORKS

When you navigate to http://localhost:8080/primefaces-test/ this is the result:

Mojarra 4.0.4: http://localhost:8080/primefaces-test/ MyFaces 4.0.1: http://localhost:8080/primefaces-test/test.xhtml?jfwid=4a6b4

You can see the proper Window ID is added to the URL.

We do NOT have to add jakarta.faces.CLIENT_WINDOW_MODE for MyFaces as it overrides the default ClientWindowFactory with our own. It seems like in Mojarra it is still loading the default ClientWindowFactory first and then overriding with our custom one thus the need to for the web.xml param.

PrimeFaces Issue: https://github.com/primefaces/primefaces/issues/10444

PrimeFaces current PR: https://github.com/primefaces/primefaces/pull/10449

Expected behavior

When I enable PrimeClientWindow it activates WITHOUT the need to set jakarta.faces.CLIENT_WINDOW_MODE

BalusC commented 1 year ago

The spec says this: https://jakarta.ee/specifications/platform/10/apidocs/jakarta/faces/lifecycle/clientwindow

The generation of ClientWindow is controlled by the value of the context-param named by the value of CLIENT_WINDOW_MODE_PARAM_NAME. If this context-param is not specified, or its value is "none", no ClientWindow instances will be generated, and the entire feature is effectively disabled for the entire application.

It doesn't say it should be auto-activated irrespective the context param when a custom client window factory is present.

@tandraschko, what's your view on this?

tandraschko commented 1 year ago

IMO for custom ClientWindow/Factory it doesnt make sense to rely on the CLIENT_WINDOW_MODE_PARAM_NAME, you would like to provide a own factory and read your own config params thats how we would migrate deltaspike clientwindow modes for Faces4

i would suggest that the impl should always, indepedent of any config, ask ClientWindowFactory to get a ClientWindow or null. the impl ClientWindowFactory should ready CLIENT_WINDOW_MODE_PARAM_NAME and decide to return a client-window for the current request or not. MyFaces even introduced own modes and behaves exactly like this: https://github.com/apache/myfaces/blob/df17470c6a90c42c3554138ec587e3e04f3d6cf9/impl/src/main/java/org/apache/myfaces/lifecycle/clientwindow/ClientWindowFactoryImpl.java#L33

arjantijms commented 1 year ago

IMO for custom ClientWindow/Factory it doesnt make sense to rely on the CLIENT_WINDOW_MODE_PARAM_NAME, you would like to provide a own factory and read your own config params

Sounds reasonable. I think we should adopt that behaviour for Mojarra 4.0.5, and then clarify the specification?

tandraschko commented 1 year ago

Yep please 🙏

mnriem commented 1 year ago

@melloware Please test @BalusC @arjantijms Please review

melloware commented 1 year ago

I will test it out.

BalusC commented 1 year ago

Excellent.