Explorar o código

LibWeb: Don't ignore non-positioned stacking contexts on step 8 of paint

...traversal. We've already fixed step 3 and 9 to not filter out
non-positioned stacking contexts, because modern CSS has more ways to
create stacking context besides being positioned with z-index (like by
using "transform", "filter" or "clip-path" properties).

See following spec issue for more details https://github.com/w3c/csswg-drafts/issues/2717

Visual improvement on https://basecamp.com/
Aliaksandr Kalenik hai 9 meses
pai
achega
a8788e5abb

+ 19 - 0
Tests/LibWeb/Ref/box-with-transform-should-be-above-other-boxes.html

@@ -0,0 +1,19 @@
+<!doctype html>
+<link rel="match" href="reference/box-with-transform-should-be-above-other-boxes-ref.html" />
+<style>
+* { outline: 1px solid black; }
+body { width: 300px; }
+#green {
+    background: rgba(0, 255, 0, 1);
+    position: absolute;
+    width: 30px;
+    height: 30px;
+}
+#orange {
+    background: orange;
+    width: 100px;
+    height: 100px;
+    transform: rotate(0deg);
+    opacity: 1;
+}
+</style><body><div id="green"></div><div id="orange">

+ 10 - 0
Tests/LibWeb/Ref/reference/box-with-transform-should-be-above-other-boxes-ref.html

@@ -0,0 +1,10 @@
+<!doctype html>
+<style>
+* { outline: 1px solid black; }
+body { width: 300px; }
+#orange {
+    background: orange;
+    width: 100px;
+    height: 100px;
+}
+</style><body><div id="orange">

+ 12 - 18
Userland/Libraries/LibWeb/Painting/StackingContext.cpp

@@ -133,20 +133,11 @@ void StackingContext::paint_descendants(PaintContext& context, Paintable const&
             return IterationDecision::Continue;
         }
 
-        if (stacking_context && z_index.value_or(0) != 0)
+        if (stacking_context)
             return IterationDecision::Continue;
         if (child.is_positioned() && z_index.value_or(0) == 0)
             return IterationDecision::Continue;
 
-        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 recurse into descendants as paint_child() will do that.
-            return IterationDecision::Continue;
-        }
-
         bool child_is_inline_or_replaced = child.is_inline() || is<Layout::ReplacedBox>(child.layout_node());
         switch (phase) {
         case StackingContextPaintPhase::BackgroundAndBorders:
@@ -223,7 +214,9 @@ void StackingContext::paint_internal(PaintContext& context) const
 
     // Stacking contexts formed by positioned descendants with negative z-indices (excluding 0) in z-index order
     // (most negative first) then tree order. (step 3)
-    // NOTE: This doesn't check if a descendant is positioned as modern CSS allows for alternative methods to establish stacking contexts.
+    // Here, we treat non-positioned stacking contexts as if they were positioned, because CSS 2.0 spec does not
+    // account for new properties like `transform` and `opacity` that can create stacking contexts.
+    // https://github.com/w3c/csswg-drafts/issues/2717
     for (auto* child : m_children) {
         if (child->paintable().computed_values().z_index().has_value() && child->paintable().computed_values().z_index().value() < 0)
             paint_child(context, *child);
@@ -239,11 +232,10 @@ void StackingContext::paint_internal(PaintContext& context) const
     paint_descendants(context, paintable(), StackingContextPaintPhase::Foreground);
 
     // Draw positioned descendants with z-index `0` or `auto` in tree order. (step 8)
-    // FIXME: There's more to this step that we have yet to understand and implement.
-    for (auto const& paintable : m_positioned_descendants_with_stack_level_0_and_stacking_contexts) {
-        if (!paintable->is_positioned())
-            continue;
-
+    // Here, we treat non-positioned stacking contexts as if they were positioned, because CSS 2.0 spec does not
+    // account for new properties like `transform` and `opacity` that can create stacking contexts.
+    // https://github.com/w3c/csswg-drafts/issues/2717
+    for (auto const& paintable : m_positioned_descendants_and_stacking_contexts_with_stack_level_0) {
         // 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_paintable = paintable->parent();
@@ -260,7 +252,9 @@ void StackingContext::paint_internal(PaintContext& context) const
 
     // 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)
-    // NOTE: This doesn't check if a descendant is positioned as modern CSS allows for alternative methods to establish stacking contexts.
+    // Here, we treat non-positioned stacking contexts as if they were positioned, because CSS 2.0 spec does not
+    // account for new properties like `transform` and `opacity` that can create stacking contexts.
+    // https://github.com/w3c/csswg-drafts/issues/2717
     for (auto* child : m_children) {
         if (child->paintable().computed_values().z_index().has_value() && child->paintable().computed_values().z_index().value() >= 1)
             paint_child(context, *child);
@@ -393,7 +387,7 @@ TraversalDecision StackingContext::hit_test(CSSPixelPoint position, HitTestType
     }
 
     // 6. the child stacking contexts with stack level 0 and the positioned descendants with stack level 0.
-    for (auto const& paintable : m_positioned_descendants_with_stack_level_0_and_stacking_contexts.in_reverse()) {
+    for (auto const& paintable : m_positioned_descendants_and_stacking_contexts_with_stack_level_0.in_reverse()) {
         if (paintable->stacking_context()) {
             if (paintable->stacking_context()->hit_test(transformed_position, type, callback) == TraversalDecision::Break)
                 return TraversalDecision::Break;

+ 1 - 1
Userland/Libraries/LibWeb/Painting/StackingContext.h

@@ -56,7 +56,7 @@ private:
     size_t m_index_in_tree_order { 0 };
     Optional<u64> m_last_paint_generation_id;
 
-    Vector<JS::NonnullGCPtr<Paintable const>> m_positioned_descendants_with_stack_level_0_and_stacking_contexts;
+    Vector<JS::NonnullGCPtr<Paintable const>> m_positioned_descendants_and_stacking_contexts_with_stack_level_0;
     Vector<JS::NonnullGCPtr<Paintable const>> m_non_positioned_floating_descendants;
 
     static void paint_child(PaintContext&, StackingContext const&);

+ 1 - 1
Userland/Libraries/LibWeb/Painting/ViewportPaintable.cpp

@@ -43,7 +43,7 @@ void ViewportPaintable::build_stacking_context_tree()
         auto* parent_context = const_cast<Paintable&>(paintable).enclosing_stacking_context();
         auto establishes_stacking_context = paintable.layout_node().establishes_stacking_context();
         if ((paintable.is_positioned() || establishes_stacking_context) && paintable.computed_values().z_index().value_or(0) == 0)
-            parent_context->m_positioned_descendants_with_stack_level_0_and_stacking_contexts.append(paintable);
+            parent_context->m_positioned_descendants_and_stacking_contexts_with_stack_level_0.append(paintable);
         if (!paintable.is_positioned() && paintable.is_floating())
             parent_context->m_non_positioned_floating_descendants.append(paintable);
         if (!establishes_stacking_context) {