Closed Bug 598439 Opened 14 years ago Closed 14 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0b1+ ---

People

(Reporter: stechz, Assigned: stechz)

References

Details

Attachments

(5 files, 1 obsolete file)

A gray area appears below the URL bar before content begins.
Attached patch fix (obsolete) (deleted) — Splinter Review
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: --- → ?
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)
Attached image test setup with viewportScale 1.0 (deleted) —
(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.
Attached image same setup with scale 2.0 (deleted) —
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.
(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+
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+
Attachment #477263 - Attachment is obsolete: true
Attachment #477989 - Flags: review?(jones.chris.g)
Attachment #477989 - Flags: review?(jones.chris.g) → review+
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Assignee: nobody → ben
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: