Closed Bug 1414125 Opened 4 years ago Closed 3 years ago

Shadows Mishandle Clips on WebRender

Categories

(Core :: Graphics: WebRender, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox57 --- unaffected
firefox58 --- unaffected
firefox60 --- fixed

People

(Reporter: kuoe0.tw, Assigned: kechen)

References

Details

(Whiteboard: [wr-reserve] [gfx-noted])

Attachments

(3 files)

When Bug 1399310 is landed, the shadow seems to be clipped with the wrong region. The issue on GitHub is [1].

[1]: https://github.com/servo/webrender/issues/1928
Whiteboard: [wr-mvp] [triage]
Whiteboard: [wr-mvp] [triage] → [wr-mvp] [triage][gfx-noted]
Whiteboard: [wr-mvp] [triage][gfx-noted] → [wr-mvp] [gfx-noted]
Attached file testcase
Assignee: nobody → kechen
Status: NEW → ASSIGNED
Priority: P2 → P1
Whiteboard: [wr-mvp] [gfx-noted] → [wr-reserve] [gfx-noted]
The root cause of this bug is that we push a clip when drawing the decoration line under the text selection status to cut the decoration line with the right length[1].
However, in webrender, shadow elements inherit the same clip with the original elements which leads the shadow to apply the wrong clip region when doing fast-shadow.

There are several ways to fix this problem:

1. Skip the fast shadow in webrender if the shadow has vertical(horizontal) offset when the text is in vertical(horizontal) format. This might affect the performance of webrender since shadow with offset might be very common.

2. Remove the clip region for the decoration line, draw the decoration line with the exact length the text segment needs. With this solution, we don't have to change the behavior in webrender, but the clip problem in webrender remains.

3. Adjust the local clip rect of PrimitiveInfo when adding shadow line primitive and apply the clip outside of the shadow to it. it seems the right in this text decoration case, but I am not sure if this is correct behavior.[2]

Glenn, Alexis, do you have any thoughts about these proposals?

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1399310
[2] https://reviewboard.mozilla.org/r/210856/diff/1#index_header
Flags: needinfo?(gwatson)
Flags: needinfo?(a.beingessner)
Yeah glenn and I were discussing this before the holidays but I don't think we ever came to a conclusion. I'm personally a fan of some version of (3) as we also need it for Bug 1403261. Note however that whenever you get one of these weird clips, it was pushed *inside* the shadow, so that gives you a hint it should be adjusted. However this information is lost to the backend, as "push clip" isn't an actual display item type.
Flags: needinfo?(a.beingessner)
I've created a pull request of proposal 3 on webrender github[1].

[1] https://github.com/servo/webrender/pull/2278
There's ongoing discussion in the GH bug. From discussing with Alexis, it sounds like the "correct" solution is probably making use of the local clip rect functionality in the API for this. That would make it quite simple for WR to adjust the local clip rects internally based on the current shadow context, without introducing any complexity involved in creating extra clip-scroll nodes. I believe Gankro is planning to discuss this with Martin this week, to work out if that seems reasonable to all.
Flags: needinfo?(gwatson)
Hello Alexis,

Does the fix meet the proposal you mentioned in [1]?
If this patch is correct, I will add some reftests for wrench and move the WR part of codes to the WR repo.


[1] https://github.com/servo/webrender/pull/2278
Attachment #8942904 - Flags: feedback?(a.beingessner)
Comment on attachment 8942904 [details] [diff] [review]
Use local clip instead of ClipNode in TextDrawTarget.

Review of attachment 8942904 [details] [diff] [review]:
-----------------------------------------------------------------

Right approach, just needs tweaks.

::: gfx/webrender/src/frame_builder.rs
@@ +1357,3 @@
>              let prim_index = self.create_primitive(
>                  &info,
>                  Vec::new(),

This should be submitted as a PR to webrender and then upstreamed in a webrender update, not submitted here.

(but yeah this seems right)

::: layout/generic/TextDrawTarget.h
@@ +287,4 @@
>  
>    // Computed facts
>    wr::LayerRect mBoundsRect;
> +  LayoutDeviceRect mClipRect;

Any reason to "cache" this when it needs to go through ToRelativeLayoutRect anyway? I'd rather just have a method for like `ClipRect()` that grabs the back of the stack and applies the ToRelativeLayoutRect transform.

@@ +287,5 @@
>  
>    // Computed facts
>    wr::LayerRect mBoundsRect;
> +  LayoutDeviceRect mClipRect;
> +  std::vector<LayoutDeviceRect> mClipStack;

This should be changed to nsTArray, and not std::vector (it's safer)
Attachment #8942904 - Flags: feedback?(a.beingessner) → feedback+
Here is the try result[1].
With this patch and the fix in [2], the result seems good.
I've already removed the fail-if tags which cause the two unexpected-passes in the last patch.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=bf264fc9dca734cbd5ae96a0869d5533c85472fd&selectedJob=156765226
[2] https://github.com/servo/webrender/pull/2278
Comment on attachment 8940627 [details]
Bug 1414125 - Use local clip instead of ClipNode in TextDrawTarget;

https://reviewboard.mozilla.org/r/210856/#review219344
Attachment #8940627 - Flags: review?(a.beingessner) → review+
We should split the reftest changes out so that we can do the following:

* land the local rect changes (no effect)
* land the wr changes upstream
* in the wr update, kats applies the reftest changes
The WR changes by themselves didn't affect any reftest results. So we can land this after that WR update gets merged, and no need to split out the reftest changes.
Depends on: 1430829
That looks good. Lee, any objections to me landing this?
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb9032b4b612
Use local clip instead of ClipNode in TextDrawTarget; r=Gankro
https://hg.mozilla.org/mozilla-central/rev/fb9032b4b612
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
You need to log in before you can comment on or make changes to this bug.