If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: stechz, Assigned: stechz)

Tracking

unspecified
Points:
---

Firefox Tracking Flags

(fennec2.0b1+)

Details

Attachments

(5 attachments, 1 obsolete attachment)

(Assignee)

Description

7 years ago
A gray area appears below the URL bar before content begins.
(Assignee)

Comment 1

7 years ago
Created attachment 477263 [details] [diff] [review]
fix
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.
Blocks: 598391
tracking-fennec: --- → ?
(Assignee)

Comment 3

7 years ago
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.
Attachment #477263 - Flags: review?(jones.chris.g)
Created attachment 477650 [details]
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.
Created attachment 477663 [details]
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.
I used http://people.mozilla.com/~cjones/grid.html to test these out.
(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 ;).
tracking-fennec: ? → 2.0b1+
Created attachment 477828 [details]
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 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).
Attachment #477263 - Flags: review?(jones.chris.g) → review+
Created attachment 477835 [details] [diff] [review]
FTR, patch used to generated screenshot 3
(Assignee)

Updated

7 years ago
Attachment #477263 - Attachment is obsolete: true
(Assignee)

Comment 11

7 years ago
Created attachment 477989 [details] [diff] [review]
Shadow layer offset is incorrect when content scroll offset is non-zero
(Assignee)

Updated

7 years ago
Attachment #477989 - Flags: review?(jones.chris.g)
Attachment #477989 - Flags: review?(jones.chris.g) → review+
(Assignee)

Comment 12

7 years ago
Pushed http://hg.mozilla.org/mozilla-central/rev/d322dec4e342
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
Duplicate of this bug: 597138

Updated

6 years ago
Assignee: nobody → ben
You need to log in before you can comment on or make changes to this bug.