Fix the computation of cells' IPD when they contain children with forced breaks.

In such cases TableCellLM.getNextKnuthElements was called several times, and thus the borders/paddings of the cell were substracted several times from the IPD.


git-svn-id: https://svn.apache.org/repos/asf/xmlgraphics/fop/trunk@530724 13f79535-47bb-0310-9956-ffa450edef68

src/java/org/apache/fop/layoutmgr/AreaAdditionUtil.java

@@ -101,6 +101,8 @@ public class AreaAdditionUtil {
 
         StackingIter childPosIter = new StackingIter(positionList.listIterator());
         while ((childLM = childPosIter.getNextChildLM()) != null) {
+            // TODO vh: the test above might be problematic in some cases. See comment in
+            // the TableCellLM.getNextKnuthElements method
             // Add the block areas to Area
             lc.setFlags(LayoutContext.FIRST_AREA, childLM == firstLM);
             lc.setFlags(LayoutContext.LAST_AREA, childLM == lastLM);

src/java/org/apache/fop/layoutmgr/table/TableCellLayoutManager.java

@@ -34,6 +34,7 @@ import org.apache.fop.layoutmgr.BlockStackingLayoutManager;
 import org.apache.fop.layoutmgr.BreakElement;
 import org.apache.fop.layoutmgr.KnuthElement;
 import org.apache.fop.layoutmgr.KnuthGlue;
+import org.apache.fop.layoutmgr.KnuthPenalty;
 import org.apache.fop.layoutmgr.LayoutContext;
 import org.apache.fop.layoutmgr.ListElement;
 import org.apache.fop.layoutmgr.PositionIterator;
@@ -175,78 +176,55 @@ public class TableCellLayoutManager extends BlockStackingLayoutManager
                 childLC.setFlags(LayoutContext.KEEP_WITH_PREVIOUS_PENDING, false);
             }
 
-            if (returnedList.size() == 1
-                    && ((ListElement)returnedList.getFirst()).isForcedBreak()) {
-                // a descendant of this block has break-before
-                if (returnList.size() == 0) {
-                    // the first child (or its first child ...) has
-                    // break-before;
-                    // all this block, including space before, will be put in
-                    // the
-                    // following page
-                }
-                contentList.addAll(returnedList);
-
-                // "wrap" the Position inside each element
-                // moving the elements from contentList to returnList
-                returnedList = new LinkedList();
-                wrapPositionElements(contentList, returnList);
-
-                //Space resolution
-                SpaceResolver.resolveElementList(returnList);
-
-                return returnList;
-            } else {
-                if (prevLM != null) {
-                    // there is a block handled by prevLM
-                    // before the one handled by curLM
-                    if (mustKeepTogether()
-                            || context.isKeepWithNextPending()
-                            || childLC.isKeepWithPreviousPending()) {
-                        //Clear keep pending flag
-                        context.setFlags(LayoutContext.KEEP_WITH_NEXT_PENDING, false);
-                        childLC.setFlags(LayoutContext.KEEP_WITH_PREVIOUS_PENDING, false);
-                        // add an infinite penalty to forbid a break between
-                        // blocks
-                        contentList.add(new BreakElement(
-                                new Position(this), KnuthElement.INFINITE, context));
-                        //contentList.add(new KnuthPenalty(0,
-                        //        KnuthElement.INFINITE, false,
-                        //        new Position(this), false));
-                    } else if (!((ListElement) contentList.getLast()).isGlue()) {
-                        // add a null penalty to allow a break between blocks
-                        contentList.add(new BreakElement(
-                                new Position(this), 0, context));
-                        //contentList.add(new KnuthPenalty(0, 0, false,
-                        //        new Position(this), false));
-                    } else {
-                        // the last element in contentList is a glue;
-                        // it is a feasible breakpoint, there is no need to add
-                        // a penalty
-                    }
-                }
-                contentList.addAll(returnedList);
-                if (returnedList.size() == 0) {
-                    //Avoid NoSuchElementException below (happens with empty blocks)
-                    continue;
-                }
-                if (((ListElement)returnedList.getLast()).isForcedBreak()) {
-                    // a descendant of this block has break-after
-                    if (curLM.isFinished()) {
-                        // there is no other content in this block;
-                        // it's useless to add space after before a page break
-                        setFinished(true);
-                    }
-
-                    returnedList = new LinkedList();
-                    wrapPositionElements(contentList, returnList);
-
-                    //Space resolution
-                    SpaceResolver.resolveElementList(returnList);
-
-                    return returnList;
+            if (prevLM != null) {
+                // there is a block handled by prevLM
+                // before the one handled by curLM
+                if (mustKeepTogether()
+                        || context.isKeepWithNextPending()
+                        || childLC.isKeepWithPreviousPending()) {
+                    //Clear keep pending flag
+                    context.setFlags(LayoutContext.KEEP_WITH_NEXT_PENDING, false);
+                    childLC.setFlags(LayoutContext.KEEP_WITH_PREVIOUS_PENDING, false);
+                    // add an infinite penalty to forbid a break between
+                    // blocks
+                    contentList.add(new BreakElement(
+                            new Position(this), KnuthElement.INFINITE, context));
+                    //contentList.add(new KnuthPenalty(0,
+                    //        KnuthElement.INFINITE, false,
+                    //        new Position(this), false));
+                } else if (!(((ListElement) contentList.getLast()).isGlue()
+                        || (((ListElement)contentList.getLast()).isPenalty()
+                                && ((KnuthPenalty)contentList.getLast()).getP() < KnuthElement.INFINITE)
+                                || (contentList.getLast() instanceof BreakElement
+                                        && ((BreakElement)contentList.getLast()).getPenaltyValue() < KnuthElement.INFINITE))) {
+                    // TODO vh: this is hacky
+                    // The getNextKnuthElements method of TableCellLM must not be called
+                    // twice, otherwise some settings like indents or borders will be
+                    // counted several times and lead to a wrong output. Anyway the
+                    // getNextKnuthElements methods should be called only once eventually
+                    // (i.e., when multi-threading the code), even when there are forced
+                    // breaks.
+                    // If we add a break possibility after a forced break the
+                    // AreaAdditionUtil.addAreas method will act on a sequence starting
+                    // with a SpaceResolver.SpaceHandlingBreakPosition element, having no
+                    // LM associated to it. Thus it will stop early instead of adding
+                    // areas for following Positions. The above test aims at preventing
+                    // such a situation from occuring. add a null penalty to allow a break
+                    // between blocks
+                    contentList.add(new BreakElement(
+                            new Position(this), 0, context));
+                    //contentList.add(new KnuthPenalty(0, 0, false,
+                    //        new Position(this), false));
+                } else {
+                    // the last element in contentList is a feasible breakpoint, there is
+                    // no need to add a penalty
                 }
             }
+            contentList.addAll(returnedList);
+            if (returnedList.size() == 0) {
+                //Avoid NoSuchElementException below (happens with empty blocks)
+                continue;
+            }
             if (childLC.isKeepWithNextPending()) {
                 //Clear and propagate
                 childLC.setFlags(LayoutContext.KEEP_WITH_NEXT_PENDING, false);
@@ -625,4 +603,3 @@ public class TableCellLayoutManager extends BlockStackingLayoutManager
     }
 
 }
-

test/layoutengine/standard-testcases/table-cell_block_break-after.xml

@@ -43,14 +43,14 @@
                   <fo:block>Cell 1</fo:block>
                   <fo:block>Cell 1</fo:block>
                 </fo:table-cell>
-                <fo:table-cell border="4pt solid black">
-                  <fo:block>Cell 2</fo:block>
+                <fo:table-cell border="6pt solid black">
+                  <fo:block>Cell 2.1</fo:block>
                   <fo:block keep-together="always" background-color="yellow">
-                    <fo:block>Cell 2</fo:block>
-                    <fo:block>Cell 2</fo:block>
-                    <fo:block break-after="page">Cell 2</fo:block>
+                    <fo:block>Cell 2.2.1</fo:block>
+                    <fo:block>Cell 2.2.2</fo:block>
+                    <fo:block break-after="page">Cell 2.2.3</fo:block>
                   </fo:block>
-                    <fo:block>Cell 2</fo:block>
+                    <fo:block>Cell 2.3</fo:block>
                 </fo:table-cell>
               </fo:table-row>
             </fo:table-body>
@@ -63,20 +63,36 @@
     <eval expected="2" xpath="count(//pageViewport)"/>
     <!-- page 1 -->
     <!-- 3 blocks in the first cell -->
-    <eval expected="3" xpath="count(//pageViewport[@nr=1]/page/regionViewport/regionBody/mainReference/span/flow/block[1]/block[1]/block)"/>
+    <eval expected="3" xpath="count(//pageViewport[@nr=1]//flow/block[1]/block[1]/block)"/>
+    <eval expected="67000" xpath="//pageViewport[@nr=1]//flow/block[1]/block[1]/@ipd"/>
+    <eval expected="75000" xpath="//pageViewport[@nr=1]//flow/block[1]/block[1]/@ipda"/>
+    <eval expected="71200" xpath="//pageViewport[@nr=1]//flow/block[1]/block[1]/@bpd"/>
+    <eval expected="79200" xpath="//pageViewport[@nr=1]//flow/block[1]/block[1]/@bpda"/>
     <!-- 2 blocks in the second cell -->
-    <eval expected="2" xpath="count(//pageViewport[@nr=1]/page/regionViewport/regionBody/mainReference/span/flow/block[1]/block[2]/block)"/>
+    <eval expected="2" xpath="count(//pageViewport[@nr=1]//flow/block[1]/block[2]/block)"/>
+    <eval expected="63000" xpath="//pageViewport[@nr=1]//flow/block[1]/block[2]/@ipd"/>
+    <eval expected="75000" xpath="//pageViewport[@nr=1]//flow/block[1]/block[2]/@ipda"/>
+    <eval expected="67200" xpath="//pageViewport[@nr=1]//flow/block[1]/block[2]/@bpd"/>
+    <eval expected="79200" xpath="//pageViewport[@nr=1]//flow/block[1]/block[2]/@bpda"/>
     <!-- 3 lines in the second block of the second cell -->
-    <eval expected="3" xpath="count(//pageViewport[@nr=1]/page/regionViewport/regionBody/mainReference/span/flow/block[1]/block[2]/block[2]//lineArea)"/>
+    <eval expected="3" xpath="count(//pageViewport[@nr=1]//flow/block[1]/block[2]/block[2]//lineArea)"/>
 
     <!-- page 2 -->
     <!-- Nothing in the first cell -->
-    <eval expected="0" xpath="count(//pageViewport[@nr=2]/page/regionViewport/regionBody/mainReference/span/flow/block[1]/block[1]/block)"/>
+    <eval expected="0" xpath="count(//pageViewport[@nr=2]//flow/block[1]/block[1]/block)"/>
+    <eval expected="67000" xpath="//pageViewport[@nr=2]//flow/block[1]/block[1]/@ipd"/>
+    <eval expected="75000" xpath="//pageViewport[@nr=2]//flow/block[1]/block[1]/@ipda"/>
+    <eval expected="20800" xpath="//pageViewport[@nr=2]//flow/block[1]/block[1]/@bpd"/>
+    <eval expected="28800" xpath="//pageViewport[@nr=2]//flow/block[1]/block[1]/@bpda"/>
     <!-- Two blocks expected in the second cell -->
-    <eval expected="2" xpath="count(//pageViewport[@nr=2]/page/regionViewport/regionBody/mainReference/span/flow/block[1]/block[2]/block)"/>
+    <eval expected="2" xpath="count(//pageViewport[@nr=2]//flow/block[1]/block[2]/block)"/>
     <!-- First block has zero bpd -->
-    <eval expected="0" xpath="//pageViewport[@nr=2]/page/regionViewport/regionBody/mainReference/span/flow/block[1]/block[2]/block[1]/@bpd"/>
+    <eval expected="0" xpath="//pageViewport[@nr=2]//flow/block[1]/block[2]/block[1]/@bpd"/>
     <!-- One line in the second block -->
-    <eval expected="1" xpath="count(//pageViewport[@nr=2]/page/regionViewport/regionBody/mainReference/span/flow/block[1]/block[2]/block[2]//lineArea)"/>
+    <eval expected="1" xpath="count(//pageViewport[@nr=2]//flow/block[1]/block[2]/block[2]//lineArea)"/>
+    <eval expected="63000" xpath="//pageViewport[@nr=2]//flow/block[1]/block[2]/@ipd"/>
+    <eval expected="75000" xpath="//pageViewport[@nr=2]//flow/block[1]/block[2]/@ipda"/>
+    <eval expected="16800" xpath="//pageViewport[@nr=2]//flow/block[1]/block[2]/@bpd"/>
+    <eval expected="28800" xpath="//pageViewport[@nr=2]//flow/block[1]/block[2]/@bpda"/>
   </checks>
 </testcase>