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. :-)
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.)
> 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?)
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.
> 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.
> 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).
> 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.
> 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.
> 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.)
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.
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
>