Bug 1507127 - Move the page-break-{before,after} properties to not us…

…e mako. r=heycam

And respect the computed value of `left` / `right` / etc.

Differential Revision: https://phabricator.services.mozilla.com/D11872

--HG--
extra : moz-landing-system : lando

gfx/src/X11UndefineNone.h

@@ -21,3 +21,8 @@
 #  endif
 #endif
 
+// X11 also defines Always, which conflicts with some style system enum variant
+// names, so get rid of that too, given we don't use it anywhere else.
+#ifdef Always
+#undef Always
+#endif

layout/base/nsCSSFrameConstructor.cpp

@@ -5788,7 +5788,7 @@ nsCSSFrameConstructor::AddFrameConstructionItemsInternal(nsFrameConstructorState
     !(aParentFrame && aParentFrame->IsGridContainerFrame()) &&
     !(bits & FCDATA_IS_TABLE_PART) && !(bits & FCDATA_IS_SVG_TEXT);
 
-  if (canHavePageBreak && display.mBreakBefore) {
+  if (canHavePageBreak && display.BreakBefore()) {
     AddPageBreakItem(aContent, aItems);
   }
 
@@ -5826,7 +5826,7 @@ nsCSSFrameConstructor::AddFrameConstructionItemsInternal(nsFrameConstructorState
   }
   item->mIsPopup = isPopup;
 
-  if (canHavePageBreak && display.mBreakAfter) {
+  if (canHavePageBreak && display.BreakAfter()) {
     AddPageBreakItem(aContent, aItems);
   }
 

layout/generic/nsFlexContainerFrame.cpp

@@ -4038,7 +4038,7 @@ nsFlexContainerFrame::GenerateFlexLines(
 
     // Honor "page-break-before", if we're multi-line and this line isn't empty:
     if (!isSingleLine && !curLine->IsEmpty() &&
-        childFrame->StyleDisplay()->mBreakBefore) {
+        childFrame->StyleDisplay()->BreakBefore()) {
       curLine = AddNewFlexLineToList(aLines, shouldInsertAtFront, aMainGapSize);
     }
 
@@ -4096,7 +4096,7 @@ nsFlexContainerFrame::GenerateFlexLines(
 
     // Honor "page-break-after", if we're multi-line and have more children:
     if (!isSingleLine && childFrame->GetNextSibling() &&
-        childFrame->StyleDisplay()->mBreakAfter) {
+        childFrame->StyleDisplay()->BreakAfter()) {
       curLine = AddNewFlexLineToList(aLines, shouldInsertAtFront, aMainGapSize);
     }
     itemIdxInContainer++;

layout/generic/nsGridContainerFrame.cpp

@@ -5284,7 +5284,7 @@ nsGridContainerFrame::ReflowInFragmentainer(GridReflowInput&     aState,
       break;
     }
     auto disp = info->mFrame->StyleDisplay();
-    if (disp->mBreakBefore) {
+    if (disp->BreakBefore()) {
       // Propagate break-before on the first row to the container unless we're
       // already at top-of-page.
       if ((itemStartRow == 0 && !isTopOfPage) || avoidBreakInside) {
@@ -5302,7 +5302,7 @@ nsGridContainerFrame::ReflowInFragmentainer(GridReflowInput&     aState,
       }
     }
     uint32_t itemEndRow = info->mArea.mRows.mEnd;
-    if (disp->mBreakAfter) {
+    if (disp->BreakAfter()) {
       if (itemEndRow != numRows) {
         if (itemEndRow > startRow && itemEndRow < endRow) {
           endRow = itemEndRow;

layout/style/ServoBindings.toml

@@ -397,6 +397,7 @@ cbindgen-types = [
     { gecko = "StyleUnicodeRange", servo = "cssparser::UnicodeRange" },
     { gecko = "StyleOverflowWrap", servo = "values::computed::OverflowWrap" },
     { gecko = "StyleUserSelect", servo = "values::computed::UserSelect" },
+    { gecko = "StyleBreakBetween", servo = "values::computed::BreakBetween" },
 ]
 
 mapped-generic-types = [

layout/style/ServoCSSPropList.mako.py

@@ -70,6 +70,7 @@ def method(prop):
     "BackgroundRepeat",
     "BackgroundSize",
     "BorderImageRepeat",
+    "BreakBetween",
     "Clear",
     "ClipRectOrAuto",
     "Color",

layout/style/nsStyleStruct.cpp

@@ -3385,8 +3385,8 @@ nsStyleDisplay::nsStyleDisplay(const nsPresContext* aContext)
   , mOriginalFloat(StyleFloat::None)
   , mBreakType(StyleClear::None)
   , mBreakInside(NS_STYLE_PAGE_BREAK_AUTO)
-  , mBreakBefore(false)
-  , mBreakAfter(false)
+  , mPageBreakBefore(StyleBreakBetween::Auto)
+  , mPageBreakAfter(StyleBreakBetween::Auto)
   , mOverflowX(NS_STYLE_OVERFLOW_VISIBLE)
   , mOverflowY(NS_STYLE_OVERFLOW_VISIBLE)
   , mOverflowClipBoxBlock(NS_STYLE_OVERFLOW_CLIP_BOX_PADDING_BOX)
@@ -3450,8 +3450,8 @@ nsStyleDisplay::nsStyleDisplay(const nsStyleDisplay& aSource)
   , mOriginalFloat(aSource.mOriginalFloat)
   , mBreakType(aSource.mBreakType)
   , mBreakInside(aSource.mBreakInside)
-  , mBreakBefore(aSource.mBreakBefore)
-  , mBreakAfter(aSource.mBreakAfter)
+  , mPageBreakBefore(aSource.mPageBreakBefore)
+  , mPageBreakAfter(aSource.mPageBreakAfter)
   , mOverflowX(aSource.mOverflowX)
   , mOverflowY(aSource.mOverflowY)
   , mOverflowClipBoxBlock(aSource.mOverflowClipBoxBlock)
@@ -3685,10 +3685,13 @@ nsStyleDisplay::CalcDifference(const nsStyleDisplay& aNewData) const
 
   // XXX the following is conservative, for now: changing float breaking shouldn't
   // necessarily require a repaint, reflow should suffice.
+  //
+  // FIXME(emilio): We definitely change the frame tree in nsCSSFrameConstructor
+  // based on break-before / break-after... Shouldn't that reframe?
   if (mBreakType != aNewData.mBreakType
       || mBreakInside != aNewData.mBreakInside
-      || mBreakBefore != aNewData.mBreakBefore
-      || mBreakAfter != aNewData.mBreakAfter
+      || mPageBreakBefore != aNewData.mPageBreakBefore
+      || mPageBreakAfter != aNewData.mPageBreakAfter
       || mAppearance != aNewData.mAppearance
       || mOrient != aNewData.mOrient
       || mOverflowClipBoxBlock != aNewData.mOverflowClipBoxBlock

layout/style/nsStyleStruct.h

@@ -2015,8 +2015,8 @@ struct MOZ_NEEDS_MEMMOVABLE_MEMBERS nsStyleDisplay
 
   mozilla::StyleClear mBreakType;
   uint8_t mBreakInside;         // NS_STYLE_PAGE_BREAK_AUTO/AVOID
-  bool mBreakBefore;
-  bool mBreakAfter;
+  mozilla::StyleBreakBetween mPageBreakBefore;
+  mozilla::StyleBreakBetween mPageBreakAfter;
   uint8_t mOverflowX;           // NS_STYLE_OVERFLOW_*
   uint8_t mOverflowY;           // NS_STYLE_OVERFLOW_*
   uint8_t mOverflowClipBoxBlock;     // NS_STYLE_OVERFLOW_CLIP_BOX_*
@@ -2324,6 +2324,36 @@ struct MOZ_NEEDS_MEMMOVABLE_MEMBERS nsStyleDisplay
     return mBackfaceVisibility == NS_STYLE_BACKFACE_VISIBILITY_HIDDEN;
   }
 
+  // FIXME(emilio): This looks slightly fishy, this had a comment from karnaze
+  // with "Temp fix for bug 24000" on it... Oh well.
+  static bool ShouldBreak(mozilla::StyleBreakBetween aBreak)
+  {
+    switch (aBreak) {
+      // "A conforming user agent may interpret the values 'left' and 'right' as
+      // 'always'." - CSS2.1, section 13.3.1
+      case mozilla::StyleBreakBetween::Left:
+      case mozilla::StyleBreakBetween::Right:
+      case mozilla::StyleBreakBetween::Always:
+        return true;
+      case mozilla::StyleBreakBetween::Auto:
+      case mozilla::StyleBreakBetween::Avoid:
+        return false;
+      default:
+        MOZ_ASSERT_UNREACHABLE("Unknown break kind");
+        return false;
+    }
+  }
+
+  bool BreakBefore() const
+  {
+    return ShouldBreak(mPageBreakBefore);
+  }
+
+  bool BreakAfter() const
+  {
+    return ShouldBreak(mPageBreakAfter);
+  }
+
   // These are defined in nsStyleStructInlines.h.
 
   // The aContextFrame argument on each of these is the frame this

layout/style/test/test_value_computation.html

@@ -45,10 +45,6 @@
 /** Test for computation of values in property database **/
 
 var gBadComputed = {
-  // These values are treated as auto.
-  "page-break-after": [ "avoid" ],
-  "page-break-before": [ "avoid" ],
-
   // This is the only SVG-length property (i.e., length allowing
   // unitless lengths) whose initial value is zero.
   "stroke-dashoffset": [ "0", "-moz-objectValue" ],

layout/tables/nsTableFrame.cpp

@@ -224,7 +224,7 @@ nsTableFrame::PageBreakAfter(nsIFrame* aSourceFrame,
   const nsStyleDisplay* display = aSourceFrame->StyleDisplay();
   nsTableRowGroupFrame* prevRg = do_QueryFrame(aSourceFrame);
   // don't allow a page break after a repeated element ...
-  if ((display->mBreakAfter || (prevRg && prevRg->HasInternalBreakAfter())) &&
+  if ((display->BreakAfter() || (prevRg && prevRg->HasInternalBreakAfter())) &&
       !IsRepeatedFrame(aSourceFrame)) {
     return !(aNextFrame && IsRepeatedFrame(aNextFrame)); // or before
   }
@@ -233,7 +233,7 @@ nsTableFrame::PageBreakAfter(nsIFrame* aSourceFrame,
     display = aNextFrame->StyleDisplay();
     // don't allow a page break before a repeated element ...
      nsTableRowGroupFrame* nextRg = do_QueryFrame(aNextFrame);
-    if ((display->mBreakBefore ||
+    if ((display->BreakBefore() ||
         (nextRg && nextRg->HasInternalBreakBefore())) &&
         !IsRepeatedFrame(aNextFrame)) {
       return !IsRepeatedFrame(aSourceFrame); // or after

layout/tables/nsTableRowGroupFrame.cpp

@@ -1630,7 +1630,7 @@ nsTableRowGroupFrame::HasInternalBreakBefore() const
  nsIFrame* firstChild = mFrames.FirstChild();
   if (!firstChild)
     return false;
-  return firstChild->StyleDisplay()->mBreakBefore;
+  return firstChild->StyleDisplay()->BreakBefore();
 }
 
 /** find page break after the last row **/
@@ -1640,7 +1640,7 @@ nsTableRowGroupFrame::HasInternalBreakAfter() const
   nsIFrame* lastChild = mFrames.LastChild();
   if (!lastChild)
     return false;
-  return lastChild->StyleDisplay()->mBreakAfter;
+  return lastChild->StyleDisplay()->BreakAfter();
 }
 /* ----- global methods ----- */
 

servo/components/style/cbindgen.toml

@@ -39,6 +39,7 @@ derive_helper_methods = true
 prefix = "Style"
 include = [
   "Appearance",
+  "BreakBetween",
   "ComputedFontStretchRange",
   "ComputedFontStyleDescriptor",
   "ComputedFontWeightRange",

servo/components/style/properties/data.py

@@ -291,6 +291,7 @@ def specified_is_copy(self):
                 "AlignItems",
                 "AlignSelf",
                 "Appearance",
+                "BreakBetween",
                 "BackgroundRepeat",
                 "BorderImageRepeat",
                 "BorderStyle",

servo/components/style/properties/gecko.mako.rs

@@ -1396,6 +1396,7 @@ impl Clone for ${style_struct.gecko_struct_name} {
 
     # Types used with predefined_type()-defined properties that we can auto-generate.
     predefined_types = {
+        "BreakBetween": impl_simple,
         "Color": impl_color,
         "ColorOrAuto": impl_color,
         "GreaterThanOrEqualToOneNumber": impl_simple,
@@ -3029,8 +3030,7 @@ fn static_assert() {
                           animation-iteration-count animation-timing-function
                           transition-duration transition-delay
                           transition-timing-function transition-property
-                          page-break-before page-break-after rotate
-                          scroll-snap-points-x scroll-snap-points-y
+                          rotate scroll-snap-points-x scroll-snap-points-y
                           scroll-snap-type-x scroll-snap-type-y scroll-snap-coordinate
                           perspective-origin -moz-binding will-change
                           offset-path overscroll-behavior-x overscroll-behavior-y
@@ -3149,35 +3149,6 @@ fn static_assert() {
 
     <%call expr="impl_coord_copy('vertical_align', 'mVerticalAlign')"></%call>
 
-    % for kind in ["before", "after"]:
-    // Temp fix for Bugzilla bug 24000.
-    // Map 'auto' and 'avoid' to false, and 'always', 'left', and 'right' to true.
-    // "A conforming user agent may interpret the values 'left' and 'right'
-    // as 'always'." - CSS2.1, section 13.3.1
-    pub fn set_page_break_${kind}(&mut self, v: longhands::page_break_${kind}::computed_value::T) {
-        use crate::computed_values::page_break_${kind}::T;
-
-        let result = match v {
-            T::Auto   => false,
-            T::Always => true,
-            T::Avoid  => false,
-            T::Left   => true,
-            T::Right  => true
-        };
-        self.gecko.mBreak${kind.title()} = result;
-    }
-
-    ${impl_simple_copy('page_break_' + kind, 'mBreak' + kind.title())}
-
-    // Temp fix for Bugzilla bug 24000.
-    // See set_page_break_before/after for detail.
-    pub fn clone_page_break_${kind}(&self) -> longhands::page_break_${kind}::computed_value::T {
-        use crate::computed_values::page_break_${kind}::T;
-
-        if self.gecko.mBreak${kind.title()} { T::Always } else { T::Auto }
-    }
-    % endfor
-
     ${impl_style_coord("scroll_snap_points_x", "mScrollSnapPointsX")}
     ${impl_style_coord("scroll_snap_points_y", "mScrollSnapPointsY")}
 

servo/components/style/properties/longhands/box.mako.rs

@@ -437,18 +437,21 @@ ${helpers.single_keyword(
     animation_value_type="discrete",
 )}
 
-// TODO add support for logical values recto and verso
-${helpers.single_keyword(
+${helpers.predefined_type(
     "page-break-after",
-    "auto always avoid left right",
+    "BreakBetween",
+    "computed::BreakBetween::Auto",
+    needs_context=False,
     products="gecko",
     spec="https://drafts.csswg.org/css2/page.html#propdef-page-break-after",
     animation_value_type="discrete",
 )}
 
-${helpers.single_keyword(
+${helpers.predefined_type(
     "page-break-before",
-    "auto always avoid left right",
+    "BreakBetween",
+    "computed::BreakBetween::Auto",
+    needs_context=False,
     products="gecko",
     spec="https://drafts.csswg.org/css2/page.html#propdef-page-break-before",
     animation_value_type="discrete",

servo/components/style/values/computed/box.rs

@@ -11,13 +11,11 @@ use crate::values::generics::box_::Perspective as GenericPerspective;
 use crate::values::generics::box_::VerticalAlign as GenericVerticalAlign;
 use crate::values::specified::box_ as specified;
 
-pub use crate::values::specified::box_::{
-    AnimationName, Appearance, Contain, Display, OverflowClipBox,
-};
+pub use crate::values::specified::box_::{AnimationName, Appearance, BreakBetween};
+pub use crate::values::specified::box_::{Contain, Display, OverflowClipBox};
 pub use crate::values::specified::box_::{Clear as SpecifiedClear, Float as SpecifiedFloat};
-pub use crate::values::specified::box_::{
-    OverscrollBehavior, ScrollSnapType, TouchAction, TransitionProperty, WillChange,
-};
+pub use crate::values::specified::box_::{OverscrollBehavior, ScrollSnapType};
+pub use crate::values::specified::box_::{TouchAction, TransitionProperty, WillChange};
 
 /// A computed value for the `vertical-align` property.
 pub type VerticalAlign = GenericVerticalAlign<LengthOrPercentage>;

servo/components/style/values/computed/mod.rs

@@ -42,7 +42,7 @@ pub use self::border::{BorderCornerRadius, BorderRadius, BorderSpacing};
 pub use self::border::{BorderImageRepeat, BorderImageSideWidth};
 pub use self::border::{BorderImageSlice, BorderImageWidth};
 pub use self::box_::{AnimationIterationCount, AnimationName, Contain};
-pub use self::box_::{Appearance, Clear, Float};
+pub use self::box_::{Appearance, BreakBetween, Clear, Float};
 pub use self::box_::{Display, TransitionProperty};
 pub use self::box_::{OverflowClipBox, OverscrollBehavior, Perspective, Resize};
 pub use self::box_::{ScrollSnapType, TouchAction, VerticalAlign, WillChange};

servo/components/style/values/specified/box.rs

@@ -1289,3 +1289,29 @@ pub enum Appearance {
     #[css(skip)]
     Count,
 }
+
+/// A kind of break between two boxes.
+///
+/// https://drafts.csswg.org/css-break/#break-between
+#[allow(missing_docs)]
+#[derive(
+    Clone,
+    Copy,
+    Debug,
+    Eq,
+    Hash,
+    MallocSizeOf,
+    Parse,
+    PartialEq,
+    SpecifiedValueInfo,
+    ToCss,
+    ToComputedValue,
+)]
+#[repr(u8)]
+pub enum BreakBetween {
+    Auto,
+    Always,
+    Avoid,
+    Left,
+    Right,
+}

servo/components/style/values/specified/mod.rs

@@ -36,7 +36,7 @@ pub use self::border::{BorderCornerRadius, BorderImageSlice, BorderImageWidth};
 pub use self::border::{BorderImageRepeat, BorderImageSideWidth};
 pub use self::border::{BorderRadius, BorderSideWidth, BorderSpacing};
 pub use self::box_::{AnimationIterationCount, AnimationName, Contain, Display};
-pub use self::box_::{Appearance, Clear, Float};
+pub use self::box_::{Appearance, BreakBetween, Clear, Float};
 pub use self::box_::{OverflowClipBox, OverscrollBehavior, Perspective, Resize};
 pub use self::box_::{ScrollSnapType, TouchAction, TransitionProperty, VerticalAlign, WillChange};
 pub use self::color::{Color, ColorPropertyValue, RGBAColor};