Closed Bug 1372321 Opened 4 years ago Closed 4 years ago

Make the clip rect and mask rect relative to the content rect when pushing a clip item

Categories

(Core :: Graphics: WebRender, enhancement, P3)

Other Branch
enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(2 files)

When defining a clip item in the WR API, there are two things passed to it: a content rect and a "clip region". The clip region contains two rects - a clip rect and a mask rect. Normally these rects are relative to the stacking context, but in the special case where the clip region is attached to a clip item (as opposed to say a rect or gradient), the clip rect and mask rect need to be relative to the content rect.

See discussion on servo/webrender#1352
Attachment #8876868 - Flags: review?(mrobinson)
Comment on attachment 8876879 [details]
Bug 1372321 - Log the mask rect when passing a WrImageMask to WR.

https://reviewboard.mozilla.org/r/148206/#review152562
Attachment #8876879 - Flags: review?(jmuizelaar) → review+
Comment on attachment 8876868 [details]
Bug 1372321 - When pushing a clip item, ensure the clip rect and mask rect are relative to the content rect.

https://reviewboard.mozilla.org/r/148196/#review152824

This looks good to me. I think this isn't causing reftest changes because internally in WebRender we are ignoring the origin of the ClipRegion rectangle. The mask origin is used though. In the future, this will likely change once https://github.com/servo/webrender/issues/1090 is fixed.

::: gfx/webrender_bindings/src/bindings.rs:1337
(Diff revision 2)
> -    let clip_rect = clip_rect.into();
> -    let mask = unsafe { mask.as_ref() }.map(|x| x.into());
> +    let content_rect: LayoutRect = rect.into();
> +
> +    // Both the clip rect and mask rect need to be relative to the
> +    // content rect when the clip region is being used as part of a clip item.
> +    let mut clip_rect: LayoutRect = rect.into();
> +    clip_rect.origin = clip_rect.origin - content_rect.origin;

Since these two rectangles appear to be the same, perhaps it would make more sense to write:

let clip_rect = LayerRect::new(LayoutPoint::zero(), content_rect.size);

I think that would be a bit clearer.
Attachment #8876868 - Flags: review?(mrobinson) → review+
Thanks. I updated the patch as you suggested.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c585f465f4b1
When pushing a clip item, ensure the clip rect and mask rect are relative to the content rect. r=mrobinson+572841
https://hg.mozilla.org/integration/autoland/rev/9f7d877140e6
Log the mask rect when passing a WrImageMask to WR. r=jrmuizel
https://hg.mozilla.org/mozilla-central/rev/c585f465f4b1
https://hg.mozilla.org/mozilla-central/rev/9f7d877140e6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.