Stop generating flows under display: none.

Because this is a bottom-up traversal it can generates flows and throw them away. To prevent that, this cascades an internal `-servo-under-display-none` property and then checks that during flow construction.  Fixes #1536.

5 files changed, 50 insertions, 10 deletions

diff --git a/components/layout/construct.rs b/components/layout/construct.rs
index 2a75c4025cb..77b388e8ade 100644
--- a/components/layout/construct.rs
+++ b/components/layout/construct.rs
@@ -1501,7 +1501,15 @@ impl<'a, ConcreteThreadSafeLayoutNode> PostorderNodeMutTraversal<ConcreteThreadS
     //
     // TODO: This should actually consult the table in that section to get the
     // final computed value for 'display'.
-    fn process(&mut self, node: &ConcreteThreadSafeLayoutNode) -> bool {
+    fn process(&mut self, node: &ConcreteThreadSafeLayoutNode) {
+        node.insert_flags(HAS_NEWLY_CONSTRUCTED_FLOW);
+
+        // Bail out if this node has an ancestor with display: none.
+        if node.style(self.style_context()).get_inheritedbox()._servo_under_display_none.0 {
+            self.set_flow_construction_result(node, ConstructionResult::None);
+            return;
+        }
+
         // Get the `display` property for this node, and determine whether this node is floated.
         let (display, float, positioning) = match node.type_id() {
             None => {
@@ -1542,12 +1550,8 @@ impl<'a, ConcreteThreadSafeLayoutNode> PostorderNodeMutTraversal<ConcreteThreadS
 
         // Switch on display and floatedness.
         match (display, float, positioning) {
-            // `display: none` contributes no flow construction result. Nuke the flow construction
-            // results of children.
+            // `display: none` contributes no flow construction result.
             (display::T::none, _, _) => {
-                for child in node.children() {
-                    self.set_flow_construction_result(&child, ConstructionResult::None);
-                }
                 self.set_flow_construction_result(node, ConstructionResult::None);
             }
 
@@ -1654,9 +1658,6 @@ impl<'a, ConcreteThreadSafeLayoutNode> PostorderNodeMutTraversal<ConcreteThreadS
                 self.set_flow_construction_result(node, construction_result)
             }
         }
-
-        node.insert_flags(HAS_NEWLY_CONSTRUCTED_FLOW);
-        true
     }
 }
 
diff --git a/components/layout/incremental.rs b/components/layout/incremental.rs
index 9670028cfa3..e0c5d804295 100644
--- a/components/layout/incremental.rs
+++ b/components/layout/incremental.rs
@@ -178,6 +178,7 @@ pub fn compute_damage(old: Option<&Arc<ServoComputedValues>>, new: &ServoCompute
                       ], [
         get_box.float, get_box.display, get_box.position, get_counters.content,
         get_counters.counter_reset, get_counters.counter_increment,
+        get_inheritedbox._servo_under_display_none,
         get_list.quotes, get_list.list_style_type,
 
         // If these text or font properties change, we need to reconstruct the flow so that
diff --git a/components/layout/traversal.rs b/components/layout/traversal.rs
index 1fe5388549e..692d956f000 100644
--- a/components/layout/traversal.rs
+++ b/components/layout/traversal.rs
@@ -72,7 +72,7 @@ impl<'lc, 'ln> DomTraversalContext<ServoLayoutNode<'ln>> for RecalcStyleAndConst
 /// A bottom-up, parallelizable traversal.
 pub trait PostorderNodeMutTraversal<ConcreteThreadSafeLayoutNode: ThreadSafeLayoutNode> {
     /// The operation to perform. Return true to continue or false to stop.
-    fn process(&mut self, node: &ConcreteThreadSafeLayoutNode) -> bool;
+    fn process(&mut self, node: &ConcreteThreadSafeLayoutNode);
 }
 
 /// The flow construction traversal, which builds flows for styled nodes.
diff --git a/components/style/properties/longhand/box.mako.rs b/components/style/properties/longhand/box.mako.rs
index f80a42c88d2..aa0b416824d 100644
--- a/components/style/properties/longhand/box.mako.rs
+++ b/components/style/properties/longhand/box.mako.rs
@@ -78,6 +78,7 @@
                                    _error_reporter: &mut StdBox<ParseErrorReporter + Send>) {
             longhands::_servo_display_for_hypothetical_box::derive_from_display(context);
             longhands::_servo_text_decorations_in_effect::derive_from_display(context);
+            longhands::_servo_under_display_none::derive_from_display(context);
         }
     % endif
 
diff --git a/components/style/properties/longhand/inherited_box.mako.rs b/components/style/properties/longhand/inherited_box.mako.rs
index 6dc6f05c2e7..655d9dac674 100644
--- a/components/style/properties/longhand/inherited_box.mako.rs
+++ b/components/style/properties/longhand/inherited_box.mako.rs
@@ -88,3 +88,40 @@ ${helpers.single_keyword("color-adjust", "economy exact", products="gecko")}
         }
     }
 </%helpers:longhand>
+
+// Used in the bottom-up flow construction traversal to avoid constructing flows for
+// descendants of nodes with `display: none`.
+<%helpers:longhand name="-servo-under-display-none" derived_from="display" products="servo">
+    use cssparser::ToCss;
+    use std::fmt;
+    use values::computed::ComputedValueAsSpecified;
+
+    #[derive(Copy, Clone, Debug, Eq, PartialEq, HeapSizeOf, Serialize, Deserialize)]
+    pub struct SpecifiedValue(pub bool);
+
+    pub mod computed_value {
+        pub type T = super::SpecifiedValue;
+    }
+    impl ComputedValueAsSpecified for SpecifiedValue {}
+
+    pub fn get_initial_value() -> computed_value::T {
+        SpecifiedValue(false)
+    }
+
+    impl ToCss for SpecifiedValue {
+        fn to_css<W>(&self, _: &mut W) -> fmt::Result where W: fmt::Write {
+            Ok(()) // Internal property
+        }
+    }
+
+    #[inline]
+    pub fn derive_from_display<Cx: TContext>(context: &mut Cx) {
+        use properties::style_struct_traits::Box;
+        use super::display::computed_value::T as Display;
+
+        if context.style().get_box().clone_display() == Display::none {
+            context.mutate_style().mutate_inheritedbox()
+                                  .set__servo_under_display_none(SpecifiedValue(true));
+        }
+    }
+</%helpers:longhand>