servo: Merge #15736 - Bug 1341083: Implement dynamic restyling for display: contents (from servo:display-contents); r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Sat, 25 Feb 2017 11:00:07 -0800
changeset 373969 048c49c1f3b3f282325b71ec3f65c4d6f50cc051
parent 373968 bb0f9802f0d0682f521f749f055a964b371f1be5
child 373970 10bfa931c92f207d44019ad984e36c116f2e6f55
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs15736, 1341083
milestone54.0a1
servo: Merge #15736 - Bug 1341083: Implement dynamic restyling for display: contents (from servo:display-contents); r=heycam Reviewed upstream by @heycam cc @bholley, didn't end up renaming the `layout_parent` thing, because it made the cascade way more verbose (and difficult to understand IMO) unnecessarily, and you said you were ok-ish with it. Source-Repo: https://github.com/servo/servo Source-Revision: b77140a0375dcad9e15d4692edf8f15d5fe4597c
servo/components/style/animation.rs
servo/components/style/gecko/media_queries.rs
servo/components/style/matching.rs
servo/components/style/properties/gecko.mako.rs
servo/components/style/properties/properties.mako.rs
servo/components/style/servo/media_queries.rs
servo/components/style/stylist.rs
servo/components/style/values/computed/mod.rs
servo/components/style/viewport.rs
servo/ports/geckolib/glue.rs
servo/tests/unit/style/parsing/image.rs
--- a/servo/components/style/animation.rs
+++ b/servo/components/style/animation.rs
@@ -425,16 +425,17 @@ fn compute_style_for_animation_step(cont
                 guard.declarations.iter().rev().map(|&(ref decl, _importance)| decl)
             };
 
             let computed =
                 properties::apply_declarations(context.viewport_size,
                                                /* is_root = */ false,
                                                iter,
                                                previous_style,
+                                               previous_style,
                                                &context.default_computed_values,
                                                /* cascade_info = */ None,
                                                context.error_reporter.clone(),
                                                /* Metrics provider */ None,
                                                CascadeFlags::empty());
             computed
         }
     }
