In this problem I have 2 tables. The first one is ending just before the page ends. It is not enough place on the page to display the header of the second table but there is enough place to display 1 row of the second table. The header2 goes to the next page but 1 row of the table is displayed on the previous page. This means the table-bodys first row is displayed BEFORE the table-header Attached is XSL/FO and PDF.
Created attachment 25952 [details] test pdf
Created attachment 25953 [details] test XSL/FO file
Created attachment 25995 [details] Reduced test case I've reduced the test case to the minimum (attached). It appears to be the same problem I've noticed when I tried to fix column balancing (Bug 49801). It appears to be triggered by the preceding span="all". Something about that algorithm restart after the span change seems to be fishy.
Created attachment 26053 [details] Minimal Test Case Test case can be reduced even more. For some reason, the algorithm starts at startIndex 1 after the overflow for line/part 2 even though at startIndex 1, there is no legal break (inf penalty). Still haven't found out why this is happening.
Created attachment 26583 [details] Minimal Test Case - Slightly Altered So I took a closer look at this one. No immediate fix in mind yet, but one observation so far that could be of use in further tracking this down. Take a look at the revised attachment. The overflow recovery/deferral mechanism is doing exactly what it is supposed to here. The issue seems to manifest itself in case an overflow condition is reached, and there is no 'real' previous node yet. The auxiliary node that is added at the start, right before the main loop in findBreakingPoints() is entered, is deactivated, and the algorithm then tries to create a 'last forced' node to restart from later (findBreakingPoints() line 577 -> recoverFromTooLong()). This all seems to work fine as long as there actually is a valid node to restart from, but something seems to go wrong in case there isn't, yet.
Operating on a hunch, I tried prepending each blockList in AbstractBreaker.doLayout() with an auxiliary penalty (w=0, p=0). At line 394, I added a simple: blockList.add(0, new KnuthPenalty(0, 0, false, null, true)); It seems to trigger correct behavior in this case, as the entire block is now deferred to the second page, where it can be kept together in one column. On the other hand, that quick hack breaks quite some testcases. All those that check for element-list content now suddenly have to deal with an extra element at position 0... Fact remains that, without this penalty, the last forced node points to position 0, which corresponds to the first box, while it should actually point to the position preceding the first box.
Created attachment 26604 [details] potential fix Looking closer at this, so far, I have not found a possibility to generate a node in BreakingAlgorithm that merely produces a break, but does not include at least one element... Comparing to the case of a forced break, or the cases where it does work as expected, the difference is always that there is a penalty element preceding the content that should be kept together. So, it looks more and more like we cannot avoid somehow inserting that auxiliary penalty. The quick hack I tried earlier, adds this penalty unconditionally, to all block lists. Another, slightly more attractive alternative would be to have the BreakingAlgorithm alter the paragraph and have it insert this penalty when appropriate. That would fix the issue without having to adapt all the testcases that observe the element lists. Attached minimal patch fixes the issue by conditionally adding the auxiliary penalty in recoverFromTooLong(). If the lastTooLong node would be the first real break, we add the penalty to enable the creation of a node representing a break before the first box. It still 'broke' one layout test: fox_disable-column-balancing.xml. Checking closer, this actually seems to fix the test, rather than break it. The failing check was followed by another, commented check. Removing the failing one, and uncommenting the other, the layout test suite passes completely. Bug 49877 also seems to be fixed by the patch, so I am going to mark that as a duplicate to begin with.
*** Bug 49877 has been marked as a duplicate of this bug. ***
Created attachment 26608 [details] revised patch Slightly more restricted: only add the penalty if the first element of the paragraph is not yet a penalty. (+ might as well extract that dummy penalty constructor into a static final that can be used elsewhere) Also included a test case, checking for some possibilities in single/multi column layout: - single column, constant bpd - single column, alternating bpd (increase sufficient to keep together) - multi column constant bpd - multi column, alternating bpd (increase sufficient to keep together) - single column, alternating bpd (increase insufficient to keep together) - multi column, alternating bpd (increase insufficient to keep together)
We have tested the revised patch, and it does fix the problem as described by a.kovacs. The minimal test case must be wrong. The output is the same with or without the patch and it behaves normally, ie rows are displayed were they should. Thanks!
Another note: bug 49877 is NOT fixed by this patch so it should be re-opened. https://issues.apache.org/bugzilla/show_bug.cgi?id=49877
> We have tested the revised patch, and it does fix the problem as described by > a.kovacs. > > The minimal test case must be wrong. The output is the same with or without the > patch and it behaves normally, ie rows are displayed were they should. Which test case are you referring to exactly? I checked all attachments before and after the patch. Before, the content was split anyway after the first line, even though it could be kept together as a whole if deferred to the next page in its entirety. Can you provide more details on why you would consider bug 49877 not to be fixed? Before the change, there was a strange overlap at the bottom of the first page. When I tried that sample after applying the patch, the offending piece of content was kept together on the next page, as expected. There is indeed still the violation of a keep-constraint. I was going to file that as a different, more general issue (= violation of keep-constraints across span changes), but didn't get around to that yet. If you would rather make that the main issue for bug 49877, or you still have other reasons, then by all means, go ahead and reopen.
(In reply to comment #13) > > The minimal test case must be wrong. The output is the same with or without the > > patch and it behaves normally, ie rows are displayed were they should. > > Which test case are you referring to exactly? I checked all attachments before > and after the patch. It just occurred to me that, if you mean the 'slightly altered' test case, you may want to read comment 5 more closely. It is expected to produce the same result before and after. More a reference sample to be able to see the difference during debugging.
(In reply to comment #14) > (In reply to comment #13) > > > The minimal test case must be wrong. The output is the same with or without the > > > patch and it behaves normally, ie rows are displayed were they should. > > > > Which test case are you referring to exactly? I checked all attachments before > > and after the patch. > > It just occurred to me that, if you mean the 'slightly altered' test case, you > may want to read comment 5 more closely. It is expected to produce the same > result before and after. More a reference sample to be able to see the > difference during debugging. I am sorry Andreas, I am not an expert in fop internals, so my response might have been too fast (and wrong) and it created some confusion. I was under the impression, that the minimal test case (by Jeremias) manifested the same behavior as Adam's original sample. The result I got from minimal, with and without, the patch is identical. So, I guess this test case does not expose the problem. I haven't tested with your revised (Slightly Altered) test, and I was not referring to it.. As for the other bug, it is my mistake, I was referring to bug https://issues.apache.org/bugzilla/show_bug.cgi?id=49801, which is still open and is a totally different case. I am sorry again for the misunderstanding. Regards, Alex
(In reply to comment #15) Alex, Added you to the CC list so that you are notified directly of updates. Hope you don't mind. > I was under the impression, that the minimal test case (by Jeremias) manifested > the same behavior as Adam's original sample. The result I got from minimal, > with and without, the patch is identical. So, I guess this test case does not > expose the problem. I haven't tested with your revised (Slightly Altered) test, > and I was not referring to it.. That's strange... I will attach the output before/after the patch here. They definitely look different on my end. After the patch, the entire block is deferred to the second page, where it can be kept together as a whole. > As for the other bug, it is my mistake, I was referring to bug > https://issues.apache.org/bugzilla/show_bug.cgi?id=49801, which is still open > and is a totally different case. OK, no problem! You can add yourself to the CC list of that bug if you want to follow up there.
Created attachment 26686 [details] Minimal Test Case - before attached patch
Created attachment 26687 [details] Minimal Test Case PDF - after attached patch
Thanks, Andreas, for looking into this. I've reviewed your solution and think you should apply it.
(In reply to comment #19) > Thanks, Andreas, for looking into this. I've reviewed your solution and think > you should apply it. OK, done. See: http://svn.apache.org/viewvc?rev=1079013&view=rev
batch transition to closed; if someone wishes to restore one of these to resolved in order to perform a verification step, then feel free to do so