Leonardo -
Thanks for this detailed analysis - lots of good information here.
I've been meaning to follow up on this for some time now. Better late
than never I guess. :-)
My thoughts inline below...
On 3/17/10 9:10 PM, Leonardo Uribe wrote:
Actually the event publishing conditions of
PostAddToViewEvent/PreRemoveFromViewEvent are not very clear. These
conditions depends if partial state saving is used or not and if
facelets updates the view or not. The problem is this fact is not
very intuitive, so for write code correctly the user needs to be aware
of that, in other words this should be documented somewhere. Also,
there are some technical questions that could be of interest on this
mailing list and myfaces dev list too.
In theory, PostAddToViewEvent is used in this cases:
- h:outputScript and h:outputStylesheet renderers uses it to relocate
component resources to UIViewRoot facet "head" or "body" target
(h:outputStylesheet by default to "head").
I have some concerns about the resource relocation implementation. As
currently implemented (in Mojarra at least - haven't checked MyFaces
yet), the <h:outputScript>/<h:outputStylesheet> components are literally
removed from their original location in the component tree and are
re-parented to the UIViewRoot. This has some non-obvious consequences,
including:
1. EL no longer evaluates in its original context.
2. When re-executing the tags during render response on postbacks, the
components are no longer found in their original location and are
re-created (on every postback).
#1 means that if an <h:outputScript>/<h:outputStylesheet> instance uses
an EL expression that references context that is set up by an ancestor
component, these EL expressions will fail to resolve correct after the
component is relocated to a new parent. Mojarra contains code that
attempts to work around this problem for one particular case: composite
component context (ie. "#{cc}"). However, composite components are not
the only components that may set up EL-reachable state.
#2 means that we are doing unnecessary work - and potentially losing
state since we re-create the component instance from scratch on each
request.
I *think* that the correct way to handle this is to leave the
<h:outputScript>/<h:outputStylesheet> components in their original
locations and to visit these before the render occurs to give these
components a chance to contribute resources while in their proper
context. This would allow ancestors to establish context that may be
necessary for proper EL evaluation. You have hinted at something like
this in the related "add component resources depending on the owner
component state" thread (which I am also hoping to review) - ie. perhaps
we need a new event/hook after tag execution but before render that we
can use to collect the rendered resources.
Note that if we are thinking about adding such an event, we need to
consider doing this in a way that allows components to opt into event
delivery. That is, instead of performing yet another full tree
traversal, ideally we should perform a partial tree visit where we only
visit components that have expressed an interest in performing
pre-render processing. (Or, if no such components are present, we can
skip the visit altogether.) This is going to be an important
optimization for the use cases that I care the most about: views with
very large/deep component trees.
Oh, and, one nice side effect to moving resource collection out of
PostAddToViewEvent and into a special pre-render processing pass is that
we could take advantage of VisitHint.SKIP_UNRENDERED to ensure that we
only visit/collect resources for components that are, in fact,
rendered. Our current approach doesn't allow for this - ie.
PostAddToViewEvents are delivered to both rendered and non-rendered
components, and as a result all resources are added regardless of
whether they are in rendered subtrees.
Note "head" and "body" facets are transient.
- cc:insertChildren and cc:insertFacet, to relocate components to some
place inside cc:implementation. The reason why we do that through a
listener attached to PostAddToViewEvent is we need to take into
account the case of nested composite components using
cc:insertFacet/cc:insertChildren. Note when the component tree is
built the first time, PostAddToViewEvent is propagated from root to nodes.
I don't fully understand why the PostAddToViewEvents approach is
necessary for implementing composite component facet/children
insertion. The current solution - relocating composite component
facets/children to a new parent inside of the composite component
implementation suffers from problem #2 above. And in this case, the
loss of state that occurs when re-creating these components is
significant. Stateful components - such as Trinidad's <tr:showDetail>:
http://myfaces.apache.org/trinidad/trinidad-api/tagdoc/tr_showDetail.html
Cannot be used as a facet/child of a JSF 2 composite component, since
any state that they maintain is lost after tags are re-executed on
postback (during render response).
Fortunately, we have a precedent for how to solve this problem - ie.
Facelets already handles this nicely for the
<ui:composition>/<ui:define>/<ui:insert> case, which is very similar to
composite component facet/children insertion. Facelets uses the
TemplateClient API to ensure that components are inserted into the
proper target location *during* tag execution. When tags are
re-executed against an existing component tree on postbacks, the
components are still in the same location (in the target insertion
location) and thus are not re-created. There is no loss of state.
We are using a similar approach here to implement ADF Faces declarative
components (our own pre-JSF2 composite component solution) and don't
suffer from any state loss problems. (Note that we would use the
TemplateClient API directly, but this was hidden in JSF2 so we ended up
hacking around this to achieve the same behavior.)
Is there a reason why the Facelets TemplateClient approach was
not/cannot be used for composite components as well?
- When Partial State Saving is enabled, it is necessary to attach a
listener to PostAddToViewEvent/PreRemoveFromViewEvent, so if some user
add/remove a component, we can keep track of those changes and
save/restore the component tree properly, in other words, support
component addition or removal programatically.
This seems like a good use of PostAddToViewEvents. (Not sure that we
really should be leveraging PostAddToViewEvents for the other two use
cases.)
Now, the instants of time where PostAddToViewEvent is published are:
- With Partial State Saving enabled
* When the view is build at first time.
* In a postback when the view is build but before the state of the
component is restored.
Right - this is during restore view when we execute tags to build up the
view.
Note that once Mojarra issue 1313 is fixed:
https://javaserverfaces.dev.java.net/issues/show_bug.cgi?id=1313
We will also re-execute tags during render response (as we already do
for full state saving), which means that we'll see additional
PostAddToViewEvents.
- With Partial State Saving disabled
* When the view is build at first time.
* In a postback when the view is "refreshed",
(This is the 1313 case - ie. we'll see this with partial state saving
enabled too once 1313 is fixed.)
because all component nodes are detached and attached to the tree. In
other words, on render response phase, vdl.buildView is called and in
this case facelets algorithm add all transient components (usually all
html markup not saved), and to do that, it detach and attach all
components to be in right order.
Hrm. Something is very off here. I sort of get the idea that Facelets
needs to detach/re-attach components to restore the original order.
(Sort of. Does JSP do this too?). However, the fact that Facelets
happens to temporarily remove and then immediately re-add components for
the purpose of preserving the original order does not, in my mind, mean
that we should be delivering PostAddToViewEvents for this scenario.
The component was added to the component tree during restore view... it
is still in the component tree, in the same parent, possibly in the same
exact location under that parent after we re-execute the tags during
render response. This does not strike me as a "component added"
situation. The fact that Facelets happens to do this should be
transparent to component authors - ie. we should not be delivering
PostAddToViewEvents for this case.
This also has some other implications, like c:if tag depends on this
to work correctly.
A developer that is not aware of this fact could think that
PostAddToViewEvent is called when the view is build.
I can see how the current implementation can be very confusing. I agree
that we need to simplify this. I believe that at least part of the
problem is that we are delivering PostAddToViewEvents for cases that
shouldn't be treated as an "add". This seems like an implementation bug
to me, though perhaps some spec clarification is also necessary.
This is true with PSS is enabled, but is not true when PSS is
disabled. On this topic:
[jsr-314-open] add component resources depending on the owner
component state
It was described why PostAddToViewEvent cannot be used in that case.
I suspect that a post tag execution/pre-render (partial) visit might
work better for this case as well.
But let's change a litte bit the problem. We have a custom component
that creates some other on PostAddToViewEvent and add it as a
children. It should work but in fact it only works when PSS is
enabled, with PSS disabled that code will cause a duplicate id exception.
Is that because we are re-delivering PostAddToViewEvents during refresh
for components which were already present in the component tree? If so,
we should stop doing that. :-)
Of course, the developer can solve it with some effort but the point
is we are not consider the element of least surprise.
Agreed. The fact that the developer needs to get creative here seems
like a sign that something is off.
But this fact this is only the tip of the iceberg. In mojarra (this
was already fixed on myfaces), components relocated by
cc:insertChildren or cc:insertFacet does not have state when PSS is
disabled, because when the view is "refreshed" the components are
created again, but facelets algorithm remove all components with no
bound to a facelet tag handler, and then the listeners attached to
PostAddToViewEvent relocates the new ones, so this effect is not
notice at first view.
Exactly! That's my #2 above. This definitely needs to be fixed. As I
mentioned above, I think the right way to solve this problem is to use
the Facelets TemplateClient approach, though I am interested to hear
whether there are reasons why we did not/could not use this solution.
Do you remember PostAddToViewEvent propagation ordering is important
by cc:insertChildren/cc:insertFacet? well, with PSS disabled, the
ordering now is from nodes to root, because all nodes are detached and
attached from nodes to root,
I don't think that we should be delivering PostAddToViewEvents for these
temporary detach/re-attach cases.
so in myfaces we did something like this (I removed some non relevant
code to be more clear):
try
{
if (refreshTransientBuild)
{
context.setProcessingEvents(false);
}
// populate UIViewRoot
_getFacelet(renderedViewId).apply(context, view);
}
finally
{
if (refreshTransientBuild)
{
context.setProcessingEvents(true);
// When the facelet is applied, all components are
removed and added from view,
// but the difference resides in the ordering.
Since the view is
// being refreshed, if we don't do this manually,
some tags like
// cc:insertChildren or cc:insertFacet will not
work correctly, because
// we expect PostAddToViewEvent will be propagated
from parent to child, and
// facelets refreshing algorithm do the opposite.
FaceletViewDeclarationLanguage._publishPreRemoveFromViewEvent(context,
view);
FaceletViewDeclarationLanguage._publishPostAddToViewEvent(context, view);
}
}
In few words, disable event processing, and fire PostAddToViewEvent
and PreRemoveFromViewEvent in the expected order to build the tree
correctly. If we don't do this, h:outputScript and h:outputStylesheet
will not work correctly (remember that UIViewRoot "head" and "body"
facets are transient, so if these components are relocated, are in
fact transient too).
I think that we should use a post tag execution/pre-render partial visit
to collect these resources.
Also, care must be taken to prevent the listener of programatic
component addition/removal register components on this time.
The conclusion based on this reasoning, it is clear the need of new
event that be specific for relocate components (keep it simple for
christ sake!). This event should be propagated for all components in
the tree after the view is build from root to nodes.
I am very nervous about adding new events that are delivered to all
components since this adds undesirable overhead for views with large #s
of components. If we do need to add new events, I would prefer that we
go with partial visits rather than full visits if at all possible.
It could be good to have some clearification about the real intention
of PostAddToViewEvent/PreRemoveFromViewEvent.
Yep, we definitely need some clarification here. Given the
inconsistencies/unexpected behaviors in the current implementation, I
can see how our users will find these events confusing.
Really, better names for those events are
PostAddToComponentTreeEvent/PreRemoveFromComponentTreeEvent.
I don't see the distinction that you are making here... ie.
"PostAddToView" and "PostAddToComponentTree" seem very similar to me.
Andy
Suggestions are welcome.
regards,
Leonardo Uribe