--- a/servo/components/style/gecko/media_queries.rs
+++ b/servo/components/style/gecko/media_queries.rs
@@ -487,16 +487,17 @@ impl Expression {
         let default_values = device.default_values();
 
         // http://dev.w3.org/csswg/mediaqueries3/#units
         // em units are relative to the initial font-size.
         let context = computed::Context {
             is_root_element: false,
             viewport_size: device.au_viewport_size(),
             inherited_style: default_values,
+            layout_parent_style: default_values,
             // This cloning business is kind of dumb.... It's because Context
             // insists on having an actual ComputedValues inside itself.
             style: default_values.clone(),
             font_metrics_provider: None
         };
 
         let required_value = match self.value {
             Some(ref v) => v,
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -438,16 +438,43 @@ pub enum StyleSharingResult {
 ///
 /// FIXME(pcwalton): Unify with `CascadeFlags`, perhaps?
 struct CascadeBooleans {
     shareable: bool,
     animate: bool,
 }
 
 trait PrivateMatchMethods: TElement {
+    /// Returns the closest parent element that doesn't have a display: contents
+    /// style (and thus generates a box).
+    ///
+    /// This is needed to correctly handle blockification of flex and grid
+    /// items.
+    ///
+    /// Returns itself if the element has no parent. In practice this doesn't
+    /// happen because the root element is blockified per spec, but it could
+    /// happen if we decide to not blockify for roots of disconnected subtrees,
+    /// which is a kind of dubious beahavior.
+    fn layout_parent(&self) -> Self {
+        let mut current = self.clone();
+        loop {
+            current = match current.parent_element() {
+                Some(el) => el,
+                None => return current,
+            };
+
+            let is_display_contents =
+                current.borrow_data().unwrap().styles().primary.values().is_display_contents();
+
+            if !is_display_contents {
+                return current;
+            }
+        }
+    }
+
     fn cascade_internal(&self,
                         context: &StyleContext<Self>,
                         primary_style: &ComputedStyle,
                         pseudo_style: &mut Option<(&PseudoElement, &mut ComputedStyle)>,
                         booleans: &CascadeBooleans)
                         -> Arc<ComputedValues> {
         let shared_context = context.shared;
         let mut cascade_info = CascadeInfo::new();
@@ -462,47 +489,66 @@ trait PrivateMatchMethods: TElement {
         // Grab the rule node.
         let rule_node = &pseudo_style.as_ref().map_or(primary_style, |p| &*p.1).rules;
 
         // Grab the inherited values.
         let parent_el;
         let parent_data;
         let inherited_values_ = if pseudo_style.is_none() {
             parent_el = self.parent_element();
-            parent_data = parent_el.as_ref().and_then(|x| x.borrow_data());
+            parent_data = parent_el.as_ref().and_then(|e| e.borrow_data());
             let parent_values = parent_data.as_ref().map(|d| {
                 // Sometimes Gecko eagerly styles things without processing
                 // pending restyles first. In general we'd like to avoid this,
                 // but there can be good reasons (for example, needing to
                 // construct a frame for some small piece of newly-added
                 // content in order to do something specific with that frame,
                 // but not wanting to flush all of layout).
                 debug_assert!(cfg!(feature = "gecko") || d.has_current_styles());
                 d.styles().primary.values()
             });
 
-            // Propagate the "can be fragmented" bit. It would be nice to
-            // encapsulate this better.
-            if let Some(ref p) = parent_values {
+            parent_values
+        } else {
+            parent_el = Some(self.clone());
+            Some(primary_style.values())
+        };
+
+        let mut layout_parent_el = parent_el.clone();
+        let layout_parent_data;
+        let mut layout_parent_style = inherited_values_;
+        if inherited_values_.map_or(false, |s| s.is_display_contents()) {
+            layout_parent_el = Some(layout_parent_el.unwrap().layout_parent());
+            layout_parent_data = layout_parent_el.as_ref().unwrap().borrow_data().unwrap();
+            layout_parent_style = Some(layout_parent_data.styles().primary.values())
+        }
+
+        let inherited_values = inherited_values_.map(|x| &**x);
+        let layout_parent_style = layout_parent_style.map(|x| &**x);
+
+        // Propagate the "can be fragmented" bit. It would be nice to
+        // encapsulate this better.
+        //
+        // Note that this is not needed for pseudos since we already do that
+        // when we resolve the non-pseudo style.
+        if pseudo_style.is_none() {
+            if let Some(ref p) = layout_parent_style {
                 let can_be_fragmented =
-                    p.is_multicol() || parent_el.unwrap().as_node().can_be_fragmented();
+                    p.is_multicol() ||
+                    layout_parent_el.as_ref().unwrap().as_node().can_be_fragmented();
                 unsafe { self.as_node().set_can_be_fragmented(can_be_fragmented); }
             }
-
-            parent_values
-        } else {
-            Some(primary_style.values())
-        };
-        let inherited_values = inherited_values_.map(|x| &**x);
+        }
 
         // Invoke the cascade algorithm.
         let values =
             Arc::new(cascade(shared_context.viewport_size,
                              rule_node,
                              inherited_values,
+                             layout_parent_style,
                              &shared_context.default_computed_values,
                              Some(&mut cascade_info),
                              shared_context.error_reporter.clone(),
                              cascade_flags));
 
         cascade_info.finish(&self.as_node());
         values
     }
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -125,16 +125,21 @@ impl ComputedValues {
             writing_mode: WritingMode::empty(), // FIXME(bz): This seems dubious
             root_font_size: longhands::font_size::get_initial_value(), // FIXME(bz): Also seems dubious?
             % for style_struct in data.style_structs:
                 ${style_struct.ident}: style_structs::${style_struct.name}::default(pres_context),
             % endfor
         })
     }
 
+    #[inline]
+    pub fn is_display_contents(&self) -> bool {
+        self.get_box().clone_display() == longhands::display::computed_value::T::contents
+    }
+
     % for style_struct in data.style_structs:
     #[inline]
     pub fn clone_${style_struct.name_lower}(&self) -> Arc<style_structs::${style_struct.name}> {
         self.${style_struct.ident}.clone()
     }
     #[inline]
     pub fn get_${style_struct.name_lower}(&self) -> &style_structs::${style_struct.name} {
         &self.${style_struct.ident}
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -1419,16 +1419,21 @@ impl ComputedValues {
     fn custom_properties(&self) -> Option<Arc<::custom_properties::ComputedValuesMap>> {
         self.custom_properties.as_ref().map(|x| x.clone())
     }
 
     /// Whether this style has a -moz-binding value. This is always false for
     /// Servo for obvious reasons.
     pub fn has_moz_binding(&self) -> bool { false }
 
+    /// Returns whether this style's display value is equal to contents.
+    ///
+    /// Since this isn't supported in Servo, this is always false for Servo.
+    pub fn is_display_contents(&self) -> bool { false }
+
     /// Get the root font size.
     fn root_font_size(&self) -> Au { self.root_font_size }
 
     /// Set the root font size.
     fn set_root_font_size(&mut self, size: Au) { self.root_font_size = size }
     /// Set the writing mode for this style.
     pub fn set_writing_mode(&mut self, mode: WritingMode) { self.writing_mode = mode; }
 
@@ -1756,24 +1761,26 @@ bitflags! {
 ///   * `parent_style`: The parent style, if applicable; if `None`, this is the root node.
 ///
 /// Returns the computed values.
 ///   * `flags`: Various flags.
 ///
 pub fn cascade(viewport_size: Size2D<Au>,
                rule_node: &StrongRuleNode,
                parent_style: Option<<&ComputedValues>,
+               layout_parent_style: Option<<&ComputedValues>,
                default_style: &Arc<ComputedValues>,
                cascade_info: Option<<&mut CascadeInfo>,
                error_reporter: StdBox<ParseErrorReporter + Send>,
                flags: CascadeFlags)
                -> ComputedValues {
-    let (is_root_element, inherited_style) = match parent_style {
-        Some(parent_style) => (false, parent_style),
-        None => (true, &**default_style),
+    debug_assert_eq!(parent_style.is_some(), layout_parent_style.is_some());
+    let (is_root_element, inherited_style, layout_parent_style) = match parent_style {
+        Some(parent_style) => (false, parent_style, layout_parent_style.unwrap()),
+        None => (true, &**default_style, &**default_style),
     };
     // Hold locks until after the apply_declarations() call returns.
     // Use filter_map because the root node has no style source.
     let lock_guards = rule_node.self_and_ancestors().filter_map(|node| {
         node.style_source().map(|source| (source.read(), node.importance()))
     }).collect::<Vec<_>>();
     let iter_declarations = || {
         lock_guards.iter().flat_map(|&(ref source, source_importance)| {
@@ -1788,29 +1795,31 @@ pub fn cascade(viewport_size: Size2D<Au>
                 }
             })
         })
     };
     apply_declarations(viewport_size,
                        is_root_element,
                        iter_declarations,
                        inherited_style,
+                       layout_parent_style,
                        default_style,
                        cascade_info,
                        error_reporter,
                        None,
                        flags)
 }
 
 /// NOTE: This function expects the declaration with more priority to appear
 /// first.
 pub fn apply_declarations<'a, F, I>(viewport_size: Size2D<Au>,
                                     is_root_element: bool,
                                     iter_declarations: F,
                                     inherited_style: &ComputedValues,
+                                    layout_parent_style: &ComputedValues,
                                     default_style: &Arc<ComputedValues>,
                                     mut cascade_info: Option<<&mut CascadeInfo>,
                                     mut error_reporter: StdBox<ParseErrorReporter + Send>,
                                     font_metrics_provider: Option<<&FontMetricsProvider>,
                                     flags: CascadeFlags)
                                     -> ComputedValues
     where F: Fn() -> I,
           I: Iterator<Item = &'a PropertyDeclaration>,
@@ -1856,16 +1865,17 @@ pub fn apply_declarations<'a, F, I>(view
                             % endfor
                             )
     };
 
     let mut context = computed::Context {
         is_root_element: is_root_element,
         viewport_size: viewport_size,
         inherited_style: inherited_style,
+        layout_parent_style: layout_parent_style,
         style: starting_style,
         font_metrics_provider: font_metrics_provider,
     };
 
     // Set computed values, overwriting earlier declarations for the same
     // property.
     //
     // NB: The cacheable boolean is not used right now, but will be once we
@@ -1938,20 +1948,17 @@ pub fn apply_declarations<'a, F, I>(view
     % endfor
 
     let mut style = context.style;
 
     let positioned = matches!(style.get_box().clone_position(),
         longhands::position::SpecifiedValue::absolute |
         longhands::position::SpecifiedValue::fixed);
     let floated = style.get_box().clone_float() != longhands::float::computed_value::T::none;
-    // FIXME(heycam): We should look past any display:contents ancestors to
-    // determine if we are a flex or grid item, but we don't have access to
-    // grandparent or higher style here.
-    let is_item = matches!(context.inherited_style.get_box().clone_display(),
+    let is_item = matches!(context.layout_parent_style.get_box().clone_display(),
         % if product == "gecko":
         computed_values::display::T::grid |
         computed_values::display::T::inline_grid |
         % endif
         computed_values::display::T::flex |
         computed_values::display::T::inline_flex);
     let (blockify_root, blockify_item) = match flags.contains(SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP) {
         false => (is_root_element, is_item),
@@ -2018,17 +2025,17 @@ pub fn apply_declarations<'a, F, I>(view
     //
     // See https://github.com/servo/servo/issues/15229
     % if product == "servo" and "align-items" in data.longhands_by_name:
     {
         use computed_values::align_self::T as align_self;
         use computed_values::align_items::T as align_items;
         if style.get_position().clone_align_self() == computed_values::align_self::T::auto && !positioned {
             let self_align =
-                match context.inherited_style.get_position().clone_align_items() {
+                match context.layout_parent_style.get_position().clone_align_items() {
                     align_items::stretch => align_self::stretch,
                     align_items::baseline => align_self::baseline,
                     align_items::flex_start => align_self::flex_start,
                     align_items::flex_end => align_self::flex_end,
                     align_items::center => align_self::center,
                 };
             style.mutate_position().set_align_self(self_align);
         }
--- a/servo/components/style/servo/media_queries.rs
+++ b/servo/components/style/servo/media_queries.rs
@@ -175,16 +175,17 @@ impl Range<specified::Length> {
                          default_values: &ComputedValues)
                          -> Range<Au> {
         // http://dev.w3.org/csswg/mediaqueries3/#units
         // em units are relative to the initial font-size.
         let context = computed::Context {
             is_root_element: false,
             viewport_size: viewport_size,
             inherited_style: default_values,
+            layout_parent_style: default_values,
             // This cloning business is kind of dumb.... It's because Context
             // insists on having an actual ComputedValues inside itself.
             style: default_values.clone(),
             font_metrics_provider: None
         };
 
         match *self {
             Range::Min(ref width) => Range::Min(width.to_computed_value(&context)),
--- a/servo/components/style/stylist.rs
+++ b/servo/components/style/stylist.rs
@@ -308,20 +308,35 @@ impl Stylist {
             None => self.rule_tree.root(),
         };
 
         let mut flags = CascadeFlags::empty();
         if inherit_all {
             flags.insert(INHERIT_ALL)
         }
 
+        // NOTE(emilio): We skip calculating the proper layout parent style
+        // here.
+        //
+        // It'd be fine to assert that this isn't called with a parent style
+        // where display contents is in effect, but in practice this is hard to
+        // do for stuff like :-moz-fieldset-content with a
+        // <fieldset style="display: contents">. That is, the computed value of
+        // display for the fieldset is "contents", even though it's not the used
+        // value, so we don't need to adjust in a different way anyway.
+        //
+        // In practice, I don't think any anonymous content can be a direct
+        // descendant of a display: contents element where display: contents is
+        // the actual used value, and the computed value of it would need
+        // blockification.
         let computed =
             properties::cascade(self.device.au_viewport_size(),
                                 &rule_node,
                                 parent.map(|p| &**p),
+                                parent.map(|p| &**p),
                                 default,
                                 None,
                                 Box::new(StdoutErrorReporter),
                                 flags);
         ComputedStyle::new(rule_node, Arc::new(computed))
     }
 
     /// Returns the style for an anonymous box of the given type.
@@ -384,20 +399,25 @@ impl Stylist {
                                           Some(pseudo),
                                           &mut declarations,
                                           &mut flags);
 
         let rule_node =
             self.rule_tree.insert_ordered_rules(
                 declarations.into_iter().map(|a| (a.source, a.level)));
 
+        // Read the comment on `precomputed_values_for_pseudo` to see why it's
+        // difficult to assert that display: contents nodes never arrive here
+        // (tl;dr: It doesn't apply for replaced elements and such, but the
+        // computed value is still "contents").
         let computed =
             properties::cascade(self.device.au_viewport_size(),
                                 &rule_node,
                                 Some(&**parent),
+                                Some(&**parent),
                                 default,
                                 None,
                                 Box::new(StdoutErrorReporter),
                                 CascadeFlags::empty());
 
         // Apply the selector flags. We should be in sequential mode already,
         // so we can directly apply the parent flags.
         if cfg!(feature = "servo") {
--- a/servo/components/style/values/computed/mod.rs
+++ b/servo/components/style/values/computed/mod.rs
@@ -38,16 +38,22 @@ pub struct Context<'a> {
     pub is_root_element: bool,
 
     /// The current viewport size.
     pub viewport_size: Size2D<Au>,
 
     /// The style we're inheriting from.
     pub inherited_style: &'a ComputedValues,
 
+    /// The style of the layout parent node. This will almost always be
+    /// `inherited_style`, except when `display: contents` is at play, in which
+    /// case it's the style of the last ancestor with a `display` value that
+    /// isn't `contents`.
+    pub layout_parent_style: &'a ComputedValues,
+
     /// Values access through this need to be in the properties "computed
     /// early": color, text-decoration, font-size, display, position, float,
     /// border-*-style, outline-style, font-family, writing-mode...
     pub style: ComputedValues,
 
     /// A font metrics provider, used to access font metrics to implement
     /// font-relative units.
     ///
--- a/servo/components/style/viewport.rs
+++ b/servo/components/style/viewport.rs
@@ -684,16 +684,17 @@ impl MaybeNew for ViewportConstraints {
         // resolved against initial values
         let initial_viewport = device.au_viewport_size();
 
         // TODO(emilio): Stop cloning `ComputedValues` around!
         let context = Context {
             is_root_element: false,
             viewport_size: initial_viewport,
             inherited_style: device.default_values(),
+            layout_parent_style: device.default_values(),
             style: device.default_values().clone(),
             font_metrics_provider: None, // TODO: Should have!
         };
 
         // DEVICE-ADAPT ยง 9.3 Resolving 'extend-to-zoom'
         let extend_width;
         let extend_height;
         if let Some(extend_zoom) = max!(initial_zoom, max_zoom) {
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -640,17 +640,20 @@ fn get_pseudo_style(element: GeckoElemen
 {
     let pseudo = PseudoElement::from_atom_unchecked(Atom::from(pseudo_tag), false);
     match SelectorImpl::pseudo_element_cascade_type(&pseudo) {
         PseudoElementCascadeType::Eager => styles.pseudos.get(&pseudo).map(|s| s.values().clone()),
         PseudoElementCascadeType::Precomputed => unreachable!("No anonymous boxes"),
         PseudoElementCascadeType::Lazy => {
             let d = doc_data.borrow_mut();
             let base = styles.primary.values();
-            d.stylist.lazily_compute_pseudo_element_style(&element, &pseudo, base, &d.default_computed_values())
+            d.stylist.lazily_compute_pseudo_element_style(&element,
+                                                          &pseudo,
+                                                          base,
+                                                          &d.default_computed_values())
                      .map(|s| s.values().clone())
         },
     }
 }
 
 #[no_mangle]
 pub extern "C" fn Servo_ComputedValues_Inherit(
   raw_data: RawServoStyleSetBorrowed,
@@ -1343,16 +1346,17 @@ pub extern "C" fn Servo_GetComputedKeyfr
     let parent_style = parent_style.as_ref().map(|r| &**ComputedValues::as_arc(&r));
     let init = ComputedValues::default_values(pres_context);
 
     let context = Context {
         is_root_element: false,
         // FIXME (bug 1303229): Use the actual viewport size here
         viewport_size: Size2D::new(Au(0), Au(0)),
         inherited_style: parent_style.unwrap_or(&init),
+        layout_parent_style: parent_style.unwrap_or(&init),
         style: (**style).clone(),
         font_metrics_provider: None,
     };
 
     for (index, keyframe) in keyframes.iter().enumerate() {
         let ref mut animation_values = computed_keyframes[index];
 
         let mut seen = PropertyBitField::new();
--- a/servo/tests/unit/style/parsing/image.rs
+++ b/servo/tests/unit/style/parsing/image.rs
@@ -44,16 +44,17 @@ fn test_linear_gradient() {
     // Note that Angle(PI) is correct for top-to-bottom rendering, whereas Angle(0) would render bottom-to-top.
     // ref: https://developer.mozilla.org/en-US/docs/Web/CSS/angle
     let container = Size2D::new(Au::default(), Au::default());
     let initial_style = ComputedValues::initial_values();
     let specified_context = Context {
         is_root_element: true,
         viewport_size: container,
         inherited_style: initial_style,
+        layout_parent_style: initial_style,
         style: initial_style.clone(),
         font_metrics_provider: None,
     };
     assert_eq!(specified::AngleOrCorner::None.to_computed_value(&specified_context),
                computed::AngleOrCorner::Angle(Angle(PI)));
 }
 
 #[test]