LibWeb: Don't paint all stacking contexts like positioned elements

Previously stacking contexts were only painted in steps 3, 8, and 9.
These steps are only meant to cover positioned elements (as per
https://www.w3.org/TR/CSS22/zindex.html). This meant that elements with
opacity (which forms a stacking context) could end up painted above
elements that actually occlude them.

Userland/Libraries/LibWeb/Painting/StackingContext.cpp

@@ -80,15 +80,34 @@ void StackingContext::paint_descendants(PaintContext& context, Layout::Node cons
         paintable->apply_clip_overflow_rect(context, to_paint_phase(phase));
     }
 
+    auto child_stacking_context = [&](auto& child) -> StackingContext const* {
+        if (auto* paintable = child.paintable()) {
+            if (auto* stacking_context = paintable->stacking_context_rooted_here())
+                return stacking_context;
+        }
+        return nullptr;
+    };
+
     box.for_each_child([&](auto& child) {
-        // If `child` establishes its own stacking context, skip over it.
+        auto* stacking_context = child_stacking_context(child);
-        if (is<Layout::Box>(child) && child.paintable() && static_cast<Layout::Box const&>(child).paintable_box()->stacking_context())
+
-            return;
-        // If `child` is positioned with a z-index of `0` or `auto`, skip over it.
         if (child.is_positioned()) {
+            // If `child` is positioned with a z-index of `0` or `auto`, skip over it.
             auto const& z_index = child.computed_values().z_index();
             if (!z_index.has_value() || z_index.value() == 0)
                 return;
+
+            // Skip positioned children with stacking contexts, these are handled in paint_internal().
+            if (stacking_context)
+                return;
+        }
+
+        if (stacking_context) {
+            // FIXME: This may not be fully correct with respect to the paint phases.
+            if (phase == StackingContextPaintPhase::Foreground)
+                paint_child(context, *stacking_context);
+            // Note: Don't further recuse into descendants as paint_child() will do that.
+            return;
         }
 
         bool child_is_inline_or_replaced = child.is_inline() || is<Layout::ReplacedBox>(child);
@@ -139,6 +158,25 @@ void StackingContext::paint_descendants(PaintContext& context, Layout::Node cons
     }
 }
 
+void StackingContext::paint_child(PaintContext& context, StackingContext const& child) const
+{
+    auto parent = child.m_box->parent();
+    auto* parent_paintable = parent ? parent->paintable() : nullptr;
+    if (parent_paintable)
+        parent_paintable->before_children_paint(context, PaintPhase::Foreground);
+    auto containing_block = child.m_box->containing_block();
+    auto* containing_block_paintable = containing_block ? containing_block->paintable() : nullptr;
+    if (containing_block_paintable)
+        containing_block_paintable->apply_clip_overflow_rect(context, PaintPhase::Foreground);
+
+    child.paint(context);
+
+    if (parent_paintable)
+        parent_paintable->after_children_paint(context, PaintPhase::Foreground);
+    if (containing_block_paintable)
+        containing_block_paintable->clear_clip_overflow_rect(context, PaintPhase::Foreground);
+}
+
 void StackingContext::paint_internal(PaintContext& context) const
 {
     // For a more elaborate description of the algorithm, see CSS 2.1 Appendix E
@@ -146,28 +184,13 @@ void StackingContext::paint_internal(PaintContext& context) const
     paint_node(m_box, context, PaintPhase::Background);
     paint_node(m_box, context, PaintPhase::Border);
 
-    auto paint_child = [&](auto* child) {
+    // Stacking contexts formed by positioned descendants with negative z-indices (excluding 0) in z-index order
-        auto parent = child->m_box->parent();
+    // (most negative first) then tree order. (step 3)
-        auto* parent_paintable = parent ? parent->paintable() : nullptr;
-        if (parent_paintable)
-            parent_paintable->before_children_paint(context, PaintPhase::Foreground);
-        auto containing_block = child->m_box->containing_block();
-        auto* containing_block_paintable = containing_block ? containing_block->paintable() : nullptr;
-        if (containing_block_paintable)
-            containing_block_paintable->apply_clip_overflow_rect(context, PaintPhase::Foreground);
-
-        child->paint(context);
-
-        if (parent_paintable)
-            parent_paintable->after_children_paint(context, PaintPhase::Foreground);
-        if (containing_block_paintable)
-            containing_block_paintable->clear_clip_overflow_rect(context, PaintPhase::Foreground);
-    };
-
-    // Draw positioned descendants with negative z-indices (step 3)
     for (auto* child : m_children) {
-        if (child->m_box->computed_values().z_index().has_value() && child->m_box->computed_values().z_index().value() < 0)
+        if (!child->paintable_box().layout_box().is_positioned())
-            paint_child(child);
+            continue;
+        if (child->paintable_box().computed_values().z_index().has_value() && child->m_box->computed_values().z_index().value() < 0)
+            paint_child(context, *child);
     }
 
     // Draw the background and borders for block-level children (step 4)
@@ -180,23 +203,18 @@ void StackingContext::paint_internal(PaintContext& context) const
     paint_descendants(context, m_box, StackingContextPaintPhase::Foreground);
 
     // Draw positioned descendants with z-index `0` or `auto` in tree order. (step 8)
-    // NOTE: Non-positioned descendants that establish stacking contexts with z-index `0` or `auto` are also painted here.
     // FIXME: There's more to this step that we have yet to understand and implement.
     m_box->paintable_box()->for_each_in_subtree([&](Paintable const& paintable) {
         auto const& layout_node = paintable.layout_node();
         auto const& z_index = paintable.computed_values().z_index();
 
-        if (auto const* child = paintable.stacking_context_rooted_here()) {
+        if (!layout_node.is_positioned() || (z_index.has_value() && z_index.value() != 0)) {
-            if (!z_index.has_value() || z_index.value() == 0)
+            return paintable.stacking_context_rooted_here()
-                paint_child(child);
+                ? TraversalDecision::SkipChildrenAndContinue
-            return TraversalDecision::SkipChildrenAndContinue;
+                : TraversalDecision::Continue;
         }
-        if (z_index.has_value() && z_index.value() != 0)
+
-            return TraversalDecision::Continue;
+        // At this point, `paintable_box` is a positioned descendant with z-index: auto.
-        if (!layout_node.is_positioned())
-            return TraversalDecision::Continue;
-        // At this point, `paintable_box` is a positioned descendant with z-index: auto
-        // but no stacking context of its own.
         // FIXME: This is basically duplicating logic found elsewhere in this same function. Find a way to make this more elegant.
         auto parent = layout_node.parent();
         auto* parent_paintable = parent ? parent->paintable() : nullptr;
@@ -206,6 +224,10 @@ void StackingContext::paint_internal(PaintContext& context) const
         auto* containing_block_paintable = containing_block ? containing_block->paintable() : nullptr;
         if (containing_block_paintable)
             containing_block_paintable->apply_clip_overflow_rect(context, PaintPhase::Foreground);
+        if (auto* child = paintable.stacking_context_rooted_here()) {
+            paint_child(context, *child);
+            return TraversalDecision::SkipChildrenAndContinue;
+        } else {
             paint_node(layout_node, context, PaintPhase::Background);
             paint_node(layout_node, context, PaintPhase::Border);
             paint_descendants(context, layout_node, StackingContextPaintPhase::BackgroundAndBorders);
@@ -216,6 +238,7 @@ void StackingContext::paint_internal(PaintContext& context) const
             paint_node(layout_node, context, PaintPhase::Outline);
             paint_node(layout_node, context, PaintPhase::Overlay);
             paint_descendants(context, layout_node, StackingContextPaintPhase::FocusAndOverlay);
+        }
         if (parent_paintable)
             parent_paintable->after_children_paint(context, PaintPhase::Foreground);
         if (containing_block_paintable)
@@ -224,10 +247,13 @@ void StackingContext::paint_internal(PaintContext& context) const
         return TraversalDecision::Continue;
     });
 
-    // Draw other positioned descendants (step 9)
+    // Stacking contexts formed by positioned descendants with z-indices greater than or equal to 1 in z-index order
+    // (smallest first) then tree order. (Step 9)
     for (auto* child : m_children) {
-        if (child->m_box->computed_values().z_index().has_value() && child->m_box->computed_values().z_index().value() >= 1)
+        if (!child->paintable_box().layout_box().is_positioned())
-            paint_child(child);
+            continue;
+        if (child->paintable_box().computed_values().z_index().has_value() && child->m_box->computed_values().z_index().value() >= 1)
+            paint_child(context, *child);
     }
 
     paint_node(m_box, context, PaintPhase::Outline);

Userland/Libraries/LibWeb/Painting/StackingContext.h

@@ -49,6 +49,7 @@ private:
     Vector<StackingContext*> m_children;
     size_t m_index_in_tree_order { 0 };
 
+    void paint_child(PaintContext&, StackingContext const&) const;
     void paint_internal(PaintContext&) const;
     Gfx::FloatMatrix4x4 get_transformation_matrix(CSS::Transformation const& transformation) const;
     Gfx::FloatMatrix4x4 combine_transformations(Vector<CSS::Transformation> const& transformations) const;