Bug 1105868 part 3 - Implement 'inline list-item' and 'inline flow-root list-item' values for the 'display' property. r=emilio
authorMats Palmgren <mats@mozilla.com>
Wed, 14 Aug 2019 14:37:03 +0000
changeset 487973 b517c3b51b9a1afeb3e53d8a43c296ea809fbd60
parent 487972 7f1a906285fc51d11eebebba43b4b541e70c1b86
child 487974 8647528a4e9d6350ba9b1ce6245d51476081c5e4
push id113895
push userbtara@mozilla.com
push dateWed, 14 Aug 2019 22:08:04 +0000
treeherdermozilla-inbound@d65454c57706 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
bugs1105868
milestone70.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1105868 part 3 - Implement 'inline list-item' and 'inline flow-root list-item' values for the 'display' property. r=emilio Differential Revision: https://phabricator.services.mozilla.com/D39832
devtools/shared/css/generated/properties-db.js
layout/base/nsCSSFrameConstructor.cpp
servo/components/style/style_adjuster.rs
servo/components/style/values/specified/box.rs
testing/web-platform/meta/css/css-display/parsing/display-valid.html.ini
--- a/devtools/shared/css/generated/properties-db.js
+++ b/devtools/shared/css/generated/properties-db.js
@@ -5890,16 +5890,18 @@ exports.CSS_PROPERTIES = {
       "block",
       "contents",
       "flex",
       "flow-root",
       "grid",
       "inherit",
       "initial",
       "inline",
+      "inline flow-root list-item",
+      "inline list-item",
       "inline-block",
       "inline-flex",
       "inline-grid",
       "inline-table",
       "list-item",
       "none",
       "revert",
       "ruby",
--- a/layout/base/nsCSSFrameConstructor.cpp
+++ b/layout/base/nsCSSFrameConstructor.cpp
@@ -11186,16 +11186,21 @@ void nsCSSFrameConstructor::BuildInlineC
   // length?  Maybe even to parentContent->GetChildCount()?
   nsFrameConstructorState::PendingBindingAutoPusher pusher(
       aState, aParentItem.mPendingBinding);
 
   ComputedStyle* const parentComputedStyle = aParentItem.mComputedStyle;
   nsIContent* const parentContent = aParentItem.mContent;
 
   if (!aItemIsWithinSVGText) {
+    if (parentComputedStyle->StyleDisplay()->IsListItem()) {
+      CreateGeneratedContentItem(aState, nullptr, *parentContent->AsElement(),
+                                 *parentComputedStyle, PseudoStyleType::marker,
+                                 aParentItem.mChildItems);
+    }
     // Probe for generated content before
     CreateGeneratedContentItem(aState, nullptr, *parentContent->AsElement(),
                                *parentComputedStyle, PseudoStyleType::before,
                                aParentItem.mChildItems);
   }
 
   uint32_t flags = ITEM_ALLOW_XBL_BASE | ITEM_ALLOW_PAGE_BREAK;
   if (aItemIsWithinSVGText) {
--- a/servo/components/style/style_adjuster.rs
+++ b/servo/components/style/style_adjuster.rs
@@ -3,16 +3,18 @@
  * file, You can obtain one at https://mozilla.org/MPL/2.0/. */
 
 //! A struct to encapsulate all the style fixups and flags propagations
 //! a computed style needs in order for it to adhere to the CSS spec.
 
 use crate::dom::TElement;
 use crate::properties::computed_value_flags::ComputedValueFlags;
 use crate::properties::longhands::display::computed_value::T as Display;
+#[cfg(feature = "gecko")]
+use crate::values::specified::box_::DisplayInside;
 use crate::properties::longhands::float::computed_value::T as Float;
 use crate::properties::longhands::overflow_x::computed_value::T as Overflow;
 use crate::properties::longhands::position::computed_value::T as Position;
 use crate::properties::{self, ComputedValues, StyleBuilder};
 use app_units::Au;
 
 /// A struct that implements all the adjustment methods.
 ///
@@ -170,17 +172,19 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
         }
 
         element.map_or(false, |e| e.skip_item_display_fixup())
     }
 
     /// Apply the blockification rules based on the table in CSS 2.2 section 9.7.
     /// <https://drafts.csswg.org/css2/visuren.html#dis-pos-flo>
     /// A ::marker pseudo-element with 'list-style-position:outside' needs to
