793692 - Implement page-break-inside:avoid for table cells

Status: NEW

(Core :: Layout: Tables, enhancement)

Description

[Mats Palmgren (inactive)]

It appears this isn't required by CSS2.1:
http://www.w3.org/TR/CSS21/page.html#propdef-page-break-inside

but is required in CSS3:
http://www.w3.org/TR/css3-break/#break-inside

So I spawned this off from bug 685012 (to keep that limited
to the minimum required by CSS2.1).

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix + reftests

If a cell is incomplete and has page-break-inside:avoid then set the
status to NS_INLINE_LINE_BREAK_BEFORE.  Make the status for the row
NS_INLINE_LINE_BREAK_BEFORE if at least one cell reported it.

(applies on top of bug 685012)

https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=40d1b6a13498

Comment 2

[fantasai]

Comment on attachment 664303 [details] [diff] [review]
fix + reftests

+  if (aPresContext->IsPaginated() &&
+      !NS_FRAME_IS_FULLY_COMPLETE(aStatus) &&
+      IsBreakInsideAvoid(aReflowState)) {
+    aStatus = NS_INLINE_LINE_BREAK_BEFORE();
+    NS_FRAME_SET_INCOMPLETE(aStatus);
+  }
+
Wrt the line that sets NS_FRAME_SET_INCOMPLETE, it's handled in the "if" clause immediately above. (If it seems easier to understand, you can switch that clause from NS_FRAME_OVERFLOW_IS_INCOMPLETE to NS_FRAME_IS_FULLY_COMPLETE, but the resulting behavior will be the same, so this here is redundant.)

+    if (isPaginated && !anyCellIsBreakInsideAvoid &&
+        cellFrame->IsBreakInsideAvoid(aReflowState)) {
+      anyCellIsBreakInsideAvoid = true;
+    }

I could be wrong--I don't remember enough about compiling if-clauses into assembly--but I think it's either better or equivalent to omit !anyCellIsBreakInsideAvoid here from the check. And that would simplify the if-clause.

+          if (isPaginated) {
+            for (nsFrameList::Enumerator e(mFrames);
+                 !anyCellIsBreakInsideAvoid && !e.AtEnd(); e.Next()) {
+              anyCellIsBreakInsideAvoid =
+                static_cast<nsFrame*>(e.get())->IsBreakInsideAvoid(aReflowState);
+            }

Why are we looping through the cells here? We're already in a loop over all the children, and you have a check for IsBreakInsideAvoid on each child in the changes right before this in the patch.

+            if (anyCellIsBreakInsideAvoid) {
+              aStatus |= NS_INLINE_LINE_BREAK_BEFORE();
+            }
+          }

And this part should be outside the ReflowChildren loop.

-  if (aPresContext->IsPaginated() && !NS_FRAME_IS_FULLY_COMPLETE(aStatus) &&
-      IsBreakInsideAvoid(aReflowState)) {
+  if (aPresContext->IsPaginated() && 
+      (NS_INLINE_IS_BREAK_BEFORE(aStatus) ||
+       (!NS_FRAME_IS_FULLY_COMPLETE(aStatus) &&
+        IsBreakInsideAvoid(aReflowState)))) {
     aStatus = NS_INLINE_LINE_BREAK_BEFORE();
   }

If NS_INLINE_IS_BREAK_BEFORE(aStatus) is already true, then why are you setting it to true again?

Comment 3

[Mats Palmgren (inactive)]

Attachment: fix + reftests

I've addressed your points as suggested.  (sorry for the delay)
https://tbpl.mozilla.org/?tree=Try&rev=7e08776ee20a

Comment 4

[fantasai]

Comment on attachment 699663 [details] [diff] [review]
fix + reftests

+ bool anyCellIsBreakInsideAvoid = false;

I'd call this avoidBreakInsideRow, it's a little clearer what it implies. You could even initialize it to whether the row has a breakInside avoid value, and remove the check in nsTableRow::Reflow.

Seems like you have two paths for a cell triggering a row break before:
  - either the cell sets its own status to NS_INLINE_LINE_BREAK_BEFORE(),
    which gets folded in after the ReflowChild call or
  - the cell sets its status to incomplete and relies on the rowFrame catching
    that in the check after the loop.
Why not just pick one method of passing up the break request? You could handle all the cases through the second method, no?

Comment 5

[fantasai]

For the tests, please:
  - Add a <title> to each test file
  - Add the following metadata to each test file:
  <link rel="author" title="Mats Palmgren" href="[mailto: or http: contact address]">
  <link rel="help" href="http://www.w3.org/TR/CSS21/page.html#page-break-props">
  <meta name="flags" content="paged">
  - Add to the reference files
  <title>CSS Reftest Reference</title>
  <link rel="author" title="Mats Palmgren" href="[mailto: or http: contact address]">

Comment 6

[fantasai]

Comment on attachment 699663 [details] [diff] [review]
fix + reftests

[waive flag due to above comments]

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.