Scissor rect incorrect with scaled viewport

VERIFIED FIXED in Firefox 14

Status

()

Core
Layout
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: tnikkel)

Tracking

Trunk
mozilla15
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(firefox14+ verified, firefox15 verified, blocking-fennec1.0 beta+)

Details

(Whiteboard: [layout][mozilla-central])

Attachments

(8 attachments, 5 obsolete attachments)

(Reporter)

Description

5 years ago
Created attachment 618830 [details] [diff] [review]
incomplete attempt to fix

I have a fix for bug 728026, but it uncovers this bug in layout.

The scissor rects we compute when we're going to be drawing are wrong. As currently written, we multiply them by the scale, which is wrong when we're downscaling (eg xx = 0.5). Unfortunately, simply removing the scale (this patch) doesn't work, because the clip rects we've got are sometimes too small for the page (eg on the desktop rollingstone.com).

If you apply this patch, then go to rollingstone.com and go to the desktop page on a Galaxy Nexus, you will see something very much like what the screenshot I'm going to attach looks like.

I don't know where to start for this. BasicLayers seemingly don't scale their clip/scissor rects, which is why I tried this patch. I'm not sure why a container rect on the desktop rollingstone.com has the idea that it needs to be smaller than the full size of the viewport.
(Reporter)

Comment 1

5 years ago
Created attachment 618832 [details]
screenshot
Layers.h:
  /**
   * CONSTRUCTION PHASE ONLY
   * Set a clip rect which will be applied to this layer as it is
   * composited to the destination. The coordinates are relative to
   * the parent layer (i.e. the contents of this layer
   * are transformed before this clip rect is applied).
   * For the root layer, the coordinates are relative to the widget,
   * in device pixels.
   * If aRect is null no clipping will be performed. 
   */
  void SetClipRect(const nsIntRect* aRect);

As far as I can tell, the code you're attempting to change is already correct. 'container' isn't drawing to its own intermediate surface, so we'll be drawing to the intermediate surface of some ancestor. That means 'this''s clip rect must be transformed by 'container's transform.
A dump of the shadow layer tree might help here.
(Reporter)

Comment 4

5 years ago
Created attachment 618892 [details]
layer dump
(Reporter)

Comment 5

