servo: Merge #14471 - adjust display style fixup to handle more Gecko cases (from heycam:blockification); r=emilio
authorCameron McCormack <cam@mcc.id.au>
Tue, 06 Dec 2016 20:19:21 -0800
changeset 340290 1ba608da113c7639625000bbb9470c39f6228839
parent 340289 808bf2778f20f6d86934b70d8dffdb4d763f35d4
child 340291 03dbb0844fe988ef837223f2691ccae7e526debe
push id31307
push usergszorc@mozilla.com
push dateSat, 04 Feb 2017 00:59:06 +0000
treeherdermozilla-central@94079d43835f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersemilio
servo: Merge #14471 - adjust display style fixup to handle more Gecko cases (from heycam:blockification); r=emilio <!-- Please describe your changes on the following line: --> This tweaks the display property fixup we do when restyling to: * handle the display values that Gecko supports that Servo doesn't * blockify grid items (like we do flex items) * skip the fixup for NAC And while I'm in the area, this sets `nsStyleDisplay::mOriginalDisplay` too. r? @bholley cc @SimonSapin --- <!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: --> - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [ ] These changes fix #__ (github issue number if applicable). <!-- Either: --> - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ <!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. --> Source-Repo: https://github.com/servo/servo Source-Revision: b5a96e6054e0669d8891847fa5f806078ffb2aa6
servo/components/script/layout_wrapper.rs
servo/components/style/dom.rs
servo/components/style/gecko/wrapper.rs
servo/components/style/matching.rs
servo/components/style/properties/gecko.mako.rs
servo/components/style/properties/properties.mako.rs
--- a/servo/components/script/layout_wrapper.rs
+++ b/servo/components/script/layout_wrapper.rs
@@ -472,16 +472,20 @@ impl<'le> TElement for ServoLayoutElemen
         unsafe {
             self.get_style_and_layout_data().map(|d| {
                 let ppld: &AtomicRefCell<PartialPersistentLayoutData> = &**d.ptr;
                 let psd: &AtomicRefCell<ElementData> = transmute(ppld);
                 psd
             })
         }
     }
+
+    fn skip_root_and_item_based_display_fixup(&self) -> bool {
+        false
+    }
 }
 
 impl<'le> PartialEq for ServoLayoutElement<'le> {
     fn eq(&self, other: &Self) -> bool {
         self.as_node() == other.as_node()
     }
 }
 
--- a/servo/components/style/dom.rs
+++ b/servo/components/style/dom.rs
@@ -264,9 +264,14 @@ pub trait TElement : PartialEq + Debug +
     fn borrow_data(&self) -> Option<AtomicRef<ElementData>> {
         self.get_data().map(|x| x.borrow())
     }
 
     /// Mutably borrows the ElementData.
     fn mutate_data(&self) -> Option<AtomicRefMut<ElementData>> {
         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;
 }
--- a/servo/components/style/gecko/wrapper.rs
+++ b/servo/components/style/gecko/wrapper.rs
@@ -366,16 +366,25 @@ impl<'le> TElement for GeckoElement<'le>
 
     fn did_process_child(&self) -> isize {
         panic!("Atomic child count not implemented in Gecko");
     }
 
     fn get_data(&self) -> Option<&AtomicRefCell<ElementData>> {
         unsafe { self.0.mServoData.get().as_ref() }
     }
+
+    fn skip_root_and_item_based_display_fixup(&self) -> bool {
+        // We don't want to fix up display values of native anonymous content.
+        // Additionally, we want to skip root-based display fixup for document
+        // level native anonymous content subtree roots, since they're not
+        // really roots from the style fixup perspective.  Checking that we
+        // are NAC handles both cases.
+        self.flags() & (NODE_IS_IN_NATIVE_ANONYMOUS_SUBTREE as u32) != 0
+    }
 }
 
 impl<'le> PartialEq for GeckoElement<'le> {
     fn eq(&self, other: &Self) -> bool {
         self.0 as *const _ == other.0 as *const _
     }
 }
 
--- a/servo/components/style/matching.rs
+++ b/servo/components/style/matching.rs
@@ -9,17 +9,17 @@
 use {Atom, LocalName};
 use animation::{self, Animation, PropertyAnimation};
 use atomic_refcell::AtomicRefMut;
 use cache::LRUCache;
 use cascade_info::CascadeInfo;
 use context::{SharedStyleContext, StyleContext};
 use data::{ComputedStyle, ElementData, ElementStyles, PseudoStyles};
 use dom::{TElement, TNode, TRestyleDamage, UnsafeNode};
-use properties::{CascadeFlags, ComputedValues, SHAREABLE, cascade};
+use properties::{CascadeFlags, ComputedValues, SHAREABLE, SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP, cascade};
 use properties::longhands::display::computed_value as display;
 use rule_tree::StrongRuleNode;
 use selector_parser::{PseudoElement, RestyleDamage, SelectorImpl};
 use selectors::MatchAttr;
 use selectors::bloom::BloomFilter;
 use selectors::matching::{AFFECTED_BY_PSEUDO_ELEMENTS, MatchingReason, StyleRelations};
 use sink::ForgetfulSink;
 use std::collections::HashMap;
