Bug 37579

Summary: footnotes within tables and listsl get lost
Product: Fop - Now in Jira Reporter: gerhard oettl <gerhard.oettl>
Component: page-master/layoutAssignee: fop-dev
Status: NEW ---    
Severity: normal CC: desrt, dev, fyodor, jaredsmith, jmt4b04d4v, joe.nemeth, Justus-bulk, mail, mdke, ron.vandenbranden
Priority: P3    
Version: trunk   
Target Milestone: ---   
Hardware: Other   
OS: other   
Bug Depends on:    
Bug Blocks: 17369, 3305    
Attachments: example for lost footnote in table-cell
1/2 patch
example fo for tables
example fo for lists
Partial patch, updated
Updated patch against FOP Trunk, which passes all layout-tests and enables two more
Patch for FootnoteBodyLayoutManager
alternative patch for footnotes in table-cells
updated patch against FOP trunk
Updated patch, TableStepper delegating more to ActiveCell
Example of lost footnote in table against trunk 660979
Example of wrong order of footnotes against trunk 768320

Description gerhard oettl 2005-11-21 18:27:48 UTC
Footnote outside of tables works, inside table-cell not.
The compliance page only speaks about restrictions within multicolumn documents.
Comment 1 gerhard oettl 2005-11-21 18:30:02 UTC
Created attachment 17010 [details]
example for lost footnote in table-cell
Comment 2 Jeremias Maerki 2005-11-22 11:02:04 UTC
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.
Comment 3 gerhard oettl 2006-01-16 14:34:33 UTC
Created attachment 17432 [details]
1/2 patch
Comment 4 gerhard oettl 2006-01-16 14:36:38 UTC
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).
Comment 5 gerhard oettl 2006-01-16 14:43:12 UTC
Created attachment 17433 [details]
example fo for tables
Comment 6 gerhard oettl 2006-01-16 14:45:21 UTC
Created attachment 17434 [details]
example fo for lists
Comment 7 Sebastian Krysmanski 2006-05-25 09:14:54 UTC
*** Bug 39633 has been marked as a duplicate of this bug. ***
Comment 8 Jeremias Maerki 2007-02-17 14:24:42 UTC
*** Bug 41634 has been marked as a duplicate of this bug. ***
Comment 9 Jeremy Hughes 2007-05-20 00:34:12 UTC
Any further progress or ETA?
Comment 10 Ih 2007-10-18 04:17:43 UTC
Is there any workaround to solve this problem? We work on a project, and we need
this feature...  :/
Comment 11 Jeremias Maerki 2007-10-18 04:55:37 UTC
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.
Comment 12 Adam Strzelecki 2008-01-31 09:29:38 UTC
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.
Comment 13 Andreas L. Delmelle 2008-02-13 12:36:27 UTC
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 bug 37579
                         (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.

Comment 14 Ron Van den Branden 2008-03-14 13:10:27 UTC
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.
Comment 15 Luca Furini 2008-05-02 08:06:47 UTC
Created attachment 21907 [details]
Partial patch, updated
Comment 16 Luca Furini 2008-05-02 08:07:37 UTC
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.
Comment 17 Andreas L. Delmelle 2008-05-02 09:27:37 UTC
(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.
Comment 18 Andreas L. Delmelle 2008-05-02 10:21:09 UTC
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...
Comment 19 Andreas L. Delmelle 2008-05-02 12:48:50 UTC
(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.
Comment 20 Andreas L. Delmelle 2008-05-02 12:51:13 UTC
Created attachment 21908 [details]
Updated patch against FOP Trunk, which passes all layout-tests and enables two more
Comment 21 Andreas L. Delmelle 2008-05-02 13:49:08 UTC
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)
Comment 22 Andreas L. Delmelle 2008-05-02 13:57:33 UTC
(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'.
Comment 23 Luca Furini 2008-05-05 05:59:03 UTC
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 ...
Comment 24 Andreas L. Delmelle 2008-05-05 07:15:17 UTC
(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.
Comment 25 Andreas L. Delmelle 2008-05-05 09:16:07 UTC
(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...
Comment 26 Andreas L. Delmelle 2008-05-05 09:41:40 UTC
(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...
Comment 27 Andreas L. Delmelle 2008-05-05 10:02:04 UTC
(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)
Comment 28 Andreas L. Delmelle 2008-05-05 10:42:16 UTC
Created attachment 21922 [details]
Patch for FootnoteBodyLayoutManager

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.
Comment 29 Andreas L. Delmelle 2008-05-12 01:52:36 UTC
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...
Comment 30 Andreas L. Delmelle 2008-05-12 13:45:00 UTC
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...
Comment 31 Andreas L. Delmelle 2008-05-13 09:35:50 UTC
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.
Comment 32 Andreas L. Delmelle 2008-05-14 00:27:45 UTC
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.
Comment 33 Luca Furini 2008-05-14 07:37:23 UTC
(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


Comment 34 Andreas L. Delmelle 2008-05-16 06:16:07 UTC
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.
Comment 35 Adrian Cumiskey 2008-05-16 08:37:11 UTC
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.
> 

Comment 36 Andreas L. Delmelle 2008-05-16 09:23:54 UTC
(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
Comment 37 Vincent Hennebert 2008-05-16 10:16:00 UTC
(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
Comment 38 Andreas L. Delmelle 2008-05-16 11:29:36 UTC
(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.
Comment 39 Andreas L. Delmelle 2008-05-16 11:37:29 UTC
(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().
Comment 40 Andreas L. Delmelle 2008-05-17 03:04:03 UTC
(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. ;-)
Comment 41 Andreas L. Delmelle 2008-05-17 13:05:46 UTC
Created attachment 21976 [details]
alternative patch for footnotes in table-cells


If I'm correct, this should be roughly what Vincent proposed (?)
Comment 42 Andreas L. Delmelle 2008-05-17 13:25:11 UTC
(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.
Comment 43 Andreas L. Delmelle 2008-05-17 14:40:35 UTC
Created attachment 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.
Comment 44 Andreas L. Delmelle 2008-05-17 16:19:07 UTC
(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?
Comment 45 Vincent Hennebert 2008-05-19 02:52:15 UTC
(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
Comment 46 Vincent Hennebert 2008-05-19 02:56:57 UTC
Created attachment 21979 [details]
Updated patch, TableStepper delegating more to ActiveCell

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.
Comment 47 Vincent Hennebert 2008-05-19 02:58:37 UTC
(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
Comment 48 Vincent Hennebert 2008-05-27 09:45:13 UTC
(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
Comment 49 Andreas L. Delmelle 2008-05-27 14:01:26 UTC
(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.
Comment 50 Vincent Hennebert 2008-05-28 08:30:54 UTC
(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
Comment 51 Vincent Hennebert 2008-05-28 08:48:08 UTC
(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...
Comment 52 Johans Marvin Taboada Villca 2008-06-08 14:44:42 UTC
(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.
Comment 53 Andreas L. Delmelle 2008-06-08 15:25:30 UTC
(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.
Comment 54 Sylvestre Ledru 2009-02-27 07:18:47 UTC
I believe this bug is now fixed. Isn't it ?
Comment 55 Vincent Hennebert 2009-03-02 02:59:40 UTC
(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
Comment 56 Dimitri Goloborodko 2009-04-14 03:30:40 UTC
Created attachment 23492 [details]
Example of lost footnote in table against trunk 660979
Comment 57 Dimitri Goloborodko 2009-04-14 03:33:19 UTC
(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?
Comment 58 Vincent Hennebert 2009-04-15 11:01:31 UTC
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
Comment 59 Vincent Hennebert 2009-04-24 07:30:08 UTC
Comment on attachment 23492 [details]
Example of lost footnote in table against trunk 660979

This bug has now been fixed.
http://svn.apache.org/viewvc?view=rev&revision=768320
Comment 60 Dimitri Goloborodko 2009-04-29 01:11:12 UTC
Created attachment 23561 [details]
Example of wrong order of footnotes against trunk 768320
Comment 61 Dimitri Goloborodko 2009-04-29 01:14:59 UTC
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
Comment 62 Vincent Hennebert 2009-04-29 03:55:03 UTC
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
Comment 63 Dimitri Goloborodko 2009-04-29 06:55:22 UTC
(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
Comment 64 Vincent Hennebert 2009-05-01 04:31:47 UTC
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
Comment 65 ToM 2011-01-10 10:02:33 UTC
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 :)
Comment 66 Glenn Adams 2012-04-07 01:41:24 UTC
resetting P2 open bugs to P3 pending further review
Comment 67 Glenn Adams 2012-04-11 06:16:41 UTC
change status from ASSIGNED to NEW for consistency