5 years ago
We have container layers with clips set on them, and these clips are being sent across correctly. However, the clips themselves seem to be wrong, and I don't know why. I don't understand enough of FrameLayerBuilder to comment meaningfully on this. :(
Whiteboard: [layout]
(Reporter)

Comment 6

5 years ago
Matt said he was unable to reproduce this, but I just updated and I still can. :(
(Reporter)

Comment 7

5 years ago
We need a fix for this before we release beta.
blocking-fennec1.0: ? → beta+
Do you have any other patches applied? My build was just trunk, plus the patch from  bug 728026 applied, not this one.

Some thoughts:

The layer dump you provided was for the content side, it would be nice to have the matching shadow layer tree. Calling LayerManager::Dump somewhere in Compositor::Composite() should work?

The content layer (the last ThebesLayer) is only 720x1557. Scaled by 0.5,0.5, this doesn't seem like it would cover the screen regardless of clipping. Since it does, I'd suggest this this region is being reported incorrectly or in the wrong coordinate space.

The 4th layer (ContainerLayer) has a clip twice the size of its visible region. This doesn't make sense either, and is probably wrong.
(Reporter)

Comment 9

5 years ago
Created attachment 619560 [details]
minimal testcase
(Reporter)

Comment 10

5 years ago
The only patch I have applied is the one from bug 728026, but note it needs unbitrotting (I will upload a new version to that bug).

Comment 11

5 years ago
Joe, can you get the ShadowLayer dump mentioned in #8?
(Reporter)

Comment 12

5 years ago
Created attachment 619976 [details]
screenshot of testcase

Note: it is equally wrong for something like this screenshot to appear. If you pan or zoom on the page, you'll see that it's actually clipping the page, though the clipped area is not black, but white.
Who owns this?

Comment 14

5 years ago
I spoke with tn about this one yesterday. Assigning...
Assignee: nobody → tnikkel
Status: NEW → ASSIGNED
(Reporter)

Comment 15

5 years ago
Created attachment 620040 [details]
content side layer tree dump
(Reporter)

Comment 16

5 years ago
Created attachment 620042 [details]
compositor side layer tree dump
(Reporter)

Comment 17

5 years ago
Created attachment 620077 [details]
content side layer tree dump
Attachment #620040 - Attachment is obsolete: true
(Reporter)

Comment 18

5 years ago
Created attachment 620079 [details]
compositor side layer tree dump
Attachment #620042 - Attachment is obsolete: true
(Reporter)

Comment 19

5 years ago
Created attachment 620094 [details]
content side layer tree dump

This dump mostly matches the compositor side dump.
Attachment #620077 - Attachment is obsolete: true
(Assignee)

Comment 20

5 years ago
Unfortunately the minimal testcase renders properly on my Galaxy S II.
It would be interesting to see the equivalent layer trees from your device Tim.

The unwanted clip is fairly obvious in Joe's layer tree examples, I wonder why you're not being affected by them.
(Reporter)

Comment 22

5 years ago
I've been able to reproduce this on a Galaxy Nexus, a Galaxy S 2, and a Nexus S, so I am sort of surprised that others have been having problems.

Tim: You've applied the patch in bug 728026, right?
(Reporter)

Comment 23

5 years ago
I'm building a try build at https://tbpl.mozilla.org/?tree=Try&rev=d0fbb635b3c7 to see if it has the same behaviour as my local builds.
(Assignee)

Comment 24

5 years ago
(In reply to Joe Drew (:JOEDREW!) from comment #22)
> Tim: You've applied the patch in bug 728026, right?

Oops, no. I'm a little dumb sometimes.
(Assignee)

Comment 25

5 years ago
(In reply to Joe Drew (:JOEDREW!) from comment #22)
> Tim: You've applied the patch in bug 728026, right?

Ok, I see the same thing as the screenshot of the minimal testcase with that patch applied.
(Assignee)

Comment 26

5 years ago
I'm debugging exactly why the clipping goes wrong here.
(Assignee)

Comment 27

5 years ago
Created attachment 620885 [details]
minimal testcase
Attachment #619560 - Attachment is obsolete: true
(Assignee)

Comment 28

5 years ago
So what actions does "width=device-width;" actually make us take?

Comment 29

5 years ago
Adding Brad, Mark who might know answer to #28.

Comment 30

5 years ago
Looks like jwir3 and dbaron are actually responsible for some device-width related code.
(In reply to Timothy Nikkel (:tn) from comment #28)
> So what actions does "width=device-width;" actually make us take?

It makes the CSS viewport be the same width as the device. i.e. If you have a device like the Galaxy Nexus with a usable screen area of 720x1038 in portrait, we will call DOMWindowUtils::setCSSViewport with a width of 720. If the user rotates the device to landscape, we call setCSSViewport again with a width of 1038.

The code for this is largely at http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2363
(Assignee)

Comment 32

5 years ago
(In reply to Kartikaya Gupta (:kats) from comment #31)
> It makes the CSS viewport be the same width as the device. i.e. If you have
> a device like the Galaxy Nexus with a usable screen area of 720x1038 in
> portrait, we will call DOMWindowUtils::setCSSViewport with a width of 720.
> If the user rotates the device to landscape, we call setCSSViewport again
> with a width of 1038.

On my Galaxy S II (wikipedia tells me it has a 480x720 screen) with the minimal testcase in this bug I see setCSSViewport calls with 320x460. So this is not the screen width but the resolution is set to 1.5, and 320 * 1.5 = 480. Is this what you would expect?
Oh, we also do some scaling of the CSS viewport because of the screen dpi, so that things don't appear tiny on high-density displays. There's a pref that can be used to override it if you want to disable that behaviour.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3675
(Assignee)

Comment 34

5 years ago
Here is what happens on my device. setCSSViewport call with 320x460. Document size is now set to that. We create clips (by the scroll frame, and for the subdocument) with this width. When we apply these clips to layers we multiply by the resolution (1.5) to get 480 as a width. The root layer has a 0.6666667 scale transform corresponding to the inverse of the resolution, so when these 480 wide clip gets to the screen it is 320 wide.
(Assignee)

Comment 35

5 years ago
So I think the problem is that CompositorParent::TransformShadowTree sets a scale transform on the main scrollable layer that the shadowable layers and layout don't know exist. We correctly scale the contents of the layer and then apply the (non-scaled) clip of that layer as comment 2 points out. Since this transform is added after layout has updated the layer tree and only on the shadow layers, layout has no idea it exists.
(Assignee)

Comment 36

5 years ago
Setting that scale transform on the parent layer fixes this bug, but you get different bugs while panning and zooming.

Comment 37

5 years ago
Adding in Benoit and Ali as well for #36

Comment 38

5 years ago
(In reply to Timothy Nikkel (:tn) from comment #36)
> Setting that scale transform on the parent layer fixes this bug, but you get
> different bugs while panning and zooming.

Setting that scale transform where? On the shadowable side? If so, how do you account for that transform changing during asynchronous zooming?

Would it help if TransformShadowTree transformed the ShadowClipRect by the same scale applied to the layer?
(Assignee)

Comment 39

5 years ago
(In reply to Ali Juma [:ajuma] from comment #38)
> (In reply to Timothy Nikkel (:tn) from comment #36)
> > Setting that scale transform on the parent layer fixes this bug, but you get
> > different bugs while panning and zooming.
> 
> Setting that scale transform where? On the shadowable side? If so, how do
> you account for that transform changing during asynchronous zooming?

On the shadow side. So where CompositorParent::TransformShadowTree calls SetShadowTransform change that call to happen on the parent of that layer basically.

> Would it help if TransformShadowTree transformed the ShadowClipRect by the
> same scale applied to the layer?

Yeah, probably. Haven't tried that yet.
(Assignee)

Comment 40

5 years ago
(In reply to Ali Juma [:ajuma] from comment #38)
> Would it help if TransformShadowTree transformed the ShadowClipRect by the
> same scale applied to the layer?

That fixes the minimal testcase here and panning zooming work fine on it but it severely screws up bugzilla pages, and I've only done minimal testing.
The problem is that the clip rect set on the scrolled layers is the intersection of clipping imposed by containers and the clipping imposed by the scrollframe itself. The latter should be adjusted to account for the shadow transform, and the former should not.

(In reply to Timothy Nikkel (:tn) from comment #34)
> Here is what happens on my device. setCSSViewport call with 320x460.
> Document size is now set to that. We create clips (by the scroll frame, and
> for the subdocument) with this width.

Seems to me we should be setting clipSubdocument to false for the <browser>, then none of this clipping would be happening.

For async scrolling of scrollframes other than the root scroll frame, we need a different solution though.
(Assignee)

Comment 42

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> The problem is that the clip rect set on the scrolled layers is the
> intersection of clipping imposed by containers and the clipping imposed by
> the scrollframe itself. The latter should be adjusted to account for the
> shadow transform, and the former should not.

Does layout know what the shadow transform is going to be? Or is it set on the compositor thread to support asynchronous panning/zooming?

> Seems to me we should be setting clipSubdocument to false for the <browser>,
> then none of this clipping would be happening.

I tried setting clipSubdocument to false, it caused a lot of other problems. But also, as the code is now, it would not remove that scrollframe clipping.
(In reply to Timothy Nikkel (:tn) from comment #42)
> Does layout know what the shadow transform is going to be?

No.

> Or is it set on the compositor thread to support asynchronous panning/zooming?

Yes.

> > Seems to me we should be setting clipSubdocument to false for the <browser>,
> > then none of this clipping would be happening.
> 
> I tried setting clipSubdocument to false, it caused a lot of other problems.
> But also, as the code is now, it would not remove that scrollframe clipping.

Why not? We would hit the "if (aBuilder->GetIgnoreScrollFrame() == mOuter || IsIgnoringViewportClipping())" path in nsGfxScrollFrameInner::BuildDisplayList.

Another approach would be to rejigger the display item setup. Currently we wrap child items in nsDisplayScrollLayers and then wrap those in nsDisplayClips. Instead we could wrap in the other order. Then I think things just might work; the overflow clip due to scrolling would be applied to the child layers of the layer created by nsDisplayScrollLayer.
(Assignee)

Comment 44

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #43)
> > I tried setting clipSubdocument to false, it caused a lot of other problems.
> > But also, as the code is now, it would not remove that scrollframe clipping.
> 
> Why not? We would hit the "if (aBuilder->GetIgnoreScrollFrame() == mOuter ||
> IsIgnoringViewportClipping())" path in
> nsGfxScrollFrameInner::BuildDisplayList.

Sorry, you are right about that. But it looks like that path also bypasses creating scroll layer items completely, which is probably why I got a completely unusable browser when I tried that.

> Another approach would be to rejigger the display item setup. Currently we
> wrap child items in nsDisplayScrollLayers and then wrap those in
> nsDisplayClips. Instead we could wrap in the other order. Then I think
> things just might work; the overflow clip due to scrolling would be applied
> to the child layers of the layer created by nsDisplayScrollLayer.

Yeah. We'd still have to do something about the subdocument clip if clipSubdocument doesn't currently work for our needs there.
(In reply to Timothy Nikkel (:tn) from comment #44)
> Sorry, you are right about that. But it looks like that path also bypasses
> creating scroll layer items completely, which is probably why I got a
> completely unusable browser when I tried that.

True.

> > Another approach would be to rejigger the display item setup. Currently we
> > wrap child items in nsDisplayScrollLayers and then wrap those in
> > nsDisplayClips. Instead we could wrap in the other order. Then I think
> > things just might work; the overflow clip due to scrolling would be applied
> > to the child layers of the layer created by nsDisplayScrollLayer.
> 
> Yeah. We'd still have to do something about the subdocument clip if
> clipSubdocument doesn't currently work for our needs there.

CompositorParent could just remove the clip rect induced by the <browser>.
(Assignee)

Comment 46

5 years ago
Created attachment 621906 [details] [diff] [review]
patch

Something like this.
(Assignee)

Comment 47

5 years ago
Created attachment 622140 [details] [diff] [review]
patch

The subdocument clip wasn't actually a problem as it's outside the subdocument, and it doesn't make it through the container layer for the subdocument.

The reftest becomes unexpected pass with this patch. I haven't investigated why, it might be just an accident, so I don't want to force people to keep it passing on desktop platforms if it was a fluke.

We also had to use the display port rect for clipping otherwise we clip that out and we notice the redrawing when panning or zooming out.

Was there any particular reason that using an nsDisplayListCollection instead of a nsDisplayListSet was important here? Because this patch makes us do the scroll layer wrapping on the final nsDisplayListSet instead of the temp nsDisplayListCollection.
Attachment #622140 - Flags: review?(roc)
(Assignee)

Comment 48

5 years ago
Try server build with this patch
https://tbpl.mozilla.org/?tree=Try&rev=4dbf3696bdfa

Try server build with this patch and bug 728026
https://tbpl.mozilla.org/?tree=Try&rev=b7e14cd82e3a
Comment on attachment 622140 [details] [diff] [review]
patch

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

very nice!

::: layout/generic/nsGfxScrollFrame.cpp
@@ +2248,2 @@
>    nsRect clip;
>    clip = mScrollPort + aBuilder->ToReferenceFrame(mOuter);

move this line down into the !usingDisplayport path

@@ +2250,5 @@
>  
> +  nscoord radii[8] = {0, 0, 0, 0, 0, 0, 0, 0};
> +
> +  if (usingDisplayport) {
> +    clip = displayPort + aBuilder->ToReferenceFrame(mOuter);

remove the array initializer and memset radii to zero here
Attachment #622140 - Flags: review?(roc) → review+
(Reporter)

Comment 50

5 years ago
In order to get tests in finite time, with Ehsan's blessing I am reusing Oak since it's not prioritized too low.
(Reporter)

Comment 51

5 years ago
https://tbpl.mozilla.org/?tree=Oak&rev=199f4171030c
(Reporter)

Comment 52

5 years ago
ftp://ftp.mozilla.org/pub/mobile/tinderbox-builds/oak-android/1336516588/ has the build with both this fix and bug 728026, ready for testing. (Still going through unit tests, though, so I haven't landed it anywhere.)
(Reporter)

Comment 53

5 years ago
https://hg.mozilla.org/mozilla-central/rev/dcecf246f732
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
status-firefox15: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
(Reporter)

Updated

5 years ago
Attachment #622140 - Flags: approval-mozilla-aurora?
status-firefox15: fixed → ---
status-firefox15: --- → fixed

Updated

5 years ago
Whiteboard: [layout] → [layout][mozilla-central]
Comment on attachment 622140 [details] [diff] [review]
patch

Let's get this landed asap for today's anticipated Beta 1 go to build.
Attachment #622140 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
status-firefox14: --- → affected
tracking-firefox14: --- → +
(Assignee)

Comment 55

5 years ago
https://hg.mozilla.org/releases/mozilla-aurora/rev/23178dc7e035
status-firefox14: affected → fixed
(Assignee)

Updated

5 years ago
Attachment #621906 - Attachment is obsolete: true
This issue seems to be fixed on the latest Nightly and Aurora build. Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-30)
Firefox 14.0a2 (2012-05-30)

Device: Galaxy Nexus
OS: Android 4.0.2
Status: RESOLVED → VERIFIED
status-firefox14: fixed → verified
status-firefox15: fixed → verified
You need to log in before you can comment on or make changes to this bug.