Description: Going to Settings > Device Information > Credits will bring up a list of many, many contributors. This very long page does not contain any scroll-bars. Repro Steps: 1) Update a Flame to 20150217074222 2) Navigate to Settings > Device Information > Credits 3) Scroll the page Actual: Scrollbar is missing Expected: When a page can be scrolled / is being scrolled a scrollbar will be present Environmental Variables: Device: Flame 3.0 Build ID: 20150217074222 Gaia: ae02fbdeae77b2002cebe33c61aedeee4b9439fd Gecko: 4bb425001d8a Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 38.0a1 (3.0) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:38.0) Gecko/38.0 Firefox/38.0 Repro frequency: 7/7 See attached: logcat -------------------------------------------------------------------------- This issue reproduces in 2.2 and 2.1 Credits were not yet implemented prior to 2.1 Device: Flame 2.2 (KK - Nightly - Full Flash) Build ID: 20150217002515 Gaia: ea64caf6d4ab03fc4472eca9f41f20d651d55fa9 Gecko: 78d823f7be4c Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 37.0a2 (2.2) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:37.0) Gecko/37.0 Firefox/37.0 Device: Flame 2.1 (KK - Nightly - Full-Flashed) Build ID: 20150217001459 Gaia: e8eba437af02820f74d122aec83b6001df6f89e3 Gecko: 9d04f9149ca4 Gonk: e7c90613521145db090dd24147afd5ceb5703190 Version: 34.0 (2.1) Firmware Version: v18D-1 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

NI on component owner for nomination decision and assignment.

I don't know the reason. This web page has scrollbar when browsing from desktop Firefox. NI browser developer.

I don't know either. I see a scroll bar on some web pages, but not others. I'm not sure how the platform decides when to show them.

(In reply to Ben Francis [:benfrancis] PTO until 3rd March from comment #3) > I don't know either. I see a scroll bar on some web pages, but not others. > I'm not sure how the platform decides when to show them. Alive, do you know the reason?

Not a gaia issue if the scrollbar is really missing. BTW, please compare with another mobile browser instead of desktop browser before you think it's difference so it's a bug - there're tons of difference between desktop browser and mobile browser.

In the android version of firefox browser on 4.4 kitkat OS, the scroll bar is seen during scrolling, so I think it qualifies as a bug.

Milan, would this be a graphics layout issue?

Kats, thoughts?

According to the layers dump, there is a scrollbar, but I don't see it. I wonder if it's getting scaled too small or something. Leaving ni? on me to investigate further. It should be easy enough to make a test case that can get us a regression window, since the credits page is only 2.2+

Here's a simpler test case. It does have to do with the viewport width; when it is 320 (on Flame) the scrollbar appears in the right spot. As it goes up the scrollbar gets pushed off to the right more.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #9) > since the credits page is only 2.2+ My mistake, credits page is 2.1 and up. Not much point getting a regression window then, might as well just fix it.

tn, mind taking this one?

Isn't the relationship that layout puts the scrollbars at the layout viewport (defined by nsIDOMWindowUtils::SetCSSViewport), and then APZC transforms there to where they should be? That's what I think is happening in general for all pages anyway. So if that is the case then it seems like layout is putting the scrollbars in the right spot.

No, I think the scrollbars are supposed to be positioned by layout at the scrollPositionClampingScrollPortSize rather than the CSS viewport. On desktop the two are always equivalent, becauase we don't support either the meta-viewport tag or zooming, so both of these are the same as the viewport/composition bounds. Even on B2G as long as the viewport is set to device-width (which it is in all the apps), these two will be the same. We probably never ran into this case before because it only shows up in the browser, and then only if there's no viewport tag or if the viewport tag is set to something other than device-width. I probably never bothered testing such a case, since all of my test pages have viewport tags. The two properties will also diverge on desktop if the CSS viewport is explicitly set to something other than default (which is what's happening in 1147038).

Also, for reference, the compositor adjusts the scrollbars in two ways: 1) For any async transform that is still pending. So in the "steady state" this is a no-op. 2) Unapplying the resolution from content, in the case where the scrollbar layer is a descendant of the content layer. This is *not* to reposition the scrollbars, but it is to prevent them from appearing "fatter"/"thinner". In all cases, the compositor expects the scrollbars to already be positioned at the right location in the steady state, and that means the scrollbars should be attached to the scrollPositionClampingScrollPortSize dimensions (or the composition bounds, which should be representing the same visual area but in a different unit/coordinate system).

Oh wait I might be wrong. It might be that the scrollbars need to be positioned at the composition bounds size (but not the scrollPositionClampingScrollPortSize). And *that* is the thing that is usually the same as the CSS viewport. Will stare at the code a bit more...

