Z index with flicker fix (#4170)

Julia created

Part of `was_top_layer` was checking if a opaque level starts with the
current stacking order, but now that each entry in the order has an id,
those comparison checks were failing as every entry generated is now
unique

I'll admit I'm still a little fuzzy on the specific reasons for this
function to be defined as it is so I'd appreciate another set of eyes

Release Notes:

- N/A

Change summary

crates/collab_ui/src/face_pile.rs       |  2 
crates/editor/src/element.rs            |  2 
crates/gpui/src/style.rs                |  8 +-
crates/gpui/src/styled.rs               |  2 
crates/gpui/src/view.rs                 |  5 +
crates/gpui/src/window.rs               | 73 +++++++++++++++++---------
crates/gpui/src/window/element_cx.rs    | 47 +++++++++-------
crates/storybook/src/stories/z_index.rs |  4 
crates/ui/src/styles/elevation.rs       |  2 
crates/workspace/src/workspace.rs       |  2 
10 files changed, 88 insertions(+), 59 deletions(-)

Detailed changes

crates/collab_ui/src/face_pile.rs 🔗

@@ -28,7 +28,7 @@ impl RenderOnce for FacePile {
             let isnt_last = ix < player_count - 1;
 
             div()
-                .z_index((player_count - ix) as u8)
+                .z_index((player_count - ix) as u16)
                 .when(isnt_last, |div| div.neg_mr_1())
                 .child(player)
         });

crates/editor/src/element.rs 🔗

