[
https://issues.apache.org/jira/browse/MYFACES-2762?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12880634#action_12880634
]
Werner Punz edited comment on MYFACES-2762 at 6/20/10 9:57 AM:
---------------------------------------------------------------
Ok now to the patch, most of it definitely looks correct, the viewstate has to
be applied to all forms which have changes (also even outer forms) elements so
applying it only to elements which have a form embedded like I did before is
not entirely correct, in fact the change trace should step up until the parent
form if it does not embed one or more forms.
secondly:
if (!document.forms || !document.forms.length) {
return null;
- } else if (1 == document.forms.length) {
- return document.forms[0];
}
+ // This will not work well on portlet case, because we cannot be sure
+ // the returned form is right one.
+ //else if (1 == document.forms.length) {
+ // return document.forms[0];
+ //}
if (!elem) {
return null;
}
This is only a fallback routine in case of an element cannot be traced back to
an original form, so your fix here, does not really solve a lot of things
because a cross portlet submit is not possible and we cannot issue a submit
unless we have a form issueing it.
I will make it differently here, I will make that one configurable, by pushing
a new optional environment variable:
myfaces_config.no_portlet_env
if this one is set to true we can use the code (it really helps in detachement
cases so it makes sense)
On a sidenote, since 90% of all cases are non portlet it should be vice versa,
but given how intrusive history of portlets I cannot really enforce that on
portlet users to set a special param to switch this off. Especially since
portlets should work without additional config params on code level.
I personally think that the portlet guys should fix their api also in regards
to javascript to allow a portlet detection within the javascripts. We should
definitely work towards such a case at least from the myfaces side. The reason
simply is, that portlets are an entirely different beast environmentally and we
should also have access to the viewroot identifier etc.. in case of a portlet
and should know that we are in a portlet environment everything else will end
up in hacks.
As for the rest of the changes they make a lot of sense and utilize the new
apis really well. Originally I wanted to push both change lists into one data
structure but now thinking about it it makes more sense to take the entire
patchset as it is.
One minor gripe, call me nitpicky, the last change to the delete, but I do not
want to have direct dom manipulating routines in the response, everything dom
manipulating has to go through dom hence I will rework the last change. This
costs a little bit of performance, but I prefer a structured separation of
concerns to the last inch of performance. So I will not really use that change.
The performance cost in that case is nullified by the browser optimizations we
have on the dom walking side.
Nevertheless I will apply all of the patches and rework from there a little
bit, thanks a lot you saved me a load of work here, you really found a huge
issue here and also added code from the portlet case handling.
was (Author: werpu):
Ok now to the patch, most of it definitely looks correct, the viewstate has
to be applied to all forms which have changes (also even outer forms) elements
so applying it only to elements which have a form embedded like I did before is
not entirely correct, in fact the change trace should step up until the parent
form if it does not embed one or more forms.
secondly:
if (!document.forms || !document.forms.length) {
return null;
- } else if (1 == document.forms.length) {
- return document.forms[0];
}
+ // This will not work well on portlet case, because we cannot be sure
+ // the returned form is right one.
+ //else if (1 == document.forms.length) {
+ // return document.forms[0];
+ //}
if (!elem) {
return null;
}
This is only a fallback routine in case of an element cannot be traced back to
an original form, so your fix here, does not really solve a lot of things
because a cross portlet submit is not possible and we cannot issue a submit
unless we have a form issuing it.
As for the rest of the changes they make a lot of sense although I would not
use two data structures for the change trace, I would push it into one list.
One minor gripe, call me nitpicky, the last change to the delete, but I do not
want to have direct dom manipulating routines in the response, everything dom
manipulating has to go through dom hence I will rework the last change. This
costs a little bit of performance, but I prefer a structured separation of
concerns to the last inch of performance. So I will not really use that change.
Nevertheless I will apply all of the patches and rework from there a little
bit, thanks a lot you saved me a load of work here, you really found a huge
issue here and also added code from the portlet case handling.
> Handling mutiple h:form and AJAX is broken
> ------------------------------------------
>
> Key: MYFACES-2762
> URL: https://issues.apache.org/jira/browse/MYFACES-2762
> Project: MyFaces Core
> Issue Type: Bug
> Components: JSR-314
> Affects Versions: 2.0.1-SNAPSHOT
> Environment: myfaces current trunk (19.6.2010)
> Reporter: Martin Kočí
> Priority: Critical
> Attachments: MYFACES-2762.patch
>
>
> Is seems that handling of
> http://www.mail-archive.com/[email protected]/msg00043.html
> (https://javaserverfaces-spec-public.dev.java.net/issues/show_bug.cgi?id=790)
> is broken in current trunk. This example:
> <h:form id="form1">
> </h:form>
>
> <h:form id="form2">
> <h:commandButton value="AJAX Submit">
> <f:ajax render="@all" />
> <f:setPropertyActionListener value="#{true}"
> target="#{viewScope.test}" />
> </h:commandButton>
> </h:form>
> <h:outputText value="#{viewScope.test}" />
> should produce output "true" with all clicks but it works only at first.
> Firebug shows that all javax.faces.ViewState elements are removed after first
> response. If I comment out first h:form it works as expected.
> Because this example works fine with 2.0.0 I think it a regression.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
