FOP
  1. FOP
  2. FOP-1099

[PATCH] footnotes within tables and lists get lost

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Resolution: Fixed
    • Affects Version/s: trunk
    • Fix Version/s: None
    • Component/s: layout/unqualified
    • Labels:
      None
    • Environment:
      Operating System: other
      Platform: Other
    • External issue ID:
      37579

      Description

      Footnote outside of tables works, inside table-cell not.
      The compliance page only speaks about restrictions within multicolumn documents.

      1. dorfchronik.fo
        4 kB
        gerhard oettl
      2. footnotespatch
        9 kB
        gerhard oettl
      3. dorfchronik.fo.table3
        12 kB
        gerhard oettl
      4. dorfchronik.fo.list3
        8 kB
        gerhard oettl
      5. footnotes.080502.diff
        10 kB
        Luca Furini
      6. bugzilla37579.patch
        10 kB
        Andreas L. Delmelle
      7. b37579_2.patch
        1 kB
        Andreas L. Delmelle
      8. b37579.diff
        5 kB
        Andreas L. Delmelle
      9. b37579.diff
        13 kB
        Andreas L. Delmelle
      10. table_list_footnotes.diff
        12 kB
        Vincent Hennebert
      11. footnote.fo
        2 kB
        Dimitri Goloborodko
      12. fussnote.fo
        2 kB
        Dimitri Goloborodko
      13. footnoteintable.patch
        6 kB
        simon steiner
      14. vtest.fo
        5 kB
        simon steiner
      15. 1044-tab-header-footnote-no-repeat.patch
        9 kB
        Matthias Reischenbacher
      16. 1044-tab-header-footnote-no-repeat.xml
        5 kB
        Matthias Reischenbacher
      17. duplicated-footnote.fo
        3 kB
        Vincent Hennebert
      18. duplicated-footnote.pdf
        6 kB
        Vincent Hennebert
      19. footnote-end-page-sequence.fo
        2 kB
        Vincent Hennebert
      20. footnote-end-page-sequence.pdf
        6 kB
        Vincent Hennebert
      21. footnote-missing_page2.fo
        5 kB
        Vincent Hennebert
      22. footnote-missing_page2.pdf
        6 kB
        Vincent Hennebert
      23. inverted-footnotes.fo
        4 kB
        Vincent Hennebert
      24. inverted-footnotes.pdf
        6 kB
        Vincent Hennebert
      25. footnote_repeated-table-heading.patch
        102 kB
        Vincent Hennebert

        Issue Links

          Activity

          Hide
          gerhard oettl added a comment -

          Attachment dorfchronik.fo has been added with description: example for lost footnote in table-cell

          Show
          gerhard oettl added a comment - Attachment dorfchronik.fo has been added with description: example for lost footnote in table-cell
          Hide
          Jeremias Maerki added a comment -

          Two test cases added to our test suite:
          http://svn.apache.org/viewcvs?rev=348138&view=rev

          The same problem exists with lists. The problem is the following:

          In table and list layout Knuth element lists are combined to a single element
          list. The footnotes contained in KnuthBlockBoxes in the original element lists
          currently don't get propagated to the combined element lists and that's why the
          footnotes get ignored. It shouldn't be too hard to fix.

          Show
          Jeremias Maerki added a comment - Two test cases added to our test suite: http://svn.apache.org/viewcvs?rev=348138&view=rev The same problem exists with lists. The problem is the following: In table and list layout Knuth element lists are combined to a single element list. The footnotes contained in KnuthBlockBoxes in the original element lists currently don't get propagated to the combined element lists and that's why the footnotes get ignored. It shouldn't be too hard to fix.
          Hide
          gerhard oettl added a comment -

          Attachment footnotespatch has been added with description: 1/2 patch

          Show
          gerhard oettl added a comment - Attachment footnotespatch has been added with description: 1/2 patch
          Hide
          gerhard oettl added a comment -

          The comitted patch causes footnotes in lists and tables to survive
          (longer?) than without.

          It is not finished, but I am at the limit of my knowledge
          and tomorrow my usual business starts, so I want to
          contribute what I have now - hoping it is a bit usefull
          for others who continue or better rewrite.

          Description:
          A) Propagate footnotes in ListItemLayoutManger when
          combined knuth elements are created.
          B) Propagate footnotes in TableStepper when
          combined knuth elements are created.
          C) Propagate footnotes in TableContentLayoutManger when
          boxes for table-header and table-footer are created.

          The shortcomings:
          1) Footnotes in list-item-label produce a
          "Cannot find LM to handle given FO for LengthBase."
          AFAICS in the getBaseLength method of AbstractBaseLayoutManger.
          2) Footnotes from list-item-body starts at the same position
          (from the starting edge) than the list-item-body itself and
          not at the starting edge of the region-body.
          3) Footnotes on table-header and table-footer are not printed
          on every page where the content of the table-header and
          table-footer is printed but only on that page that holds
          the end of the table.
          4) Footnotes on table-header and table-footer are ordered after
          the footnotes of table-body.
          5) There is one situation with my example fo-file for tables that
          produce an endless loop (see comments there).

          Show
          gerhard oettl added a comment - The comitted patch causes footnotes in lists and tables to survive (longer?) than without. It is not finished, but I am at the limit of my knowledge and tomorrow my usual business starts, so I want to contribute what I have now - hoping it is a bit usefull for others who continue or better rewrite. Description: A) Propagate footnotes in ListItemLayoutManger when combined knuth elements are created. B) Propagate footnotes in TableStepper when combined knuth elements are created. C) Propagate footnotes in TableContentLayoutManger when boxes for table-header and table-footer are created. The shortcomings: 1) Footnotes in list-item-label produce a "Cannot find LM to handle given FO for LengthBase." AFAICS in the getBaseLength method of AbstractBaseLayoutManger. 2) Footnotes from list-item-body starts at the same position (from the starting edge) than the list-item-body itself and not at the starting edge of the region-body. 3) Footnotes on table-header and table-footer are not printed on every page where the content of the table-header and table-footer is printed but only on that page that holds the end of the table. 4) Footnotes on table-header and table-footer are ordered after the footnotes of table-body. 5) There is one situation with my example fo-file for tables that produce an endless loop (see comments there).
          Hide
          gerhard oettl added a comment -

          Attachment dorfchronik.fo.table3 has been added with description: example fo for tables

          Show
          gerhard oettl added a comment - Attachment dorfchronik.fo.table3 has been added with description: example fo for tables
          Hide
          gerhard oettl added a comment -

          Attachment dorfchronik.fo.list3 has been added with description: example fo for lists

          Show
          gerhard oettl added a comment - Attachment dorfchronik.fo.list3 has been added with description: example fo for lists
          Hide
          Sebastian Krysmanski added a comment -
              • FOP-1190 has been marked as a duplicate of this bug. ***
          Show
          Sebastian Krysmanski added a comment - FOP-1190 has been marked as a duplicate of this bug. ***
          Hide
          Jeremias Maerki added a comment -
              • FOP-1310 has been marked as a duplicate of this bug. ***
          Show
          Jeremias Maerki added a comment - FOP-1310 has been marked as a duplicate of this bug. ***
          Hide
          Jeremy Hughes added a comment -

          Any further progress or ETA?

          Show
          Jeremy Hughes added a comment - Any further progress or ETA?
          Hide
          Ih added a comment -

          Is there any workaround to solve this problem? We work on a project, and we need
          this feature... :/

          Show
          Ih added a comment - Is there any workaround to solve this problem? We work on a project, and we need this feature... :/
          Hide
          Jeremias Maerki added a comment -

          I don't think there's any work-around. It's really a non-trivial problem with
          our layout approach. As far as I know, nobody's working on this at the moment. I
          you're fearless, you can try to do it yourself based on the work Gerhard Oettl
          started.

          Show
          Jeremias Maerki added a comment - I don't think there's any work-around. It's really a non-trivial problem with our layout approach. As far as I know, nobody's working on this at the moment. I you're fearless, you can try to do it yourself based on the work Gerhard Oettl started.
          Hide
          Adam Strzelecki added a comment -

          Wouldn't be better to include at least Gerhard's patch into the code? Correct, me if I'm wrong, but it's
          better to have limited functionality of list & tables footnotes than none.
          Having footnotes in lists is simply necessary thing when writing bigger projects documentation.

          Cheers.

          Show
          Adam Strzelecki added a comment - Wouldn't be better to include at least Gerhard's patch into the code? Correct, me if I'm wrong, but it's better to have limited functionality of list & tables footnotes than none. Having footnotes in lists is simply necessary thing when writing bigger projects documentation. Cheers.
          Hide
          Andreas L. Delmelle added a comment -

          Workaround as offered by Ron Van den Branden on fop-users@ :

          A way in which the problem can be avoided, is by generating fo:footnote areas for those footnotes outside the areas of their
          containing lists and tables. If those fo:footnote areas don't contain any text in their fo:inline footnote markers, the latter don't
          show up in the inline text, while the fo:footnote-body does end up in the footnote region at the bottom of the page. [Note: this
          use of footnotes is inspired by solutions to other vertical alignment issues like
          <http://www.dpawson.co.uk/xsl/sect3/fofixedposn.html#d12878e43>]
          Stylesheet-wise this involves a two-way treatment of footnotes inside lists and tables: 1) generate the footnote markers inline
          (just a fo:inline containing the footnote marker suffices), 2) generate the fo:footnote areas for each of those footnotes out-of-
          line, inside a fo:block after the affected fo:list-block / fo:table.

          I'll illustrate with following code, in which the first footnote doesn't get rendered, while the second one does:

          <fo:root xmlns:fo="http://www.w3.org/1999/XSL/Format">
          <fo:layout-master-set>
          <fo:simple-page-master master-name="simple" page-height="5in" page-width="5in">
          <fo:region-body/>
          </fo:simple-page-master>
          </fo:layout-master-set>
          <fo:page-sequence master-reference="simple">
          <fo:flow flow-name="xsl-region-body">
          <fo:list-block provisional-distance-between-starts="50pt" provisional-label-separation="10pt">
          <fo:list-item>
          <fo:list-item-label end-indent="label-end()">
          <fo:block>label</fo:block>
          </fo:list-item-label>
          <fo:list-item-body start-indent="body-start()">
          <fo:block>
          <!-- This fo:block contains a 'regular' fo:footnote inside a fo:list-block. Note that the fo:footnote-body
          doesn't get rendered in the output, due to FOP-1099
          (http://issues.apache.org/bugzilla/show_bug.cgi?id=37579) -->
          List item with a footnote<fo:footnote>
          <fo:inline font-size="60%" baseline-shift="super">1)</fo:inline>
          <fo:footnote-body>
          <fo:block start-indent="0.5cm" text-indent="-0.5cm">
          <fo:inline font-size="60%" baseline-shift="super">1)</fo:inline> This footnote doesn't get rendered.</fo:block>
          </fo:footnote-body>
          </fo:footnote>.
          </fo:block>
          </fo:list-item-body>
          </fo:list-item>
          <fo:list-item>
          <fo:list-item-label end-indent="label-end()">
          <fo:block>label</fo:block>
          </fo:list-item-label>
          <fo:list-item-body start-indent="body-start()">
          <fo:block>
          <!-- this footnote is only marked inline by a fo:inline marker -->
          List item with a footnote<fo:inline font-size="60%" baseline-shift="super">2)</fo:inline>.
          </fo:block>
          </fo:list-item-body>
          </fo:list-item> </fo:list-block>
          <!-- this block contains the fo:footnote areas for all separate footnotes in the previous fo:list-block -->
          <fo:block>
          <fo:footnote>
          <!-- this fo:inline footnote marker is empty to avoid it getting output after the fo:list-block -->
          <fo:inline/>
          <fo:footnote-body>
          <fo:block start-indent="0.5cm" text-indent="-0.5cm">
          <fo:inline font-size="60%" baseline-shift="super">2)</fo:inline> This footnote does get rendered.</fo:block>
          </fo:footnote-body>
          </fo:footnote>
          </fo:block>
          </fo:flow>
          </fo:page-sequence>
          </fo:root>

          Note that this approach will require some refinements. For example, for lists / tables that span multiple pages, the footnotes
          will all end up before / after the affected list / table (depending on the placement of their containing block). In order to avoid
          this, the fo:list-block / fo:table areas could be generated at the lower level of the list items, i.e. the input list / table will not
          generate a fo:list-block / fo:table, but each list item / table row will. Each of those fo:list-block / fo:table areas can then be
          followed by a fo:block containing the relevant fo:footnote area. Of course this produces a lot of one-item lists / one-row tables,
          but it also guarantees that footnotes will show at the right page when they occur in long lists / tables. I haven't tested it in my
          production XSL-FO stylesheets, and of course treatment of nested lists would demand further consideration, but I think the
          principle works.

          Show
          Andreas L. Delmelle added a comment - Workaround as offered by Ron Van den Branden on fop-users@ : A way in which the problem can be avoided, is by generating fo:footnote areas for those footnotes outside the areas of their containing lists and tables. If those fo:footnote areas don't contain any text in their fo:inline footnote markers, the latter don't show up in the inline text, while the fo:footnote-body does end up in the footnote region at the bottom of the page. [Note: this use of footnotes is inspired by solutions to other vertical alignment issues like < http://www.dpawson.co.uk/xsl/sect3/fofixedposn.html#d12878e43 >] Stylesheet-wise this involves a two-way treatment of footnotes inside lists and tables: 1) generate the footnote markers inline (just a fo:inline containing the footnote marker suffices), 2) generate the fo:footnote areas for each of those footnotes out-of- line, inside a fo:block after the affected fo:list-block / fo:table. I'll illustrate with following code, in which the first footnote doesn't get rendered, while the second one does: <fo:root xmlns:fo="http://www.w3.org/1999/XSL/Format"> <fo:layout-master-set> <fo:simple-page-master master-name="simple" page-height="5in" page-width="5in"> <fo:region-body/> </fo:simple-page-master> </fo:layout-master-set> <fo:page-sequence master-reference="simple"> <fo:flow flow-name="xsl-region-body"> <fo:list-block provisional-distance-between-starts="50pt" provisional-label-separation="10pt"> <fo:list-item> <fo:list-item-label end-indent="label-end()"> <fo:block>label</fo:block> </fo:list-item-label> <fo:list-item-body start-indent="body-start()"> <fo:block> <!-- This fo:block contains a 'regular' fo:footnote inside a fo:list-block. Note that the fo:footnote-body doesn't get rendered in the output, due to FOP-1099 ( http://issues.apache.org/bugzilla/show_bug.cgi?id=37579 ) --> List item with a footnote<fo:footnote> <fo:inline font-size="60%" baseline-shift="super">1)</fo:inline> <fo:footnote-body> <fo:block start-indent="0.5cm" text-indent="-0.5cm"> <fo:inline font-size="60%" baseline-shift="super">1)</fo:inline> This footnote doesn't get rendered.</fo:block> </fo:footnote-body> </fo:footnote>. </fo:block> </fo:list-item-body> </fo:list-item> <fo:list-item> <fo:list-item-label end-indent="label-end()"> <fo:block>label</fo:block> </fo:list-item-label> <fo:list-item-body start-indent="body-start()"> <fo:block> <!-- this footnote is only marked inline by a fo:inline marker --> List item with a footnote<fo:inline font-size="60%" baseline-shift="super">2)</fo:inline>. </fo:block> </fo:list-item-body> </fo:list-item> </fo:list-block> <!-- this block contains the fo:footnote areas for all separate footnotes in the previous fo:list-block --> <fo:block> <fo:footnote> <!-- this fo:inline footnote marker is empty to avoid it getting output after the fo:list-block --> <fo:inline/> <fo:footnote-body> <fo:block start-indent="0.5cm" text-indent="-0.5cm"> <fo:inline font-size="60%" baseline-shift="super">2)</fo:inline> This footnote does get rendered.</fo:block> </fo:footnote-body> </fo:footnote> </fo:block> </fo:flow> </fo:page-sequence> </fo:root> Note that this approach will require some refinements. For example, for lists / tables that span multiple pages, the footnotes will all end up before / after the affected list / table (depending on the placement of their containing block). In order to avoid this, the fo:list-block / fo:table areas could be generated at the lower level of the list items, i.e. the input list / table will not generate a fo:list-block / fo:table, but each list item / table row will. Each of those fo:list-block / fo:table areas can then be followed by a fo:block containing the relevant fo:footnote area. Of course this produces a lot of one-item lists / one-row tables, but it also guarantees that footnotes will show at the right page when they occur in long lists / tables. I haven't tested it in my production XSL-FO stylesheets, and of course treatment of nested lists would demand further consideration, but I think the principle works.
          Hide
          ron van den branden added a comment -

          In the mean time, I have further investigated the approach mentioned in the previous comment, and documented this research in a blog post at <http://rvdb.wordpress.com/2008/03/07/rendering-footnotes-in-tables-and-lists-with-fop/>.
          It describes how far it got me in circumventing the bug and indicates where problems remain. It also provides a link to a downloadable zip
          package containing sample source XML documents and an XSLT stylesheet
          implementing the techniques discussed. I hope this can a) help others
          looking for a solution to this problem and b) lead to a better solution.

          Show
          ron van den branden added a comment - In the mean time, I have further investigated the approach mentioned in the previous comment, and documented this research in a blog post at < http://rvdb.wordpress.com/2008/03/07/rendering-footnotes-in-tables-and-lists-with-fop/ >. It describes how far it got me in circumventing the bug and indicates where problems remain. It also provides a link to a downloadable zip package containing sample source XML documents and an XSLT stylesheet implementing the techniques discussed. I hope this can a) help others looking for a solution to this problem and b) lead to a better solution.
          Hide
          Luca Furini added a comment -

          Attachment footnotes.080502.diff has been added with description: Partial patch, updated

          Show
          Luca Furini added a comment - Attachment footnotes.080502.diff has been added with description: Partial patch, updated
          Hide
          Luca Furini added a comment -

          I'm attaching an updated patch, that can be applied to the trunk as it is now.

          It basically does the same thing as the old one, just in a slightly different way due to the new classes involved in the table management.

          The 5 "shortcomings" listed by Gerhard Oettl in comment #4 are still present, although I'm not sure whether the second one (indentation of notes whose citation is in the list-item-body) is a bug or the right behaviour, because of indent inheritance ...

          I've tried to track down the LengthBase error message, but I could not find where the problem really is: the strange thing is that it happens only for footnotes in the list-item-label, and not for those in the list-item-body ...

          Footnotes on table headers / footers will probably need a special handling, as they behave quite differently from the "normal" ones, being repeated.

          Show
          Luca Furini added a comment - I'm attaching an updated patch, that can be applied to the trunk as it is now. It basically does the same thing as the old one, just in a slightly different way due to the new classes involved in the table management. The 5 "shortcomings" listed by Gerhard Oettl in comment #4 are still present, although I'm not sure whether the second one (indentation of notes whose citation is in the list-item-body) is a bug or the right behaviour, because of indent inheritance ... I've tried to track down the LengthBase error message, but I could not find where the problem really is: the strange thing is that it happens only for footnotes in the list-item-label, and not for those in the list-item-body ... Footnotes on table headers / footers will probably need a special handling, as they behave quite differently from the "normal" ones, being repeated.
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #16)
          > I'm attaching an updated patch, that can be applied to the trunk as it is now.

          Thanks! Good to see you back in action

          >
          > It basically does the same thing as the old one, just in a slightly different
          > way due to the new classes involved in the table management.
          >
          > The 5 "shortcomings" listed by Gerhard Oettl in comment #4 are still present,
          > although I'm not sure whether the second one (indentation of notes whose
          > citation is in the list-item-body) is a bug or the right behaviour, because of
          > indent inheritance ...

          It is correct behavior. No special rules are defined for property-inheritance on fo:footnotes, so default inheritance applies. Stylesheet authors should reset the indent to zero on the fo:footnote, if so desired.

          >
          > I've tried to track down the LengthBase error message, but I could not find
          > where the problem really is: the strange thing is that it happens only for
          > footnotes in the list-item-label, and not for those in the list-item-body ...

          The reason seems to occur due to resolution of the label-end() function that is triggered from within FootnoteBodyLM.
          Looking at org.apache.fop.fo.expr.LabelEndFunction, there is a component PercentLength being created. Resolution of this percentage during normal layout is done by looking up the associated ancestor LM, but since (if I interpret correctly) the FootnoteBodyLM is not a descendant of the original ListItemLayoutManager, this resolution fails...

          Setting the fo:footnote's end-indent to "0pt" eliminates the dependency on label-end() in the footnote, and thus also the warnings.

          >
          > Footnotes on table headers / footers will probably need a special handling, as
          > they behave quite differently from the "normal" ones, being repeated.
          >

          Right, my first instinct would be to include footnotes for the table-header only on the first page that is spanned by the table, and for the table-footer only on the last page.

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #16) > I'm attaching an updated patch, that can be applied to the trunk as it is now. Thanks! Good to see you back in action > > It basically does the same thing as the old one, just in a slightly different > way due to the new classes involved in the table management. > > The 5 "shortcomings" listed by Gerhard Oettl in comment #4 are still present, > although I'm not sure whether the second one (indentation of notes whose > citation is in the list-item-body) is a bug or the right behaviour, because of > indent inheritance ... It is correct behavior. No special rules are defined for property-inheritance on fo:footnotes, so default inheritance applies. Stylesheet authors should reset the indent to zero on the fo:footnote, if so desired. > > I've tried to track down the LengthBase error message, but I could not find > where the problem really is: the strange thing is that it happens only for > footnotes in the list-item-label, and not for those in the list-item-body ... The reason seems to occur due to resolution of the label-end() function that is triggered from within FootnoteBodyLM. Looking at org.apache.fop.fo.expr.LabelEndFunction, there is a component PercentLength being created. Resolution of this percentage during normal layout is done by looking up the associated ancestor LM, but since (if I interpret correctly) the FootnoteBodyLM is not a descendant of the original ListItemLayoutManager, this resolution fails... Setting the fo:footnote's end-indent to "0pt" eliminates the dependency on label-end() in the footnote, and thus also the warnings. > > Footnotes on table headers / footers will probably need a special handling, as > they behave quite differently from the "normal" ones, being repeated. > Right, my first instinct would be to include footnotes for the table-header only on the first page that is spanned by the table, and for the table-footer only on the last page.
          Hide
          Andreas L. Delmelle added a comment -


          Just ran the layoutengine test-suite after having applied the patch, and it seems this causes quite a few tests to break. One of the reasons is an IndexOutOfBoundsException at TableStepper line#237...

          Show
          Andreas L. Delmelle added a comment - Just ran the layoutengine test-suite after having applied the patch, and it seems this causes quite a few tests to break. One of the reasons is an IndexOutOfBoundsException at TableStepper line#237...
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #18)
          > Just ran the layoutengine test-suite after having applied the patch, and it
          > seems this causes quite a few tests to break. One of the reasons is an
          > IndexOutOfBoundsException at TableStepper line#237...
          >

          Found the cause of this: the error was that the additional code in TableStepper assumed the list of activeCells to be always equal to the number of columns. Changing the code-fragment to be based on activeCells.size() instead of columnCount did the trick.

          Also, but that's maybe more of a personal preference, IMO the ElementListUtils.collectFootnodeBodyLMs() implementations should better be turned around for reasons of code-clarity.
          Right now, the implementation dealing with a single List creates one-element arrays and delegates to the implementation for an array of List. Somehow, I like the opposite where a call to the latter implementation expands into multiple calls to the 'simple' implementation.

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #18) > Just ran the layoutengine test-suite after having applied the patch, and it > seems this causes quite a few tests to break. One of the reasons is an > IndexOutOfBoundsException at TableStepper line#237... > Found the cause of this: the error was that the additional code in TableStepper assumed the list of activeCells to be always equal to the number of columns. Changing the code-fragment to be based on activeCells.size() instead of columnCount did the trick. Also, but that's maybe more of a personal preference, IMO the ElementListUtils.collectFootnodeBodyLMs() implementations should better be turned around for reasons of code-clarity. Right now, the implementation dealing with a single List creates one-element arrays and delegates to the implementation for an array of List. Somehow, I like the opposite where a call to the latter implementation expands into multiple calls to the 'simple' implementation.
          Hide
          Andreas L. Delmelle added a comment -

          Attachment bugzilla37579.patch has been added with description: Updated patch against FOP Trunk, which passes all layout-tests and enables two more

          Show
          Andreas L. Delmelle added a comment - Attachment bugzilla37579.patch has been added with description: Updated patch against FOP Trunk, which passes all layout-tests and enables two more
          Hide
          Andreas L. Delmelle added a comment -

          Further investigation into the ordering of footnotes for table-header and table-footer revealed that they are added to the first/last page in case we add to the table:

          table-omit-header-at-break="true"

          The reason, if I judge correctly, is that in this case, the header's element-list is added to the head of the combined element list. In the default case, with the header repeated at each break, that list will be the next-to-last element in the combined list. The footer will be the last, if present.

          The infinite loop seems to point to a borderline-case where there is 'just (not) enough' room to fit both the table and all its footnotes on one page. Further investigation pending...

          The problem with getLengthBase() seems to point to a difficulty with property-inheritance: strictly speaking, the footnote should inherit the computed value for start-indent(), but during property resolution, this value is not yet known if the specified value is "label-end()". Since LabelEndFunction makes use of a PercentLength that can only be evaluated during layout, the descendants of the list-item-label inherit the expression:

          reference-area-width - (provisional-distance-between-starts + start-indent - provisional-label-separation)

          Show
          Andreas L. Delmelle added a comment - Further investigation into the ordering of footnotes for table-header and table-footer revealed that they are added to the first/last page in case we add to the table: table-omit-header-at-break="true" The reason, if I judge correctly, is that in this case, the header's element-list is added to the head of the combined element list. In the default case, with the header repeated at each break, that list will be the next-to-last element in the combined list. The footer will be the last, if present. The infinite loop seems to point to a borderline-case where there is 'just (not) enough' room to fit both the table and all its footnotes on one page. Further investigation pending... The problem with getLengthBase() seems to point to a difficulty with property-inheritance: strictly speaking, the footnote should inherit the computed value for start-indent(), but during property resolution, this value is not yet known if the specified value is "label-end()". Since LabelEndFunction makes use of a PercentLength that can only be evaluated during layout, the descendants of the list-item-label inherit the expression: reference-area-width - (provisional-distance-between-starts + start-indent - provisional-label-separation)
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #21)
          > The problem with getLengthBase() seems to point to a difficulty with
          > property-inheritance: strictly speaking, the footnote should inherit the
          > computed value for start-indent(),

          Sorry, I obviously meant "end-indent".

          That said, taking indent-inheritance into account, not resetting end-indent on an fo:footnote that is a descendant of an fo:list-item-label is very likely to count as 'bad practice'.

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #21) > The problem with getLengthBase() seems to point to a difficulty with > property-inheritance: strictly speaking, the footnote should inherit the > computed value for start-indent(), Sorry, I obviously meant "end-indent". That said, taking indent-inheritance into account, not resetting end-indent on an fo:footnote that is a descendant of an fo:list-item-label is very likely to count as 'bad practice'.
          Hide
          Luca Furini added a comment -

          Thank you for all your comments and additions, Andreas!
          It's good to be back after quite a long time (enough to forget the good old habit of using JUnit!)

          (In reply to comment #22)

          > (In reply to comment #21)
          > > The problem with getLengthBase() seems to point to a difficulty with
          > > property-inheritance: strictly speaking, the footnote should inherit the
          > > computed value for start-indent(),
          >
          > Sorry, I obviously meant "end-indent".

          What I still cannot understand is why there is no such problem with start-indent = "body-start", which is resolved ok ...

          (In reply to comment #17)

          > Right, my first instinct would be to include footnotes for the table-header
          > only on the first page that is spanned by the table, and for the table-footer
          > only on the last page.

          It's an interesting idea, and probably the easiest to implement.
          Personally, I would have placed them all in the first page spanned by the table, although this would be a bit more problematic in terms of relative order between the footnotes.
          What do other think in this regard?

          Anyway, I think this is a situation not perfectly covered by the specs, which forbid footnotes in static-contents but say nothing about footnotes in table headers / footers, which are not so different.

          The condition defining where a block area returned by fo:footnote is permitted does not explicitly take into account the situation when a single fo:foonote generates several anchor areas in different pages, although the definition

          "The term anchor-area is defined to mean the last area that is generated and returned by the fo:inline child of the fo:footnote."

          could maybe be read as a justification for placing all the notes in the last page where the table appears ...

          Show
          Luca Furini added a comment - Thank you for all your comments and additions, Andreas! It's good to be back after quite a long time (enough to forget the good old habit of using JUnit!) (In reply to comment #22) > (In reply to comment #21) > > The problem with getLengthBase() seems to point to a difficulty with > > property-inheritance: strictly speaking, the footnote should inherit the > > computed value for start-indent(), > > Sorry, I obviously meant "end-indent". What I still cannot understand is why there is no such problem with start-indent = "body-start", which is resolved ok ... (In reply to comment #17) > Right, my first instinct would be to include footnotes for the table-header > only on the first page that is spanned by the table, and for the table-footer > only on the last page. It's an interesting idea, and probably the easiest to implement. Personally, I would have placed them all in the first page spanned by the table, although this would be a bit more problematic in terms of relative order between the footnotes. What do other think in this regard? Anyway, I think this is a situation not perfectly covered by the specs, which forbid footnotes in static-contents but say nothing about footnotes in table headers / footers, which are not so different. The condition defining where a block area returned by fo:footnote is permitted does not explicitly take into account the situation when a single fo:foonote generates several anchor areas in different pages, although the definition "The term anchor-area is defined to mean the last area that is generated and returned by the fo:inline child of the fo:footnote." could maybe be read as a justification for placing all the notes in the last page where the table appears ...
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #23)
          > Thank you for all your comments and additions, Andreas!
          > It's good to be back after quite a long time (enough to forget the good old
          > habit of using JUnit!)

          No problem. That's why it's A Good Thing that there's more of us.

          > > (In reply to comment #21)
          > > > The problem with getLengthBase() seems to point to a difficulty with
          > > > property-inheritance: strictly speaking, the footnote should inherit the
          > > > computed value for start-indent(),
          > >
          > > Sorry, I obviously meant "end-indent".
          >
          > What I still cannot understand is why there is no such problem with
          > start-indent = "body-start", which is resolved ok ...

          They are implemented differently: see org.apache.fop.fo.expr.BodyStartFunction and .LabelEndFunction. label-end() makes explicit use of a percentage, body-start() does not...

          It is only that percentage which causes the error. For 'normal' list-item-label descendants, the base-length is found by climbing up the LM tree until the corresponding LM is found. That's what happens in AbstractBaseLM.getBaseLength().
          Come to think of it, maybe a way to avoid the warning would be to reimplement getBaseLength() in FootnoteBodyLM, to do something else than try out all ancestors... (long shot)

          > > Right, my first instinct would be to include footnotes for the table-header
          > > only on the first page that is spanned by the table, and for the table-footer
          > > only on the last page.
          >
          > It's an interesting idea, and probably the easiest to implement.
          > Personally, I would have placed them all in the first page spanned by the
          > table, although this would be a bit more problematic in terms of relative order
          > between the footnotes.

          Yours is probably the better idea... Since the footer can appear on the first page, it would indeed be confusing to have its footnote appear maybe 10 pages after the citation first appears.

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #23) > Thank you for all your comments and additions, Andreas! > It's good to be back after quite a long time (enough to forget the good old > habit of using JUnit!) No problem. That's why it's A Good Thing that there's more of us. > > (In reply to comment #21) > > > The problem with getLengthBase() seems to point to a difficulty with > > > property-inheritance: strictly speaking, the footnote should inherit the > > > computed value for start-indent(), > > > > Sorry, I obviously meant "end-indent". > > What I still cannot understand is why there is no such problem with > start-indent = "body-start", which is resolved ok ... They are implemented differently: see org.apache.fop.fo.expr.BodyStartFunction and .LabelEndFunction. label-end() makes explicit use of a percentage, body-start() does not... It is only that percentage which causes the error. For 'normal' list-item-label descendants, the base-length is found by climbing up the LM tree until the corresponding LM is found. That's what happens in AbstractBaseLM.getBaseLength(). Come to think of it, maybe a way to avoid the warning would be to reimplement getBaseLength() in FootnoteBodyLM, to do something else than try out all ancestors... (long shot) > > Right, my first instinct would be to include footnotes for the table-header > > only on the first page that is spanned by the table, and for the table-footer > > only on the last page. > > It's an interesting idea, and probably the easiest to implement. > Personally, I would have placed them all in the first page spanned by the > table, although this would be a bit more problematic in terms of relative order > between the footnotes. Yours is probably the better idea... Since the footer can appear on the first page, it would indeed be confusing to have its footnote appear maybe 10 pages after the citation first appears.
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #24)
          > ...
          > It is only that percentage which causes the error. For 'normal' list-item-label
          > descendants, the base-length is found by climbing up the LM tree until the
          > corresponding LM is found. That's what happens in
          > AbstractBaseLM.getBaseLength().
          > Come to think of it, maybe a way to avoid the warning would be to reimplement
          > getBaseLength() in FootnoteBodyLM, to do something else than try out all
          > ancestors... (long shot)

          Did some further research, and the ancestor tree for both footnotes' LMs (label and body) looks like:
          ListItemContentLayoutManager
          -> BlockLayoutManager
          -> LineLayoutManager
          -> FootnoteLayoutManager
          -> FootnoteBodyLayoutManager

          The default getBaseLength() in percentage resolution relies on the LM's getFObj() method to find the LM corresponding to the FO on which the percentage is based (the one that is passed as an argument to the PercentLength constructor in LabelEndFunction).

          For the ListItemContentLayoutManager, this method always returns the ListItemBody (never the ListItemLabel).

          It would probably work as expected if we had separate ListItemLabelLM and ListItemBodyLM. Similar issues occur with the table-related objects BTW, where we also do not have a 1-1 correspondence between FO and LM...

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #24) > ... > It is only that percentage which causes the error. For 'normal' list-item-label > descendants, the base-length is found by climbing up the LM tree until the > corresponding LM is found. That's what happens in > AbstractBaseLM.getBaseLength(). > Come to think of it, maybe a way to avoid the warning would be to reimplement > getBaseLength() in FootnoteBodyLM, to do something else than try out all > ancestors... (long shot) Did some further research, and the ancestor tree for both footnotes' LMs (label and body) looks like: ListItemContentLayoutManager -> BlockLayoutManager -> LineLayoutManager -> FootnoteLayoutManager -> FootnoteBodyLayoutManager The default getBaseLength() in percentage resolution relies on the LM's getFObj() method to find the LM corresponding to the FO on which the percentage is based (the one that is passed as an argument to the PercentLength constructor in LabelEndFunction). For the ListItemContentLayoutManager, this method always returns the ListItemBody (never the ListItemLabel). It would probably work as expected if we had separate ListItemLabelLM and ListItemBodyLM. Similar issues occur with the table-related objects BTW, where we also do not have a 1-1 correspondence between FO and LM...
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #25)
          >
          > Did some further research, and the ancestor tree for both footnotes' LMs (label
          > and body) looks like:
          > ListItemContentLayoutManager
          > -> BlockLayoutManager
          > -> LineLayoutManager
          > -> FootnoteLayoutManager
          > -> FootnoteBodyLayoutManager

          Strike that... Stopped debugging too early. The label's LM does appear if I leave it running...

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #25) > > Did some further research, and the ancestor tree for both footnotes' LMs (label > and body) looks like: > ListItemContentLayoutManager > -> BlockLayoutManager > -> LineLayoutManager > -> FootnoteLayoutManager > -> FootnoteBodyLayoutManager Strike that... Stopped debugging too early. The label's LM does appear if I leave it running...
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #26)
          >
          > Strike that... Stopped debugging too early. The label's LM does appear if I
          > leave it running...
          >

          At the point where getBaseLength() fails, the ancestor tree looks like:

          FlowLayoutManager
          -> FootnoteBodyLM

          So, the LM is reparented (in PageBreaker, line 166), and somewhere after that, we get a call to PercentLength.getValue(footnoteBodyLayoutManager)

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #26) > > Strike that... Stopped debugging too early. The label's LM does appear if I > leave it running... > At the point where getBaseLength() fails, the ancestor tree looks like: FlowLayoutManager -> FootnoteBodyLM So, the LM is reparented (in PageBreaker, line 166), and somewhere after that, we get a call to PercentLength.getValue(footnoteBodyLayoutManager)
          Hide
          Andreas L. Delmelle added a comment -

          Naively tried adding an override for getParent() to FootnoteBodyLM that always returns the associated FootnoteLM, and then it works without errors, but as I expected, the footnote's end-indent is then equal to that of the label. May count as a fix, though...
          I haven't checked yet if there's a situation where FootnoteBodyLM.getParent() is supposed to return the FlowLM.

          Show
          Andreas L. Delmelle added a comment - Naively tried adding an override for getParent() to FootnoteBodyLM that always returns the associated FootnoteLM, and then it works without errors, but as I expected, the footnote's end-indent is then equal to that of the label. May count as a fix, though... I haven't checked yet if there's a situation where FootnoteBodyLM.getParent() is supposed to return the FlowLM.
          Hide
          Andreas L. Delmelle added a comment -

          Attachment b37579_2.patch has been added with description: Patch for FootnoteBodyLayoutManager

          Show
          Andreas L. Delmelle added a comment - Attachment b37579_2.patch has been added with description: Patch for FootnoteBodyLayoutManager
          Hide
          Andreas L. Delmelle added a comment -

          In the meantime, I managed to track down the point of origin for the infinite loop. No solution yet, but it happens in PageBreakingAlgorithm.createFootnotePages(), when it is called for the second page (which I presume to be a footnote-only page).
          The member 'insertedFootnotesLength' never changes, and it is less than the 'totalFootnotesLength', so the loop body is never exited.

          Adding a third variable to check whether the first value has actually changed compared to the previous iteration, makes the loop finish, but apparently triggers creation of so many nodes, that no matter how much heap you give it, it still runs out of memory...

          Show
          Andreas L. Delmelle added a comment - In the meantime, I managed to track down the point of origin for the infinite loop. No solution yet, but it happens in PageBreakingAlgorithm.createFootnotePages(), when it is called for the second page (which I presume to be a footnote-only page). The member 'insertedFootnotesLength' never changes, and it is less than the 'totalFootnotesLength', so the loop body is never exited. Adding a third variable to check whether the first value has actually changed compared to the previous iteration, makes the loop finish, but apparently triggers creation of so many nodes, that no matter how much heap you give it, it still runs out of memory...
          Hide
          Andreas L. Delmelle added a comment -

          Good news: No idea why exactly, but I just tried again to reproduce the infinite loop with the latest trunk, and couldn't. Maybe has something to do with the changes Vincent has committed to the table-layout code earlier today

          The bad news is that the footnote in the table-footer is now skipped/swallowed...

          Show
          Andreas L. Delmelle added a comment - Good news: No idea why exactly, but I just tried again to reproduce the infinite loop with the latest trunk, and couldn't. Maybe has something to do with the changes Vincent has committed to the table-layout code earlier today The bad news is that the footnote in the table-footer is now skipped/swallowed...
          Hide
          Andreas L. Delmelle added a comment -

          Just had another look, and it seems to have been a fluke...

          The infinite loop remains. Debugging it, I can almost 'see' what's going wrong, but any attempt to bypass it so far, have failed. If not for this case, then for the unit-tests for split footnotes.

          The story goes:
          -> after the page-breaking loop completes, the first page is finished, and its footnotes added as much as possible (4 footnotes in this case)
          -> after that, we see there's still one footnote left which has its citation on that page (PBA.finish(): (node.totalFootnotes < PBA.totalFootnotesLength))
          -> so createFootnotePages() is called, which starts at the last footnote that was added to the previous page; could be a split footnote

          => We hang indefinitely in:
          ...
          while (insertedFootnotesLength < totalFootnotesLength) {
          tmpLength = ((Integer)lengthList.get(footnoteListIndex)).intValue();
          // try adding some more content
          if ((tmpLength - insertedFootnotesLength) <= availableBPD)

          { // add a whole footnote availableBPD -= tmpLength - insertedFootnotesLength; insertedFootnotesLength = tmpLength; footnoteElementIndex = ((LinkedList)footnotesList.get(footnoteListIndex)).size() - 1; ... ... }

          insertedFootnotesLength never changes, availableBPD always remains the same (tmpLength == insertedFootnotesLength), so the condition to break the while-loop is never reached.

          Show
          Andreas L. Delmelle added a comment - Just had another look, and it seems to have been a fluke... The infinite loop remains. Debugging it, I can almost 'see' what's going wrong, but any attempt to bypass it so far, have failed. If not for this case, then for the unit-tests for split footnotes. The story goes: -> after the page-breaking loop completes, the first page is finished, and its footnotes added as much as possible (4 footnotes in this case) -> after that, we see there's still one footnote left which has its citation on that page (PBA.finish(): (node.totalFootnotes < PBA.totalFootnotesLength)) -> so createFootnotePages() is called, which starts at the last footnote that was added to the previous page; could be a split footnote => We hang indefinitely in: ... while (insertedFootnotesLength < totalFootnotesLength) { tmpLength = ((Integer)lengthList.get(footnoteListIndex)).intValue(); // try adding some more content if ((tmpLength - insertedFootnotesLength) <= availableBPD) { // add a whole footnote availableBPD -= tmpLength - insertedFootnotesLength; insertedFootnotesLength = tmpLength; footnoteElementIndex = ((LinkedList)footnotesList.get(footnoteListIndex)).size() - 1; ... ... } insertedFootnotesLength never changes, availableBPD always remains the same (tmpLength == insertedFootnotesLength), so the condition to break the while-loop is never reached.
          Hide
          Andreas L. Delmelle added a comment -

          In the meantime, I also compared the behavior of the table-testcase with our layout-test for footnote splits.

          The one notable difference between the two cases: if a footnote appears in a one line block, and none of the lines of the footnote fit on the page, then the entire block is moved. If there is a keep-with-previous.within-page="always" on the block, then the preceding block is moved along. Only if at least one line of the footnote fits, a part of the block is also put on the first page.
          Seems like something similar is happening with the footnote in the table-footer. The footnote does not fit, not even one line, but the table-footer cannot be moved (either completely or partially) to the next page either.

          Show
          Andreas L. Delmelle added a comment - In the meantime, I also compared the behavior of the table-testcase with our layout-test for footnote splits. The one notable difference between the two cases: if a footnote appears in a one line block, and none of the lines of the footnote fit on the page, then the entire block is moved. If there is a keep-with-previous.within-page="always" on the block, then the preceding block is moved along. Only if at least one line of the footnote fits, a part of the block is also put on the first page. Seems like something similar is happening with the footnote in the table-footer. The footnote does not fit, not even one line, but the table-footer cannot be moved (either completely or partially) to the next page either.
          Hide
          Luca Furini added a comment -

          (In reply to comment #32)

          > The footnote does not fit, not even one line, but the
          > table-footer cannot be moved (either completely or partially) to the next page
          > either.

          Brilliant investigation, Andreas!
          So there are two main differences between "normal" footnotes and those in table-header / footer (when table-omit-*-at-break = false), as the latters:

          • need to be repeated in each page where their related table appears
          • cannot be "delayed" to the next page as this would lead to a duplication, nor they can be moved together with the header

          From the implementation point of view, this seems to suggest that, as the table-header (or footer) and its footnotes are actually indivisible , their overall length could be used to generate the table elements ... altough this would not be 100% accurate as the proper space resolution with the other surrounding footnotes would be lost.

          Mmhh, I need to think about this some more time ...

          Well, there is at least a case when partially deferring table-footer footnotes would not be completely ugly: when the table content is finished, so that there would not be footnote repetition in the next page

          Show
          Luca Furini added a comment - (In reply to comment #32) > The footnote does not fit, not even one line, but the > table-footer cannot be moved (either completely or partially) to the next page > either. Brilliant investigation, Andreas! So there are two main differences between "normal" footnotes and those in table-header / footer (when table-omit-*-at-break = false), as the latters: need to be repeated in each page where their related table appears cannot be "delayed" to the next page as this would lead to a duplication, nor they can be moved together with the header From the implementation point of view, this seems to suggest that, as the table-header (or footer) and its footnotes are actually indivisible , their overall length could be used to generate the table elements ... altough this would not be 100% accurate as the proper space resolution with the other surrounding footnotes would be lost. Mmhh, I need to think about this some more time ... Well, there is at least a case when partially deferring table-footer footnotes would not be completely ugly: when the table content is finished, so that there would not be footnote repetition in the next page
          Hide
          Andreas L. Delmelle added a comment -

          Update:
          Adrian contacted me off-list to see if we could at least partially apply the attached patches.
          My proposal would be to integrate the changes for footnote support in lists and the table-body, but leave headers/footers out for the moment. This restriction should obviously be reflected in the documentation.

          Unless any fop-devs object to this, I'll go ahead, create some additional checks in the existing jUnit tests, update the docs, and apply those changes in the weekend.

          Show
          Andreas L. Delmelle added a comment - Update: Adrian contacted me off-list to see if we could at least partially apply the attached patches. My proposal would be to integrate the changes for footnote support in lists and the table-body, but leave headers/footers out for the moment. This restriction should obviously be reflected in the documentation. Unless any fop-devs object to this, I'll go ahead, create some additional checks in the existing jUnit tests, update the docs, and apply those changes in the weekend.
          Hide
          Adrian Cumiskey added a comment -

          I think this patch should only be applied when it is ready, looks like there is still quite a bit of cleanup to do. Lets try and have a model that encapsulates a complete solution to the problem in all cases otherwise supporting this feature will be more difficult and confusing for users. I think ElementListUtils.collectFootnoteBodyLMs() could be revised somewhat.

          (In reply to comment #34)
          > Update:
          > Adrian contacted me off-list to see if we could at least partially apply the
          > attached patches.
          > My proposal would be to integrate the changes for footnote support in lists and
          > the table-body, but leave headers/footers out for the moment. This restriction
          > should obviously be reflected in the documentation.
          >
          > Unless any fop-devs object to this, I'll go ahead, create some additional
          > checks in the existing jUnit tests, update the docs, and apply those changes in
          > the weekend.
          >

          Show
          Adrian Cumiskey added a comment - I think this patch should only be applied when it is ready, looks like there is still quite a bit of cleanup to do. Lets try and have a model that encapsulates a complete solution to the problem in all cases otherwise supporting this feature will be more difficult and confusing for users. I think ElementListUtils.collectFootnoteBodyLMs() could be revised somewhat. (In reply to comment #34) > Update: > Adrian contacted me off-list to see if we could at least partially apply the > attached patches. > My proposal would be to integrate the changes for footnote support in lists and > the table-body, but leave headers/footers out for the moment. This restriction > should obviously be reflected in the documentation. > > Unless any fop-devs object to this, I'll go ahead, create some additional > checks in the existing jUnit tests, update the docs, and apply those changes in > the weekend. >
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #35)
          > I think this patch should only be applied when it is ready, looks like there is
          > still quite a bit of cleanup to do. Lets try and have a model that
          > encapsulates a complete solution to the problem in all cases otherwise
          > supporting this feature will be more difficult and confusing for users. I
          > think ElementListUtils.collectFootnoteBodyLMs() could be revised somewhat.

          I'd be happy to look into that. Can you be more specific? The method in question is a general-purpose utility method that can potentially be used by all related LayoutManagers to extract the list of footnotes from an element-list generated by one of their descendants. I don't really see what could or should be revised, so if you have any suggestions...

          It works like a charm anyway, apart from the mentioned anomaly with table-footers.

          Of course, we could also leave it another two years... ;-P

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #35) > I think this patch should only be applied when it is ready, looks like there is > still quite a bit of cleanup to do. Lets try and have a model that > encapsulates a complete solution to the problem in all cases otherwise > supporting this feature will be more difficult and confusing for users. I > think ElementListUtils.collectFootnoteBodyLMs() could be revised somewhat. I'd be happy to look into that. Can you be more specific? The method in question is a general-purpose utility method that can potentially be used by all related LayoutManagers to extract the list of footnotes from an element-list generated by one of their descendants. I don't really see what could or should be revised, so if you have any suggestions... It works like a charm anyway, apart from the mentioned anomaly with table-footers. Of course, we could also leave it another two years... ;-P
          Hide
          Vincent Hennebert added a comment -

          (In reply to comment #36)
          > (In reply to comment #35)
          > > I think this patch should only be applied when it is ready, looks like there is
          > > still quite a bit of cleanup to do. Lets try and have a model that
          > > encapsulates a complete solution to the problem in all cases otherwise
          > > supporting this feature will be more difficult and confusing for users. I
          > > think ElementListUtils.collectFootnoteBodyLMs() could be revised somewhat.
          >
          > I'd be happy to look into that. Can you be more specific? The method in
          > question is a general-purpose utility method that can potentially be used by
          > all related LayoutManagers to extract the list of footnotes from an
          > element-list generated by one of their descendants. I don't really see what
          > could or should be revised, so if you have any suggestions...
          >
          > It works like a charm anyway, apart from the mentioned anomaly with
          > table-footers.

          I'm sorry to chime in so late, but so far I haven't had the time and energy to approach this topic.

          Anyway, I've just had a look at the patch and I'm afraid I don't think it's ready to be committed.
          I see mainly the following problems:

          • from a high-level point of view first: list- and table-related code should remain totally footnote-agnostic. The footnote-handling code should remain in a single class and not be spread over the codebase, which would make it error-prone and difficult to maintain and understand.
          • not sure the collectFootnoteBodyLMs taking array parameters is any useful. In the calling code a new array is created and populated with the element lists to parse, and in the method this array is iterated over to get back to the single element lists...
            Below I will only speak for the table code, because it's the only one in the code impacted by the patch that I know well, but I think the changes don't fit well in it:
          • in TableStepper, ActiveCells aren't ordered by their column indices! Instead they are appended to the list once they start contributing content for the merging algorithm. This means that new cells will be found /after/ cells starting on earlier rows and spanning over the current one, even if they are in earlier columns. So basically the code added in TableStepper doesn't work...
          • in CellPart, there's no reason why the getStartIndex and getEndIndex methods should be made public, package-private would be enough. But in the first place footnotes shouldn't really be handled that way. There is a negative impact on performance since at every iteration, the cells' (linked) lists of elements will be re-skimmed through from the beginning up to the start index.

          An intermediate solution could probably be implemented the following way:

          • in ActiveCell.Step, add a List field that would contain the FootnoteBodyLMs encountered during the last call to gotoNextLegalBreak;
          • in TableStepper.getCombinedKnuthElements, when iterating over the active cells to create the CellPart instances, get those FootnoteBodyLMs in the same time. A small detail to pay attention to is to not re-get them if the active cell doesn't contribute new content to the step. If there are some footnotes, create a KnuthBlockBox, otherwise create a normal Box.
            And that should be it basically...
            I'll see if I can submit a patch to illustrate my ideas in the next days.

          Vincent

          Show
          Vincent Hennebert added a comment - (In reply to comment #36) > (In reply to comment #35) > > I think this patch should only be applied when it is ready, looks like there is > > still quite a bit of cleanup to do. Lets try and have a model that > > encapsulates a complete solution to the problem in all cases otherwise > > supporting this feature will be more difficult and confusing for users. I > > think ElementListUtils.collectFootnoteBodyLMs() could be revised somewhat. > > I'd be happy to look into that. Can you be more specific? The method in > question is a general-purpose utility method that can potentially be used by > all related LayoutManagers to extract the list of footnotes from an > element-list generated by one of their descendants. I don't really see what > could or should be revised, so if you have any suggestions... > > It works like a charm anyway, apart from the mentioned anomaly with > table-footers. I'm sorry to chime in so late, but so far I haven't had the time and energy to approach this topic. Anyway, I've just had a look at the patch and I'm afraid I don't think it's ready to be committed. I see mainly the following problems: from a high-level point of view first: list- and table-related code should remain totally footnote-agnostic. The footnote-handling code should remain in a single class and not be spread over the codebase, which would make it error-prone and difficult to maintain and understand. not sure the collectFootnoteBodyLMs taking array parameters is any useful. In the calling code a new array is created and populated with the element lists to parse, and in the method this array is iterated over to get back to the single element lists... Below I will only speak for the table code, because it's the only one in the code impacted by the patch that I know well, but I think the changes don't fit well in it: in TableStepper, ActiveCells aren't ordered by their column indices! Instead they are appended to the list once they start contributing content for the merging algorithm. This means that new cells will be found /after/ cells starting on earlier rows and spanning over the current one, even if they are in earlier columns. So basically the code added in TableStepper doesn't work... in CellPart, there's no reason why the getStartIndex and getEndIndex methods should be made public, package-private would be enough. But in the first place footnotes shouldn't really be handled that way. There is a negative impact on performance since at every iteration, the cells' (linked) lists of elements will be re-skimmed through from the beginning up to the start index. An intermediate solution could probably be implemented the following way: in ActiveCell.Step, add a List field that would contain the FootnoteBodyLMs encountered during the last call to gotoNextLegalBreak; in TableStepper.getCombinedKnuthElements, when iterating over the active cells to create the CellPart instances, get those FootnoteBodyLMs in the same time. A small detail to pay attention to is to not re-get them if the active cell doesn't contribute new content to the step. If there are some footnotes, create a KnuthBlockBox, otherwise create a normal Box. And that should be it basically... I'll see if I can submit a patch to illustrate my ideas in the next days. Vincent
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #37)
          >
          > I'm sorry to chime in so late, but so far I haven't had the time and energy to
          > approach this topic.

          No problem.

          >
          > Anyway, I've just had a look at the patch and I'm afraid I don't think it's
          > ready to be committed.
          > I see mainly the following problems:
          > - from a high-level point of view first: list- and table-related code should
          > remain totally footnote-agnostic. The footnote-handling code should remain in a
          > single class and not be spread over the codebase, which would make it
          > error-prone and difficult to maintain and understand.

          Now that you mention it...
          The current implementation has the related code (footnote gathering) in PageBreaker.getNextKnuthElements(). The only reason so far that it didn't work for lists and tables is that they did not yet propagate the footnotes for their descendants upwards.
          I'm wondering how can this be done without making either of them footnote-aware... They are agnostic, that is precisely why it does not work now. Both generate combined element-lists for their parts, but the parts' footnotes are not picked up.

          > Below I will only speak for the table code, because it's the only one in the
          > code impacted by the patch that I know well, but I think the changes don't fit
          > well in it:

          Good! That's why I was hoping you'd chime in.

          > - in TableStepper, ActiveCells aren't ordered by their column indices! Instead
          > they are appended to the list once they start contributing content for the
          > merging algorithm. This means that new cells will be found /after/ cells
          > starting on earlier rows and spanning over the current one, even if they are in
          > earlier columns. So basically the code added in TableStepper doesn't work...

          OK, I see where this becomes a problem. Had not yet tested such cases, only simple grids.

          > An intermediate solution could probably be implemented the following way:
          > - in ActiveCell.Step, add a List field that would contain the FootnoteBodyLMs
          > encountered during the last call to gotoNextLegalBreak;
          > - in TableStepper.getCombinedKnuthElements, when iterating over the active
          > cells to create the CellPart instances, get those FootnoteBodyLMs in the same
          > time. A small detail to pay attention to is to not re-get them if the active
          > cell doesn't contribute new content to the step. If there are some footnotes,
          > create a KnuthBlockBox, otherwise create a normal Box.
          > And that should be it basically...
          > I'll see if I can submit a patch to illustrate my ideas in the next days.

          Cool, we await your input.

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #37) > > I'm sorry to chime in so late, but so far I haven't had the time and energy to > approach this topic. No problem. > > Anyway, I've just had a look at the patch and I'm afraid I don't think it's > ready to be committed. > I see mainly the following problems: > - from a high-level point of view first: list- and table-related code should > remain totally footnote-agnostic. The footnote-handling code should remain in a > single class and not be spread over the codebase, which would make it > error-prone and difficult to maintain and understand. Now that you mention it... The current implementation has the related code (footnote gathering) in PageBreaker.getNextKnuthElements(). The only reason so far that it didn't work for lists and tables is that they did not yet propagate the footnotes for their descendants upwards. I'm wondering how can this be done without making either of them footnote-aware... They are agnostic, that is precisely why it does not work now. Both generate combined element-lists for their parts, but the parts' footnotes are not picked up. > Below I will only speak for the table code, because it's the only one in the > code impacted by the patch that I know well, but I think the changes don't fit > well in it: Good! That's why I was hoping you'd chime in. > - in TableStepper, ActiveCells aren't ordered by their column indices! Instead > they are appended to the list once they start contributing content for the > merging algorithm. This means that new cells will be found /after/ cells > starting on earlier rows and spanning over the current one, even if they are in > earlier columns. So basically the code added in TableStepper doesn't work... OK, I see where this becomes a problem. Had not yet tested such cases, only simple grids. > An intermediate solution could probably be implemented the following way: > - in ActiveCell.Step, add a List field that would contain the FootnoteBodyLMs > encountered during the last call to gotoNextLegalBreak; > - in TableStepper.getCombinedKnuthElements, when iterating over the active > cells to create the CellPart instances, get those FootnoteBodyLMs in the same > time. A small detail to pay attention to is to not re-get them if the active > cell doesn't contribute new content to the step. If there are some footnotes, > create a KnuthBlockBox, otherwise create a normal Box. > And that should be it basically... > I'll see if I can submit a patch to illustrate my ideas in the next days. Cool, we await your input.
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #38)
          <snip />
          > Now that you mention it...
          > The current implementation has the related code (footnote gathering) in
          > PageBreaker.getNextKnuthElements().

          To be complete: the first steps are taken in LineLayoutManager.postProcessLineBreaks().

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #38) <snip /> > Now that you mention it... > The current implementation has the related code (footnote gathering) in > PageBreaker.getNextKnuthElements(). To be complete: the first steps are taken in LineLayoutManager.postProcessLineBreaks().
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #38)
          >
          > > An intermediate solution could probably be implemented the following way:
          > > - in ActiveCell.Step, add a List field that would contain the FootnoteBodyLMs
          > > encountered during the last call to gotoNextLegalBreak;
          > > - in TableStepper.getCombinedKnuthElements, when iterating over the active
          > > cells to create the CellPart instances, get those FootnoteBodyLMs in the same
          > > time. A small detail to pay attention to is to not re-get them if the active
          > > cell doesn't contribute new content to the step. If there are some footnotes,
          > > create a KnuthBlockBox, otherwise create a normal Box.
          > > And that should be it basically...
          > > I'll see if I can submit a patch to illustrate my ideas in the next days.
          >
          > Cool, we await your input.

          Well, I did not really wait...
          I've already tried this approach, and I got this working, apart from 'not re-get them if they do not contribute content'. If you could tell me what condition I need to check for, that would save me the time to go looking.

          Right now, I have:
          -> added a footnoteList member to ActiveCell, with accompanying accessor
          -> modified gotoNextLegalBreak(): if the element is a KnuthBlockBox and has anchors, then add the footnotes to the added member-list
          > modified TableStepper (loop +/ line 207) to use the accessor; if the ActiveCell has footnotes, a KnuthBlockBox is generated further on, else a normal Box.

          Thanks for the helpful feedback so far! It's giving me better insight into table-layout as well, which could come in handy at some point.

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #38) > > > An intermediate solution could probably be implemented the following way: > > - in ActiveCell.Step, add a List field that would contain the FootnoteBodyLMs > > encountered during the last call to gotoNextLegalBreak; > > - in TableStepper.getCombinedKnuthElements, when iterating over the active > > cells to create the CellPart instances, get those FootnoteBodyLMs in the same > > time. A small detail to pay attention to is to not re-get them if the active > > cell doesn't contribute new content to the step. If there are some footnotes, > > create a KnuthBlockBox, otherwise create a normal Box. > > And that should be it basically... > > I'll see if I can submit a patch to illustrate my ideas in the next days. > > Cool, we await your input. Well, I did not really wait... I've already tried this approach, and I got this working, apart from 'not re-get them if they do not contribute content'. If you could tell me what condition I need to check for, that would save me the time to go looking. Right now, I have: -> added a footnoteList member to ActiveCell, with accompanying accessor -> modified gotoNextLegalBreak(): if the element is a KnuthBlockBox and has anchors, then add the footnotes to the added member-list > modified TableStepper (loop +/ line 207) to use the accessor; if the ActiveCell has footnotes, a KnuthBlockBox is generated further on, else a normal Box. Thanks for the helpful feedback so far! It's giving me better insight into table-layout as well, which could come in handy at some point.
          Hide
          Andreas L. Delmelle added a comment -


          If I'm correct, this should be roughly what Vincent proposed

          Show
          Andreas L. Delmelle added a comment - If I'm correct, this should be roughly what Vincent proposed
          Hide
          Andreas L. Delmelle added a comment -

          Attachment b37579.diff has been added with description: alternative patch for footnotes in table-cells

          Show
          Andreas L. Delmelle added a comment - Attachment b37579.diff has been added with description: alternative patch for footnotes in table-cells
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #41)
          > Created an attachment (id=21976) [details]
          > alternative patch for footnotes in table-cells
          >
          >
          > If I'm correct, this should be roughly what Vincent proposed
          >

          Just noticed: after this patch, there does not seem to be a good reason anymore to have the ElementListUtils.collectFootnoteBodyLMs() methods. The only class accessing it, is ListItemLayoutManager, so we may as well put it there in a private method, or inline... Updated patch to follow.

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #41) > Created an attachment (id=21976) [details] > alternative patch for footnotes in table-cells > > > If I'm correct, this should be roughly what Vincent proposed > Just noticed: after this patch, there does not seem to be a good reason anymore to have the ElementListUtils.collectFootnoteBodyLMs() methods. The only class accessing it, is ListItemLayoutManager, so we may as well put it there in a private method, or inline... Updated patch to follow.
          Hide
          Andreas L. Delmelle added a comment -

          Updated patch, without support for footnotes in table-header or -footer, which removes ElementListUtils from the picture.

          For list-items, I also took the liberty of making one more modification. Not sure if this agrees with everyone, but if the idea is to improve element access performance in getNextStep(), and we're not modifying the lists anyway, we might as well switch to plain arrays instead of copying the LinkedLists to ArrayLists.

          Show
          Andreas L. Delmelle added a comment - Updated patch, without support for footnotes in table-header or -footer, which removes ElementListUtils from the picture. For list-items, I also took the liberty of making one more modification. Not sure if this agrees with everyone, but if the idea is to improve element access performance in getNextStep(), and we're not modifying the lists anyway, we might as well switch to plain arrays instead of copying the LinkedLists to ArrayLists.
          Hide
          Andreas L. Delmelle added a comment -

          Attachment b37579.diff has been added with description: updated patch against FOP trunk

          Show
          Andreas L. Delmelle added a comment - Attachment b37579.diff has been added with description: updated patch against FOP trunk
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #38)
          > (In reply to comment #37)

          > > - from a high-level point of view first: list- and table-related code should
          > > remain totally footnote-agnostic. The footnote-handling code should remain in a
          > > single class and not be spread over the codebase, which would make it
          > > error-prone and difficult to maintain and understand.
          >
          <snip />
          > I'm wondering how can this be done without making either of them
          > footnote-aware...

          Been doing some more thinking, and what if we were to introduce something like a 'FootnoteCollector'? I think something like this would also address Adrian's concern about a complete solution...
          Right now, the LineLayoutManager separates the footnotes from their citations, and attaches them as a member-list to the KnuthBlockBoxes. The same approach is now copied to list- and table-layout: extraction of the footnotes from the boxes, and copying them to higher-level block-boxes.

          What if we pass a FootnoteCollector down from the PageBreaker, which contains a Map<KnuthBox, List<FootnoteBodyLM>>, or maybe simply Map<KnuthBox, FootnoteBodyLM[]>.

          The LineLayoutManager would do something like:

          footnoteCollector.collect( KnuthBox, List<KnuthBox> );

          which would use the box as a key, and put the resulting list as a value in the map.

          Something similar would be done by the list- and table-related LMs.

          The iterations that are now spread over LineLM, PageBreaker, ActiveCell, TableStepper and ListItemLM can then be confined to one single class.

          PageBreaker could do something like:

          footnoteCollector.getFootnotesFor( List<KnuthBox> )

          to obtain a combined list of footnotes for all the boxes in the list.

          WDYT?

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #38) > (In reply to comment #37) > > - from a high-level point of view first: list- and table-related code should > > remain totally footnote-agnostic. The footnote-handling code should remain in a > > single class and not be spread over the codebase, which would make it > > error-prone and difficult to maintain and understand. > <snip /> > I'm wondering how can this be done without making either of them > footnote-aware... Been doing some more thinking, and what if we were to introduce something like a 'FootnoteCollector'? I think something like this would also address Adrian's concern about a complete solution... Right now, the LineLayoutManager separates the footnotes from their citations, and attaches them as a member-list to the KnuthBlockBoxes. The same approach is now copied to list- and table-layout: extraction of the footnotes from the boxes, and copying them to higher-level block-boxes. What if we pass a FootnoteCollector down from the PageBreaker, which contains a Map<KnuthBox, List<FootnoteBodyLM>>, or maybe simply Map<KnuthBox, FootnoteBodyLM[]>. The LineLayoutManager would do something like: footnoteCollector.collect( KnuthBox, List<KnuthBox> ); which would use the box as a key, and put the resulting list as a value in the map. Something similar would be done by the list- and table-related LMs. The iterations that are now spread over LineLM, PageBreaker, ActiveCell, TableStepper and ListItemLM can then be confined to one single class. PageBreaker could do something like: footnoteCollector.getFootnotesFor( List<KnuthBox> ) to obtain a combined list of footnotes for all the boxes in the list. WDYT?
          Hide
          Vincent Hennebert added a comment -

          (In reply to comment #43)
          > Created an attachment (id=21977) [details]
          > updated patch against FOP trunk
          >
          >
          > Updated patch, without support for footnotes in table-header or -footer, which
          > removes ElementListUtils from the picture.
          >
          > For list-items, I also took the liberty of making one more modification. Not
          > sure if this agrees with everyone, but if the idea is to improve element access
          > performance in getNextStep(), and we're not modifying the lists anyway, we
          > might as well switch to plain arrays instead of copying the LinkedLists to
          > ArrayLists.
          >

          (In reply to comment #40)
          > (In reply to comment #38)
          > >
          > > > An intermediate solution could probably be implemented the following way:
          > > > - in ActiveCell.Step, add a List field that would contain the FootnoteBodyLMs
          > > > encountered during the last call to gotoNextLegalBreak;
          > > > - in TableStepper.getCombinedKnuthElements, when iterating over the active
          > > > cells to create the CellPart instances, get those FootnoteBodyLMs in the same
          > > > time. A small detail to pay attention to is to not re-get them if the active
          > > > cell doesn't contribute new content to the step. If there are some footnotes,
          > > > create a KnuthBlockBox, otherwise create a normal Box.
          > > > And that should be it basically...
          > > > I'll see if I can submit a patch to illustrate my ideas in the next days.
          > >
          > > Cool, we await your input.
          >
          > Well, I did not really wait...
          > I've already tried this approach, and I got this working, apart from 'not
          > re-get them if they do not contribute content'. If you could tell me what
          > condition I need to check for, that would save me the time to go looking.
          >
          > Right now, I have:
          > -> added a footnoteList member to ActiveCell, with accompanying accessor
          > -> modified gotoNextLegalBreak(): if the element is a KnuthBlockBox and has
          > anchors, then add the footnotes to the added member-list
          > > modified TableStepper (loop +/ line 207) to use the accessor; if the
          > ActiveCell has footnotes, a KnuthBlockBox is generated further on, else a
          > normal Box.

          This is already much better, but this can still be improved IMO. TableStepper is still taking too much responsibility: instead of asking ActiveCells if they have any footnotes, and then asking them for their footnotes, it can just provide them with a list to which they can add their own footnotes, if they have any, and if they contribute content to the next step. Kind of inversion of control principle.

          > Thanks for the helpful feedback so far! It's giving me better insight into
          > table-layout as well, which could come in handy at some point.
          >

          Vincent

          Show
          Vincent Hennebert added a comment - (In reply to comment #43) > Created an attachment (id=21977) [details] > updated patch against FOP trunk > > > Updated patch, without support for footnotes in table-header or -footer, which > removes ElementListUtils from the picture. > > For list-items, I also took the liberty of making one more modification. Not > sure if this agrees with everyone, but if the idea is to improve element access > performance in getNextStep(), and we're not modifying the lists anyway, we > might as well switch to plain arrays instead of copying the LinkedLists to > ArrayLists. > (In reply to comment #40) > (In reply to comment #38) > > > > > An intermediate solution could probably be implemented the following way: > > > - in ActiveCell.Step, add a List field that would contain the FootnoteBodyLMs > > > encountered during the last call to gotoNextLegalBreak; > > > - in TableStepper.getCombinedKnuthElements, when iterating over the active > > > cells to create the CellPart instances, get those FootnoteBodyLMs in the same > > > time. A small detail to pay attention to is to not re-get them if the active > > > cell doesn't contribute new content to the step. If there are some footnotes, > > > create a KnuthBlockBox, otherwise create a normal Box. > > > And that should be it basically... > > > I'll see if I can submit a patch to illustrate my ideas in the next days. > > > > Cool, we await your input. > > Well, I did not really wait... > I've already tried this approach, and I got this working, apart from 'not > re-get them if they do not contribute content'. If you could tell me what > condition I need to check for, that would save me the time to go looking. > > Right now, I have: > -> added a footnoteList member to ActiveCell, with accompanying accessor > -> modified gotoNextLegalBreak(): if the element is a KnuthBlockBox and has > anchors, then add the footnotes to the added member-list > > modified TableStepper (loop +/ line 207) to use the accessor; if the > ActiveCell has footnotes, a KnuthBlockBox is generated further on, else a > normal Box. This is already much better, but this can still be improved IMO. TableStepper is still taking too much responsibility: instead of asking ActiveCells if they have any footnotes, and then asking them for their footnotes, it can just provide them with a list to which they can add their own footnotes, if they have any, and if they contribute content to the next step. Kind of inversion of control principle. > Thanks for the helpful feedback so far! It's giving me better insight into > table-layout as well, which could come in handy at some point. > Vincent
          Hide
          Vincent Hennebert added a comment -

          Patch showing how TableStepper can delegate more responsibility to ActiveCell. I didn't touch the modification to list-related class. However, I reverted the modifications to FootnoteBodyLM since they were breaking the testsuite.

          Show
          Vincent Hennebert added a comment - Patch showing how TableStepper can delegate more responsibility to ActiveCell. I didn't touch the modification to list-related class. However, I reverted the modifications to FootnoteBodyLM since they were breaking the testsuite.
          Hide
          Vincent Hennebert added a comment -

          Attachment table_list_footnotes.diff has been added with description: Updated patch, TableStepper delegating more to ActiveCell

          Show
          Vincent Hennebert added a comment - Attachment table_list_footnotes.diff has been added with description: Updated patch, TableStepper delegating more to ActiveCell
          Hide
          Vincent Hennebert added a comment -

          (In reply to comment #43)
          > Created an attachment (id=21977) [details]
          > updated patch against FOP trunk
          >
          >
          > Updated patch, without support for footnotes in table-header or -footer, which
          > removes ElementListUtils from the picture.
          >
          > For list-items, I also took the liberty of making one more modification. Not
          > sure if this agrees with everyone, but if the idea is to improve element access
          > performance in getNextStep(), and we're not modifying the lists anyway, we
          > might as well switch to plain arrays instead of copying the LinkedLists to
          > ArrayLists.

          I don't really mind, but this should be done separately from the implementation of footnotes (atomicity of commits).

          Vincent

          Show
          Vincent Hennebert added a comment - (In reply to comment #43) > Created an attachment (id=21977) [details] > updated patch against FOP trunk > > > Updated patch, without support for footnotes in table-header or -footer, which > removes ElementListUtils from the picture. > > For list-items, I also took the liberty of making one more modification. Not > sure if this agrees with everyone, but if the idea is to improve element access > performance in getNextStep(), and we're not modifying the lists anyway, we > might as well switch to plain arrays instead of copying the LinkedLists to > ArrayLists. I don't really mind, but this should be done separately from the implementation of footnotes (atomicity of commits). Vincent
          Hide
          Vincent Hennebert added a comment -

          (In reply to comment #27)
          > (In reply to comment #26)
          > >
          > > Strike that... Stopped debugging too early. The label's LM does appear if I
          > > leave it running...
          > >
          >
          > At the point where getBaseLength() fails, the ancestor tree looks like:
          >
          > FlowLayoutManager
          > -> FootnoteBodyLM
          >
          > So, the LM is reparented (in PageBreaker, line 166), and somewhere after that,
          > we get a call to PercentLength.getValue(footnoteBodyLayoutManager)

          AFAIU the reason why FootnoteBodyLM is re-parented is that it put its areas at the right place (as children of the footnote-reference-area, instead of the block containing the footnote). Simply moving the setParent call after the call to getNextKnuthElements makes the warnings disappear, and doesn't break any test.
          Confirmation from specialists of this part of the code would be appreciated

          Vincent

          Show
          Vincent Hennebert added a comment - (In reply to comment #27) > (In reply to comment #26) > > > > Strike that... Stopped debugging too early. The label's LM does appear if I > > leave it running... > > > > At the point where getBaseLength() fails, the ancestor tree looks like: > > FlowLayoutManager > -> FootnoteBodyLM > > So, the LM is reparented (in PageBreaker, line 166), and somewhere after that, > we get a call to PercentLength.getValue(footnoteBodyLayoutManager) AFAIU the reason why FootnoteBodyLM is re-parented is that it put its areas at the right place (as children of the footnote-reference-area, instead of the block containing the footnote). Simply moving the setParent call after the call to getNextKnuthElements makes the warnings disappear, and doesn't break any test. Confirmation from specialists of this part of the code would be appreciated Vincent
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #48)
          >
          > AFAIU the reason why FootnoteBodyLM is re-parented is that it put its areas at
          > the right place (as children of the footnote-reference-area, instead of the
          > block containing the footnote). Simply moving the setParent call after the call
          > to getNextKnuthElements makes the warnings disappear, and doesn't break any
          > test.
          > Confirmation from specialists of this part of the code would be appreciated

          Seems reasonable to me. Cleaner than overriding getParent() anyway.

          In the meantime, I've also been playing with adding an interface FootnoteCitationHolder. Such an interface could then be implemented by KnuthBlockBox and ActiveCell. The interface methods can be used by LineLayoutManager, PageBreaker, ListItemLayoutManager, TableStepper...

          The methods would roughly be:
          hasAnchors()
          getFootnoteBodyLMs()
          addFootnotes(List<KnuthElement>)
          addFootnotes(FootnoteCitationHolder)
          addFootnote(FootnoteCitationHolder.Citation)
          expandFootnotes(LayoutManager, LayoutContext, int)

          While this would still leave the related portions of code distributed over those classes, the interface makes it a bit easier to locate them in an IDE, and makes those pieces of code a bit more uniform.

          Most of the loops we see now, would move to KnuthBlockBox, as the only complete implementation. ActiveCell would only implement what is needed to make the citations accessible to the box created by TableStepper. Slight compromise in comparison to the last patch is that, in the iteration over the active cells, we would only create a temporary list with those having citations. If the list is empty, we create a regular box. If not, then we iterate over that temporary list of cells, and instruct the created KnuthBlockBox to add the citations from those cells. The same pattern can be used by ListItemLayoutManager:

          • create a temporary list of FootnoteCitationHolders for which hasAnchors() returns true
          • if non-empty, iterate over that temporary list
          • for each element, ask the higher level FootnoteCitationHolder (KnuthBlockBox) to extract the citations, and add them to its own list.

          I'll see if I can attach a patch to demonstrate one of these days.

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #48) > > AFAIU the reason why FootnoteBodyLM is re-parented is that it put its areas at > the right place (as children of the footnote-reference-area, instead of the > block containing the footnote). Simply moving the setParent call after the call > to getNextKnuthElements makes the warnings disappear, and doesn't break any > test. > Confirmation from specialists of this part of the code would be appreciated Seems reasonable to me. Cleaner than overriding getParent() anyway. In the meantime, I've also been playing with adding an interface FootnoteCitationHolder. Such an interface could then be implemented by KnuthBlockBox and ActiveCell. The interface methods can be used by LineLayoutManager, PageBreaker, ListItemLayoutManager, TableStepper... The methods would roughly be: hasAnchors() getFootnoteBodyLMs() addFootnotes(List<KnuthElement>) addFootnotes(FootnoteCitationHolder) addFootnote(FootnoteCitationHolder.Citation) expandFootnotes(LayoutManager, LayoutContext, int) While this would still leave the related portions of code distributed over those classes, the interface makes it a bit easier to locate them in an IDE, and makes those pieces of code a bit more uniform. Most of the loops we see now, would move to KnuthBlockBox, as the only complete implementation. ActiveCell would only implement what is needed to make the citations accessible to the box created by TableStepper. Slight compromise in comparison to the last patch is that, in the iteration over the active cells, we would only create a temporary list with those having citations. If the list is empty, we create a regular box. If not, then we iterate over that temporary list of cells, and instruct the created KnuthBlockBox to add the citations from those cells. The same pattern can be used by ListItemLayoutManager: create a temporary list of FootnoteCitationHolders for which hasAnchors() returns true if non-empty, iterate over that temporary list for each element, ask the higher level FootnoteCitationHolder (KnuthBlockBox) to extract the citations, and add them to its own list. I'll see if I can attach a patch to demonstrate one of these days.
          Hide
          Vincent Hennebert added a comment -

          (In reply to comment #49)
          > (In reply to comment #48)
          > >
          > > AFAIU the reason why FootnoteBodyLM is re-parented is that it put its areas at
          > > the right place (as children of the footnote-reference-area, instead of the
          > > block containing the footnote). Simply moving the setParent call after the call
          > > to getNextKnuthElements makes the warnings disappear, and doesn't break any
          > > test.
          > > Confirmation from specialists of this part of the code would be appreciated
          >
          > Seems reasonable to me. Cleaner than overriding getParent() anyway.
          >
          > In the meantime, I've also been playing with adding an interface
          > FootnoteCitationHolder. Such an interface could then be implemented by
          > KnuthBlockBox and ActiveCell. The interface methods can be used by
          > LineLayoutManager, PageBreaker, ListItemLayoutManager, TableStepper...
          >
          > The methods would roughly be:
          > hasAnchors()
          > getFootnoteBodyLMs()
          > addFootnotes(List<KnuthElement>)
          > addFootnotes(FootnoteCitationHolder)
          > addFootnote(FootnoteCitationHolder.Citation)
          > expandFootnotes(LayoutManager, LayoutContext, int)
          >
          > While this would still leave the related portions of code distributed over
          > those classes, the interface makes it a bit easier to locate them in an IDE,
          > and makes those pieces of code a bit more uniform.
          >
          > Most of the loops we see now, would move to KnuthBlockBox, as the only complete
          > implementation. ActiveCell would only implement what is needed to make the
          > citations accessible to the box created by TableStepper. Slight compromise in
          > comparison to the last patch is that, in the iteration over the active cells,
          > we would only create a temporary list with those having citations. If the list
          > is empty, we create a regular box. If not, then we iterate over that temporary
          > list of cells, and instruct the created KnuthBlockBox to add the citations from
          > those cells. The same pattern can be used by ListItemLayoutManager:
          >
          > - create a temporary list of FootnoteCitationHolders for which hasAnchors()
          > returns true
          > - if non-empty, iterate over that temporary list
          > - for each element, ask the higher level FootnoteCitationHolder (KnuthBlockBox)
          > to extract the citations, and add them to its own list.
          >
          > I'll see if I can attach a patch to demonstrate one of these days.

          I've been asked to look into this issue, so I committed a partial and temporary fix based on the latest patch:
          http://svn.apache.org/viewvc?view=rev&revision=660979
          Footnotes in table headers and footers are not handled yet, and anyway I think it's best to wait for clarification from xsl-editors before implementing anything (which gives us a couple of months ).

          That doesn't prevent you from exploring your ideas above, though. I await your patch.

          Vincent

          Show
          Vincent Hennebert added a comment - (In reply to comment #49) > (In reply to comment #48) > > > > AFAIU the reason why FootnoteBodyLM is re-parented is that it put its areas at > > the right place (as children of the footnote-reference-area, instead of the > > block containing the footnote). Simply moving the setParent call after the call > > to getNextKnuthElements makes the warnings disappear, and doesn't break any > > test. > > Confirmation from specialists of this part of the code would be appreciated > > Seems reasonable to me. Cleaner than overriding getParent() anyway. > > In the meantime, I've also been playing with adding an interface > FootnoteCitationHolder. Such an interface could then be implemented by > KnuthBlockBox and ActiveCell. The interface methods can be used by > LineLayoutManager, PageBreaker, ListItemLayoutManager, TableStepper... > > The methods would roughly be: > hasAnchors() > getFootnoteBodyLMs() > addFootnotes(List<KnuthElement>) > addFootnotes(FootnoteCitationHolder) > addFootnote(FootnoteCitationHolder.Citation) > expandFootnotes(LayoutManager, LayoutContext, int) > > While this would still leave the related portions of code distributed over > those classes, the interface makes it a bit easier to locate them in an IDE, > and makes those pieces of code a bit more uniform. > > Most of the loops we see now, would move to KnuthBlockBox, as the only complete > implementation. ActiveCell would only implement what is needed to make the > citations accessible to the box created by TableStepper. Slight compromise in > comparison to the last patch is that, in the iteration over the active cells, > we would only create a temporary list with those having citations. If the list > is empty, we create a regular box. If not, then we iterate over that temporary > list of cells, and instruct the created KnuthBlockBox to add the citations from > those cells. The same pattern can be used by ListItemLayoutManager: > > - create a temporary list of FootnoteCitationHolders for which hasAnchors() > returns true > - if non-empty, iterate over that temporary list > - for each element, ask the higher level FootnoteCitationHolder (KnuthBlockBox) > to extract the citations, and add them to its own list. > > I'll see if I can attach a patch to demonstrate one of these days. I've been asked to look into this issue, so I committed a partial and temporary fix based on the latest patch: http://svn.apache.org/viewvc?view=rev&revision=660979 Footnotes in table headers and footers are not handled yet, and anyway I think it's best to wait for clarification from xsl-editors before implementing anything (which gives us a couple of months ). That doesn't prevent you from exploring your ideas above, though. I await your patch. Vincent
          Hide
          Vincent Hennebert added a comment -

          (In reply to comment #48)
          > (In reply to comment #27)
          > > (In reply to comment #26)
          > > At the point where getBaseLength() fails, the ancestor tree looks like:
          > >
          > > FlowLayoutManager
          > > -> FootnoteBodyLM
          > >
          > > So, the LM is reparented (in PageBreaker, line 166), and somewhere after that,
          > > we get a call to PercentLength.getValue(footnoteBodyLayoutManager)
          >
          > AFAIU the reason why FootnoteBodyLM is re-parented is that it put its areas at
          > the right place (as children of the footnote-reference-area, instead of the
          > block containing the footnote). Simply moving the setParent call after the call
          > to getNextKnuthElements makes the warnings disappear, and doesn't break any
          > test.

          This is more complicated than that. For most of the properties that can take a percentage value, the percentage refers to the containing area's (or nearest ancestor reference area's) ipd or bpd. The only notable exception is font-size, which refers to the parent element.

          So if we take end-indent, for instance, if it's not specified inside footnote-body, then it takes the /computed/ value of the parent element (e.g., the block containing the footnote). /But/ if it has a specified percentage value, then the percentage shall be resolved against the footnote-reference-area. In many cases this will lead to the same result, but not when footnotes are declared inside lists, tables, block-containers, or if the region-body has several columns, etc.

          This issue is not related to lists and tables only, but is more general. If PageBreaker is left as is, then percentages specified inside footnotes should resolve correctly, but not inherited values. If we move the setParent call like explained above, then this is the other way around...

          Show
          Vincent Hennebert added a comment - (In reply to comment #48) > (In reply to comment #27) > > (In reply to comment #26) > > At the point where getBaseLength() fails, the ancestor tree looks like: > > > > FlowLayoutManager > > -> FootnoteBodyLM > > > > So, the LM is reparented (in PageBreaker, line 166), and somewhere after that, > > we get a call to PercentLength.getValue(footnoteBodyLayoutManager) > > AFAIU the reason why FootnoteBodyLM is re-parented is that it put its areas at > the right place (as children of the footnote-reference-area, instead of the > block containing the footnote). Simply moving the setParent call after the call > to getNextKnuthElements makes the warnings disappear, and doesn't break any > test. This is more complicated than that. For most of the properties that can take a percentage value, the percentage refers to the containing area's (or nearest ancestor reference area's) ipd or bpd. The only notable exception is font-size, which refers to the parent element. So if we take end-indent, for instance, if it's not specified inside footnote-body, then it takes the /computed/ value of the parent element (e.g., the block containing the footnote). /But/ if it has a specified percentage value, then the percentage shall be resolved against the footnote-reference-area. In many cases this will lead to the same result, but not when footnotes are declared inside lists, tables, block-containers, or if the region-body has several columns, etc. This issue is not related to lists and tables only, but is more general. If PageBreaker is left as is, then percentages specified inside footnotes should resolve correctly, but not inherited values. If we move the setParent call like explained above, then this is the other way around...
          Hide
          Johans Marvin Taboada Villca added a comment -

          (In reply to comment #50)
          >
          > I've been asked to look into this issue, so I committed a partial and temporary
          > fix based on the latest patch:
          > http://svn.apache.org/viewvc?view=rev&revision=660979
          > Footnotes in table headers and footers are not handled yet, and anyway I think
          > it's best to wait for clarification from xsl-editors before implementing
          > anything (which gives us a couple of months ).

          First, many thanks to all for your great efforts.

          As I've seen, fop-0.95beta has been released by 2008-03-25 so these temporally fixes (2008-05-28) are still not present in it.

          Is there any preliminar release date for beta2 or something like that?. A couple of months?. Should I use trunk?

          Sorry for the noise.

          Show
          Johans Marvin Taboada Villca added a comment - (In reply to comment #50) > > I've been asked to look into this issue, so I committed a partial and temporary > fix based on the latest patch: > http://svn.apache.org/viewvc?view=rev&revision=660979 > Footnotes in table headers and footers are not handled yet, and anyway I think > it's best to wait for clarification from xsl-editors before implementing > anything (which gives us a couple of months ). First, many thanks to all for your great efforts. As I've seen, fop-0.95beta has been released by 2008-03-25 so these temporally fixes (2008-05-28) are still not present in it. Is there any preliminar release date for beta2 or something like that?. A couple of months?. Should I use trunk? Sorry for the noise.
          Hide
          Andreas L. Delmelle added a comment -

          (In reply to comment #52)
          >
          > As I've seen, fop-0.95beta has been released by 2008-03-25 so these temporally
          > fixes (2008-05-28) are still not present in it.

          Correct.

          >
          > Is there any preliminar release date for beta2 or something like that?. A
          > couple of months?. Should I use trunk?

          If you really need this feature, then you should indeed use the trunk. 0.95final is very close, but the changes have not applied to the release branch.

          Show
          Andreas L. Delmelle added a comment - (In reply to comment #52) > > As I've seen, fop-0.95beta has been released by 2008-03-25 so these temporally > fixes (2008-05-28) are still not present in it. Correct. > > Is there any preliminar release date for beta2 or something like that?. A > couple of months?. Should I use trunk? If you really need this feature, then you should indeed use the trunk. 0.95final is very close, but the changes have not applied to the release branch.
          Hide
          Sylvestre Ledru added a comment -

          I believe this bug is now fixed. Isn't it ?

          Show
          Sylvestre Ledru added a comment - I believe this bug is now fixed. Isn't it ?
          Hide
          Vincent Hennebert added a comment -

          (In reply to comment #54)
          > I believe this bug is now fixed. Isn't it ?

          No. Footnotes have not been implemented yet in table headers and footnotes, as we are waiting for clarification from the W3C whether they should appear only once or every time the header/footer is repeated. Plus there currently are issues with percentages and inherited values inside footnotes (see comment #51 above).

          Vincent

          Show
          Vincent Hennebert added a comment - (In reply to comment #54) > I believe this bug is now fixed. Isn't it ? No. Footnotes have not been implemented yet in table headers and footnotes, as we are waiting for clarification from the W3C whether they should appear only once or every time the header/footer is repeated. Plus there currently are issues with percentages and inherited values inside footnotes (see comment #51 above). Vincent
          Hide
          Dimitri Goloborodko added a comment -

          Attachment footnote.fo has been added with description: Example of lost footnote in table against trunk 660979

          Show
          Dimitri Goloborodko added a comment - Attachment footnote.fo has been added with description: Example of lost footnote in table against trunk 660979
          Hide
          Dimitri Goloborodko added a comment -

          (In reply to comment #50)
          > (In reply to comment #49)
          > > (In reply to comment #48)
          > > >
          > > > AFAIU the reason why FootnoteBodyLM is re-parented is that it put its areas at
          > > > the right place (as children of the footnote-reference-area, instead of the
          > > > block containing the footnote). Simply moving the setParent call after the call
          > > > to getNextKnuthElements makes the warnings disappear, and doesn't break any
          > > > test.
          > > > Confirmation from specialists of this part of the code would be appreciated
          > >
          > > Seems reasonable to me. Cleaner than overriding getParent() anyway.
          > >
          > > In the meantime, I've also been playing with adding an interface
          > > FootnoteCitationHolder. Such an interface could then be implemented by
          > > KnuthBlockBox and ActiveCell. The interface methods can be used by
          > > LineLayoutManager, PageBreaker, ListItemLayoutManager, TableStepper...
          > >
          > > The methods would roughly be:
          > > hasAnchors()
          > > getFootnoteBodyLMs()
          > > addFootnotes(List<KnuthElement>)
          > > addFootnotes(FootnoteCitationHolder)
          > > addFootnote(FootnoteCitationHolder.Citation)
          > > expandFootnotes(LayoutManager, LayoutContext, int)
          > >
          > > While this would still leave the related portions of code distributed over
          > > those classes, the interface makes it a bit easier to locate them in an IDE,
          > > and makes those pieces of code a bit more uniform.
          > >
          > > Most of the loops we see now, would move to KnuthBlockBox, as the only complete
          > > implementation. ActiveCell would only implement what is needed to make the
          > > citations accessible to the box created by TableStepper. Slight compromise in
          > > comparison to the last patch is that, in the iteration over the active cells,
          > > we would only create a temporary list with those having citations. If the list
          > > is empty, we create a regular box. If not, then we iterate over that temporary
          > > list of cells, and instruct the created KnuthBlockBox to add the citations from
          > > those cells. The same pattern can be used by ListItemLayoutManager:
          > >
          > > - create a temporary list of FootnoteCitationHolders for which hasAnchors()
          > > returns true
          > > - if non-empty, iterate over that temporary list
          > > - for each element, ask the higher level FootnoteCitationHolder (KnuthBlockBox)
          > > to extract the citations, and add them to its own list.
          > >
          > > I'll see if I can attach a patch to demonstrate one of these days.
          >
          > I've been asked to look into this issue, so I committed a partial and temporary
          > fix based on the latest patch:
          > http://svn.apache.org/viewvc?view=rev&revision=660979
          > Footnotes in table headers and footers are not handled yet, and anyway I think
          > it's best to wait for clarification from xsl-editors before implementing
          > anything (which gives us a couple of months ).
          >
          > That doesn't prevent you from exploring your ideas above, though. I await your
          > patch.
          >
          > Vincent

          I try to use the trunk 660979 and find a case, when a footnote defined in table-body disappears. An example of this is attached. I look for solution since some days, but don’t get much with my very modest knowledge of fop. Do you have any ideas about that?

          Show
          Dimitri Goloborodko added a comment - (In reply to comment #50) > (In reply to comment #49) > > (In reply to comment #48) > > > > > > AFAIU the reason why FootnoteBodyLM is re-parented is that it put its areas at > > > the right place (as children of the footnote-reference-area, instead of the > > > block containing the footnote). Simply moving the setParent call after the call > > > to getNextKnuthElements makes the warnings disappear, and doesn't break any > > > test. > > > Confirmation from specialists of this part of the code would be appreciated > > > > Seems reasonable to me. Cleaner than overriding getParent() anyway. > > > > In the meantime, I've also been playing with adding an interface > > FootnoteCitationHolder. Such an interface could then be implemented by > > KnuthBlockBox and ActiveCell. The interface methods can be used by > > LineLayoutManager, PageBreaker, ListItemLayoutManager, TableStepper... > > > > The methods would roughly be: > > hasAnchors() > > getFootnoteBodyLMs() > > addFootnotes(List<KnuthElement>) > > addFootnotes(FootnoteCitationHolder) > > addFootnote(FootnoteCitationHolder.Citation) > > expandFootnotes(LayoutManager, LayoutContext, int) > > > > While this would still leave the related portions of code distributed over > > those classes, the interface makes it a bit easier to locate them in an IDE, > > and makes those pieces of code a bit more uniform. > > > > Most of the loops we see now, would move to KnuthBlockBox, as the only complete > > implementation. ActiveCell would only implement what is needed to make the > > citations accessible to the box created by TableStepper. Slight compromise in > > comparison to the last patch is that, in the iteration over the active cells, > > we would only create a temporary list with those having citations. If the list > > is empty, we create a regular box. If not, then we iterate over that temporary > > list of cells, and instruct the created KnuthBlockBox to add the citations from > > those cells. The same pattern can be used by ListItemLayoutManager: > > > > - create a temporary list of FootnoteCitationHolders for which hasAnchors() > > returns true > > - if non-empty, iterate over that temporary list > > - for each element, ask the higher level FootnoteCitationHolder (KnuthBlockBox) > > to extract the citations, and add them to its own list. > > > > I'll see if I can attach a patch to demonstrate one of these days. > > I've been asked to look into this issue, so I committed a partial and temporary > fix based on the latest patch: > http://svn.apache.org/viewvc?view=rev&revision=660979 > Footnotes in table headers and footers are not handled yet, and anyway I think > it's best to wait for clarification from xsl-editors before implementing > anything (which gives us a couple of months ). > > That doesn't prevent you from exploring your ideas above, though. I await your > patch. > > Vincent I try to use the trunk 660979 and find a case, when a footnote defined in table-body disappears. An example of this is attached. I look for solution since some days, but don’t get much with my very modest knowledge of fop. Do you have any ideas about that?
          Hide
          Vincent Hennebert added a comment -

          Hi Dimitri,

          (In reply to comment #57)
          <snip/>>
          > I try to use the trunk 660979 and find a case, when a footnote defined in
          > table-body disappears. An example of this is attached. I look for solution
          > since some days, but don’t get much with my very modest knowledge of fop. Do
          > you have any ideas about that?

          This is an oversight. The special code that is executed to handle the forced height set on the table row doesn't handle footnotes. And setting the row height to 33mm is enough to include the line that contains the footnote reference. When the height is set to 32mm that line is handled by the 'normal' code, that knows about footnotes.

          I'll see if I can provide a fix in the next days. Thanks for the very simple test case.

          Vincent

          Show
          Vincent Hennebert added a comment - Hi Dimitri, (In reply to comment #57) <snip/>> > I try to use the trunk 660979 and find a case, when a footnote defined in > table-body disappears. An example of this is attached. I look for solution > since some days, but don’t get much with my very modest knowledge of fop. Do > you have any ideas about that? This is an oversight. The special code that is executed to handle the forced height set on the table row doesn't handle footnotes. And setting the row height to 33mm is enough to include the line that contains the footnote reference. When the height is set to 32mm that line is handled by the 'normal' code, that knows about footnotes. I'll see if I can provide a fix in the next days. Thanks for the very simple test case. Vincent
          Hide
          Vincent Hennebert added a comment -
          Show
          Vincent Hennebert added a comment - This bug has now been fixed. http://svn.apache.org/viewvc?view=rev&revision=768320
          Hide
          Dimitri Goloborodko added a comment -

          Attachment fussnote.fo has been added with description: Example of wrong order of footnotes against trunk 768320

          Show
          Dimitri Goloborodko added a comment - Attachment fussnote.fo has been added with description: Example of wrong order of footnotes against trunk 768320
          Hide
          Dimitri Goloborodko added a comment -

          Hi Vincent,

          thank you for the patch. This time another issue with a wrong order of
          footnotes. There is a two column table in the attached example, both columns
          have footnotes. Sometimes the footnote from the second column precedes the
          footnote from the first one. If you delete one block from the first column, the
          order will be right.

          Dimitri

          Show
          Dimitri Goloborodko added a comment - Hi Vincent, thank you for the patch. This time another issue with a wrong order of footnotes. There is a two column table in the attached example, both columns have footnotes. Sometimes the footnote from the second column precedes the footnote from the first one. If you delete one block from the first column, the order will be right. Dimitri
          Hide
          Vincent Hennebert added a comment -

          Hi Dimitri,

          (In reply to comment #61)
          > Hi Vincent,
          >
          > thank you for the patch. This time another issue with a wrong order of
          > footnotes. There is a two column table in the attached example, both columns
          > have footnotes. Sometimes the footnote from the second column precedes the
          > footnote from the first one. If you delete one block from the first column, the
          > order will be right.

          It all depends on what order you should be expecting. If you scan the page in its whole width starting from the top you will find the footnote labeled 2 before the footnote labeled 1. This is basically what FOP is doing.

          Of course, it may seem more natural to start from the leftmost column, then go to the following one, etc. But this is particular to that case. With a right-to-left language it will be more natural to start from the rightmost column. Sometimes, the content will be such that the method above will be more natural.

          So this is a grey area, and the Recommendation doesn't say anything about that. Your best bet is to re-number the footnotes. Or use something else than footnotes (you may be happy with putting the notes in regular blocks just after the table, for example).

          HTH,
          Vincent

          Show
          Vincent Hennebert added a comment - Hi Dimitri, (In reply to comment #61) > Hi Vincent, > > thank you for the patch. This time another issue with a wrong order of > footnotes. There is a two column table in the attached example, both columns > have footnotes. Sometimes the footnote from the second column precedes the > footnote from the first one. If you delete one block from the first column, the > order will be right. It all depends on what order you should be expecting. If you scan the page in its whole width starting from the top you will find the footnote labeled 2 before the footnote labeled 1. This is basically what FOP is doing. Of course, it may seem more natural to start from the leftmost column, then go to the following one, etc. But this is particular to that case. With a right-to-left language it will be more natural to start from the rightmost column. Sometimes, the content will be such that the method above will be more natural. So this is a grey area, and the Recommendation doesn't say anything about that. Your best bet is to re-number the footnotes. Or use something else than footnotes (you may be happy with putting the notes in regular blocks just after the table, for example). HTH, Vincent
          Hide
          Dimitri Goloborodko added a comment -

          (In reply to comment #62)
          > Hi Dimitri,
          >
          > (In reply to comment #61)
          > > Hi Vincent,
          > >
          > > thank you for the patch. This time another issue with a wrong order of
          > > footnotes. There is a two column table in the attached example, both columns
          > > have footnotes. Sometimes the footnote from the second column precedes the
          > > footnote from the first one. If you delete one block from the first column, the
          > > order will be right.
          >
          > It all depends on what order you should be expecting. If you scan the page in
          > its whole width starting from the top you will find the footnote labeled 2
          > before the footnote labeled 1. This is basically what FOP is doing.
          >
          > Of course, it may seem more natural to start from the leftmost column, then go
          > to the following one, etc. But this is particular to that case. With a
          > right-to-left language it will be more natural to start from the rightmost
          > column. Sometimes, the content will be such that the method above will be more
          > natural.
          >
          > So this is a grey area, and the Recommendation doesn't say anything about that.
          > Your best bet is to re-number the footnotes. Or use something else than
          > footnotes (you may be happy with putting the notes in regular blocks just after
          > the table, for example).
          >
          > HTH,
          > Vincent

          Hi Vincent,

          I understand your point of view, you are probably right. Anyway, current implementation is not very reliable. If you leave only two blocks in the left column in my example, footnote labeled 1 will be output first, although, according to FOP behavior, we can expect it should be footnote labeled 2.

          Dimitri

          Show
          Dimitri Goloborodko added a comment - (In reply to comment #62) > Hi Dimitri, > > (In reply to comment #61) > > Hi Vincent, > > > > thank you for the patch. This time another issue with a wrong order of > > footnotes. There is a two column table in the attached example, both columns > > have footnotes. Sometimes the footnote from the second column precedes the > > footnote from the first one. If you delete one block from the first column, the > > order will be right. > > It all depends on what order you should be expecting. If you scan the page in > its whole width starting from the top you will find the footnote labeled 2 > before the footnote labeled 1. This is basically what FOP is doing. > > Of course, it may seem more natural to start from the leftmost column, then go > to the following one, etc. But this is particular to that case. With a > right-to-left language it will be more natural to start from the rightmost > column. Sometimes, the content will be such that the method above will be more > natural. > > So this is a grey area, and the Recommendation doesn't say anything about that. > Your best bet is to re-number the footnotes. Or use something else than > footnotes (you may be happy with putting the notes in regular blocks just after > the table, for example). > > HTH, > Vincent Hi Vincent, I understand your point of view, you are probably right. Anyway, current implementation is not very reliable. If you leave only two blocks in the left column in my example, footnote labeled 1 will be output first, although, according to FOP behavior, we can expect it should be footnote labeled 2. Dimitri
          Hide
          Vincent Hennebert added a comment -

          Hi Dimitri,

          (In reply to comment #63)
          > (In reply to comment #62)
          > > Hi Dimitri,
          > >
          > > (In reply to comment #61)
          > > > Hi Vincent,
          > > >
          > > > thank you for the patch. This time another issue with a wrong order of
          > > > footnotes. There is a two column table in the attached example, both columns
          > > > have footnotes. Sometimes the footnote from the second column precedes the
          > > > footnote from the first one. If you delete one block from the first column, the
          > > > order will be right.
          > >
          > > It all depends on what order you should be expecting. If you scan the page in
          > > its whole width starting from the top you will find the footnote labeled 2
          > > before the footnote labeled 1. This is basically what FOP is doing.
          > >
          > > Of course, it may seem more natural to start from the leftmost column, then go
          > > to the following one, etc. But this is particular to that case. With a
          > > right-to-left language it will be more natural to start from the rightmost
          > > column. Sometimes, the content will be such that the method above will be more
          > > natural.
          > >
          > > So this is a grey area, and the Recommendation doesn't say anything about that.
          > > Your best bet is to re-number the footnotes. Or use something else than
          > > footnotes (you may be happy with putting the notes in regular blocks just after
          > > the table, for example).
          > >
          > > HTH,
          > > Vincent
          >
          > Hi Vincent,
          >
          > I understand your point of view, you are probably right. Anyway, current
          > implementation is not very reliable. If you leave only two blocks in the left
          > column in my example, footnote labeled 1 will be output first, although,
          > according to FOP behavior, we can expect it should be footnote labeled 2.

          The change I made introduced another bug. It should be fixed now in revision 770635. Sorry about that.

          That said, I can think of certain situations involving row-spanning cells where the basic 'rule' stated above does no longer hold. I won't enter the details because they are a bit technical, but interesting issues may arise regarding accessibility, order of reading, etc. (That was a note to self )

          Vincent

          Show
          Vincent Hennebert added a comment - Hi Dimitri, (In reply to comment #63) > (In reply to comment #62) > > Hi Dimitri, > > > > (In reply to comment #61) > > > Hi Vincent, > > > > > > thank you for the patch. This time another issue with a wrong order of > > > footnotes. There is a two column table in the attached example, both columns > > > have footnotes. Sometimes the footnote from the second column precedes the > > > footnote from the first one. If you delete one block from the first column, the > > > order will be right. > > > > It all depends on what order you should be expecting. If you scan the page in > > its whole width starting from the top you will find the footnote labeled 2 > > before the footnote labeled 1. This is basically what FOP is doing. > > > > Of course, it may seem more natural to start from the leftmost column, then go > > to the following one, etc. But this is particular to that case. With a > > right-to-left language it will be more natural to start from the rightmost > > column. Sometimes, the content will be such that the method above will be more > > natural. > > > > So this is a grey area, and the Recommendation doesn't say anything about that. > > Your best bet is to re-number the footnotes. Or use something else than > > footnotes (you may be happy with putting the notes in regular blocks just after > > the table, for example). > > > > HTH, > > Vincent > > Hi Vincent, > > I understand your point of view, you are probably right. Anyway, current > implementation is not very reliable. If you leave only two blocks in the left > column in my example, footnote labeled 1 will be output first, although, > according to FOP behavior, we can expect it should be footnote labeled 2. The change I made introduced another bug. It should be fixed now in revision 770635. Sorry about that. That said, I can think of certain situations involving row-spanning cells where the basic 'rule' stated above does no longer hold. I won't enter the details because they are a bit technical, but interesting issues may arise regarding accessibility, order of reading, etc. (That was a note to self ) Vincent
          Hide
          ToM added a comment -

          Footnotes are working now for tables and lists however i still experience that bug when using table-header elements. (The body part doesn't show up at all).

          Hope this will get fixed soon

          Show
          ToM added a comment - Footnotes are working now for tables and lists however i still experience that bug when using table-header elements. (The body part doesn't show up at all). Hope this will get fixed soon
          Hide
          Glenn Adams added a comment -

          resetting P2 open bugs to P3 pending further review

          Show
          Glenn Adams added a comment - resetting P2 open bugs to P3 pending further review
          Hide
          Glenn Adams added a comment -

          change status from ASSIGNED to NEW for consistency

          Show
          Glenn Adams added a comment - change status from ASSIGNED to NEW for consistency
          Hide
          Chris Bowditch added a comment -

          The W3C XSL-FO sub group has been suspended, so we aren't going to get an answer from them any time soon. I suggest we drive the behaviour from user/customer requirements instead. I have a customer that wants to be able to repeat footnotes for every page that a table spans. Putting the footnote in the header is the perfect way to achieve this. If the user didnt want it repeating for every page, they can move the footnote into the first table-row after the table-header. So making the footnotes in headers repeat give the user the choice of which behaviour they want. Therefore it makes sense to implement footnotes in headers so that they repeat, allowing either requirement to be fulfilled.

          Show
          Chris Bowditch added a comment - The W3C XSL-FO sub group has been suspended, so we aren't going to get an answer from them any time soon. I suggest we drive the behaviour from user/customer requirements instead. I have a customer that wants to be able to repeat footnotes for every page that a table spans. Putting the footnote in the header is the perfect way to achieve this. If the user didnt want it repeating for every page, they can move the footnote into the first table-row after the table-header. So making the footnotes in headers repeat give the user the choice of which behaviour they want. Therefore it makes sense to implement footnotes in headers so that they repeat, allowing either requirement to be fulfilled.
          Hide
          Vincent Hennebert added a comment -

          Repeating the footnote on every page creates all sorts of problems: what do we do if a footnote doesn't fit on the current page and has to be deferred to the next one? The next page will end up having two identical footnotes. Also, how would footnote numbering work? The number would be duplicated?

          Putting the footnote into the first table-row after the table-header is well and good, but then the footnote mark will no longer appear in the header, which will probably not be what the user wants.

          It seems to me that this kind of requirement is better solved by using markers.

          Show
          Vincent Hennebert added a comment - Repeating the footnote on every page creates all sorts of problems: what do we do if a footnote doesn't fit on the current page and has to be deferred to the next one? The next page will end up having two identical footnotes. Also, how would footnote numbering work? The number would be duplicated? Putting the footnote into the first table-row after the table-header is well and good, but then the footnote mark will no longer appear in the header, which will probably not be what the user wants. It seems to me that this kind of requirement is better solved by using markers.
          Hide
          Jesper Jakobsen added a comment -

          I would like to see a fix for the header footnote body disappearing.

          I think the best and simplest solution would be to treat them like normal footnotes that are printed once. Each repeat of the header should then disregard footnotes, perhaps leaving the inline part of the footnote.

          Show
          Jesper Jakobsen added a comment - I would like to see a fix for the header footnote body disappearing. I think the best and simplest solution would be to treat them like normal footnotes that are printed once. Each repeat of the header should then disregard footnotes, perhaps leaving the inline part of the footnote.
          Hide
          simon steiner added a comment -

          Footnote in table header patch, not 100% perfect I was new to Layout Engine

          Show
          simon steiner added a comment - Footnote in table header patch, not 100% perfect I was new to Layout Engine
          Hide
          simon steiner added a comment -

          I was following XEP behaviour

          Show
          simon steiner added a comment - I was following XEP behaviour
          Hide
          Matthias Reischenbacher added a comment -

          I was preparing for hours and hours of debugging FOP internals , when I found your patch, Simon. Thanks a lot! It worked perfectly.

          Show
          Matthias Reischenbacher added a comment - I was preparing for hours and hours of debugging FOP internals , when I found your patch, Simon. Thanks a lot! It worked perfectly.
          Hide
          Matthias Reischenbacher added a comment -

          After more detailed testing I discovered that footnotes inside the table header were listed after footnotes inside the table body. I reworked the patch form Simon in order to fix that. I removed the new code dependency between page breaking and table related classes, which means that the repetition of footnotes in case of tables spanning multiple pages is also gone. I also attempted to reuse some footnote related code in a FootnoteUtil class. It would be nice to combine both patches and make the repetition configurable but that exceeds by far my knowledge of the layout related code. I'm attaching my patch in case someone wants to continue working on that.

          Show
          Matthias Reischenbacher added a comment - After more detailed testing I discovered that footnotes inside the table header were listed after footnotes inside the table body. I reworked the patch form Simon in order to fix that. I removed the new code dependency between page breaking and table related classes, which means that the repetition of footnotes in case of tables spanning multiple pages is also gone. I also attempted to reuse some footnote related code in a FootnoteUtil class. It would be nice to combine both patches and make the repetition configurable but that exceeds by far my knowledge of the layout related code. I'm attaching my patch in case someone wants to continue working on that.
          Hide
          Vincent Hennebert added a comment -

          Hi Matthias,

          Thanks for your patch! I've applied it in rev. 1599344.

          I've made some modifications, essentially clean-up and javadoc, plus I added a test case and fixed one mistake: the KnuthBlockBox to add to the list when the header is repeated at page breaks should be of height 0, since the header height is already taken into account in headerAsSecondToLast.

          I didn't apply Simon's patch as it has several faults. See attached files. For example the footnote from the header will be added after any footnote from the body (inverted-footnotes.fo). Or the footnote will be found at the very end of the page sequence instead of being on the same page as the table.

          Those test cases are relatively simple, so I don't think we can proceed in this state. Basically the code that deals with footnotes in repeated table headers/footers doesn't integrate in the general footnote code, which can only cause issues.

          The fact that repeated headers/footers are handled by using a penalty whose height corresponds to the additional heights of the header and footer will inevitably be a problem for obtaining the correct order of footnotes between the table header and the rest of the document.

          So more work is needed, and the fact that the footnote code is extremely complex will make it... interesting

          Vincent

          Show
          Vincent Hennebert added a comment - Hi Matthias, Thanks for your patch! I've applied it in rev. 1599344 . I've made some modifications, essentially clean-up and javadoc, plus I added a test case and fixed one mistake: the KnuthBlockBox to add to the list when the header is repeated at page breaks should be of height 0, since the header height is already taken into account in headerAsSecondToLast. I didn't apply Simon's patch as it has several faults. See attached files. For example the footnote from the header will be added after any footnote from the body (inverted-footnotes.fo). Or the footnote will be found at the very end of the page sequence instead of being on the same page as the table. Those test cases are relatively simple, so I don't think we can proceed in this state. Basically the code that deals with footnotes in repeated table headers/footers doesn't integrate in the general footnote code, which can only cause issues. The fact that repeated headers/footers are handled by using a penalty whose height corresponds to the additional heights of the header and footer will inevitably be a problem for obtaining the correct order of footnotes between the table header and the rest of the document. So more work is needed, and the fact that the footnote code is extremely complex will make it... interesting Vincent
          Hide
          Vincent Hennebert added a comment -

          I had another go at this problem and came up with the attached patch. It leverages the patch from Matthias and adds support for repeated table headers/footers.

          For the first header/last footer, footnotes are treated the usual way. That means that if the table is not broken over pages then footnotes coming from its header/footer will be handled just like any other footnote.

          For repeated headers/footers, I added a mechanism that registers footnotes separately and inserts them at the right place at area creation time. That is, footnotes from repeated headers first, then other footnotes, then footnotes from repeated footers.

          The size of the footnote is accounted for in the penalty that represents the header/footer. As a consequence, the footnote will always be on the same page as the header/footer and will never be deferred to later pages. The regular footnote-handling mechanism is essentially by-passed here.

          This works well except in the case of nested tables, when the outer table has a repeated table-footer. Gaps will be visible between that footer and the inner table, and the outer footer may even be mixed with the footnotes.

          I guess we’ll have to leave with this limitation. Footnotes in repeated table headers/footers already is a degenerate case IMO, so I would say footnotes in repeated headers/footers in nested tables is pushing too far...

          If people could try out this patch and give feedback, that would be great. I’ll leave a few days for peer review and then, if nobody has objected, I'll commit it to trunk.

          Show
          Vincent Hennebert added a comment - I had another go at this problem and came up with the attached patch. It leverages the patch from Matthias and adds support for repeated table headers/footers. For the first header/last footer, footnotes are treated the usual way. That means that if the table is not broken over pages then footnotes coming from its header/footer will be handled just like any other footnote. For repeated headers/footers, I added a mechanism that registers footnotes separately and inserts them at the right place at area creation time. That is, footnotes from repeated headers first, then other footnotes, then footnotes from repeated footers. The size of the footnote is accounted for in the penalty that represents the header/footer. As a consequence, the footnote will always be on the same page as the header/footer and will never be deferred to later pages. The regular footnote-handling mechanism is essentially by-passed here. This works well except in the case of nested tables, when the outer table has a repeated table-footer. Gaps will be visible between that footer and the inner table, and the outer footer may even be mixed with the footnotes. I guess we’ll have to leave with this limitation. Footnotes in repeated table headers/footers already is a degenerate case IMO, so I would say footnotes in repeated headers/footers in nested tables is pushing too far... If people could try out this patch and give feedback, that would be great. I’ll leave a few days for peer review and then, if nobody has objected, I'll commit it to trunk.
          Hide
          Vincent Hennebert added a comment -

          Patch committed in rev. 1603386.

          Show
          Vincent Hennebert added a comment - Patch committed in rev. 1603386 .

            People

            • Assignee:
              Unassigned
              Reporter:
              gerhard oettl
            • Votes:
              0 Vote for this issue
              Watchers:
              14 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development