Issue Details (XML | Word | Printable)

Key: JAVASERVERFACES-1684
Type: Bug Bug
Status: Closed Closed
Resolution: Duplicate
Priority: Critical Critical
Assignee: Ed Burns
Reporter: mst_70
Votes: 0
Watchers: 0
Operations

If you were logged in you would be able to see more operations.
javaserverfaces

Nested templates broken when the named used in ui:define for both

Created: 21/May/10 12:26 PM   Updated: 01/Mar/12 03:08 PM   Resolved: 14/Jul/10 06:58 AM
Component/s: facelets
Affects Version/s: current
Fix Version/s: unscheduled

Time Tracking:
Not Specified

Environment:

Operating System: All
Platform: All

Issue Links:
Related
 

Issuezilla Id: 1,684
Tags:
Participants: Ed Burns, Jakob Korherr, Manfred Riem and mst_70


 Description  « Hide

To reproduce the issue, create the simplest page with two nested templates
(ui:composition):

Top-level page (test.xhtml):
===========================
<ui:composition xmlns="http://www.w3.org/1999/xhtml"
xmlns:f="http://java.sun.com/jsf/core"
xmlns:h="http://java.sun.com/jsf/html"
xmlns:ui="http://java.sun.com/jsf/facelets"
template="inner_1.xhtml" >
<ui:define name="test">
<h:outputText value = "Defined in the Top Page"/>
</ui:define>
</ui:composition>

First-level template (inner_1.xhtml):
====================================
<ui:composition template="inner_2.xhtml"
xmlns="http://www.w3.org/1999/xhtml"
xmlns:f="http://java.sun.com/jsf/core"
xmlns:h="http://java.sun.com/jsf/html"
xmlns:ui="http://java.sun.com/jsf/facelets">
<ui:define name="test">
<h:panelGroup>
<ui:insert name ="test"/>
<br/>
<span>Defined in the first-level template</span>
</h:panelGroup>
</ui:define>
</ui:composition>

Second-level template (inner_2.xhtml):
======================================
<ui:composition xmlns="http://www.w3.org/1999/xhtml"
xmlns:f="http://java.sun.com/jsf/core"
xmlns:h="http://java.sun.com/jsf/html"
xmlns:ui="http://java.sun.com/jsf/facelets">
<html>
<body>
<ui:insert name="test"/>
</body>
</html>
</ui:composition>

The correct behavior should be that ui:insert in the second-level template
(inner_2.xhtml) gets ui:define content from the first-level template
(inner_1.xhtml). Then inner_1.xhtml should get ui:define content from the
top-level page. However, when the same name is used for the ui:define elements
in the top-level page and in the first-level template, Facelets gets confused.
Instead of getting content from inner1_xhtml, ui:insert in inner_2.xhtml gets it
straight from the top-level page.

The expected output is:
Defined in the Top Page
Defined in the first-level template

The current output is:
Defined in the Top Page

A quick analysis shows that DefaultFaceletContext does not implement proper
stack handling for the TemplateClients. The correct use of stack would require
the following:

[push TemplateClient]

Include Template->includeDefinition->[pop TemplateClient]->apply() on the
ui:define content->[push TemplateClient]

[pop TemplateClient]

There should be only one current TemplateClient at any moment. When apply() is
called on the ui:define content during includeDefinition(), we have to pop the
TemplateClient to reflect the state of the document containing the ui:define
element.

Sadly, DefaultFaceletContext looks at every TemplateClient on the stack during
includeDefinition():

for (int i = 0, size = this.clients.size(); i < size && !found; i++) { client = this.clients.get(i); //noinspection EqualsBetweenInconvertibleTypes if (client.managesFacelet(this.facelet)) continue; found = client.apply(this, parent, name); }

Note that we always start search at the index 0. This is why the code ends up
finding the top-level TemplateClient in the test case above (CompositionHandler
calls extendClient() to put the TemplateClient at the end of the list, so the
correct TemplateClient ends up at the index 1).

I believe the entire area related to the TemplateClient stack needs to be
cleaned up. We will also need to answer the following questions:
1) What is the purpose of extendClient() and why just having pushClient() was
not enough?
2) Why does CompositionHandler call extendClient(), while DecorateHandler calls
pushClient()?
3) Why does InsertHandler call extendClient()() and why does it implement
TemplateClient?



Ed Burns added a comment - 22/Jun/10 08:52 PM

reassign


Jakob Korherr added a comment - 14/Jul/10 02:58 AM

Hi,

We have the exact same issue on MyFaces (see MYFACES-2753) and I already worked
on it a couple of hours.

On my opinion extendClient() does not make any sence at all. I think it was
introduced to handle the "fallback" content for <ui:insert>, which is the body
content of this tag. This would explain your third question, but looking at the
(normal) apply method of InsertHandler, it does not make any sence, because we
have the fallback check there too. Furthermore I found out that the
TemplateClient.apply() method on InsertHandler is never called anyway, because
of the following if on includeDefinition() (which by the way is broken OO
design, because equals() does not do what it normally should):

if (client.equals(owner)) // checks: this._owner == o || this._target == o;
continue;

Furthermore the order of fallback TemplateClients would be wrong anyway if you
have more than one layer (like in the example of Max). One option would be to
create a separate stack here, but if you think about it, a stack does not make
any sence here at all, because the code is initially triggered from the
IncludeHandler anyway, thus he can control the fallback himself.

I have also investigated your second question for some time and I found the
following commit on the faceletes codebase which changes pushClient() to
extendClient() on ui:composition:

https://facelets.dev.java.net/source/browse/facelets/src/java/com/sun/facelets/tag/ui/CompositionHandler.java?r1=1.9&r2=1.9.4.1

However without a commit message, thus I have no clue why this was changed, and
if you think about it, it does not make any sence at all.

I think this area needs a major refactoring, on MyFaces, Mojarra and on the
facelets codebase. On my opinion, the best way to solve this problem is to:

  • remove extendClient()
  • remove TemplateClient code from InsertHandler

I already proposed a patch with the above changes for MyFaces, sadly we have
some disagreements about it and thus we think it is the best idea to discuss
this issue together.

Regards,
Jakob


Jakob Korherr added a comment - 14/Jul/10 03:11 AM

issue #1708 describes exactly the same problem


Ed Burns added a comment - 14/Jul/10 06:58 AM

Per Jakob, close this as duplicate of 1708.

      • This issue has been marked as a duplicate of 1708 ***

Manfred Riem added a comment - 01/Mar/12 03:08 PM

Closing issue out