servo: Merge #19666 - style: Simplify the skip item based display fixup adjustment (from emilio:root-and-item); r=upsuper

In practice the only NAC that possibly inherits from a grid or flex container
are pseudos.

In Gecko, if the root element is an item container, custom anon content would
also sometimes incorrectly inherit from that (see bug 1405635), but that's fixed
in Stylo.

We remove the IS_ROOT_ELEMENT blockification from the "skip display fixup"
check, since the root element is never NAC or anything like that, so there's no
need for the check.

This also fixes some reparenting fishiness related to pseudo-elements. We were
only skipping the fixup when reparenting anon boxes, not when reparenting normal
element styles, nor when reparenting other pseudo styles which are not anon
boxes.

Source-Repo: https://github.com/servo/servo
Source-Revision: 1970e82b0d310128eabe8466d39d42cc20e7ae4b

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 6633420bdbfffc8a284fa7d804cb48ed0ab20cae

servo/components/layout_thread/dom_wrapper.rs

@@ -460,10 +460,6 @@ impl<'le> TElement for ServoLayoutElement<'le> {
         }
     }
 
-    fn skip_root_and_item_based_display_fixup(&self) -> bool {
-        false
-    }
-
     unsafe fn set_selector_flags(&self, flags: ElementSelectorFlags) {
         self.element.insert_selector_flags(flags);
     }

servo/components/style/dom.rs

@@ -670,11 +670,6 @@ pub trait TElement
         self.get_data().map(|x| x.borrow_mut())
     }
 
-    /// Whether we should skip any root- or item-based display property
-    /// blockification on this element.  (This function exists so that Gecko
-    /// native anonymous content can opt out of this style fixup.)
-    fn skip_root_and_item_based_display_fixup(&self) -> bool;
-
     /// Sets selector flags, which indicate what kinds of selectors may have
     /// matched on this element and therefore what kind of work may need to
     /// be performed when DOM state changes.

servo/components/style/gecko/wrapper.rs

@@ -1256,19 +1256,6 @@ impl<'le> TElement for GeckoElement<'le> {
         }
     }
 
-    #[inline]
-    fn skip_root_and_item_based_display_fixup(&self) -> bool {
-        if !self.is_native_anonymous() {
-            return false;
-        }
-
-        if let Some(p) = self.implemented_pseudo_element() {
-            return p.skip_item_based_display_fixup();
-        }
-
-        self.is_root_of_native_anonymous_subtree()
-    }
-
     unsafe fn set_selector_flags(&self, flags: ElementSelectorFlags) {
         debug_assert!(!flags.is_empty());
         self.set_flags(selector_flags_to_node_flags(flags));

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

@@ -3060,14 +3060,10 @@ bitflags! {
     pub struct CascadeFlags: u8 {
         /// Whether to inherit all styles from the parent. If this flag is not
         /// present, non-inherited styles are reset to their initial values.
-        const INHERIT_ALL = 1;
-
-        /// Whether to skip any display style fixup for root element, flex/grid
-        /// item, and ruby descendants.
-        const SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP = 1 << 1;
+        const INHERIT_ALL = 1 << 0;
 
         /// Whether to only cascade properties that are visited dependent.
-        const VISITED_DEPENDENT_ONLY = 1 << 2;
+        const VISITED_DEPENDENT_ONLY = 1 << 1;
 
         /// Whether the given element we're styling is the document element,
         /// that is, matches :root.
@@ -3077,15 +3073,15 @@ bitflags! {
         ///
         /// This affects some style adjustments, like blockification, and means
         /// that it may affect global state, like the Device's root font-size.
-        const IS_ROOT_ELEMENT = 1 << 3;
+        const IS_ROOT_ELEMENT = 1 << 2;
 
         /// Whether we're computing the style of a link, either visited or
         /// unvisited.
-        const IS_LINK = 1 << 4;
+        const IS_LINK = 1 << 3;
 
         /// Whether we're computing the style of a link element that happens to
         /// be visited.
-        const IS_VISITED_LINK = 1 << 5;
+        const IS_VISITED_LINK = 1 << 4;
     }
 }
 

servo/components/style/style_adjuster.rs

@@ -28,6 +28,12 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
         StyleAdjuster { style }
     }
 
+    /// Whether to skip any display style fixup flex/grid item, and ruby
+    /// descendants.
+    fn skip_item_based_display_fixup(&self) -> bool {
+        self.style.pseudo.as_ref().map_or(false, |p| p.skip_item_based_display_fixup())
+    }
+
     /// <https://fullscreen.spec.whatwg.org/#new-stacking-layer>
     ///
     ///    Any position value other than 'absolute' and 'fixed' are
@@ -66,10 +72,11 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
             }
         }
 
