From 2a067b5601681fdb643732ac319a42f8b006300e Mon Sep 17 00:00:00 2001 From: MacDue Date: Sun, 13 Aug 2023 18:56:56 +0100 Subject: [PATCH] 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. --- .../LibWeb/Painting/StackingContext.cpp | 124 +++++++++++------- .../LibWeb/Painting/StackingContext.h | 1 + 2 files changed, 76 insertions(+), 49 deletions(-) diff --git a/Userland/Libraries/LibWeb/Painting/StackingContext.cpp b/Userland/Libraries/LibWeb/Painting/StackingContext.cpp index 5246428eb2..eb4270991b 100644 --- a/Userland/Libraries/LibWeb/Painting/StackingContext.cpp +++ b/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. - if (is(child) && child.paintable() && static_cast(child).paintable_box()->stacking_context()) - return; - // If `child` is positioned with a z-index of `0` or `auto`, skip over it. + auto* stacking_context = child_stacking_context(child); + 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(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) { - 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); - }; - - // Draw positioned descendants with negative z-indices (step 3) + // Stacking contexts formed by positioned descendants with negative z-indices (excluding 0) in z-index order + // (most negative first) then tree order. (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) - paint_child(child); + if (!child->paintable_box().layout_box().is_positioned()) + 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 (!z_index.has_value() || z_index.value() == 0) - paint_child(child); - return TraversalDecision::SkipChildrenAndContinue; + if (!layout_node.is_positioned() || (z_index.has_value() && z_index.value() != 0)) { + return paintable.stacking_context_rooted_here() + ? TraversalDecision::SkipChildrenAndContinue + : TraversalDecision::Continue; } - if (z_index.has_value() && z_index.value() != 0) - return TraversalDecision::Continue; - 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. + + // At this point, `paintable_box` is a positioned descendant with z-index: auto. // 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,16 +224,21 @@ 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); - paint_node(layout_node, context, PaintPhase::Background); - paint_node(layout_node, context, PaintPhase::Border); - paint_descendants(context, layout_node, StackingContextPaintPhase::BackgroundAndBorders); - paint_descendants(context, layout_node, StackingContextPaintPhase::Floats); - paint_descendants(context, layout_node, StackingContextPaintPhase::BackgroundAndBordersForInlineLevelAndReplaced); - paint_node(layout_node, context, PaintPhase::Foreground); - paint_descendants(context, layout_node, StackingContextPaintPhase::Foreground); - paint_node(layout_node, context, PaintPhase::Outline); - paint_node(layout_node, context, PaintPhase::Overlay); - paint_descendants(context, layout_node, StackingContextPaintPhase::FocusAndOverlay); + 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); + paint_descendants(context, layout_node, StackingContextPaintPhase::Floats); + paint_descendants(context, layout_node, StackingContextPaintPhase::BackgroundAndBordersForInlineLevelAndReplaced); + paint_node(layout_node, context, PaintPhase::Foreground); + paint_descendants(context, layout_node, StackingContextPaintPhase::Foreground); + 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) - paint_child(child); + if (!child->paintable_box().layout_box().is_positioned()) + 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); diff --git a/Userland/Libraries/LibWeb/Painting/StackingContext.h b/Userland/Libraries/LibWeb/Painting/StackingContext.h index 7016862f92..e0d7f4b3bb 100644 --- a/Userland/Libraries/LibWeb/Painting/StackingContext.h +++ b/Userland/Libraries/LibWeb/Painting/StackingContext.h @@ -49,6 +49,7 @@ private: Vector 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 const& transformations) const;