@@ -3169,7 +3169,7 @@ pub struct CursorName {
     string: SharedString,
     color: Hsla,
     is_top_row: bool,
-    z_index: u8,
+    z_index: u16,
 }
 
 impl Cursor {

crates/gpui/src/style.rs 🔗

@@ -110,7 +110,7 @@ pub struct Style {
     /// The mouse cursor style shown when the mouse pointer is over an element.
     pub mouse_cursor: Option<CursorStyle>,
 
-    pub z_index: Option<u8>,
+    pub z_index: Option<u16>,
 
     #[cfg(debug_assertions)]
     pub debug: bool,
@@ -337,7 +337,7 @@ impl Style {
 
         let background_color = self.background.as_ref().and_then(Fill::color);
         if background_color.map_or(false, |color| !color.is_transparent()) {
-            cx.with_z_index(0, |cx| {
+            cx.with_z_index(1, |cx| {
                 let mut border_color = background_color.unwrap_or_default();
                 border_color.a = 0.;
                 cx.paint_quad(quad(
@@ -350,12 +350,12 @@ impl Style {
             });
         }
 
-        cx.with_z_index(0, |cx| {
+        cx.with_z_index(2, |cx| {
             continuation(cx);
         });
 
         if self.is_border_visible() {
-            cx.with_z_index(0, |cx| {
+            cx.with_z_index(3, |cx| {
                 let corner_radii = self.corner_radii.to_pixels(bounds.size, rem_size);
                 let border_widths = self.border_widths.to_pixels(rem_size);
                 let max_border_width = border_widths.max();

crates/gpui/src/styled.rs 🔗

@@ -12,7 +12,7 @@ pub trait Styled: Sized {
 
     gpui_macros::style_helpers!();
 
-    fn z_index(mut self, z_index: u8) -> Self {
+    fn z_index(mut self, z_index: u16) -> Self {
         self.style().z_index = Some(z_index);
         self
     }

crates/gpui/src/view.rs 🔗

@@ -23,6 +23,7 @@ impl<V> Sealed for View<V> {}
 #[doc(hidden)]
 pub struct AnyViewState {
     root_style: Style,
+    next_stacking_order_id: u16,
     cache_key: Option<ViewCacheKey>,
     element: Option<AnyElement>,
 }
@@ -292,6 +293,7 @@ impl Element for AnyView {
             let root_style = cx.layout_style(layout_id).unwrap().clone();
             let state = AnyViewState {
                 root_style,
+                next_stacking_order_id: 0,
                 cache_key: None,
                 element: Some(element),
             };
@@ -314,7 +316,7 @@ impl Element for AnyView {
                     && !cx.window.dirty_views.contains(&self.entity_id())
                     && !cx.window.refreshing
                 {
-                    cx.reuse_view();
+                    cx.reuse_view(state.next_stacking_order_id);
                     return;
                 }
             }
@@ -326,6 +328,7 @@ impl Element for AnyView {
                 element.draw(bounds.origin, bounds.size.into(), cx);
             }
 
+            state.next_stacking_order_id = cx.window.next_frame.next_stacking_order_id;
             state.cache_key = Some(ViewCacheKey {
                 bounds,
                 stacking_order: cx.stacking_order().clone(),

crates/gpui/src/window.rs 🔗

@@ -39,30 +39,23 @@ use util::{measure, ResultExt};
 mod element_cx;
 pub use element_cx::*;
 
-const ACTIVE_DRAG_Z_INDEX: u8 = 1;
+const ACTIVE_DRAG_Z_INDEX: u16 = 1;
 
 /// A global stacking order, which is created by stacking successive z-index values.
 /// Each z-index will always be interpreted in the context of its parent z-index.
-#[derive(Deref, DerefMut, Clone, Ord, PartialOrd, PartialEq, Eq, Default)]
-pub struct StackingOrder {
-    #[deref]
-    #[deref_mut]
-    context_stack: SmallVec<[u8; 64]>,
-    pub(crate) id: u32,
+#[derive(Debug, Deref, DerefMut, Clone, Ord, PartialOrd, PartialEq, Eq, Default)]
+pub struct StackingOrder(SmallVec<[StackingContext; 64]>);
+
+/// A single entry in a primitive's z-index stacking order
+#[derive(Clone, Ord, PartialOrd, PartialEq, Eq, Default)]
+pub struct StackingContext {
+    pub(crate) z_index: u16,
+    pub(crate) id: u16,
 }
 
-impl std::fmt::Debug for StackingOrder {
+impl std::fmt::Debug for StackingContext {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        let mut stacks = self.context_stack.iter().peekable();
-        write!(f, "[({}): ", self.id)?;
-        while let Some(z_index) = stacks.next() {
-            write!(f, "{z_index}")?;
-            if stacks.peek().is_some() {
-                write!(f, "->")?;
-            }
-        }
-        write!(f, "]")?;
-        Ok(())
+        write!(f, "{{{}.{}}} ", self.z_index, self.id)
     }
 }
 
@@ -807,18 +800,39 @@ impl<'a> WindowContext<'a> {
     }
 
     /// Returns true if there is no opaque layer containing the given point
-    /// on top of the given level. Layers whose level is an extension of the
-    /// level are not considered to be on top of the level.
-    pub fn was_top_layer(&self, point: &Point<Pixels>, level: &StackingOrder) -> bool {
-        for (opaque_level, _, bounds) in self.window.rendered_frame.depth_map.iter() {
-            if level >= opaque_level {
-                break;
+    /// on top of the given level. Layers who are extensions of the queried layer
+    /// are not considered to be on top of queried layer.
+    pub fn was_top_layer(&self, point: &Point<Pixels>, layer: &StackingOrder) -> bool {
+        // Precondition: the depth map is ordered from topmost to bottomost.
+
+        for (opaque_layer, _, bounds) in self.window.rendered_frame.depth_map.iter() {
+            if layer >= opaque_layer {
+                // The queried layer is either above or is the same as the this opaque layer.
+                // Anything after this point is guaranteed to be below the queried layer.
+                return true;
+            }
+
+            if !bounds.contains(point) {
+                // This opaque layer is above the queried layer but it doesn't contain
+                // the given position, so we can ignore it even if it's above.
+                continue;
             }
 
-            if bounds.contains(point) && !opaque_level.starts_with(level) {
+            // At this point, we've established that this opaque layer is on top of the queried layer
+            // and contains the position:
+            // - If the opaque layer is an extension of the queried layer, we don't want
+            // to consider the opaque layer to be on top and so we ignore it.
+            // - Else, we will bail early and say that the queried layer wasn't the top one.
+            let opaque_layer_is_extension_of_queried_layer = opaque_layer.len() >= layer.len()
+                && opaque_layer
+                    .iter()
+                    .zip(layer.iter())
+                    .all(|(a, b)| a.z_index == b.z_index);
+            if !opaque_layer_is_extension_of_queried_layer {
                 return false;
             }
         }
+
         true
     }
 
@@ -831,11 +845,16 @@ impl<'a> WindowContext<'a> {
             if level >= opaque_level {
                 break;
             }
-            if opaque_level.starts_with(&[ACTIVE_DRAG_Z_INDEX]) {
+
+            if opaque_level
+                .first()
+                .map(|c| c.z_index == ACTIVE_DRAG_Z_INDEX)
+                .unwrap_or(false)
+            {
                 continue;
             }
 
-            if bounds.contains(point) && !opaque_level.starts_with(level) {
+            if bounds.contains(point) {
                 return false;
             }
         }

crates/gpui/src/window/element_cx.rs 🔗

@@ -19,9 +19,9 @@ use crate::{
     EntityId, FocusHandle, FocusId, FontId, GlobalElementId, GlyphId, Hsla, ImageData,
     InputHandler, IsZero, KeyContext, KeyEvent, LayoutId, MonochromeSprite, MouseEvent, PaintQuad,
     Path, Pixels, PlatformInputHandler, Point, PolychromeSprite, Quad, RenderGlyphParams,
-    RenderImageParams, RenderSvgParams, Scene, Shadow, SharedString, Size, StackingOrder, Style,
-    Surface, TextStyleRefinement, Underline, UnderlineStyle, Window, WindowContext,
-    SUBPIXEL_VARIANTS,
+    RenderImageParams, RenderSvgParams, Scene, Shadow, SharedString, Size, StackingContext,
+    StackingOrder, Style, Surface, TextStyleRefinement, Underline, UnderlineStyle, Window,
+    WindowContext, SUBPIXEL_VARIANTS,
 };
 
 type AnyMouseListener = Box<dyn FnMut(&dyn Any, DispatchPhase, &mut ElementContext) + 'static>;
@@ -45,8 +45,8 @@ pub(crate) struct Frame {
     pub(crate) scene: Scene,
     pub(crate) depth_map: Vec<(StackingOrder, EntityId, Bounds<Pixels>)>,
     pub(crate) z_index_stack: StackingOrder,
-    pub(crate) next_stacking_order_id: u32,
-    pub(crate) next_root_z_index: u8,
+    pub(crate) next_stacking_order_id: u16,
+    pub(crate) next_root_z_index: u16,
     pub(crate) content_mask_stack: Vec<ContentMask<Pixels>>,
     pub(crate) element_offset_stack: Vec<Point<Pixels>>,
     pub(crate) requested_input_handler: Option<RequestedInputHandler>,
@@ -292,7 +292,7 @@ impl<'a> VisualContext for ElementContext<'a> {
 }
 
 impl<'a> ElementContext<'a> {
-    pub(crate) fn reuse_view(&mut self) {
+    pub(crate) fn reuse_view(&mut self, next_stacking_order_id: u16) {
         let view_id = self.parent_view_id();
         let grafted_view_ids = self
             .cx
@@ -333,6 +333,9 @@ impl<'a> ElementContext<'a> {
                 self.window.next_frame.requested_cursor_style = Some(style);
             }
         }
+
+        debug_assert!(next_stacking_order_id >= self.window.next_frame.next_stacking_order_id);
+        self.window.next_frame.next_stacking_order_id = next_stacking_order_id;
     }
 
     pub fn with_text_style<F, R>(&mut self, style: Option<TextStyleRefinement>, f: F) -> R
@@ -410,36 +413,40 @@ impl<'a> ElementContext<'a> {
                 size: self.window().viewport_size,
             },
         };
+
+        let new_root_z_index = post_inc(&mut self.window_mut().next_frame.next_root_z_index);
         let new_stacking_order_id =
             post_inc(&mut self.window_mut().next_frame.next_stacking_order_id);
-        let new_root_z_index = post_inc(&mut self.window_mut().next_frame.next_root_z_index);
+        let new_context = StackingContext {
+            z_index: new_root_z_index,
+            id: new_stacking_order_id,
+        };
+
         let old_stacking_order = mem::take(&mut self.window_mut().next_frame.z_index_stack);
-        self.window_mut().next_frame.z_index_stack.id = new_stacking_order_id;
-        self.window_mut()
-            .next_frame
-            .z_index_stack
-            .push(new_root_z_index);
+
+        self.window_mut().next_frame.z_index_stack.push(new_context);
         self.window_mut().next_frame.content_mask_stack.push(mask);
         let result = f(self);
         self.window_mut().next_frame.content_mask_stack.pop();
         self.window_mut().next_frame.z_index_stack = old_stacking_order;
+
         result
     }
 
     /// Called during painting to invoke the given closure in a new stacking context. The given
     /// z-index is interpreted relative to the previous call to `stack`.
-    pub fn with_z_index<R>(&mut self, z_index: u8, f: impl FnOnce(&mut Self) -> R) -> R {
+    pub fn with_z_index<R>(&mut self, z_index: u16, f: impl FnOnce(&mut Self) -> R) -> R {
         let new_stacking_order_id =
             post_inc(&mut self.window_mut().next_frame.next_stacking_order_id);
-        let old_stacking_order_id = mem::replace(
-            &mut self.window_mut().next_frame.z_index_stack.id,
-            new_stacking_order_id,
-        );
-        self.window_mut().next_frame.z_index_stack.id = new_stacking_order_id;
-        self.window_mut().next_frame.z_index_stack.push(z_index);
+        let new_context = StackingContext {
+            z_index,
+            id: new_stacking_order_id,
+        };
+
+        self.window_mut().next_frame.z_index_stack.push(new_context);
         let result = f(self);
-        self.window_mut().next_frame.z_index_stack.id = old_stacking_order_id;
         self.window_mut().next_frame.z_index_stack.pop();
+
         result
     }
 

crates/storybook/src/stories/z_index.rs 🔗

@@ -76,7 +76,7 @@ impl Styles for Div {}
 
 #[derive(IntoElement)]
 struct ZIndexExample {
-    z_index: u8,
+    z_index: u16,
 }
 
 impl RenderOnce for ZIndexExample {
@@ -166,7 +166,7 @@ impl RenderOnce for ZIndexExample {
 }
 
 impl ZIndexExample {
-    pub fn new(z_index: u8) -> Self {
+    pub fn new(z_index: u16) -> Self {
         Self { z_index }
     }
 }

crates/ui/src/styles/elevation.rs 🔗

@@ -20,7 +20,7 @@ pub enum ElevationIndex {
 }
 
 impl ElevationIndex {
-    pub fn z_index(self) -> u8 {
+    pub fn z_index(self) -> u16 {
         match self {
             ElevationIndex::Background => 0,
             ElevationIndex::Surface => 42,

crates/workspace/src/workspace.rs 🔗

@@ -4326,7 +4326,7 @@ impl Element for DisconnectedOverlay {
         overlay: &mut Self::State,
         cx: &mut ElementContext,
     ) {
-        cx.with_z_index(u8::MAX, |cx| {
+        cx.with_z_index(u16::MAX, |cx| {
             cx.add_opaque_layer(bounds);
             overlay.paint(cx);
         })