Re: [jsr-314-open-mirror] [jsr-314-open] PostAddToViewEvent publishing conditions

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 > >