Auto merge of #7587 - mrobinson:layer-id, r=pcwalton

Ensure unique LayerIds for pseudo-elements

Currently pseudo-elements, like the fragments created for ::before and
::after, with layers will have the same LayerId as the body of their
owning fragments. Instead all LayerIds should be unique.

Fixes #2010.

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7587)
<!-- Reviewable:end -->

components/gfx/display_list/mod.rs

@@ -692,7 +692,7 @@ impl StackingContextLayerCreator {
     fn finish_building_current_layer(&mut self, stacking_context: &mut StackingContext) {
         if let Some(display_list) = self.display_list_for_next_layer.take() {
             let next_layer_id =
-                    stacking_context.display_list.layered_children.back().unwrap().id.next_layer_id();
+                stacking_context.display_list.layered_children.back().unwrap().id.companion_layer_id();
             let child_stacking_context =
                 Arc::new(stacking_context.create_layered_child(next_layer_id, display_list));
             stacking_context.display_list.layered_children.push_back(

components/layout/block.rs

@@ -48,10 +48,11 @@ use layout_debug;
 use layout_task::DISPLAY_PORT_SIZE_FACTOR;
 use model::{IntrinsicISizes, MarginCollapseInfo};
 use model::{MaybeAuto, CollapsibleMargins, specified, specified_or_none};
+use wrapper::PseudoElementType;
 
 use euclid::{Point2D, Rect, Size2D};
 use gfx::display_list::{ClippingRegion, DisplayList};
-use msg::compositor_msg::LayerId;
+use msg::compositor_msg::{LayerId, LayerType};
 use rustc_serialize::{Encoder, Encodable};
 use std::cmp::{max, min};
 use std::fmt;
@@ -1977,7 +1978,7 @@ impl Flow for BlockFlow {
         let stacking_relative_position_of_display_port_for_children =
             if is_stacking_context || self.is_root() {
                 let visible_rect =
-                    match layout_context.shared.visible_rects.get(&self.layer_id(0)) {
+                    match layout_context.shared.visible_rects.get(&self.layer_id()) {
                         Some(visible_rect) => *visible_rect,
                         None => Rect::new(Point2D::zero(), layout_context.shared.screen_size),
                     };
@@ -2075,11 +2076,17 @@ impl Flow for BlockFlow {
         (self.fragment.border_box - self.fragment.style().logical_border_width()).size
     }
 
-    fn layer_id(&self, fragment_index: u32) -> LayerId {
-        // FIXME(#2010, pcwalton): This is a hack and is totally bogus in the presence of pseudo-
-        // elements. But until we have incremental reflow we can't do better--we recreate the flow
-        // for every DOM node so otherwise we nuke layers on every reflow.
-        LayerId(self.fragment.node.id() as usize, fragment_index, 0)
+    fn layer_id(&self) -> LayerId {
+        let layer_type = match self.fragment.pseudo {
+            PseudoElementType::Normal => LayerType::FragmentBody,
+            PseudoElementType::Before(_) => LayerType::BeforePseudoContent,
+            PseudoElementType::After(_) => LayerType::AfterPseudoContent
+        };
+        LayerId::new_of_type(layer_type, self.fragment.node.id() as usize)
+    }
+
+    fn layer_id_for_overflow_scroll(&self) -> LayerId {
+        LayerId::new_of_type(LayerType::OverflowScroll, self.fragment.node.id() as usize)
     }
 
     fn is_absolute_containing_block(&self) -> bool {

components/layout/display_list_builder.rs

@@ -66,11 +66,6 @@ use util::logical_geometry::{LogicalPoint, LogicalRect, LogicalSize, WritingMode
 use util::opts;
 use util::range::Range;
 
-/// The fake fragment ID we use to indicate the inner display list for `overflow: scroll`.
-///
-/// FIXME(pcwalton): This is pretty ugly. Consider modifying `LayerId` somehow.
-const FAKE_FRAGMENT_ID_FOR_OVERFLOW_SCROLL: u32 = 1000000;
-
 /// The logical width of an insertion point: at the moment, a one-pixel-wide line.
 const INSERTION_POINT_LOGICAL_WIDTH: Au = Au(1 * AU_PER_PX);
 
@@ -1641,7 +1636,7 @@ impl BlockFlowDisplayListBuilding for BlockFlow {
                 &self.base,
                 display_list,
                 layout_context,
-                StackingContextLayerNecessity::Always(self.layer_id(0), scroll_policy),
+                StackingContextLayerNecessity::Always(self.layer_id(), scroll_policy),
                 StackingContextCreationMode::Normal);
             DisplayListBuildingResult::StackingContext(stacking_context)
         } else if self.fragment.establishes_stacking_context() {
@@ -1650,7 +1645,7 @@ impl BlockFlowDisplayListBuilding for BlockFlow {
                     &self.base,
                     display_list,
                     layout_context,
-                    StackingContextLayerNecessity::IfCanvas(self.layer_id(0)),
+                    StackingContextLayerNecessity::IfCanvas(self.layer_id()),
                     StackingContextCreationMode::Normal))
         } else {
             match self.fragment.style.get_box().position {
@@ -1727,7 +1722,7 @@ impl BlockFlowDisplayListBuilding for BlockFlow {
                             &self.base,
                             display_list,
                             layout_context,
-                            StackingContextLayerNecessity::IfCanvas(self.layer_id(0)),
+                            StackingContextLayerNecessity::IfCanvas(self.layer_id()),
                             StackingContextCreationMode::Normal));
             }
             return
@@ -1747,9 +1742,9 @@ impl BlockFlowDisplayListBuilding for BlockFlow {
         };
 
         let layer_id = if outer_display_list_for_overflow_scroll.is_some() {
-            self.layer_id(FAKE_FRAGMENT_ID_FOR_OVERFLOW_SCROLL)
+            self.layer_id_for_overflow_scroll()
         } else {
-            self.layer_id(0)
+            self.layer_id()
         };
         let stacking_context = self.fragment.create_stacking_context(
             &self.base,
@@ -1766,7 +1761,7 @@ impl BlockFlowDisplayListBuilding for BlockFlow {
                     &self.base,
                     outer_display_list_for_overflow_scroll,
                     layout_context,
-                    StackingContextLayerNecessity::Always(self.layer_id(0), scroll_policy),
+                    StackingContextLayerNecessity::Always(self.layer_id(), scroll_policy),
                     StackingContextCreationMode::OuterScrollWrapper)
             }
             None => stacking_context,
@@ -1792,7 +1787,7 @@ impl BlockFlowDisplayListBuilding for BlockFlow {
                     &self.base,
                     display_list,
                     layout_context,
-                    StackingContextLayerNecessity::IfCanvas(self.layer_id(0)),
+                    StackingContextLayerNecessity::IfCanvas(self.layer_id()),
                     StackingContextCreationMode::Normal))
         } else {
             DisplayListBuildingResult::Normal(display_list)
@@ -1892,7 +1887,7 @@ impl InlineFlowDisplayListBuilding for InlineFlow {
                     &self.base,
                     display_list,
                     layout_context,
-                    StackingContextLayerNecessity::IfCanvas(self.layer_id(0)),
+                    StackingContextLayerNecessity::IfCanvas(self.layer_id()),
                     StackingContextCreationMode::Normal))
         } else {
             DisplayListBuildingResult::Normal(display_list)

components/layout/flow.rs

@@ -48,7 +48,7 @@ use wrapper::{PseudoElementType, ThreadSafeLayoutNode};
 
 use euclid::{Point2D, Rect, Size2D};
 use gfx::display_list::ClippingRegion;
-use msg::compositor_msg::LayerId;
+use msg::compositor_msg::{LayerId, LayerType};
 use msg::constellation_msg::ConstellationChan;
 use rustc_serialize::{Encoder, Encodable};
 use std::fmt;
@@ -359,9 +359,16 @@ pub trait Flow: fmt::Debug + Sync + Send + 'static {
 
     /// Returns a layer ID for the given fragment.
     #[allow(unsafe_code)]
-    fn layer_id(&self, fragment_id: u32) -> LayerId {
+    fn layer_id(&self) -> LayerId {
         let obj = unsafe { mem::transmute::<&&Self, &raw::TraitObject>(&self) };
-        LayerId(obj.data as usize, fragment_id, 0)
+        LayerId::new_of_type(LayerType::FragmentBody, obj.data as usize)
+    }
+
+    /// Returns a layer ID for the given fragment.
+    #[allow(unsafe_code)]
+    fn layer_id_for_overflow_scroll(&self) -> LayerId {
+        let obj = unsafe { mem::transmute::<&&Self, &raw::TraitObject>(&self) };
+        LayerId::new_of_type(LayerType::OverflowScroll, obj.data as usize)
     }
 
     /// Attempts to perform incremental fixup of this flow by replacing its fragment's style with

components/layout/layout_task.rs

@@ -1057,7 +1057,7 @@ impl LayoutTask {
                     .display_list_building_result
                     .add_to(&mut *display_list);
                 let origin = Rect::new(Point2D::new(Au(0), Au(0)), root_size);
-                let layer_id = layout_root.layer_id(0);
+                let layer_id = layout_root.layer_id();
                 let stacking_context = Arc::new(StackingContext::new(display_list,
                                                                      &origin,
                                                                      &origin,

components/msg/compositor_msg.rs

@@ -35,36 +35,63 @@ impl FrameTreeId {
     }
 }
 
+#[derive(Clone, PartialEq, Eq, Copy, Hash, Deserialize, Serialize, HeapSizeOf)]
+pub enum LayerType {
+    /// A layer for the fragment body itself.
+    FragmentBody,
+    /// An extra layer created for a DOM fragments with overflow:scroll.
+    OverflowScroll,
+    /// A layer created to contain ::before pseudo-element content.
+    BeforePseudoContent,
+    /// A layer created to contain ::after pseudo-element content.
+    AfterPseudoContent,
+}
+
 #[derive(Clone, PartialEq, Eq, Copy, Hash, Deserialize, Serialize, HeapSizeOf)]
 pub struct LayerId(
-    /// A base layer ID, currently derived from DOM element pointer address.
-    pub usize,
-
-    /// FIXME(#2010, pcwalton): A marker for overflow scroll layers.
-    pub u32,
-
-    /// A sub ID, which is used for synthesizing new layers for content that
-    /// belongs on top of this layer. This prevents accidentally making colliding
-    /// layer ids.
-    pub u32
+    /// The type of the layer. This serves to differentiate layers that share fragments.
+    LayerType,
+    /// The identifier for this layer's fragment, derived from the fragment memory address.
+    usize,
+    /// Whether or not this layer is a companion layer, synthesized to ensure that
+    /// content on top of this layer's fragment has the proper rendering order.
+    bool
 );
 
 impl Debug for LayerId {
     fn fmt(&self, f: &mut Formatter) -> fmt::Result {
-        let LayerId(a, b, c) = *self;
-        write!(f, "Layer({}, {}, {})", a, b, c)
+        let LayerId(layer_type, id, companion) = *self;
+        let type_string = match layer_type {
+            LayerType::FragmentBody => "-FragmentBody",
+            LayerType::OverflowScroll => "-OverflowScroll",
+            LayerType::BeforePseudoContent => "-BeforePseudoContent",
+            LayerType::AfterPseudoContent => "-AfterPseudoContent",
+        };
+
+        let companion_string = if companion {
+            "-companion"
+        } else {
+            ""
+        };
+
+        write!(f, "{}{}{}", id, type_string, companion_string)
     }
 }
 
 impl LayerId {
     /// FIXME(#2011, pcwalton): This is unfortunate. Maybe remove this in the future.
     pub fn null() -> LayerId {
-        LayerId(0, 0, 0)
+        LayerId(LayerType::FragmentBody, 0, false)
     }
 
-    pub fn next_layer_id(&self) -> LayerId {
-        let LayerId(a, b, sub_id) = *self;
-        LayerId(a, b, sub_id + 1)
+    pub fn new_of_type(layer_type: LayerType, fragment_id: usize) -> LayerId {
+        LayerId(layer_type, fragment_id, false)
+    }
+
+    pub fn companion_layer_id(&self) -> LayerId {
+        let LayerId(layer_type, id, companion) = *self;
+        assert!(!companion);
+        LayerId(layer_type, id, true)
     }
 }
 

tests/ref/basic.list

@@ -312,6 +312,7 @@ flaky_cpu == linebreak_simple_a.html linebreak_simple_b.html
 == position_relative_vertical_percentage_overflow_a.html position_relative_vertical_percentage_overflow_ref.html
 == pre_ignorable_whitespace_a.html pre_ignorable_whitespace_ref.html
 == pre_with_tab.html pre_with_tab_ref.html
+== pseudo_content_with_layers.html pseudo_content_with_layers_ref.html
 == pseudo_element_a.html pseudo_element_b.html
 == pseudo_inherit.html pseudo_inherit_ref.html
 == quotes_none_a.html quotes_none_ref.html

tests/ref/pseudo_content_with_layers.html

@@ -0,0 +1,32 @@
+<!DOCTYPE html>
+<html>
+    <body>
+        <style>
+        .before-test::before { 
+            content: " ";
+            position: fixed;
+            width: 100px;
+            height: 100px;
+            background: green;
+        }
+
+        .after-test::after { 
+            content: " ";
+            position: fixed;
+            width: 100px;
+            height: 100px;
+            background: green;
+        }
+
+        div {
+            position: fixed;
+            width: 110px;
+            height: 110px;
+            background: blue;
+        }
+        </style>
+
+        <div class="before-test"> </div>
+        <div style="left: 150px;" class="after-test"> </div>
+    </body>
+</html>

tests/ref/pseudo_content_with_layers_ref.html

@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+<html>
+    <body>
+        <style>
+        div {
+            position: absolute;
+            width: 100px;
+            height: 100px;
+            background: green;
+            border-right: 10px blue solid;
+            border-bottom: 10px blue solid;
+        }
+        </style>
+
+        <div> </div>
+        <div style="left: 150px;"> </div>
+    </body>
+</html>