916125 - When displayports are set on elements, their layers are not always clipped correctly

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

+++ This bug was initially created as a clone of Bug #898444 +++

Based on snorp's investigation in bug 898444 and some more digging around I did, I believe that the code in AsyncCompositionManager.cpp is missing some code to set a clip rect on the shadow layers. In a nutshell, if a subframe is async-scrollable and has a displayport, then the visible region for the layer will be the entire displayport (which is the painted area). Prior to multi-apzc this never happened so we never hit this case. But now that we do this, we also need to clip that layer so that instead of drawing the entire displayport it will only draw the part of it that's inside the scroll port (which should be the mCompositionBounds, possibly transformed into the right coordinate space).

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Just from a quick code examination, I think what needs to be done here is to modify this code: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/composite/AsyncCompositionManager.cpp#162

Instead of just setting the clip rect (possibly with translation) on the shadow layer, it should also intersect the (pre-translation) clip rect with the composition bounds of the layer, if the layer is a scrollable container layer. I suspect that the composition bounds you want to use there is mCompositionBounds / mFrameMetrics.GetParentResolution() but I'm not even 60% sure of that.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

This might be related to the clipping problem in bug 886969.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Matt, what do you think of comment 1?

Comment 4

[Matt Woodrow (:mattwoodrow)]

I'm not sure I understand the problem entirely, but if we're creating a scrollable layer that's bigger than its viewport, then the clipping to restrict compositing to that rect probably should be set up by content.

I'd expect nsGfxScrollFrame (where we decide on the display port) to also setup clipping to the viewport.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

If nsGfxScrollFrame sets up the clip, will the clip be transformed appropriately when the layer is async-scrolled? I don't really understand how the clip is affected by transformations.

Comment 6

[Matt Woodrow (:mattwoodrow)]

Why does the clip need to change? Isn't the viewport (scrollport?) staying in the same spot?

Comment 7

[Kartikaya Gupta (email:[e-mail])]

The scrollport of a layer stays in the same spot if you scroll that layer, yes. If you scroll a parent layer then the scrollport might move. So it is possible for the clip will move relative to the top-left corner of the layer as well as relative to the physical screen. I don't know if the clip is defined in such a way that both of these are handled automatically or not.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

I don't think I'm the right person to be assigned this bug. I don't know enough about the clipping code to really work on this.

Note that there are existing clipping bugs in Fennec - bug 866585, bug 854873 (+dupes, dependencies) that may or may not be related, so maybe somebody who knows the code should tackle all of them together.

Comment 9

[Milan Sreckovic [:milan] (needinfo for best results)]

Nick, is this something you can handle?  1.2 blocker.

Comment 10

[Nick Cameron [:nrc]]

I hate to shirk on a bug, but I don't think I can fix this quickly (I'm not familiar with either apzc or layout's clipping) and I only have two days until I go on PTO. This doesn't look like it is related with the clipping bugs I fixed last week.

Comment 11

[Milan Sreckovic [:milan] (needinfo for best results)]

Jet, who can look at this layout clipping bug?

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

Timothy, is this in the part of the code you know?

Comment 13

[Timothy Nikkel (:tnikkel)]

Hmm, depends on which code needs changing. AsyncCompositionManager.cpp not so much. But while looking into this now I noticed that in nsGfxScrollFrameInner::BuildDisplayList the clip that we use it the display port, which doesn't seem right. If we don't need the clip to be updated async-ly then we should be able to use the right clip in nsGfxScrollFrameInner::BuildDisplayList. The clip shouldn't change if we are just aynsc scrolling the sub-element, but if we are async scrolling the parent that might not work.

kats, comment 0 implies we should be applying this clip in AsyncCompositionManager, but if the clip doesn't need to be updated async then it seems like it would make sense to do it in nsGfxScrollFrameInner::BuildDisplayList. What do you think?

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think ScrollLayerWrapper should set a cliprect on the nsDisplayScrollLayers it creates, clipping them to the scrollport. That should take care of everything.

Comment 15

[Kartikaya Gupta (email:[e-mail])]

(In reply to Timothy Nikkel (:tn) from comment #13)
> But while looking into this now I noticed that in
> nsGfxScrollFrameInner::BuildDisplayList the clip that we use it the display
> port, which doesn't seem right.

Yeah that doesn't seem right.

> If we don't need the clip to be updated
> async-ly then we should be able to use the right clip in
> nsGfxScrollFrameInner::BuildDisplayList. The clip shouldn't change if we are
> just aynsc scrolling the sub-element, but if we are async scrolling the
> parent that might not work.
>
> kats, comment 0 implies we should be applying this clip in
> AsyncCompositionManager, but if the clip doesn't need to be updated async
> then it seems like it would make sense to do it in
> nsGfxScrollFrameInner::BuildDisplayList. What do you think?

I still don't know what the clip is interpreted relative to so it makes it hard to answer this. Say you have a container layer A with a child layer B, and there is a clip set on layer B. Because of the clip you can currently only see the centermost pixel of layer B's contents. Now, if we set a transform on layer B, do you still see the centermost pixel of layer B? What if you set a transform on layer A? If the answers are "no" and "yes" respectively then I think yes, the clip should be settable in layout code and everything will work.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)
> I still don't know what the clip is interpreted relative to so it makes it
> hard to answer this. Say you have a container layer A with a child layer B,
> and there is a clip set on layer B. Because of the clip you can currently
> only see the centermost pixel of layer B's contents. Now, if we set a
> transform on layer B, do you still see the centermost pixel of layer B?

No. Layer::SetClipRect sets a clip that is applied after B's transform. See the comment in Layers.h:
The coordinates are relative to
   * the parent layer (i.e. the contents of this layer
   * are transformed before this clip rect is applied).

> What
> if you set a transform on layer A? If the answers are "no" and "yes"
> respectively then I think yes, the clip should be settable in layout code
> and everything will work.

Those are the answers. Yes, comment #14 should work :-).

Comment 17

[Timothy Nikkel (:tnikkel)]

Attachment: patch

This seems to fix the problem.

Comment 18

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2668b492d852

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2668b492d852

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/b0d9654813fe

Comment 21

[Kartikaya Gupta (email:[e-mail])]

\o/ Thanks! Verified this fixes the issue on my test page. Tested on 1.2 on a hamachi device.