Adjust the clip region for shadow elements. #2278

GuanWen-Chen · 2018-01-10T07:32:17Z

This patch is intend to fix #1928.

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.
To fix this, when adding a shadow line primitive, I adjust PrimitiveInfo's local_clip_rect to fit the correct clip region and apply the clip outside of the shadow to the shadow line primitive.

This change is

GuanWen-Chen · 2018-01-10T07:34:45Z

r? @glennw

glennw · 2018-01-10T19:53:29Z

Does this seem like a reasonable solution @gankro ?

@GuanWen-Chen Thanks for the patch! Could you also add a wrench reftest to cover this case, and a gecko try run with this patch applied?

Gankra · 2018-01-11T05:35:02Z

I don't understand how the clip scroll tree works well enough anymore to say if this is right, but from what I remember it shouldn't be?

How does this correctly handle:

push_clip(A)
  push_shadow
    push_clip(B)
      line
    pop_clip
    text
  pop_shadow
pop_clip

(I believe this roughly corresponds to the commands sent for <div overflow:hidden><span text-shadow; underline>AAAA</span></div>)

Specifically only B should be offset, while A should still apply unchanged. It seems like the interesection you're doing is potentially applying this logic, but I thought the push_clip commands were too lossy today to reliably show up in the clipscroll tree?

Also whatever the correct logic, it needs to be applied to text run shadows as well, as partial ligatures do the same trick.

glennw · 2018-01-11T09:52:56Z

Sorry, didn't get to this today - will read through it in detail tomorrow.

GuanWen-Chen · 2018-01-11T09:56:12Z

Hello @gankro

Yes, I think only B (every clip nodes within shadow) should be offset, and I will apply this logic to text.
I am not sure what do you mean by "the push_clip commands were too lossy today to reliably show up in the clipscroll tree"? Does it mean that the clip id passed by push_clip command may not show up in clipscroll tree? And how does it affect the result?

However, I found a case this solution couldn't handle.
Consider a more complicated case(Not sure if this case will happen in practice):

push_clip(A)
  push_shadow
    push_clip(B)
      push_clip(C)
        line
      pop_clip
    pop_clip
    text
  pop_shadow
pop_clip

In this case, according to current logic, when handling the line's shadow, we will adjust the line's shadow's local_clip rect to clip C's region and apply clip B; but the clip B is the clip without shadow offset.
To correct this, we should recursively intersect the local_clip_rect to its clip and ascent parents until the clip outside the shadow.
But not sure if it will cause any side effects.

Gankra · 2018-01-11T15:41:25Z

@mrobinson you're still the clip expert, right?

mrobinson · 2018-01-11T16:37:01Z

webrender/src/frame_builder.rs

+                fast_clip_and_scroll = clip_and_scroll;
+            } else {
+                let node = clip_scroll_tree.nodes.get(&clip_and_scroll.clip_node_id()).unwrap();
+                let mut clip_rect = node.local_viewport_rect.translate(&shadow_offset);


Hrm. This feels a bit wrong to me. How do we know that the ClipScrollNode has the same coordinate system as the shadow? What about the other clips in the clip hierarchy? I'm not sure this is the correct approach here, but it could be that I simply don't understand the problem. I worry that this is merely hiding the overall issue.

Doesn't it make sense to render

mrobinson · 2018-01-11T16:38:01Z

Sorry for the late reply. Another question I have is how we know which clips should affect the shadow and which ones should not?

Gankra · 2018-01-11T16:46:46Z

@mrobinson the idea is that during DL construction, some clips are pushed while a text-shadow is still pushed, and those should have the shadow offset applied when rendering the shadow. This "inner" clip is used to make wavy/dashed lines from sibling elements "line up", and to draw a glyph where different parts have different styles (by drawing it multiple times with different clips).

Here's some examples of this in action in firefox today:

wavy lines from multiple spans cleanly merge:

style changes mid-ligature:

mrobinson · 2018-01-11T16:54:29Z

@gankro I suppose it isn't possible to modify Gecko to produce another clip with the proper offset and apply it to the shadow? I worry about building assumptions that only hold true for Gecko into WebRender...

glennw · 2018-01-12T01:39:57Z

I discussed this briefly with @gankro on IRC. Conversation here for posterity:

<Gankro> gw: mrobinson and I are gonna have a call about clipping stuff next week if that's the topic
<gw> Gankro: so ... returning to shadows + clips, our favourite topic :)
<gw> Gankro: ah, ok
<gw> Gankro: might be worth referencing https://github.com/servo/webrender/pull/1935#issuecomment-342058148 in your discussion
<Gankro> gw: we're currently hoping we can restrict the transform to the local clip rect only
<gw> Gankro: but doesn't gecko not use local clip at all? (and we were considering removing local clip because of that)?
<Gankro> yes it will need (hopefully) minor gecko changes
<gw> Gankro: ok. that would certainly resolve my concern in the comment referenced above, since there is no clip ID mapping in that case
<gw> Gankro: and, I guess, this use case only needs clip rects (not *rounded* rects), right?
<Gankro> gw: yeah exactly
<gw> Gankro: ok, which would still allow us to remove the complex local clip items from the display list as an optimization...
<Gankro> mhm
<gw> Gankro: great, ok, we can wait on the outcome of that discussion then, thanks!

