Uploaded image for project: 'Tapestry 5'
  1. Tapestry 5
  2. TAP5-2500

Label broken when inside zone and attached to a Field after that zone

Details

    • Bug
    • Status: Open
    • Major
    • Resolution: Unresolved
    • 5.4
    • None
    • tapestry-core
    • None

    Description

      This is just one example, since the same problem is observed for TriggerFrament inside a zone when used in conjunction with a FormFragment outside the zone (and after it).

          <t:zone t:id="myZone">
              <t:label for="myField">Label</t:label>
          </t:zone>
          <t:textfield t:id="myField"/>
      

      The reason this doesn't work is because Label.updateAttributes() uses @HeartbeatDeferred and assumes this invocation will be deferred until the end of the page render. Alas, since the label is inside a zone, which for some reason starts and ends its own heartbeat (nested within, and occluding, the page heartbeat), the invocation is deferred only until the end of the zone render, i.e. - too early.

      Apparently this bug was introduced in this commit, which in turn was addressing TAP5-940.

      Attachments

        1. patch
          7 kB
          Jochen Kemnade

        Issue Links

          Activity

            jkemnade Jochen Kemnade added a comment -

            Are you sure that the TriggerFragment/FormFragment issue still happens with 5.4-beta-36?

            jkemnade Jochen Kemnade added a comment - Are you sure that the TriggerFragment/FormFragment issue still happens with 5.4-beta-36?
            annorax I D added a comment -

            No, we're still using beta 33 and can't upgrade due to a different issue, which I'll report separately.

            However, as I said, this issue isn't related specifically to FormFragment/TriggerFragment. Rather, it thwarts any component using @HeartbeatDeferred when such a component is nested within a Zone, since the Zone's use of Heartbeat breaks the assumption that the deferrence will be until the end of the page render.

            annorax I D added a comment - No, we're still using beta 33 and can't upgrade due to a different issue, which I'll report separately. However, as I said, this issue isn't related specifically to FormFragment/TriggerFragment. Rather, it thwarts any component using @HeartbeatDeferred when such a component is nested within a Zone, since the Zone's use of Heartbeat breaks the assumption that the deferrence will be until the end of the page render.
            jkemnade Jochen Kemnade added a comment -

            Looks as if Heartbeat is not flexible enough. The first thing that comes to my mind is different types of Heartbeats, some of which act as "stoppers" for defer() while others let the Runnable fall through until the next stopper. But that's still quite fuzzy inside my head. It might be a better idea to take this to the dev mailing list.

            jkemnade Jochen Kemnade added a comment - Looks as if Heartbeat is not flexible enough. The first thing that comes to my mind is different types of Heartbeats, some of which act as "stoppers" for defer() while others let the Runnable fall through until the next stopper. But that's still quite fuzzy inside my head. It might be a better idea to take this to the dev mailing list.
            annorax I D added a comment -

            I think the first question we should ask ourselves is whether any component should be allowed to use its own heartbeat, or whether that privilege should be reserved for the "Heartbeat" MarkupRendererFilter / PartialMarkupRendererFilter, as I think was originally intended.

            I.e.: should Heartbeat be a stack, as currently implemented, or can we change it to simply have one "ongoing" heartbeat at a time, and possibly make it an internal service.

            Currently only the following Tapestry built-in components create (start and stop) their own Heartbeats:

            • Zone (as we've already discussed)
            • Loop
            • Tree
            • PropertyEditor
            annorax I D added a comment - I think the first question we should ask ourselves is whether any component should be allowed to use its own heartbeat, or whether that privilege should be reserved for the "Heartbeat" MarkupRendererFilter / PartialMarkupRendererFilter, as I think was originally intended. I.e.: should Heartbeat be a stack, as currently implemented, or can we change it to simply have one "ongoing" heartbeat at a time, and possibly make it an internal service. Currently only the following Tapestry built-in components create (start and stop) their own Heartbeats: Zone (as we've already discussed) Loop Tree PropertyEditor
            jkemnade Jochen Kemnade added a comment -

            If we only have a single Heartbeat, we need another way to defer rendering tasks to the end of an iteration of a containing rendering task, e.g.

            <t:loop source="list:[1,2]" />
              <t:label for="tf />
              <t:textfield id="tf"/>
            </t:loop>
            

            We cannot write the for parameter until after we have assigned a client id to the textfield, but we don't want to defer that to the end of the MarkupRendererFilter, because if we ded, all labels would get the id of the textfield in the last iteration of the loop component.
            I recently fixed the TriggerFragment/FormFragment issue by allocation the client id on first access (https://git-wip-us.apache.org/repos/asf?p=tapestry-5.git;a=commitdiff;h=4a9b171531f9fa271cb540555ead87d6b7242cca) and resetting it in the AfterRender phase (https://git-wip-us.apache.org/repos/asf?p=tapestry-5.git;a=commitdiff;h=33f9e65b933654b32d4a67ef6dcbc6b357eb7393). If we did the same for AbstractField, that would probably solve your issue.
            I'll also add a check for a null fieldId to the Label component, so this error doesn't go unnoticed at least.

            jkemnade Jochen Kemnade added a comment - If we only have a single Heartbeat, we need another way to defer rendering tasks to the end of an iteration of a containing rendering task, e.g. <t:loop source= "list:[1,2]" /> <t:label for ="tf /> <t:textfield id= "tf" /> </t:loop> We cannot write the for parameter until after we have assigned a client id to the textfield, but we don't want to defer that to the end of the MarkupRendererFilter, because if we ded, all labels would get the id of the textfield in the last iteration of the loop component. I recently fixed the TriggerFragment/FormFragment issue by allocation the client id on first access ( https://git-wip-us.apache.org/repos/asf?p=tapestry-5.git;a=commitdiff;h=4a9b171531f9fa271cb540555ead87d6b7242cca ) and resetting it in the AfterRender phase ( https://git-wip-us.apache.org/repos/asf?p=tapestry-5.git;a=commitdiff;h=33f9e65b933654b32d4a67ef6dcbc6b357eb7393 ). If we did the same for AbstractField, that would probably solve your issue. I'll also add a check for a null fieldId to the Label component, so this error doesn't go unnoticed at least.
            jkemnade Jochen Kemnade added a comment -

            No, we're still using beta 33 and can't upgrade due to a different issue, which I'll report separately.

            Looking through the changes between beta 33 and 34, I wonder what that issue might be and I'm getting the feeling that it's probably a nasty one.

            jkemnade Jochen Kemnade added a comment - No, we're still using beta 33 and can't upgrade due to a different issue, which I'll report separately. Looking through the changes between beta 33 and 34, I wonder what that issue might be and I'm getting the feeling that it's probably a nasty one.

            Commit c4202982a11925b656e595cce4a276530e9c47d7 in tapestry-5's branch refs/heads/master from jkemnade
            [ https://git-wip-us.apache.org/repos/asf?p=tapestry-5.git;h=c420298 ]

            TAP5-2500: throw an exception if the Label component cannot retrieve the Field's client id

            jira-bot ASF subversion and git services added a comment - Commit c4202982a11925b656e595cce4a276530e9c47d7 in tapestry-5's branch refs/heads/master from jkemnade [ https://git-wip-us.apache.org/repos/asf?p=tapestry-5.git;h=c420298 ] TAP5-2500 : throw an exception if the Label component cannot retrieve the Field's client id
            hudson Hudson added a comment -

            ABORTED: Integrated in tapestry-trunk-freestyle #1492 (See https://builds.apache.org/job/tapestry-trunk-freestyle/1492/)
            TAP5-2500: throw an exception if the Label component cannot retrieve the Field's client id (jochen.kemnade: rev c4202982a11925b656e595cce4a276530e9c47d7)

            • tapestry-core/src/main/java/org/apache/tapestry5/corelib/components/Label.java
            hudson Hudson added a comment - ABORTED: Integrated in tapestry-trunk-freestyle #1492 (See https://builds.apache.org/job/tapestry-trunk-freestyle/1492/ ) TAP5-2500 : throw an exception if the Label component cannot retrieve the Field's client id (jochen.kemnade: rev c4202982a11925b656e595cce4a276530e9c47d7) tapestry-core/src/main/java/org/apache/tapestry5/corelib/components/Label.java
            jkemnade Jochen Kemnade added a comment -

            The patch adds a test and fix for the exact issue. However it changes the client id allocation logic in AbstractField and that's why I'd like some more eyes to go over the change first.
            The point is to allocate the client id when it is first requested (so that the label component can use it if it is located before the field) and clear it after the field has rendered (so that multiple renderings of the field use different ids) but to delay the resetting until the end of the heartbeat (so that the label component can use it if it is located after the field).

            jkemnade Jochen Kemnade added a comment - The patch adds a test and fix for the exact issue. However it changes the client id allocation logic in AbstractField and that's why I'd like some more eyes to go over the change first. The point is to allocate the client id when it is first requested (so that the label component can use it if it is located before the field) and clear it after the field has rendered (so that multiple renderings of the field use different ids) but to delay the resetting until the end of the heartbeat (so that the label component can use it if it is located after the field).
            annorax I D added a comment -

            I'm afraid I tried this exact fix in the past in the context of other issues related to clientId assignment, and trust me - calling JavascriptSupport.allocateClientId() any earlier than setupRender() is a bad idea.

            Why? Because the ID you will get will not be guaranteed to be unique. You will simply get the component ID (possible namespaced), but without the unique suffix. I don't know why this happens, but it probably has to do with the internals of IdAllocator.

            If I were you, I'd revert this (also for FormFragment).

            annorax I D added a comment - I'm afraid I tried this exact fix in the past in the context of other issues related to clientId assignment, and trust me - calling JavascriptSupport.allocateClientId() any earlier than setupRender() is a bad idea. Why? Because the ID you will get will not be guaranteed to be unique. You will simply get the component ID (possible namespaced), but without the unique suffix. I don't know why this happens, but it probably has to do with the internals of IdAllocator. If I were you, I'd revert this (also for FormFragment).

            Commit 18ea654718212d4f65d98fb7a106bfbbecc34d47 in tapestry-5's branch refs/heads/master from jkemnade
            [ https://git-wip-us.apache.org/repos/asf?p=tapestry-5.git;h=18ea654 ]

            TAP5-2509, TAP5-2500: log a warning in development mode instead of throwing an exception if a Label is used with a Field that does not return its clientId (in time)

            jira-bot ASF subversion and git services added a comment - Commit 18ea654718212d4f65d98fb7a106bfbbecc34d47 in tapestry-5's branch refs/heads/master from jkemnade [ https://git-wip-us.apache.org/repos/asf?p=tapestry-5.git;h=18ea654 ] TAP5-2509 , TAP5-2500 : log a warning in development mode instead of throwing an exception if a Label is used with a Field that does not return its clientId (in time)
            hudson Hudson added a comment -

            FAILURE: Integrated in tapestry-trunk-freestyle #1528 (See https://builds.apache.org/job/tapestry-trunk-freestyle/1528/)
            TAP5-2509, TAP5-2500: log a warning in development mode instead of (jochen.kemnade: rev 18ea654718212d4f65d98fb7a106bfbbecc34d47)

            • tapestry-core/src/main/java/org/apache/tapestry5/corelib/components/Label.java
            hudson Hudson added a comment - FAILURE: Integrated in tapestry-trunk-freestyle #1528 (See https://builds.apache.org/job/tapestry-trunk-freestyle/1528/ ) TAP5-2509 , TAP5-2500 : log a warning in development mode instead of (jochen.kemnade: rev 18ea654718212d4f65d98fb7a106bfbbecc34d47) tapestry-core/src/main/java/org/apache/tapestry5/corelib/components/Label.java

            People

              Unassigned Unassigned
              annorax I D
              Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

                Created:
                Updated: