javaserverfaces
  1. javaserverfaces
  2. JAVASERVERFACES-1684

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

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Duplicate
    • Affects Version/s: current
    • Fix Version/s: unscheduled
    • Component/s: facelets
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

    • Issuezilla Id:
      1,684

      Description

      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?

        Issue Links

          Activity

          Hide
          Ed Burns added a comment -

          reassign

          Show
          Ed Burns added a comment - reassign
          Hide
          Jakob Korherr added a comment -

          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

          Show
          Jakob Korherr added a comment - 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
          Hide
          Jakob Korherr added a comment -

          issue #1708 describes exactly the same problem

          Show
          Jakob Korherr added a comment - issue #1708 describes exactly the same problem
          Hide
          Ed Burns added a comment -

          Per Jakob, close this as duplicate of 1708.

              • This issue has been marked as a duplicate of 1708 ***
          Show
          Ed Burns added a comment - Per Jakob, close this as duplicate of 1708. This issue has been marked as a duplicate of 1708 ***
          Hide
          Manfred Riem added a comment -

          Closing issue out

          Show
          Manfred Riem added a comment - Closing issue out

            People

            • Assignee:
              Ed Burns
              Reporter:
              mst_70
            • Votes:
              0 Vote for this issue
              Watchers:
              0 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved: