598439 - Shadow layer offset is incorrect when content scroll offset is non-zero

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Benjamin Stover (:stechz)]

A gray area appears below the URL bar before content begins.

Comment 1

[Benjamin Stover (:stechz)]

Attachment: fix

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Can you paste in the displayport+setResolution+viewportScroll+viewportScale configuration that triggers this bug?  It's not immediately clear to me what's wrong here, want to look in the ipc testapp.

Comment 3

[Benjamin Stover (:stechz)]

Comment on attachment 477263 [details] [diff] [review]
fix

As discussed on IRC, the reason this fixes our problem:
* mScrollOffset (the content scroll offset) is in CSS pixels
* mViewportScrollOffset (the viewportScroll*) is in device pixels

So we must convert one to the other before we add them together. Since we translate before we scale, we want them both in device pixels. Of course, there are other ways to do this.

From what I understand, there's no objection to leaving mViewportScrollOffset in device pixels, so let me know what I need to do to clean up this patch.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: test setup with viewportScale 1.0

(In reply to comment #3)
> Comment on attachment 477263 [details] [diff] [review]
> fix
> As discussed on IRC, the reason this fixes our problem:
> * mScrollOffset (the content scroll offset) is in CSS pixels
> * mViewportScrollOffset (the viewportScroll*) is in device pixels

The bug here is that mScrollOffset is in units of content-CSS pixels; it should be converted to device pixels in the content process.  mViewportScrollOffset is technically in app units, and it's properly treated from chrome-CSS pixels by the browser.scrollViewportTo/By API, and converted correctly here to device pixels.  The problem is we're computing the offset based on mixed units, content-CSS-pixels vs. device pixels.  However, this isn't the "real bug".

> So we must convert one to the other before we add them together. Since we
> translate before we scale, we want them both in device pixels. Of course, there
> are other ways to do this.
> 

Even if we fixed the content-CSS vs. device pixel mismatch in the translation computation here, it wouldn't change anything in normal cases; we're going to have one CSS pixel per device pixel except with fullZoom, which isn't a normal case on mobile ;).  The scale you're applying here to viewportScroll* is a fundamental change to spec that's orthogonal to the unit mismatch. 

> From what I understand, there's no objection to leaving mViewportScrollOffset
> in device pixels, so let me know what I need to do to clean up this patch.

It's not in device pixels: mViewportScrollOffset is in app units, and viewportScroll* are in chrome-CSS pixels.

Let's not worry about fullZoom for b1; let's assume content-CSS and chrome-CSS pixels are the same (setting a custom dpi was broken too last time I checked, so no need to worry about that either).

Currently I think that viewportScroll* is doing what you think it's doing, but it's necessarily clear that's what we want.  The attached screenshot is set up as shown in its params; the viewportScale is 1.0 and we have a compensating translate of <100,200> - <200,300> device pixels = <-100, -100> device pixels.  This is working as we would both expect I think.  The next screenshot I attach will be the meat of the discussion.

Comment 5

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: same setup with scale 2.0

Same as previous: we have a compensating translate of <100,200> - <200,300> device pixels = <-100, -100> device pixels.  However, the scale is 2.0; because of that, the content-CSS pixel <150, 250> is at top-left, because we're translating by exactly <-100, -100> device pixels.  This is unlike the previous testcase which put content-CSS pixel <100,200> at top-left.

So, we can either change the spec to make the compensating translation in the platform take viewportScale into account, as your patch does, or have the frontend do it.  I'm open to either way as long as it's documented.  Doing this automatically in the platform is probably more in the spirit of the original intent of these APIs, but let's have this discussion now.  Note that if we make this change, then viewportScroll* will *not* be in chrome-CSS (device) pixels anymore.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I used http://people.mozilla.com/~cjones/grid.html to test these out.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #4)
> Currently I think that viewportScroll* is doing what you think it's doing, but
> it's necessarily clear that's what we want.

*not* necessarily clear ;).

Comment 8

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: same as previous (scale=2.0) with a fixed-up version of Ben's patch

This changes the spec for viewportScroll* so that with scale 1.0 and scale 2.0, with the same viewportScroll*, the same pixel is drawn to the top-left of the <browser>.

If we agree that this is what we want, I'll request the change to Ben's patch in a review comment.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 477263 [details] [diff] [review]
fix

Please edit this comment in nsIFrameLoader

   * The viewport scroll values are in units of content-document CSS
   * pixels.

to make it clear that they're in units of chrome CSS pixels, and that the content-document's scroll offset has viewportScale applied to it (to highlight that viewportScroll* does *not* have the scale applied).

Comment 10

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Attachment: FTR, patch used to generated screenshot 3

Comment 11

[Benjamin Stover (:stechz)]

Attachment: Shadow layer offset is incorrect when content scroll offset is non-zero

Comment 12

[Benjamin Stover (:stechz)]

Pushed http://hg.mozilla.org/mozilla-central/rev/d322dec4e342