Hi
2011/6/10 Andy Schwartz <andy.schwartz_at_oracle.com>:
> On 5/9/11 2:02 PM, Ed Burns wrote:
>>
>> LU> The first thing to notice is the description here is not what is
>> LU> happening right now in Mojarra (2.0 and 2.1). The description found
>> LU> in UIComponent.setParent is more close to the reality (the
>> LU> conditions related to parent.isInView() ) but the part that says
>> LU> "... but only if any of the following conditions are true ..." is
>> LU> not being preserved.
>>
>> When reading Leonadro's post from 23 Jan 2011 [1], the above paragraph
>> is the first actionable text I encountered. The desired action seems to
>> be to fix this bug in mojarra.
>>
>> Leonardo then goes on to assert that our working assumption, that we *do
>> not* want to deliver PostAddToViewEvent during renderResponse when
>> Facelets temporarily removes/re-adds existing components from the tree,
>> is false for certain cases.
>>
>
> Hrm. It isn't clear to me that this is Leonardo's concern. Though
> hopefully he can clarify. :-)
>
In few words, PostAddToViewEvent, should be published when:
1. For all components when the view is build for first time.
2. A component is added, as a result of c:if, ui:include src="#{...}", when
the view is being refreshed.
3. Programatic additions.
> The fact that Facelets temporarily removes/re-adds existing components is an
> implementation detail that should be completely transparent to the
> application developer. (The fact that Facelets removes/re-adds existing
> components is bad thing in many cases, but let's save that for a separate
> discussion.)
>
I totally agree with you but note this problem is related to PostAddToViewEvent
publishing conditions too.
>> LU> there are valid scenarios in facelets when it is required to throw
>> LU> PostAddToViewEvent (for example, an ui:include src="#{someEL}"
>> LU> should propagate that event so the view could be restored).
>>
>
> I think I understand what Leonardo is getting at here, but just to be sure,
> I'll try to work through this in more detail...
>
> I believe that the use case that Leonardo is concerned about is:
>
> - Page has an EL-bound include
> - On postback, a listener (eg. action listener) modifies some condition that
> results in new content being included.
> - During the render-response, this content is included (and new components
> are created/added) for the first time.
>
> Note that there are two very different types of component adds that occur on
> postback/render-response:
>
> 1. Exisiting omponents that are temporarily removed/re-added by Facelets.
> 2. New components that are being added for the first time.
>
> We don't want to deliver PostAddToviewEvents for #1.
>
> We do want to deliver PostAddToViewEvents for #2.
>
> (Leonardo - does that sound right?)
Yes, that's right. In fact, MyFaces implementations do that.
>
> Unfortunately the spec wording is overly specific about the conditions under
> which PostAddToViewEvents should be delivered. At a high-level the
> intention seems clear (ie. these events should be delivered any time a
> component is newly added), but the spec language doesn't facilitate this.
>
Yes, we need to fix the spec to reflect the high-level intention.
>> Leonardo asserts that this case does not work in Mojarra for the
>> following reason.
>>
>> LU> Mojarra still uses a Listener attached to PostAddToViewEvent, making
>> LU> UIComponent.getClientId() "unstable".
>>
>> Are you talking about inner class AddRemoveListener in class
>> StateContext?
>>
>
> Since Leonardo also references "relocation" below, I suspect that he is
> referring to the PostAddToViewEvent listener used by Mojarra's
> implementation of InsertFacetHandler/InsertChildrenHandler.
>
Yes. Below there is a more detailed description of the problem
(copied from other email):
Look the javadoc documentation for PostAddToViewEvent, it says
something like this:
"... Therefore, the implementation may assume it is safe to call
UIComponent.getParent(), UIComponent.getClientId(), and other methods
that depend upon the component instance being added into the view. ..."
Now consider what happen with a component relocation, like the
one activated by cc:insertChildre or cc:insertFacet. If a parent component
call getClientId() for a nested child component and relocation occur for an
ancestor between the parent and the child, since getClientId() is cached
the value will be wrong, because the ancestor hierarchy changed.
In practice, call getClientId() in PostAddToViewEvent could lead to
inconsistencies.
To track view changes MyFaces uses a listener called
PostAddPreRemoveFromViewListener on
org.apache.myfaces.view.facelets.DefaultFaceletsStateManagementStrategy
class, but if you take a look to this class, you'll notice that MyFaces
never calls getClientId() for PostAddToViewEvent. I have checked how
Mojarra AddRemoveListener works, and as I was expecting, it uses
getClientId(). I remember this cause state get lost for composite
components (it is a subtle effect, but there is).
I'm taking about cc:insertChildren and cc:insertFacet
RelocateChildrenListener / RelocateFacetListener. But the mistake
is not on those listeners, instead the problem is about how
PostAddToViewEvent is propagated when the view is refreshed.
When the view is built by first time, PostAddToViewEvent is propagated
from root to leaves, but when the view is refreshed, the opposite
happens, PostAddToViewEvent is propagated from leaves to root.
This create a full mess on those listeners, I think you have already
seen that. To make it work, the best is use other internal event
that could be propagated correctly: from root to leaves. In MyFaces
I was able to tune up this algorithm, and make it bullet proof, but do
that change is critical to make it (note MyFaces now uses other
strategy, so that code was removed, but there is still some
comments).
>> LU> If the developer has a listener that is executed before the one
>> LU> doing relocation, UIComponent.getClientId() call will not work
>> LU> correctly.
>>
>
> The issue here is that if getClientId() is called just before the relocation
> occurs (from an application-specified PostAddToViewEvent listener), the
> component will compute/cache the client id from the original location.
> However, this client id is not stable, since the component is being
> relocated into the composite component - ie. into a different naming
> container (and thus should end up with a different client id).
>
Yes. The important thing to note is the spec is clear about getClientId()
should be stable on PostAddToViewEvent, and relocation hack breaks
this. From spec point of view, any different alternative to relocation should
be used to preserve getClientId() calls.
>> Leonardo goes on to propose a concrete solution to this apparent
>> implementation bug.
>
>
> The solution for this particular issue that we discussed last year is to
> replace the PostAddToViewEvent-based relocation strategy with a
> TemplateClient-based approach. The end result is that the inserted
> components are only ever added to one parent - ie. to the target parent
> inside of the component component.
>
> While I think that this is the right solution, it is worth pointing out that
> there are other issues/trade-offs here - eg. the act of relocating the
> component from the application-specified parent into a different parent
> inside of the composite component definition violates the abstraction that
> the application developer expects. Ken P and I had an extended discussion
> of this topic on jsr-314-open a couple of years ago, but never reached a
> satisfactory resolution for what to do about this.
>
My personal opinion, based on the experience implementing it in MyFaces and
comparing it to the previous relocation alternative, is TemplateClient-based
approach is the best choice. It is clean, stable, and allow to solve a lot of
difficult issues related to interaction between ui:xxx tags and composite
components. I know it is a "big jump", and big jumps are risky and takes time
but it pays of.
>
>> I am having a hard time understanding the root
>> cause of the problem.
>>
>> Is the root cause that we do not distinguish between Add/Remove events
>> done by the framework and those that happen at the request of
>> developers?
>
> Seems that there are a few issues here:
>
> - Spec issue: as currently worded, the spec does not allow for events to be
> delivered in response to "real" adds that occur during
> postback/render-response.
> - Implementation issue: Mojarra should ensure that PostAddToViewEvents are
> delivered for "real" adds that occur during postback/render-response.
> - Implementation issue: Mojarra's insert facet/children implementation
> should not rely on PostAddToViewEvents for component relocation - at least
> in the cc case. (Aside from the concerns that Leonardo raises, the other
> issue is that this results in the relocated components being unnecessarily
> re-created on every postback request.)
>
>
>> This seems to be the what Leonardo is suggesting when he
>> says:
>>
>> LU> The current behavior of Mojarra, from the point of view of the spec
>> LU> should be consider a bug. How to do fix it? I propose do the
>> LU> following for Mojarra:
>>
>> LU> 1. Check when a component is temporally removed/added on
>> LU> ComponentTagHandlerDelegate implementation. If that so, disable
>> LU> event processing using FacesContext.setProcessingEvents() method
>> LU> temporally while removal and addition is done.
>>
>
>
> Right. This would suppress remove/add event delivery for the transient
> remove/re-adds performed by Facelets but allow these events to be delivered
> for application-related component tree modifications.
>
Yes.
>
>> LU> 2. Propagate another internal event (for example
>> LU> PostAddToViewOnRefreshBuild or something like that) to notify the
>> LU> listeners used for relocation (cc:insertChildren, cc:insertFacet,
>> LU> h:outputScript, h:outputStylesheet) to do their job like before.
>>
>
>
> Hrm. I was thinking that we could use the TemplateClient approach, at least
> for the cc cases. Unfortunately this won't work for the
> h:outputScript/h:outputStyleSheet cases. Not sure what to do here, though I
> am not happy with the though of adding new system events to solve this. (We
> are seeing performance regressions due to system event delivery overhead
> when comparing 1.2 to 2.0. Not really acceptable to regress further in
> 2.2.)
Note this event will be propagated only in a postback. The only way to make
relocation hack works is add that event. So, in theory if it is
possible to find a
way to make h:outputScript/h:outputStyleSheet without use relocation, and just
do not allow relocation hack anymore, this event could be skipped.
Note relocation hack is only valid inside JSF impls for now.
>
> FWIW, in retrospect I think that it was a mistake to use component
> relocation for resources. We would have been better off finding a way to
> allow resources to be added to the various document targets that did not
> require the component itself to be removed/re-added. I realize that it is
> unlikely we'll be able to do anything to address this now.
>
I totally agree with you. From spec point of view, there is still a pending
problem about how to make some parts of the view "dynamic", that means
that those parts will not be included on PSS algorithm and all components
there could be saved and restored completely. I'll send another mail for
this one soon. Note for users it is not clear how to create programatically
components in JSF (how, from where?, can i use a listener?).
Leonardo Uribe
> Andy
>
>
>
>> Do we have a testcase that reproduces this problem? Are any of the test
>> cases on JAVASERVERFACES-1826 showing this problem?
>>
>> Can someone more clearly explain the actions Leonardo is suggesting we
>> take?
>>
>> Ed
>>
>> [1]
>> http://lists.jboss.org/pipermail/jsr-314-open-mirror/2011-January/000716.html
>>
>
>