Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ClientWindow: Custom PrimeClientWindowFactory not being respected #5316

Closed
melloware opened this issue Sep 17, 2023 · 7 comments
Closed

ClientWindow: Custom PrimeClientWindowFactory not being respected #5316

melloware opened this issue Sep 17, 2023 · 7 comments

Comments

@melloware
Copy link
Contributor

melloware commented Sep 17, 2023

Description

Mojarra Version: 4.0.4

Original Issue reported for 2.3.21: #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: primefaces/primefaces#10444

PrimeFaces current PR: primefaces/primefaces#10449

Expected behavior

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

@BalusC
Copy link
Contributor

BalusC commented Sep 23, 2023

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
Copy link
Contributor

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
Copy link
Contributor

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
Copy link
Contributor

Yep please 🙏

@mnriem
Copy link
Contributor

mnriem commented Sep 29, 2023

@melloware Please test @BalusC @arjantijms Please review

@melloware
Copy link
Contributor Author

I will test it out.

@mnriem mnriem added the 5.0 label Sep 30, 2023
@BalusC
Copy link
Contributor

BalusC commented Sep 30, 2023

Excellent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants