Bugfix: padding-top and padding-bottom on list-items could lead to wrong page break decisions and thus to overlapping text.

Bugfix: break-before and break-after were ignored on list-items.


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

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

@@ -1,5 +1,5 @@
 /*
- * Copyright 1999-2005 The Apache Software Foundation.
+ * Copyright 1999-2006 The Apache Software Foundation.
  * 
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -338,11 +338,6 @@ public abstract class BlockStackingLayoutManager extends AbstractLayoutManager
                 contentList.addAll(returnedList);
                 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);
-                    }
 
                     /* extension: conversione di tutta la sequenza fin'ora ottenuta */
                     if (bpUnit > 0) {
@@ -356,11 +351,6 @@ public abstract class BlockStackingLayoutManager extends AbstractLayoutManager
 
                     return returnList;
                 }
-                /*
-                if (allocatedSpace.min > context.getStackLimit().max) {
-                    log.debug("Allocated space exceeds stack limit, returning early.");
-                    return returnList;
-                }*/
             }
             // propagate and clear
             context.setFlags(LayoutContext.KEEP_WITH_NEXT_PENDING, childLC.isKeepWithNextPending());
@@ -863,6 +853,8 @@ public abstract class BlockStackingLayoutManager extends AbstractLayoutManager
     /**
      * Creates Knuth elements for before border padding and adds them to the return list.
      * @param returnList return list to add the additional elements to
+     * @param isFirst true if this is the first time a layout manager instance needs to generate 
+     *                border and padding
      */
     protected void addKnuthElementsForBorderPaddingBefore(LinkedList returnList, boolean isFirst) {
         //Border and Padding (before)
@@ -888,6 +880,8 @@ public abstract class BlockStackingLayoutManager extends AbstractLayoutManager
     /**
      * Creates Knuth elements for after border padding and adds them to the return list.
      * @param returnList return list to add the additional elements to
+     * @param isLast true if this is the last time a layout manager instance needs to generate 
+     *               border and padding
      */
     protected void addKnuthElementsForBorderPaddingAfter(LinkedList returnList, boolean isLast) {
         //Border and Padding (after)
@@ -913,6 +907,7 @@ public abstract class BlockStackingLayoutManager extends AbstractLayoutManager
     /**
      * Creates Knuth elements for break-before and adds them to the return list.
      * @param returnList return list to add the additional elements to
+     * @param context the layout context
      * @return true if an element has been added due to a break-before.
      */
     protected boolean addKnuthElementsForBreakBefore(LinkedList returnList, 
@@ -922,6 +917,10 @@ public abstract class BlockStackingLayoutManager extends AbstractLayoutManager
             breakBefore = ((org.apache.fop.fo.flow.Block) fobj).getBreakBefore();
         } else if (fobj instanceof org.apache.fop.fo.flow.BlockContainer) {
             breakBefore = ((org.apache.fop.fo.flow.BlockContainer) fobj).getBreakBefore();
+        } else if (fobj instanceof org.apache.fop.fo.flow.ListBlock) {
+            breakBefore = ((org.apache.fop.fo.flow.ListBlock) fobj).getBreakBefore();
+        } else if (fobj instanceof org.apache.fop.fo.flow.ListItem) {
+            breakBefore = ((org.apache.fop.fo.flow.ListItem) fobj).getBreakBefore();
         } else if (fobj instanceof org.apache.fop.fo.flow.Table) {
             breakBefore = ((org.apache.fop.fo.flow.Table) fobj).getBreakBefore();
         }
@@ -932,8 +931,6 @@ public abstract class BlockStackingLayoutManager extends AbstractLayoutManager
             // return a penalty element, representing a forced page break
             returnList.add(new BreakElement(getAuxiliaryPosition(), 
                     0, -KnuthElement.INFINITE, breakBefore, context));
-            //returnList.add(new KnuthPenalty(0, -KnuthElement.INFINITE, false,
-            //        breakBefore, getAuxiliaryPosition(), false));
             return true;
         } else {
             return false;
@@ -943,6 +940,7 @@ public abstract class BlockStackingLayoutManager extends AbstractLayoutManager
     /**
      * Creates Knuth elements for break-after and adds them to the return list.
      * @param returnList return list to add the additional elements to
+     * @param context the layout context
      * @return true if an element has been added due to a break-after.
      */
     protected boolean addKnuthElementsForBreakAfter(LinkedList returnList, 
@@ -952,6 +950,10 @@ public abstract class BlockStackingLayoutManager extends AbstractLayoutManager
             breakAfter = ((org.apache.fop.fo.flow.Block) fobj).getBreakAfter();
         } else if (fobj instanceof org.apache.fop.fo.flow.BlockContainer) {
             breakAfter = ((org.apache.fop.fo.flow.BlockContainer) fobj).getBreakAfter();
+        } else if (fobj instanceof org.apache.fop.fo.flow.ListBlock) {
+            breakAfter = ((org.apache.fop.fo.flow.ListBlock) fobj).getBreakAfter();
+        } else if (fobj instanceof org.apache.fop.fo.flow.ListItem) {
+            breakAfter = ((org.apache.fop.fo.flow.ListItem) fobj).getBreakAfter();
         } else if (fobj instanceof org.apache.fop.fo.flow.Table) {
             breakAfter = ((org.apache.fop.fo.flow.Table) fobj).getBreakAfter();
         }
@@ -962,8 +964,6 @@ public abstract class BlockStackingLayoutManager extends AbstractLayoutManager
             // add a penalty element, representing a forced page break
             returnList.add(new BreakElement(getAuxiliaryPosition(), 
                     0, -KnuthElement.INFINITE, breakAfter, context));
-            //returnList.add(new KnuthPenalty(0, -KnuthElement.INFINITE, false,
-            //        breakAfter, getAuxiliaryPosition(), false));
             return true;
         } else {
             return false;

src/java/org/apache/fop/layoutmgr/list/ListItemLayoutManager.java

@@ -179,8 +179,21 @@ public class ListItemLayoutManager extends BlockStackingLayoutManager
         
         LinkedList returnList = new LinkedList();
         
+        if (!breakBeforeServed) {
+            try {
+                if (addKnuthElementsForBreakBefore(returnList, context)) {
+                    return returnList;
+                }
+            } finally {
+                breakBeforeServed = true;
+            }
+        }
+
         addKnuthElementsForSpaceBefore(returnList, alignment);
         
+        addKnuthElementsForBorderPaddingBefore(returnList, !firstVisibleMarkServed);
+        firstVisibleMarkServed = true;
+
         //Spaces, border and padding to be repeated at each break
         addPendingMarks(context);
 
@@ -222,7 +235,9 @@ public class ListItemLayoutManager extends BlockStackingLayoutManager
         // "wrap" the Position inside each element
         wrapPositionElements(returnedList, returnList, true);
         
+        addKnuthElementsForBorderPaddingAfter(returnList, true);
         addKnuthElementsForSpaceAfter(returnList, alignment);
+        addKnuthElementsForBreakAfter(returnList, context);
 
         if (keepWithNextPendingOnLabel || keepWithNextPendingOnBody || mustKeepWithNext()) {
             context.setFlags(LayoutContext.KEEP_WITH_NEXT_PENDING);

status.xml

@@ -27,6 +27,13 @@
 
   <changes>
     <release version="FOP Trunk">
+      <action context="Code" dev="JM" type="fix">
+        Bugfix: padding-top and padding-bottom on list-items could lead to wrong page
+        break decisions and thus to overlapping text.
+      </action>
+      <action context="Code" dev="JM" type="fix">
+        Bugfix: break-before and break-after were ignored on list-items.
+      </action>
       <action context="Code" dev="LF" type="fix" fixes-bug="38507">
         Bugfix: The elements representing a non-breaking space weren't always correct.
       </action>

test/layoutengine/standard-testcases/list-item_break-before_break-after.xml

@@ -0,0 +1,83 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  Copyright 2006 The Apache Software Foundation
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+-->
+<!-- $Id$ -->
+<testcase>
+  <info>
+    <p>
+      This test checks element list generation with breaks on list-items.
+    </p>
+  </info>
+  <fo>
+    <fo:root xmlns:fo="http://www.w3.org/1999/XSL/Format">
+      <fo:layout-master-set>
+        <fo:simple-page-master master-name="normal" page-width="5in" page-height="5in">
+          <fo:region-body/>
+        </fo:simple-page-master>
+      </fo:layout-master-set>
+      <fo:page-sequence master-reference="normal">
+        <fo:flow flow-name="xsl-region-body">
+          <fo:list-block provisional-distance-between-starts="1in" provisional-label-separation="0.1in">
+            <fo:list-item background-color="yellow" break-after="page">
+              <fo:list-item-label end-indent="label-end()">
+                <fo:block>label 1</fo:block>
+              </fo:list-item-label>
+              <fo:list-item-body start-indent="body-start()">
+                <fo:block>
+                  body 1: We'll break after this yellow list item.
+                </fo:block>
+              </fo:list-item-body>
+            </fo:list-item>
+            <fo:list-item>
+              <fo:list-item-label end-indent="label-end()">
+                <fo:block>label 2</fo:block>
+              </fo:list-item-label>
+              <fo:list-item-body start-indent="body-start()">
+                <fo:block>
+                  body 2: This is just a boring list item.
+                </fo:block>
+              </fo:list-item-body>
+            </fo:list-item>
+            <fo:list-item background-color="orange" break-before="page">
+              <fo:list-item-label end-indent="label-end()">
+                <fo:block>label 3</fo:block>
+              </fo:list-item-label>
+              <fo:list-item-body start-indent="body-start()">
+                <fo:block>
+                  body 3: We had a break before this orange list item.
+                </fo:block>
+              </fo:list-item-body>
+            </fo:list-item>
+          </fo:list-block>
+        </fo:flow>
+      </fo:page-sequence>
+    </fo:root>
+  </fo>
+  <checks>
+    <element-list category="breaker" index="0">
+      <box w="14400"/>
+      <skip>3</skip>
+    </element-list>
+    <element-list category="breaker" index="1">
+      <box w="14400"/>
+      <skip>3</skip>
+    </element-list>
+    <element-list category="breaker" index="2">
+      <box w="14400"/>
+      <skip>3</skip>
+    </element-list>
+  </checks>
+</testcase>

test/layoutengine/standard-testcases/list-item_padding.xml

@@ -0,0 +1,82 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<!--
+  Copyright 2006 The Apache Software Foundation
+
+  Licensed under the Apache License, Version 2.0 (the "License");
+  you may not use this file except in compliance with the License.
+  You may obtain a copy of the License at
+
+       http://www.apache.org/licenses/LICENSE-2.0
+
+  Unless required by applicable law or agreed to in writing, software
+  distributed under the License is distributed on an "AS IS" BASIS,
+  WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+  See the License for the specific language governing permissions and
+  limitations under the License.
+-->
+<!-- $Id$ -->
+<testcase>
+  <info>
+    <p>
+      This test checks element list generation with padding in lists (list-item).
+    </p>
+  </info>
+  <fo>
+    <fo:root xmlns:fo="http://www.w3.org/1999/XSL/Format">
+      <fo:layout-master-set>
+        <fo:simple-page-master master-name="normal" page-width="5in" page-height="5in">
+          <fo:region-body/>
+        </fo:simple-page-master>
+      </fo:layout-master-set>
+      <fo:page-sequence master-reference="normal">
+        <fo:flow flow-name="xsl-region-body">
+          <fo:list-block provisional-distance-between-starts="1in" provisional-label-separation="0.1in">
+            <fo:list-item padding-top="5pt" padding-bottom="8pt" background-color="yellow">
+              <fo:list-item-label end-indent="label-end()">
+                <fo:block>label 1</fo:block>
+              </fo:list-item-label>
+              <fo:list-item-body start-indent="body-start()">
+                <fo:block>
+                  body 1: We should have 5pt padding-before and 8pt padding-after for this yellow list item.
+                </fo:block>
+              </fo:list-item-body>
+            </fo:list-item>
+            <fo:list-item padding-top="5pt" padding-bottom="8pt">
+              <fo:list-item-label end-indent="label-end()">
+                <fo:block>label 2</fo:block>
+              </fo:list-item-label>
+              <fo:list-item-body start-indent="body-start()">
+                <fo:block>
+                  body 2: We should have 5pt padding-before and 8pt padding-after for this list item.
+                </fo:block>
+              </fo:list-item-body>
+            </fo:list-item>
+          </fo:list-block>
+        </fo:flow>
+      </fo:page-sequence>
+    </fo:root>
+  </fo>
+  <checks>
+    <element-list category="breaker">
+      <box w="0"/> <!-- Starting SpaceHandlingPosition -->
+      <penalty w="0" p="INF"/>
+      <glue w="5000" y="0" z="0"/>
+      <box w="28800"/>
+      <penalty w="0" p="INF"/>
+      <glue w="8000" y="0" z="0"/>
+      
+      <penalty w="0" p="0"/>
+      
+      <box w="0"/>
+      <penalty w="0" p="INF"/>
+      <glue w="5000" y="0" z="0"/>
+      <box w="28800"/>
+      <box w="0"/> <!-- SpaceHandlingPosition -->
+      <penalty w="0" p="INF"/>
+      <glue w="8000" y="0" z="0"/>
+      <box w="0"/>
+      
+      <skip>3</skip>
+    </element-list>
+  </checks>
+</testcase>