myfaces-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andy Schwartz <andy.schwa...@oracle.com>
Subject Re: [jsr-314-open] PostAddToViewEvent publishing conditions
Date Wed, 19 May 2010 02:27:42 GMT
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
>


Mime
View raw message