-    /// have its 'display' blockified.
+    /// have its 'display' blockified, unless the ::marker is for an inline
+    /// list-item (for which 'list-style-position:outside' behaves as 'inside').
+    /// https://drafts.csswg.org/css-lists-3/#list-style-position-property
     fn blockify_if_necessary<E>(&mut self, layout_parent_style: &ComputedValues, element: Option<E>)
     where
         E: TElement,
     {
         use crate::computed_values::list_style_position::T as ListStylePosition;
 
         let mut blockify = false;
         macro_rules! blockify_if {
@@ -189,31 +193,29 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
                     blockify = $if_what;
                 }
             };
         }
 
         let is_root = self.style.pseudo.is_none() && element.map_or(false, |e| e.is_root());
         blockify_if!(is_root);
         if !self.skip_item_display_fixup(element) {
-            blockify_if!(layout_parent_style
-                .get_box()
-                .clone_display()
-                .is_item_container());
+            let parent_display = layout_parent_style.get_box().clone_display();
+            blockify_if!(parent_display.is_item_container());
         }
 
         let is_item_or_root = blockify;
 
         blockify_if!(self.style.floated());
         blockify_if!(self.style.out_of_flow_positioned());
+        #[cfg(feature = "gecko")]
         blockify_if!(
             self.style.pseudo.map_or(false, |p| p.is_marker()) &&
-                self.style.get_parent_list().clone_list_style_position() ==
-                    ListStylePosition::Outside
-        );
+             self.style.get_parent_list().clone_list_style_position() == ListStylePosition::Outside &&
+             layout_parent_style.get_box().clone_display().inside() != DisplayInside::Inline);
 
         if !blockify {
             return;
         }
 
         let display = self.style.get_box().clone_display();
         let blockified_display = display.equivalent_block_display(is_root);
         if display != blockified_display {
--- a/servo/components/style/values/specified/box.rs
+++ b/servo/components/style/values/specified/box.rs
@@ -388,65 +388,76 @@ impl Display {
             #[cfg(feature = "gecko")]
             Display::Contents | Display::Ruby | Display::RubyBaseContainer => true,
             _ => false,
         }
     }
 
     /// Convert this display into an equivalent block display.
     ///
-    /// Also used for style adjustments.
+    /// Also used for :root style adjustments.
     pub fn equivalent_block_display(&self, _is_root_element: bool) -> Self {
+        #[cfg(feature = "gecko")]
+        {
+            // Special handling for `contents` and `list-item`s on the root element.
+            if _is_root_element && (*self == Display::Contents || self.is_list_item()) {
+                return Display::Block;
+            }
+            match self.outside() {
+                DisplayOutside::Inline => {
+                    let inside =  match self.inside() {
+                        DisplayInside::Inline | DisplayInside::FlowRoot => DisplayInside::Block,
+                        // FIXME: we don't handle `block ruby` in layout yet, remove this when we do:
+                        DisplayInside::Ruby => DisplayInside::Block,
+                        inside => inside,
+                    };
+                    Display::from3(DisplayOutside::Block, inside, self.is_list_item())
+                },
+                DisplayOutside::Block | DisplayOutside::None => *self,
+                _ => Display::Block,
+            }
+        }
+        #[cfg(not(feature = "gecko"))]
         match *self {
             // Values that have a corresponding block-outside version.
-            #[cfg(any(feature = "gecko", feature = "servo-layout-2013"))]
+            #[cfg(feature = "servo-layout-2013")]
             Display::InlineTable => Display::Table,
-            #[cfg(any(feature = "gecko", feature = "servo-layout-2013"))]
+            #[cfg(feature = "servo-layout-2013")]
             Display::InlineFlex => Display::Flex,
 
-            #[cfg(feature = "gecko")]
-            Display::InlineGrid => Display::Grid,
-            #[cfg(feature = "gecko")]
-            Display::WebkitInlineBox => Display::WebkitBox,
-
-            // Special handling for contents and list-item on the root
-            // element for Gecko.
-            #[cfg(feature = "gecko")]
-            Display::Contents | Display::ListItem if _is_root_element => Display::Block,
-
             // These are not changed by blockification.
             Display::None | Display::Block => *self,
-            #[cfg(any(feature = "gecko", feature = "servo-layout-2013"))]
+            #[cfg(feature = "servo-layout-2013")]
             Display::Flex | Display::ListItem | Display::Table => *self,
 
-            #[cfg(feature = "gecko")]
-            Display::Contents | Display::FlowRoot | Display::Grid | Display::WebkitBox => *self,
-
             // Everything else becomes block.
             _ => Display::Block,
         }
     }
 
