Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Local clips on box shadows are ignored #2310

Closed
mstange opened this issue Jan 16, 2018 · 4 comments
Closed

Local clips on box shadows are ignored #2310

mstange opened this issue Jan 16, 2018 · 4 comments

Comments

@mstange
Copy link
Contributor

@mstange mstange commented Jan 16, 2018

https://bugzilla.mozilla.org/show_bug.cgi?id=1416535

When Gecko has a shadow on an inline element that is broken up over multiple lines, it wants to make it look as if there really was just a single shadow, but sliced up into pieces and distributed over the multiple lines. It does that by drawing the entire shadow for each of the pieces and clipping it to just the size of that piece.

That clipping is done using a local clip:

#[no_mangle]
pub extern "C" fn wr_dp_push_box_shadow(state: &mut WrState,
                                        rect: LayoutRect,
                                        clip: LayoutRect,
                                        // ...
                                        clip_mode: BoxShadowClipMode) {

    let mut prim_info = LayoutPrimitiveInfo::with_clip_rect(rect, clip.into());

However, it looks like WebRender ignores that local clip, and the broken up inset box shadows end up being displayed completely. This can lead to surprisingly long underlines, for example.

Here's a wrench reftest that fails but which I'd expect to pass:

diff --git a/wrench/reftests/boxshadow/box-shadow-clip-ref.yaml b/wrench/reftests/boxshadow/box-shadow-clip-ref.yaml
new file mode 100644
index 00000000..7b181c80
--- /dev/null
+++ b/wrench/reftests/boxshadow/box-shadow-clip-ref.yaml
@@ -0,0 +1,40 @@
+---
+root:
+  items:
+    - type: stacking-context
+      bounds: [0, 0, 1000, 1000]
+      items:
+        - type: clip
+          bounds: [0, 0, 100, 200]
+          items:
+            - type: box-shadow
+              bounds: [ 20, 20, 200, 80 ]
+              color: blue
+              offset: [10, 20]
+              clip-mode: outset
+        - type: clip
+          bounds: [200, 0, 100, 200]
+          items:
+            - type: box-shadow
+              bounds: [ 220, 20, 200, 80 ]
+              color: red
+              offset: [10, 20]
+              blur-radius: 10
+              clip-mode: outset
+        - type: clip
+          bounds: [0, 200, 100, 200]
+          items:
+            - type: box-shadow
+              bounds: [ 20, 220, 200, 80 ]
+              color: green
+              offset: [10, 20]
+              clip-mode: inset
+        - type: clip
+          bounds: [200, 200, 100, 200]
+          items:
+            - type: box-shadow
+              bounds: [ 220, 220, 200, 80 ]
+              color: yellow
+              offset: [10, 20]
+              blur-radius: 10
+              clip-mode: inset
diff --git a/wrench/reftests/boxshadow/box-shadow-clip.yaml b/wrench/reftests/boxshadow/box-shadow-clip.yaml
new file mode 100644
index 00000000..fd5a419f
--- /dev/null
+++ b/wrench/reftests/boxshadow/box-shadow-clip.yaml
@@ -0,0 +1,33 @@
+---
+root:
+  items:
+    - type: stacking-context
+      bounds: [0, 0, 1000, 1000]
+      items:
+        - type: box-shadow
+          bounds: [ 20, 20, 200, 80 ]
+          clip-rect: [0, 0, 100, 200]
+          color: blue
+          offset: [10, 20]
+          clip-mode: outset
+        - type: box-shadow
+          bounds: [ 220, 20, 200, 80 ]
+          clip-rect: [200, 0, 100, 200]
+          color: red
+          offset: [10, 20]
+          blur-radius: 10
+          clip-mode: outset
+        - type: box-shadow
+          bounds: [ 20, 220, 200, 80 ]
+          clip-rect: [0, 200, 100, 200]
+          color: green
+          offset: [10, 20]
+          clip-mode: inset
+        - type: box-shadow
+          bounds: [ 220, 220, 200, 80 ]
+          clip-rect: [200, 200, 100, 200]
+          color: yellow
+          offset: [10, 20]
+          blur-radius: 10
+          clip-mode: inset
+
diff --git a/wrench/reftests/boxshadow/reftest.list b/wrench/reftests/boxshadow/reftest.list
index d08ca9b1..a1ad4d45 100644
--- a/wrench/reftests/boxshadow/reftest.list
+++ b/wrench/reftests/boxshadow/reftest.list
@@ -13,6 +13,7 @@ fuzzy(1,396) == inset-large-offset.yaml inset-large-offset-ref.png
 fuzzy(1,6388) == rounding.yaml rounding-ref.yaml
 == box-shadow-border-radii.yaml box-shadow-border-radii.png
 == box-shadow-spread.yaml box-shadow-spread.png
+== box-shadow-clip.yaml box-shadow-clip-ref.yaml
 == invalid.yaml invalid-ref.yaml
 == inset-subpx.yaml inset-subpx.png
 == inset-downscale.yaml inset-downscale.png
@mstange
Copy link
Contributor Author

@mstange mstange commented Jan 17, 2018

@demo99 has started working on this bug.

@mstange
Copy link
Contributor Author

@mstange mstange commented Jan 19, 2018

Here's Ethan's patch from his try push:

# HG changeset patch
# User Ethan Lin <ethlin@mozilla.com>
# Date 1516180055 -28800
# Node ID 57610bed06574df9d1856c8eb613ee4b8ad137bc
# Parent  acc729d8b3d47505bdb19c340f411920a8500df5
fix box shadow clip

MozReview-Commit-ID: DrC0NMvGNd4


diff --git a/gfx/webrender/src/box_shadow.rs b/gfx/webrender/src/box_shadow.rs
--- a/gfx/webrender/src/box_shadow.rs
+++ b/gfx/webrender/src/box_shadow.rs
@@ -58,16 +58,17 @@ impl FrameBuilder {
         mut blur_radius: f32,
         spread_radius: f32,
         border_radius: BorderRadius,
         clip_mode: BoxShadowClipMode,
     ) {
         if color.a == 0.0 {
             return;
         }
+        let local_clip = *prim_info.local_clip.clip_rect();
 
         let (spread_amount, brush_clip_mode) = match clip_mode {
             BoxShadowClipMode::Outset => {
                 (spread_radius, ClipMode::Clip)
             }
             BoxShadowClipMode::Inset => {
                 (-spread_radius, ClipMode::ClipOut)
             }
@@ -92,39 +93,47 @@ impl FrameBuilder {
                 BoxShadowClipMode::Outset => {
                     // TODO(gw): Add a fast path for ClipOut + zero border radius!
                     clips.push(ClipSource::new_rounded_rect(
                         prim_info.rect,
                         border_radius,
                         ClipMode::ClipOut
                     ));
 
+                    let local_clip = local_clip.intersection(&shadow_rect);
+                    if local_clip.is_none() {
+                        return;
+                    }
                     LayerPrimitiveInfo::with_clip(
                         shadow_rect,
                         LocalClip::RoundedRect(
-                            shadow_rect,
+                            local_clip.unwrap(),
                             ComplexClipRegion::new(
                                 shadow_rect,
                                 shadow_radius,
                                 ClipMode::Clip,
                             ),
                         ),
                     )
                 }
                 BoxShadowClipMode::Inset => {
                     clips.push(ClipSource::new_rounded_rect(
                         shadow_rect,
                         shadow_radius,
                         ClipMode::ClipOut
                     ));
 
+                    let local_clip = local_clip.intersection(&prim_info.rect);
+                    if local_clip.is_none() {
+                        return;
+                    }
                     LayerPrimitiveInfo::with_clip(
                         prim_info.rect,
                         LocalClip::RoundedRect(
-                            prim_info.rect,
+                            local_clip.unwrap(),
                             ComplexClipRegion::new(
                                 prim_info.rect,
                                 border_radius,
                                 ClipMode::Clip
                             ),
                         ),
                     )
                 }
@@ -262,17 +271,20 @@ impl FrameBuilder {
                     //           most of the pixels inside and just
                     //           clipping out along the edges.
                     extra_clips.push(ClipSource::new_rounded_rect(
                         prim_info.rect,
                         border_radius,
                         ClipMode::ClipOut,
                     ));
 
-                    let pic_info = LayerPrimitiveInfo::new(pic_rect);
+                    let pic_info = LayerPrimitiveInfo::with_clip_rect(
+                        pic_rect,
+                        local_clip
+                    );
                     self.add_primitive(
                         clip_and_scroll,
                         &pic_info,
                         extra_clips,
                         PrimitiveContainer::Picture(pic_prim),
                     );
                 }
                 BoxShadowClipMode::Inset => {
@@ -329,24 +341,29 @@ impl FrameBuilder {
                         cache_key,
                         pipeline_id,
                     );
                     pic_prim.add_primitive(
                         brush_prim_index,
                         clip_and_scroll
                     );
 
+                    let local_clip = local_clip.intersection(&prim_info.rect);
+                    if local_clip.is_none() {
+                        return;
+                    }
+
                     // Draw the picture one pixel outside the original
                     // rect to account for the inflate above. This
                     // extra edge will be clipped by the local clip
                     // rect set below.
                     let pic_rect = prim_info.rect.inflate(inflate_size + box_offset.x.abs(), inflate_size + box_offset.y.abs());
                     let pic_info = LayerPrimitiveInfo::with_clip_rect(
                         pic_rect,
-                        prim_info.rect
+                        local_clip.unwrap()
                     );
 
                     // Add a normal clip to ensure nothing gets drawn
                     // outside the primitive rect.
                     if !border_radius.is_zero() {
                         extra_clips.push(ClipSource::new_rounded_rect(
                             prim_info.rect,
                             border_radius,
@demo99
Copy link
Contributor

@demo99 demo99 commented Jan 20, 2018

Markus, thanks. I have a branch for it: demo99@b765e98. There are two wrench tests for this clipping problem in the branch. IIRC, there is another wrench test that needs to be modified, but I lost my latest patch. I probably can't keep working on this issue. It would be good if you or anyone want to finish the patch. Thanks.

@Darkspirit
Copy link

@Darkspirit Darkspirit commented Feb 20, 2018

This was fixed by #2405.

@glennw glennw closed this Feb 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants
You can’t perform that action at this time.