@@ -400,16 +400,19 @@ trait PrivateMatchMethods: TElement {
                                             -> Arc<ComputedValues>
                                             where Ctx: StyleContext<'a> {
         let shared_context = context.shared_context();
         let mut cascade_info = CascadeInfo::new();
         let mut cascade_flags = CascadeFlags::empty();
         if booleans.shareable {
             cascade_flags.insert(SHAREABLE)
         }
+        if self.skip_root_and_item_based_display_fixup() {
+            cascade_flags.insert(SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP)
+        }
 
         let this_style = match parent_style {
             Some(ref parent_style) => {
                 cascade(shared_context.viewport_size,
                         rule_node,
                         Some(&***parent_style),
                         Some(&mut cascade_info),
                         shared_context.error_reporter.clone(),
--- a/servo/components/style/properties/gecko.mako.rs
+++ b/servo/components/style/properties/gecko.mako.rs
@@ -1030,17 +1030,34 @@ fn static_assert() {
                                             "table-column table-cell table-caption list-item flex none " +
                                             "inline-flex grid inline-grid ruby ruby-base ruby-base-container " +
                                             "ruby-text ruby-text-container contents -webkit-box -webkit-inline-box " +
                                             "-moz-box -moz-inline-box -moz-grid -moz-inline-grid -moz-grid-group " +
                                             "-moz-grid-line -moz-stack -moz-inline-stack -moz-deck -moz-popup " +
                                             "-moz-groupbox",
                                             gecko_enum_prefix="StyleDisplay",
                                             gecko_strip_moz_prefix=False) %>
-    ${impl_keyword('display', 'mDisplay', display_keyword, True)}
+
+    pub fn set_display(&mut self, v: longhands::display::computed_value::T) {
+        use properties::longhands::display::computed_value::T as Keyword;
+        // FIXME(bholley): Align binary representations and ditch |match| for cast + static_asserts
+        let result = match v {
+            % for value in display_keyword.values_for('gecko'):
+                Keyword::${to_rust_ident(value)} =>
+                    structs::${display_keyword.gecko_constant(value)},
+            % endfor
+        };
+        self.gecko.mDisplay = result;
+        self.gecko.mOriginalDisplay = result;
+    }
+    pub fn copy_display_from(&mut self, other: &Self) {
+        self.gecko.mDisplay = other.gecko.mDisplay;
+        self.gecko.mOriginalDisplay = other.gecko.mOriginalDisplay;
+    }
+    <%call expr="impl_keyword_clone('display', 'mDisplay', display_keyword)"></%call>
 
     // overflow-y is implemented as a newtype of overflow-x, so we need special handling.
     // We could generalize this if we run into other newtype keywords.
     <% overflow_x = data.longhands_by_name["overflow-x"] %>
     pub fn set_overflow_y(&mut self, v: longhands::overflow_y::computed_value::T) {
         use properties::longhands::overflow_x::computed_value::T as BaseType;
         // FIXME(bholley): Align binary representations and ditch |match| for cast + static_asserts
         self.gecko.mOverflowY = match v.0 {
--- a/servo/components/style/properties/properties.mako.rs
+++ b/servo/components/style/properties/properties.mako.rs
@@ -1431,16 +1431,18 @@ static CASCADE_PROPERTY: [CascadePropert
 bitflags! {
     pub flags CascadeFlags: u8 {
         /// Whether the `ComputedValues` structure to be constructed should be considered
         /// shareable.
         const SHAREABLE = 0x01,
         /// 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 = 0x02,
+        /// Whether to skip any root element and flex/grid item display style fixup.
+        const SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP = 0x04,
     }
 }
 
 /// Performs the CSS cascade, computing new styles for an element from its parent style.
 ///
 /// The arguments are:
 ///
 ///   * `viewport_size`: The size of the initial viewport.
@@ -1625,40 +1627,60 @@ pub fn apply_declarations<'a, F, I>(view
     });
 
     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::SpecifiedValue::none;
-    let is_flex_item =
-        context.inherited_style.get_box().clone_display() == computed_values::display::T::flex;
-    if positioned || floated || is_root_element || is_flex_item {
+    // 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(),
+        % if product == "gecko":
+        computed_values::display::T::grid |
+        % endif
+        computed_values::display::T::flex);
+    let (blockify_root, blockify_item) = match flags.contains(SKIP_ROOT_AND_ITEM_BASED_DISPLAY_FIXUP) {
+        false => (is_root_element, is_item),
+        true => (false, false),
+    };
+    if positioned || floated || blockify_root || blockify_item {
         use computed_values::display::T;
 
         let specified_display = style.get_box().clone_display();
         let computed_display = match specified_display {
-            T::inline_table => {
-                Some(T::table)
-            }
-            T::inline | T::inline_block |
-            T::table_row_group | T::table_column |
-            T::table_column_group | T::table_header_group |
-            T::table_footer_group | T::table_row | T::table_cell |
-            T::table_caption => {
-                Some(T::block)
-            }
-            _ => None
+            // Values that have a corresponding block-outside version.
+            T::inline_table => Some(T::table),
+            % if product == "gecko":
+            T::inline_flex => Some(T::flex),
+            T::inline_grid => Some(T::grid),
+            T::_webkit_inline_box => Some(T::_webkit_box),
+            % endif
+
+            // Special handling for contents and list-item on the root element for Gecko.
+            % if product == "gecko":
+            T::contents | T::list_item if blockify_root => Some(T::block),
+            % endif
+
+            // Values that are not changed by blockification.
+            T::block | T::flex | T::list_item | T::table => None,
+            % if product == "gecko":
+            T::contents | T::grid | T::_webkit_box => None,
+            % endif
+
+            // Everything becomes block.
+            _ => Some(T::block),
         };
         if let Some(computed_display) = computed_display {
             let box_ = style.mutate_box();
             box_.set_display(computed_display);
             % if product == "servo":
-                box_.set__servo_display_for_hypothetical_box(if is_root_element || is_flex_item {
+                box_.set__servo_display_for_hypothetical_box(if blockify_root || blockify_item {
                     computed_display
                 } else {
                     specified_display
                 });
             % endif
         }
     }