This seems to mostly work, but there is a little bit of work to do on the compositor side to fix up the scrollbars.

This seems to fix it on the compositor side although to be honest I don't really know why :p. I didn't do the math, just tried a few different scenarios to isolate the factor that was causing the incorrect behaviour.

Comment on attachment 8594043 [details] [diff] [review] layout scroll bars Markus, do you want to review this? (I'll remove the commented out lines of course.)

Is it possible to write a test for this?

I can review it, but a comment or two in the code would be nice. And in my opinion, good tests for this are more valuable than a good implementation. (The test should also test RTL scrollbars. And I don't see the value in keeping the "true ? " thing for the unreachable scrollbars-on-top case...)

I think I should be able to come up with some reftests for this with a little creativity. I can add some comments, but they will basically say "scale this do that so the compositor can scale and place the scroll bars properly" because I don't really know what it's doing, or why it works to lay out the scrollbars like this.

Comment on attachment 8594043 [details] [diff] [review] layout scroll bars Sounds good to me.

Is your patch ready for review?

Comment on attachment 8594908 [details] [diff] [review] compositor-side scrollbar fix Review of attachment 8594908 [details] [diff] [review]: ----------------------------------------------------------------- Botond, thoughts on this? Goes with the layout-side change.

This is really hard to test. I can't think of a way to test this with reftests because I can't think of a change to make to the page that would make the test pass when the scrollbars are in the right place but fail when they are not. We cannot put any content over the scrollbars (gecko will always one up us if we attempt to). Doing a snapshot in a mochitest (and manually inspecting the pixel data to make sure scrollbars are in the right place) can't work either. Taking a snapshot of a content document with the WIDGET_LAYERS flag of a root content doc (the only kind where we can get zooming so we can see this problem) is forbidden by this line http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp?rev=d22a980dd1e1#4898 and if we can't use widget layers then we don't add the scrollbars in ScrollFrameHelper::BuildDisplayList at all. And special powers is not allowed in reftests, so we can't do a window snapshot and inspect the data in a reftest either.

(In reply to Timothy Nikkel (:tn) from comment #26) > This is really hard to test. I can't think of a way to test this with > reftests because I can't think of a change to make to the page that would > make the test pass when the scrollbars are in the right place but fail when > they are not. Can you compare an async-zoomed page to a non-zoomed one?

FYI we have bug 1151617 filed about writing tests for the scrollbar positioning.

Comment on attachment 8594908 [details] [diff] [review] compositor-side scrollbar fix Review of attachment 8594908 [details] [diff] [review]: ----------------------------------------------------------------- Discussed this with Kats on IRC. The correspondence between this change and the layout-side change isn't clear to us, nor do we have a clear picture of what, if anything, goes wrong without this patch, so I'm going to drop the review flag until we test that.

Comment on attachment 8594908 [details] [diff] [review] compositor-side scrollbar fix This patch doesn't belong on this bug. The wonky behaviour was pre-existing, and I filed bug 1158933 for it.

tn, feel free to land your patch.

These work in that they fail before this patch and pass after, but I'm not completely happy with them. Something is better than nothing though as I was unable to make anything better.

Comment on attachment 8599971 [details] [diff] [review] some reftests I was hoping for something with reftest-async-zoom (which I now realize we don't even have), but sure, this is better than nothing.

(In reply to Markus Stange [:mstange] from comment #33) > I was hoping for something with reftest-async-zoom (which I now realize we > don't even have), but sure, this is better than nothing. I implemented this but when my tests passed without the patch in this bug I realized that it does not test the problem. The steady-state position of the scrollbars is wrong. Not the async state. Still it seems like a useful enough feature, I'll land it in another bug.

Oh I see. Cool, I think it will be useful.

(In reply to Timothy Nikkel (:tn) from comment #34) > (In reply to Markus Stange [:mstange] from comment #33) > > I was hoping for something with reftest-async-zoom (which I now realize we > > don't even have), but sure, this is better than nothing. > > I implemented this but when my tests passed without the patch in this bug I > realized that it does not test the problem. The steady-state position of the > scrollbars is wrong. Not the async state. Still it seems like a useful > enough feature, I'll land it in another bug. It'll come in very useful for bug 1151617!

Filed reftest-async-zoom as bug 1160642.

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf932c7a258d https://hg.mozilla.org/integration/mozilla-inbound/rev/37a740847aed

https://hg.mozilla.org/mozilla-central/rev/cf932c7a258d https://hg.mozilla.org/mozilla-central/rev/37a740847aed

We might need this backed out because it is causing a blocker bug 1162648.