Closed
Bug 916125
Opened 11 years ago
Closed 11 years ago
When displayports are set on elements, their layers are not always clipped correctly
Categories
(Core :: Graphics: Layers, defect)
Tracking
()
People
(Reporter: kats, Assigned: tnikkel)
References
Details
(Keywords: regression)
Attachments
(1 file)
(deleted),
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
+++ 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).
Reporter | ||
Comment 1•11 years ago
|
||
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.
Reporter | ||
Comment 2•11 years ago
|
||
This might be related to the clipping problem in bug 886969.
Comment 4•11 years ago
|
||
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.
Flags: needinfo?(matt.woodrow)
Reporter | ||
Comment 5•11 years ago
|
||
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•11 years ago
|
||
Why does the clip need to change? Isn't the viewport (scrollport?) staying in the same spot?
Reporter | ||
Comment 7•11 years ago
|
||
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.
Updated•11 years ago
|
blocking-b2g: koi? → ---
Updated•11 years ago
|
Assignee: nobody → bugmail.mozilla
blocking-b2g: --- → koi+
Reporter | ||
Comment 8•11 years ago
|
||
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.
Flags: needinfo?(milan)
Comment 9•11 years ago
|
||
Nick, is this something you can handle? 1.2 blocker.
Assignee: bugmail.mozilla → ncameron
Flags: needinfo?(milan) → needinfo?(ncameron)
Comment 10•11 years ago
|
||
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.
Flags: needinfo?(ncameron)
Comment 12•11 years ago
|
||
Timothy, is this in the part of the code you know?
Flags: needinfo?(bugs) → needinfo?(tnikkel)
Assignee | ||
Comment 13•11 years ago
|
||
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?
Flags: needinfo?(tnikkel) → needinfo?(bugmail.mozilla)
I think ScrollLayerWrapper should set a cliprect on the nsDisplayScrollLayers it creates, clipping them to the scrollport. That should take care of everything.
Assignee: ncameron → tnikkel
Reporter | ||
Comment 15•11 years ago
|
||
(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.
Flags: needinfo?(bugmail.mozilla)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 :-).
Assignee | ||
Comment 17•11 years ago
|
||
This seems to fix the problem.
Attachment #818695 -
Flags: review?(roc)
Attachment #818695 -
Flags: review?(roc) → review+
Assignee | ||
Comment 18•11 years ago
|
||
Comment 19•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Comment 20•11 years ago
|
||
status-b2g-v1.2:
--- → fixed
status-firefox25:
--- → wontfix
status-firefox26:
--- → fixed
status-firefox27:
--- → fixed
Reporter | ||
Comment 21•11 years ago
|
||
\o/ Thanks! Verified this fixes the issue on my test page. Tested on 1.2 on a hamachi device.
Updated•11 years ago
|
Blocks: gaia-apzc-2
Updated•11 years ago
|
No longer blocks: gaia-apzc-2
You need to log in
before you can comment on or make changes to this bug.
Description
•