[jsr-314-open-mirror] [jsr-314-open] PostAddToViewEvent publishing conditions
Leonardo Uribe
lu4242 at gmail.com
Wed May 19 00:51:41 EDT 2010
Hi
2010/5/18 Andy Schwartz <andy.schwartz at oracle.com>
> 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.
>
>
I agree with you. I was thinking as a temporal fix for 2.0 to create a
"copy" of the component created by <h:outputScript>/<h:outputStylesheet> and
add them as resource as a result of a PostAddToViewEvent, but your
suggestion looks better. The important here is do not relocate components.
> 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?
>
>
Good idea! I think it is possible, but I don't know the details behind
TemplateClient. I'll try it.
>
> - 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.
>
>
Totally agree. I'll fix this stuff on myfaces, because after doing some work
with tomahawk (t:autoScroll), I conclude it is more important publish
PostAddToViewEvent correctly (I want to create a view listener from
PostAddToViewEvent that attach a client behavior based on some conditions).
>
> 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.
>
>
Leonardo
> Andy
>
>
> Suggestions are welcome.
>>
>> regards,
>>
>> Leonardo Uribe
>>
>>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/jsr-314-open-mirror/attachments/20100518/b00c806e/attachment-0002.html
More information about the jsr-314-open-mirror
mailing list