Gankra · 2018-01-15T18:39:03Z

Ok so discussed this @mrobinson and @mstange today, and we think we've worked out what should work here. A key detail here is @demo99 (ethan lin) recently cleaned up the TextDrawTarget clipping, eliminating one of my points of confusion about the different clip sources: https://bugzilla.mozilla.org/show_bug.cgi?id=1424673

The high level idea is that we want shadows to offset only the local clip rect, and PushClip/PopClip on TextDrawTarget will manipulate this value.

Add a stack of Rects to TextDrawTarget which PushClip/PopClip manipulate
Make all uses of mClipRect instead submit the intersection of this stack (it will likely only ever have 1-2 rects)
Change this patch to just offset each element's local clip rect

One word of caution is that this introduces a semantic distinction between ClipScrollNodes and local clips; the latter will be offset by shadows. This means it isn't always a correct optimization to squash clips into children. If such an optimization is desirable, we can potentially add machinery to remember if we're currently adding things to a shadow to supress it when necessary. (cc @kats)

Ideally we should add tests to that will break if anyone tries to do this without thinking about it.

Does that make sense to you, @GuanWen-Chen ?

GuanWen-Chen · 2018-01-16T15:02:19Z

Ok, I will modify the code in both gecko and WR sides.

GuanWen-Chen · 2018-01-17T05:59:15Z

r? @gankro

Gankra · 2018-01-17T16:54:52Z

@bors-servo r+

awesome!

bors-servo · 2018-01-17T16:54:53Z

📌 Commit c8cbae0 has been approved by Gankro

bors-servo · 2018-01-17T17:26:08Z

⌛ Testing commit c8cbae0 with merge 2547f6f...

… r=Gankro Adjust the clip region for shadow elements. This patch is intend to fix #1928. 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. To fix this, when adding a shadow line primitive, I adjust PrimitiveInfo's local_clip_rect to fit the correct clip region and apply the clip outside of the shadow to the shadow line primitive.  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2278)

bors-servo · 2018-01-17T18:36:35Z

☀️ Test successful - status-appveyor, status-taskcluster
Approved by: Gankro
Pushing 2547f6f to master...

mrobinson reviewed Jan 11, 2018

View reviewed changes

GuanWen-Chen force-pushed the 1928-Fast_Shadows_Mishandle_Clips branch from 7100ccb to efa3048 Compare January 17, 2018 03:46

Adjust the local clip region for shadow elements.

c8cbae0

GuanWen-Chen force-pushed the 1928-Fast_Shadows_Mishandle_Clips branch from efa3048 to c8cbae0 Compare January 17, 2018 04:11

bors-servo merged commit c8cbae0 into servo:master Jan 17, 2018

Adjust the clip region for shadow elements. #2278

Adjust the clip region for shadow elements. #2278

Uh oh!

Conversation

GuanWen-Chen commented Jan 10, 2018 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

GuanWen-Chen commented Jan 10, 2018

Uh oh!

glennw commented Jan 10, 2018

Uh oh!

Gankra commented Jan 11, 2018

Uh oh!

glennw commented Jan 11, 2018

Uh oh!

GuanWen-Chen commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Gankra commented Jan 11, 2018

Uh oh!

mrobinson Jan 11, 2018

Choose a reason for hiding this comment

Uh oh!

mrobinson commented Jan 11, 2018

Uh oh!

Gankra commented Jan 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mrobinson commented Jan 11, 2018

Uh oh!

glennw commented Jan 12, 2018

Uh oh!

Gankra commented Jan 15, 2018

Uh oh!

GuanWen-Chen commented Jan 16, 2018

Uh oh!

GuanWen-Chen commented Jan 17, 2018

Uh oh!

Gankra commented Jan 17, 2018

Uh oh!

bors-servo commented Jan 17, 2018

Uh oh!

bors-servo commented Jan 17, 2018

Uh oh!

bors-servo commented Jan 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

GuanWen-Chen commented Jan 10, 2018 •

edited by larsbergstrom

Loading

GuanWen-Chen commented Jan 11, 2018 •

edited

Loading

Gankra commented Jan 11, 2018 •

edited

Loading