-    /// Convert this display into an inline-outside display.
-    ///
-    /// Ideally it should implement spec: https://drafts.csswg.org/css-display/#inlinify
-    /// but the spec isn't stable enough, so we copy what Gecko does for now.
+    /// Convert this display into an equivalent inline-outside display.
+    /// https://drafts.csswg.org/css-display/#inlinify
     #[cfg(feature = "gecko")]
     pub fn inlinify(&self) -> Self {
-        match *self {
-            Display::Block | Display::FlowRoot => Display::InlineBlock,
-            Display::Table => Display::InlineTable,
-            Display::Flex => Display::InlineFlex,
-            Display::Grid => Display::InlineGrid,
-            // XXX bug 1105868 this should probably be InlineListItem:
-            Display::ListItem => Display::Inline,
-            Display::MozBox => Display::MozInlineBox,
-            Display::MozStack => Display::MozInlineStack,
-            Display::WebkitBox => Display::WebkitInlineBox,
-            other => other,
+        match self.outside() {
+            DisplayOutside::Block => {
+                let inside = match self.inside() {
+                    DisplayInside::Block => DisplayInside::FlowRoot,
+                    inside => inside,
+                };
+                Display::from3(DisplayOutside::Inline, inside, self.is_list_item())
+            },
+            DisplayOutside::XUL => {
+                match self.inside() {
+                    DisplayInside::MozBox => Display::MozInlineBox,
+                    DisplayInside::MozStack => Display::MozInlineStack,
+                    _ => *self,
+                }
+            },
+            _ => *self,
         }
     }
 
     /// Returns true if the value is `Contents`
     #[inline]
     pub fn is_contents(&self) -> bool {
         match *self {
             #[cfg(feature = "gecko")]
@@ -491,18 +502,25 @@ impl ToCss for Display {
                     dest.write_str("inline-")?;
                     inside.to_css(dest)
                 }
                 (DisplayOutside::Block, DisplayInside::Ruby) => {
                     dest.write_str("block ruby")
                 }
                 (_, inside) => {
                     if self.is_list_item() {
-                        debug_assert_eq!(inside, DisplayInside::FlowRoot);
-                        dest.write_str("flow-root list-item")
+                        if outside != DisplayOutside::Block {
+                            outside.to_css(dest)?;
+                            dest.write_str(" ")?;
+                        }
+                        if inside != DisplayInside::Flow {
+                            inside.to_css(dest)?;
+                            dest.write_str(" ")?;
+                        }
+                        dest.write_str("list-item")
                     } else {
                         inside.to_css(dest)
                     }
                 }
             }
         }
     }
 }
@@ -542,45 +560,20 @@ fn parse_display_inside_for_block<'i, 't
 /// https://drafts.csswg.org/css-display/#typedef-display-outside
 #[cfg(feature = "gecko")]
 fn parse_display_outside<'i, 't>(
     input: &mut Parser<'i, 't>,
 ) -> Result<DisplayOutside, ParseError<'i>> {
     Ok(try_match_ident_ignore_ascii_case! { input,
         "block" => DisplayOutside::Block,
         "inline" => DisplayOutside::Inline,
-        // FIXME: not supported in layout yet:
-        //"run-in" => DisplayOutside::RunIn,
-    })
-}
-
-/// FIXME: this can be replaced with parse_display_outside once we
-/// support all its values for list items.
-#[cfg(feature = "gecko")]
-fn parse_display_outside_for_list_item<'i, 't>(
-    input: &mut Parser<'i, 't>,
-) -> Result<DisplayOutside, ParseError<'i>> {
-    Ok(try_match_ident_ignore_ascii_case! { input,
-        "block" => DisplayOutside::Block,
-        // FIXME(bug 1105868): not supported in layout yet:
-        //"inline" => DisplayOutside::Inline,
         // FIXME(bug 2056): not supported in layout yet:
         //"run-in" => DisplayOutside::RunIn,
     })
 }
