Bug 775618 - Implement page-break-{before,after} as legacy shorthands for {before,after}. r=heycam

This is all the style-system work needed for this. This implements the concept of legacy shorthands, teaches tests to understand it, and adds a few more tests for these properties in particular. The WPT even caught a few WebKit / Blink bugs: https://bugs.chromium.org/p/chromium/issues/detail?id=906336 https://bugs.webkit.org/show_bug.cgi?id=191803 This doesn't change the layout behavior for page-break-before: always, since it'd stop breaking in multicol and such. Similarly, break-before / break-after: column and page still behave the same, I'll file followups for those given comment 22. Differential Revision: https://phabricator.services.mozilla.com/D12211 --HG-- extra : moz-landing-system : lando
2024-11-30 16:22:00 +00:00 · 2018-11-30 05:35:47 +00:00 · 2018-11-30 05:35:47 +00:00 · c279a94301
commit c279a94301
parent cd451a8c96
12 changed files with 296 additions and 39 deletions
--- a/devtools/server/actors/animation-type-longhand.js
+++ b/devtools/server/actors/animation-type-longhand.js
@ -114,9 +114,9 @@ exports.ANIMATION_TYPE_FOR_LONGHANDS = [
    "overflow-y",
    "overscroll-behavior-x",
    "overscroll-behavior-y",
-    "page-break-after",
-    "page-break-before",
-    "page-break-inside",
+    "break-after",
+    "break-before",
+    "break-inside",
    "paint-order",
    "pointer-events",
    "position",
--- a/devtools/shared/css/generated/properties-db.js
+++ b/devtools/shared/css/generated/properties-db.js
@ -2940,9 +2940,9 @@ exports.CSS_PROPERTIES = {
      "overscroll-behavior-x",
      "overscroll-behavior-y",
      "isolation",
-      "page-break-after",
-      "page-break-before",
-      "page-break-inside",
+      "break-after",
+      "break-before",
+      "break-inside",
      "resize",
      "perspective",
      "perspective-origin",
@ -4775,6 +4775,56 @@ exports.CSS_PROPERTIES = {
      "unset"
    ]
  },
+  "break-after": {
+    "isInherited": false,
+    "subproperties": [
+      "break-after"
+    ],
+    "supports": [],
+    "values": [
+      "always",
+      "auto",
+      "avoid",
+      "inherit",
+      "initial",
+      "left",
+      "page",
+      "right",
+      "unset"
+    ]
+  },
+  "break-before": {
+    "isInherited": false,
+    "subproperties": [
+      "break-before"
+    ],
+    "supports": [],
+    "values": [
+      "always",
+      "auto",
+      "avoid",
+      "inherit",
+      "initial",
+      "left",
+      "page",
+      "right",
+      "unset"
+    ]
+  },
+  "break-inside": {
+    "isInherited": false,
+    "subproperties": [
+      "break-inside"
+    ],
+    "supports": [],
+    "values": [
+      "auto",
+      "avoid",
+      "inherit",
+      "initial",
+      "unset"
+    ]
+  },
  "caption-side": {
    "isInherited": true,
    "subproperties": [
@ -7772,7 +7822,7 @@ exports.CSS_PROPERTIES = {
  "page-break-after": {
    "isInherited": false,
    "subproperties": [
-      "page-break-after"
+      "break-after"
    ],
    "supports": [],
    "values": [
@ -7782,6 +7832,7 @@ exports.CSS_PROPERTIES = {
      "inherit",
      "initial",
      "left",
+      "page",
      "right",
      "unset"
    ]
@ -7789,7 +7840,7 @@ exports.CSS_PROPERTIES = {
  "page-break-before": {
    "isInherited": false,
    "subproperties": [
-      "page-break-before"
+      "break-before"
    ],
    "supports": [],
    "values": [
@ -7799,6 +7850,7 @@ exports.CSS_PROPERTIES = {
      "inherit",
      "initial",
      "left",
+      "page",
      "right",
      "unset"
    ]
@ -7806,7 +7858,7 @@ exports.CSS_PROPERTIES = {
  "page-break-inside": {
    "isInherited": false,
    "subproperties": [
-      "page-break-inside"
+      "break-inside"
    ],
    "supports": [],
    "values": [
--- a/layout/style/nsStyleStruct.cpp
+++ b/layout/style/nsStyleStruct.cpp
@ -3385,8 +3385,8 @@ nsStyleDisplay::nsStyleDisplay(const nsPresContext* aContext)
  , mOriginalFloat(StyleFloat::None)
  , mBreakType(StyleClear::None)
  , mBreakInside(StyleBreakWithin::Auto)
-  , mPageBreakBefore(StyleBreakBetween::Auto)
-  , mPageBreakAfter(StyleBreakBetween::Auto)
+  , mBreakBefore(StyleBreakBetween::Auto)
+  , mBreakAfter(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)
-  , mPageBreakBefore(aSource.mPageBreakBefore)
-  , mPageBreakAfter(aSource.mPageBreakAfter)
+  , mBreakBefore(aSource.mBreakBefore)
+  , mBreakAfter(aSource.mBreakAfter)
  , mOverflowX(aSource.mOverflowX)
  , mOverflowY(aSource.mOverflowY)
  , mOverflowClipBoxBlock(aSource.mOverflowClipBoxBlock)
@ -3690,8 +3690,8 @@ nsStyleDisplay::CalcDifference(const nsStyleDisplay& aNewData) const
  // based on break-before / break-after... Shouldn't that reframe?
  if (mBreakType != aNewData.mBreakType
      || mBreakInside != aNewData.mBreakInside
-      || mPageBreakBefore != aNewData.mPageBreakBefore
-      || mPageBreakAfter != aNewData.mPageBreakAfter
+      || mBreakBefore != aNewData.mBreakBefore
+      || mBreakAfter != aNewData.mBreakAfter
      || mAppearance != aNewData.mAppearance
      || mOrient != aNewData.mOrient
      || mOverflowClipBoxBlock != aNewData.mOverflowClipBoxBlock
--- a/layout/style/nsStyleStruct.h
+++ b/layout/style/nsStyleStruct.h
@ -2015,8 +2015,8 @@ struct MOZ_NEEDS_MEMMOVABLE_MEMBERS nsStyleDisplay

  mozilla::StyleClear mBreakType;
  mozilla::StyleBreakWithin mBreakInside;
-  mozilla::StyleBreakBetween mPageBreakBefore;
-  mozilla::StyleBreakBetween mPageBreakAfter;
+  mozilla::StyleBreakBetween mBreakBefore;
+  mozilla::StyleBreakBetween mBreakAfter;
  uint8_t mOverflowX;           // NS_STYLE_OVERFLOW_*
  uint8_t mOverflowY;           // NS_STYLE_OVERFLOW_*
  uint8_t mOverflowClipBoxBlock;     // NS_STYLE_OVERFLOW_CLIP_BOX_*
@ -2324,15 +2324,14 @@ 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.
+  // FIXME(emilio): This should be more fine-grained on each caller to
+  // BreakBefore() / BreakAfter().
  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::Page:
      case mozilla::StyleBreakBetween::Always:
        return true;
      case mozilla::StyleBreakBetween::Auto:
@ -2346,12 +2345,12 @@ struct MOZ_NEEDS_MEMMOVABLE_MEMBERS nsStyleDisplay

  bool BreakBefore() const
  {
-    return ShouldBreak(mPageBreakBefore);
+    return ShouldBreak(mBreakBefore);
  }

  bool BreakAfter() const
  {
-    return ShouldBreak(mPageBreakAfter);
+    return ShouldBreak(mBreakAfter);
  }

  // These are defined in nsStyleStructInlines.h.
--- a/layout/style/test/property_database.js
+++ b/layout/style/test/property_database.js
@ -36,6 +36,11 @@ const CSS_TYPE_TRUE_SHORTHAND = 1;
 // the current spec or earlier versions of the spec.
 const CSS_TYPE_SHORTHAND_AND_LONGHAND = 2;

+// Legacy shorthand properties, that behave mostly like an alias
+// (CSS_TYPE_SHORTHAND_AND_LONGHAND) but not quite because their syntax may not
+// match, plus they shouldn't serialize in cssText.
+const CSS_TYPE_LEGACY_SHORTHAND = 3;
+
 // Each property has the following fields:
 //   domProp: The name of the relevant member of nsIDOM[NS]CSS2Properties
 //   inherited: Whether the property is inherited by default (stated as
@ -4299,18 +4304,44 @@ var gCSSProperties = {
  "page-break-after": {
    domProp: "pageBreakAfter",
    inherited: false,
-    type: CSS_TYPE_LONGHAND,
+    type: CSS_TYPE_LEGACY_SHORTHAND,
+    alias_for: "break-after",
+    subproperties: [ "break-after" ],
    initial_values: [ "auto" ],
    other_values: [ "always", "avoid", "left", "right" ],
-    invalid_values: []
+    legacy_mapping: {
+      always: "page",
+    },
+    invalid_values: [ "page", "column" ]
  },
  "page-break-before": {
    domProp: "pageBreakBefore",
    inherited: false,
-    type: CSS_TYPE_LONGHAND,
+    type: CSS_TYPE_LEGACY_SHORTHAND,
+    alias_for: "break-before",
+    subproperties: [ "break-before" ],
    initial_values: [ "auto" ],
    other_values: [ "always", "avoid", "left", "right" ],
-    invalid_values: []
+    legacy_mapping: {
+      always: "page",
+    },
+    invalid_values: [ "page", "column" ]
+  },
+  "break-after": {
+    domProp: "breakAfter",
+    inherited: false,
+    type: CSS_TYPE_LONGHAND,
+    initial_values: [ "auto" ],
+    other_values: [ "always", "page", "avoid", "left", "right" ],
+    invalid_values: [ ]
+  },
+  "break-before": {
+    domProp: "breakBefore",
+    inherited: false,
+    type: CSS_TYPE_LONGHAND,
+    initial_values: [ "auto" ],
+    other_values: [ "always", "page", "avoid", "left", "right" ],
+    invalid_values: [ ]
  },
  "break-inside": {
    domProp: "breakInside",
--- a/layout/style/test/test_property_database.html
+++ b/layout/style/test/test_property_database.html
@ -60,8 +60,9 @@ for (var idx in gShorthandProperties) {
     "'" + prop.name + "' listed in gCSSProperties");
  if (present) {
    ok(gCSSProperties[prop.name].type == CSS_TYPE_TRUE_SHORTHAND ||
+       gCSSProperties[prop.name].type == CSS_TYPE_LEGACY_SHORTHAND ||
       gCSSProperties[prop.name].type == CSS_TYPE_SHORTHAND_AND_LONGHAND,
-       "'" + prop.name + "' listed as CSS_TYPE_TRUE_SHORTHAND or CSS_TYPE_SHORTHAND_AND_LONGHAND");
+       "'" + prop.name + "' listed as CSS_TYPE_TRUE_SHORTHAND, CSS_TYPE_LEGACY_SHORTHAND, or CSS_TYPE_SHORTHAND_AND_LONGHAND");
    ok(gCSSProperties[prop.name].domProp == prop.prop,
       "'" + prop.name + "' listed with correct DOM property name");
  }
--- a/layout/style/test/test_value_storage.html
+++ b/layout/style/test/test_value_storage.html
@ -182,7 +182,8 @@ function test_property(property)
      for (idx in info.subproperties) {
        var subprop = info.subproperties[idx];
        if (value_has_variable_reference &&
-            (!info.alias_for || info.type == CSS_TYPE_TRUE_SHORTHAND)) {
+            (!info.alias_for || info.type == CSS_TYPE_TRUE_SHORTHAND ||
+            info.type == CSS_TYPE_LEGACY_SHORTHAND)) {
          is(gDeclaration.getPropertyValue(subprop), "",
             "setting '" + value + "' on '" + property + "' (for '" + subprop + "')");
          test_other_shorthands_empty(value, subprop);
@ -197,7 +198,13 @@ function test_property(property)
    var expected_serialization = "";
    if (step1val != "") {
      if ("alias_for" in info) {
-        expected_serialization = info.alias_for + colon + step1val + ";";
+        let value = info.legacy_mapping && info.legacy_mapping[step1val]
+          ? info.legacy_mapping[step1val] : step1val;
+        // FIXME(emilio): This is a bit unfortunate:
+        // https://github.com/w3c/csswg-drafts/issues/3332
+        if (info.type == CSS_TYPE_LEGACY_SHORTHAND && value_has_variable_reference)
+          value = "";
+        expected_serialization = info.alias_for + colon + value + ";";
      } else {
        expected_serialization = property + colon + step1val + ";";
      }
@ -221,8 +228,6 @@ function test_property(property)
        // Using setProperty over subproperties is not sufficient for
        // system fonts, since the shorthand does more than its parts.
        !is_system_font &&
-        // Likewise for special compatibility values of transform
-        (property != "-moz-transform" || !value.match(/^matrix.*(px|em|%)/)) &&
        !value_has_variable_reference) {
      gDeclaration.removeProperty(property);
      for (idx in info.subproperties) {
@ -244,8 +249,9 @@ function test_property(property)
         property + colon + value + "'");
    }

+    // FIXME(emilio): Why is mask special?
    if (info.type != CSS_TYPE_TRUE_SHORTHAND &&
-          property != "mask") {
+        property != "mask") {
      gDeclaration.removeProperty(property);
      gDeclaration.setProperty(property, step1comp, "");
      var func = xfail_compute(property, value) ? todo_is : is;
--- a/servo/components/style/properties/longhands/box.mako.rs
+++ b/servo/components/style/properties/longhands/box.mako.rs
@ -438,22 +438,22 @@ ${helpers.single_keyword(
 )}

 ${helpers.predefined_type(
-    "page-break-after",
+    "break-after",
    "BreakBetween",
    "computed::BreakBetween::Auto",
    needs_context=False,
    products="gecko",
-    spec="https://drafts.csswg.org/css2/page.html#propdef-page-break-after",
+    spec="https://drafts.csswg.org/css-break/#propdef-break-after",
    animation_value_type="discrete",
 )}

 ${helpers.predefined_type(
-    "page-break-before",
+    "break-before",
    "BreakBetween",
    "computed::BreakBetween::Auto",
    needs_context=False,
    products="gecko",
-    spec="https://drafts.csswg.org/css2/page.html#propdef-page-break-before",
+    spec="https://drafts.csswg.org/css-break/#propdef-break-before",
    animation_value_type="discrete",
 )}

--- a/servo/components/style/properties/shorthands/box.mako.rs
+++ b/servo/components/style/properties/shorthands/box.mako.rs
@ -447,3 +447,49 @@ macro_rules! try_parse_one {
        }
    }
 </%helpers:shorthand>
+
+<%helpers:shorthand
+    name="page-break-before"
+    flags="SHORTHAND_IN_GETCS IS_LEGACY_SHORTHAND"
+    sub_properties="break-before"
+    spec="https://drafts.csswg.org/css2/page.html#propdef-page-break-before"
+>
+    pub fn parse_value<'i>(
+        _: &ParserContext,
+        input: &mut Parser<'i, '_>,
+    ) -> Result<Longhands, ParseError<'i>> {
+        use crate::values::specified::box_::BreakBetween;
+        Ok(expanded! {
+            break_before: BreakBetween::parse_legacy(input)?,
+        })
+    }
+
+    impl<'a> ToCss for LonghandsToSerialize<'a> {
+        fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result where W: fmt::Write {
+            self.break_before.to_css_legacy(dest)
+        }
+    }
+</%helpers:shorthand>
+
+<%helpers:shorthand
+    name="page-break-after"
+    flags="SHORTHAND_IN_GETCS IS_LEGACY_SHORTHAND"
+    sub_properties="break-after"
+    spec="https://drafts.csswg.org/css2/page.html#propdef-page-break-after"
+>
+    pub fn parse_value<'i>(
+        _: &ParserContext,
+        input: &mut Parser<'i, '_>,
+    ) -> Result<Longhands, ParseError<'i>> {
+        use crate::values::specified::box_::BreakBetween;
+        Ok(expanded! {
+            break_after: BreakBetween::parse_legacy(input)?,
+        })
+    }
+
+    impl<'a> ToCss for LonghandsToSerialize<'a> {
+        fn to_css<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result where W: fmt::Write {
+            self.break_after.to_css_legacy(dest)
+        }
+    }
+</%helpers:shorthand>
--- a/servo/components/style/values/specified/box.rs
+++ b/servo/components/style/values/specified/box.rs
@ -1286,13 +1286,60 @@ pub enum Appearance {
 )]
 #[repr(u8)]
 pub enum BreakBetween {
-    Auto,
    Always,
+    Auto,
+    Page,
    Avoid,
    Left,
    Right,
 }

+impl BreakBetween {
+    /// Parse a legacy break-between value for `page-break-*`.
+    ///
+    /// See https://drafts.csswg.org/css-break/#page-break-properties.
+    #[inline]
+    pub fn parse_legacy<'i>(
+        input: &mut Parser<'i, '_>,
+    ) -> Result<Self, ParseError<'i>> {
+        let location = input.current_source_location();
+        let ident = input.expect_ident()?;
+        let break_value = match BreakBetween::from_ident(ident) {
+            Ok(v) => v,
+            Err(()) => return Err(location.new_custom_error(
+                SelectorParseErrorKind::UnexpectedIdent(ident.clone())
+            )),
+        };
+        match break_value {
+            BreakBetween::Always => Ok(BreakBetween::Page),
+            BreakBetween::Auto |
+            BreakBetween::Avoid |
+            BreakBetween::Left |
+            BreakBetween::Right => Ok(break_value),
+            BreakBetween::Page => {
+                Err(location.new_custom_error(SelectorParseErrorKind::UnexpectedIdent(ident.clone())))
+            }
+        }
+    }
+
+    /// Serialize a legacy break-between value for `page-break-*`.
+    ///
+    /// See https://drafts.csswg.org/css-break/#page-break-properties.
+    pub fn to_css_legacy<W>(&self, dest: &mut CssWriter<W>) -> fmt::Result
+    where
+        W: Write,
+    {
+        match *self {
+            BreakBetween::Auto |
+            BreakBetween::Avoid |
+            BreakBetween::Left |
+            BreakBetween::Right => self.to_css(dest),
+            BreakBetween::Page => dest.write_str("always"),
+            BreakBetween::Always => Ok(()),
+        }
+    }
+}
+
 /// A kind of break within a box.
 ///
 /// https://drafts.csswg.org/css-break/#break-within
--- a/testing/web-platform/meta/css/css-break/inheritance.html.ini
+++ b/testing/web-platform/meta/css/css-break/inheritance.html.ini
@ -1,4 +1,4 @@
 [inheritance.html]
-  [Inheritance of CSS Fragmentation properties]
+  [Property break-after does not inherit]
    expected: FAIL

--- a/testing/web-platform/tests/css/css-break/page-break-legacy-shorthands.html
+++ b/testing/web-platform/tests/css/css-break/page-break-legacy-shorthands.html
@ -0,0 +1,75 @@
+<!doctype html>
+<meta charset="utf-8">
+<title>Test for the page-break-* legacy shorthands.</title>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="author" title="Mozilla" href="https://mozilla.org">
+<link rel="help" href="https://drafts.csswg.org/css-cascade-4/#legacy-shorthand">
+<link rel="help" href="https://drafts.csswg.org/css-break/#page-break-properties">
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<div></div>
+<script>
+const NEW_VALUES = ["page", "column"].filter(v => CSS.supports("break-before", v));
+const LEGACY_VALUES = ["always", "auto", "left", "right", "avoid"];
+const LEGACY_MAPPING = { "always": "page" };
+const REVERSE_LEGACY_MAPPING = { "page": "always" };
+const div = document.querySelector("div");
+const cs = getComputedStyle(div);
+
+test(function() {
+  for (const property of ["break-before", "break-after"]) {
+    for (const val of LEGACY_VALUES) {
+      const mapped_value = LEGACY_MAPPING[val] || val;
+
+      div.style["page-" + property] = val;
+
+      assert_equals(div.style["page-" + property], val);
+      assert_equals(div.style[property], mapped_value);
+      assert_equals(cs.getPropertyValue("page-" + property), val);
+      assert_equals(cs.getPropertyValue(property), mapped_value);
+      assert_not_equals(div.style.cssText.indexOf(property + ": " + mapped_value + ";"), -1);
+      assert_equals(div.style.cssText.indexOf("page-" + property), -1,
+                    "Legacy shorthands don't appear in cssText");
+    }
+  }
+}, "Legacy values of the shorthands work as expected")
+
+test(function() {
+  for (const property of ["break-before", "break-after"]) {
+    for (const val of NEW_VALUES) {
+      const mapped_value = REVERSE_LEGACY_MAPPING[val] || "";
+
+      div.style[property] = val;
+
+      assert_equals(div.style[property], val);
+      assert_equals(div.style["page-" + property], mapped_value);
+      assert_equals(cs.getPropertyValue("page-" + property), mapped_value);
+      assert_equals(cs.getPropertyValue(property), val);
+    }
+  }
+}, "New values work on the new longhands, but serialize to the empty string in the legacy shorthands");
+
+test(function() {
+  for (const property of ["break-before", "break-after"]) {
+    for (const val of NEW_VALUES) {
+      div.style["page-" + property] = "";
+      div.style["page-" + property] = val;
+      assert_equals(div.style["page-" + property], "");
+      assert_equals(div.style[property], "");
+      assert_equals(cs.getPropertyValue("page-" + property), "auto");
+      assert_equals(cs.getPropertyValue(property), "auto");
+    }
+  }
+}, "New values of the break longhands don't work on legacy shorthands");
+
+// See https://github.com/w3c/csswg-drafts/issues/3332
+test(function() {
+  for (const property of ["break-before", "break-after"]) {
+    div.style["page-" + property] = "var(--a)";
+
+    assert_equals(div.style["page-" + property], "var(--a)");
+    assert_equals(div.style[property], "");
+    assert_equals(div.style.cssText.indexOf("page-" + property), -1);
+  }
+}, "Legacy shorthands really never appear on cssText, even when there are variable references");
+</script>