-        if !flags.contains(CascadeFlags::SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP) {
-            blockify_if!(flags.contains(CascadeFlags::IS_ROOT_ELEMENT));
-            blockify_if!(layout_parent_style.get_box().clone_display().is_item_container());
-        }
+        blockify_if!(flags.contains(CascadeFlags::IS_ROOT_ELEMENT));
+        blockify_if!(
+            !self.skip_item_based_display_fixup() &&
+            layout_parent_style.get_box().clone_display().is_item_container()
+        );
 
         let is_item_or_root = blockify;
 
@@ -447,9 +454,7 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
     fn adjust_for_ruby(
         &mut self,
         layout_parent_style: &ComputedValues,
-        flags: CascadeFlags,
     ) {
-        use properties::CascadeFlags;
         use properties::computed_value_flags::ComputedValueFlags;
         use properties::longhands::unicode_bidi::computed_value::T as UnicodeBidi;
 
@@ -458,7 +463,7 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
         if self.should_suppress_linebreak(layout_parent_style) {
             self.style.flags.insert(ComputedValueFlags::SHOULD_SUPPRESS_LINEBREAK);
             // Inlinify the display type if allowed.
-            if !flags.contains(CascadeFlags::SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP) {
+            if !self.skip_item_based_display_fixup() {
                 let inline_display = self_display.inlinify();
                 if self_display != inline_display {
                     self.style.mutate_box().set_display(inline_display);
@@ -594,7 +599,7 @@ impl<'a, 'b: 'a> StyleAdjuster<'a, 'b> {
         self.adjust_for_text_decoration_lines(layout_parent_style);
         #[cfg(feature = "gecko")]
         {
-            self.adjust_for_ruby(layout_parent_style, flags);
+            self.adjust_for_ruby(layout_parent_style);
         }
         self.set_bits();
     }

servo/components/style/style_resolver.rs

@@ -542,11 +542,6 @@ where
 
         let mut cascade_flags = CascadeFlags::empty();
 
-        if self.element.skip_root_and_item_based_display_fixup() ||
-           pseudo.map_or(false, |p| p.skip_item_based_display_fixup()) {
-            cascade_flags.insert(CascadeFlags::SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP);
-        }
-
         if pseudo.is_none() && self.element.is_link() {
             cascade_flags.insert(CascadeFlags::IS_LINK);
             if self.element.is_visited_link() &&

servo/ports/geckolib/glue.rs

@@ -2019,12 +2019,11 @@ pub extern "C" fn Servo_ComputedValues_GetForAnonymousBox(parent_style_or_null:
         page_decls,
     );
 
-    let cascade_flags = CascadeFlags::SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP;
     data.stylist.precomputed_values_for_pseudo_with_rule_node(
         &guards,
         &pseudo,
         parent_style_or_null.map(|x| &*x),
-        cascade_flags,
+        CascadeFlags::empty(),
         &metrics,
         rule_node
     ).into()
@@ -3620,10 +3619,17 @@ pub extern "C" fn Servo_ReparentStyle(
     let element = element.map(GeckoElement);
 
     let mut cascade_flags = CascadeFlags::empty();
-    if style_to_reparent.is_anon_box() {
-        cascade_flags.insert(CascadeFlags::SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP);
-    }
     if let Some(element) = element {
+        // NOTE(emilio): This relies on element.is_some() => pseudo.is_none(),
+        // which the caller guarantees, fortunately. But this doesn't handle the
+        // IS_ROOT_ELEMENT flag correctly!
+        //
+        // I think it's not possible to wrap a root element in a first-line
+        // frame (and reparenting only happens for ::first-line and its
+        // descendants), so this may be fine...
+        //
+        // We should just get rid of all these flags which pass element / pseudo
+        // state.
         if element.is_link() {
             cascade_flags.insert(CascadeFlags::IS_LINK);
             if element.is_visited_link() && doc_data.visited_styles_enabled() {