[jsr-314-open-mirror] [jsr-314-open] PostAddToViewEvent publishing conditions
Andy Schwartz
andy.schwartz at oracle.com
Tue May 18 22:27:42 EDT 2010
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
>
More information about the jsr-314-open-mirror
mailing list