-/// Test a <display-outside> Result for same values as above.
-#[cfg(feature = "gecko")]
-fn is_valid_outside_for_list_item<'i>(
-    outside: &Result<DisplayOutside, ParseError<'i>>,
-) -> bool {
-    match outside {
-        Ok(DisplayOutside::Block) => true,
-        _ => false,
-    }
-}
 
 /// FIXME: this can be replaced with parse_display_outside once we
 /// support all its values for `ruby`.
 #[cfg(feature = "gecko")]
 fn parse_display_outside_for_ruby<'i, 't>(
     input: &mut Parser<'i, 't>,
 ) -> Result<DisplayOutside, ParseError<'i>> {
     Ok(try_match_ident_ignore_ascii_case! { input,
@@ -632,41 +625,38 @@ impl Parse for Display {
         } else {
             input.try(parse_display_inside)
         };
         // <display-listitem> = <display-outside>? && [ flow | flow-root ]? && list-item
         // https://drafts.csswg.org/css-display/#typedef-display-listitem
         if !got_list_item && is_valid_inside_for_list_item(&inside) {
             got_list_item = input.try(parse_list_item).is_ok();
         }
-        let outside = if got_list_item {
-            input.try(parse_display_outside_for_list_item)
-        } else {
-            match inside {
-                Ok(DisplayInside::Ruby) => input.try(parse_display_outside_for_ruby),
-                _ => input.try(parse_display_outside),
-            }
+        let outside = match inside {
+            // FIXME we don't handle `block ruby` in layout yet.
+            Ok(DisplayInside::Ruby) => input.try(parse_display_outside_for_ruby),
+            _ => input.try(parse_display_outside),
         };
-        if !got_list_item && is_valid_outside_for_list_item(&outside) {
-            got_list_item = input.try(parse_list_item).is_ok();
-        }
-        if outside.is_ok() && inside.is_err(){
-            inside = if got_list_item {
-                input.try(parse_display_inside_for_list_item)
-            } else {
-                match outside {
-                    // FIXME we don't handle `block ruby` in layout yet.
-                    Ok(DisplayOutside::Block) => input.try(parse_display_inside_for_block),
-                    _ => input.try(parse_display_inside),
+        if outside.is_ok() {
+            if !got_list_item && (inside.is_err() || is_valid_inside_for_list_item(&inside)) {
+                got_list_item = input.try(parse_list_item).is_ok();
+            }
+            if inside.is_err() {
+                inside = if got_list_item {
+                    input.try(parse_display_inside_for_list_item)
+                } else {
+                    match outside {
+                        // FIXME we don't handle `block ruby` in layout yet.
+                        Ok(DisplayOutside::Block) => input.try(parse_display_inside_for_block),
+                        _ => input.try(parse_display_inside),
+                    }
+                };
+                if !got_list_item && is_valid_inside_for_list_item(&inside) {
+                    got_list_item = input.try(parse_list_item).is_ok();
                 }
-            };
-            if !got_list_item &&
-                is_valid_outside_for_list_item(&outside) &&
-                is_valid_inside_for_list_item(&inside) {
-                got_list_item = input.try(parse_list_item).is_ok();
             }
         }
         if got_list_item || inside.is_ok() || outside.is_ok() {
             let inside = inside.unwrap_or(DisplayInside::Flow);
             let outside = outside.unwrap_or(
                 match inside {
                     // "If <display-outside> is omitted, the element’s outside display type
                     // defaults to block — except for ruby, which defaults to inline."
@@ -725,16 +715,18 @@ impl SpecifiedValueInfo for Display {
           "flex",
           "flow-root",
           "grid",
           "inline",
           "inline-block",
           "inline-flex",
           "inline-grid",
           "inline-table",
+          "inline list-item",
+          "inline flow-root list-item",
           "list-item",
           "none",
           "ruby",
           "ruby-base",
           "ruby-base-container",
           "ruby-text",
           "ruby-text-container",
           "table",
--- a/testing/web-platform/meta/css/css-display/parsing/display-valid.html.ini
+++ b/testing/web-platform/meta/css/css-display/parsing/display-valid.html.ini
@@ -1,12 +1,9 @@
 [display-valid.html]
-  [e.style['display'\] = "list-item inline flow" should set the property value]
-    expected: FAIL
-
   [e.style['display'\] = "ruby block" should set the property value]
     expected: FAIL
 
   [e.style['display'\] = "run-in flow-root list-item" should set the property value]
     expected: FAIL
 
   [e.style['display'\] = "run-in table" should set the property value]
     expected: FAIL
@@ -18,91 +15,58 @@
     expected: FAIL
 
   [e.style['display'\] = "block ruby" should set the property value]
     expected: FAIL
 
   [e.style['display'\] = "run-in flow list-item" should set the property value]
     expected: FAIL
 
-  [e.style['display'\] = "list-item flow-root inline" should set the property value]
-    expected: FAIL
-
   [e.style['display'\] = "table run-in" should set the property value]
     expected: FAIL
 
-  [e.style['display'\] = "flow inline list-item" should set the property value]
-    expected: FAIL
-
-  [e.style['display'\] = "flow-root inline list-item" should set the property value]
-    expected: FAIL
-
-  [e.style['display'\] = "inline list-item" should set the property value]
-    expected: FAIL
-
   [e.style['display'\] = "list-item run-in" should set the property value]
     expected: FAIL
 
-  [e.style['display'\] = "inline flow-root list-item" should set the property value]
-    expected: FAIL
-
   [e.style['display'\] = "grid run-in" should set the property value]
     expected: FAIL
 
-  [e.style['display'\] = "inline flow list-item" should set the property value]
-    expected: FAIL
-
   [e.style['display'\] = "flow run-in" should set the property value]
     expected: FAIL
 
   [e.style['display'\] = "run-in list-item flow-root" should set the property value]
     expected: FAIL
 
   [e.style['display'\] = "run-in flow-root" should set the property value]
     expected: FAIL
 
-  [e.style['display'\] = "inline list-item flow" should set the property value]
-    expected: FAIL
-
   [e.style['display'\] = "run-in grid" should set the property value]
     expected: FAIL
 
   [e.style['display'\] = "list-item run-in flow" should set the property value]
     expected: FAIL
 
   [e.style['display'\] = "flow run-in list-item" should set the property value]
     expected: FAIL
 
-  [e.style['display'\] = "flow list-item inline" should set the property value]
-    expected: FAIL
-
   [e.style['display'\] = "flow-root run-in list-item" should set the property value]
     expected: FAIL
 
-  [e.style['display'\] = "flow-root list-item inline" should set the property value]
-    expected: FAIL
-
   [e.style['display'\] = "list-item flow-root run-in" should set the property value]
     expected: FAIL
 
-  [e.style['display'\] = "list-item inline flow-root" should set the property value]
-    expected: FAIL
-
   [e.style['display'\] = "flow-root list-item run-in" should set the property value]
     expected: FAIL
 
   [e.style['display'\] = "flow-root run-in" should set the property value]
     expected: FAIL
 
   [e.style['display'\] = "run-in ruby" should set the property value]
     expected: FAIL
 
-  [e.style['display'\] = "list-item flow inline" should set the property value]
-    expected: FAIL
-
   [e.style['display'\] = "flex run-in" should set the property value]
     expected: FAIL
 
   [e.style['display'\] = "run-in" should set the property value]
     expected: FAIL
 
   [e.style['display'\] = "list-item flow run-in" should set the property value]
     expected: FAIL
@@ -114,17 +78,11 @@
     expected: FAIL
 
   [e.style['display'\] = "flow list-item run-in" should set the property value]
     expected: FAIL
 
   [e.style['display'\] = "ruby run-in" should set the property value]
     expected: FAIL
 
-  [e.style['display'\] = "inline list-item flow-root" should set the property value]
-    expected: FAIL
-
-  [e.style['display'\] = "list-item inline" should set the property value]
-    expected: FAIL
-
   [e.style['display'\] = "run-in flex" should set the property value]
     expected: FAIL