Currently, due to the dynamic sizing of the rocketbar, pages are always scrollable, even when they would fit the screen if the browser presented itself with the correspondingly smaller height. We should use the capability that bug 757859 will add and only allow scrolling if the page actually needs it. My plan for this would be as follows: - Set the browser iframe size so that it doesn't cause rocketbar scrolling. - As soon as the page size is large enough that the rocketbar could collapse, change the browser iframe size to allow the rocketbar to collapse. - Reset the browser iframe size on locationchange/firstpaint. This is really necessary for pages like Google Maps, which don't allow scrolling and where currently we end up cutting off content at the bottom. Nominating this as a 2.1 blocker so it doesn't get lost.

Thanks for filing this, I've also been noticing this for the Uber mobile site, and I think this would fix that site as well.

[Blocking Requested - why for this release]: Blocks other blocking 2.1 bugs, somewhat breaks interaction with some major sites.

Kevin, I think you wanted to review this? I'm thinking Vivien is probably the most suitable reviewer though, so I'm flagging both and you guys can decide between you :) This requires the Gecko patch in bug 757859.

Comment on attachment 8480047 [details] Allow small pages not to scroll in system browser Mostly nits but before r+'ing I would like to understand 2 things: - Does the scrollable class is necessary for apps ? - Does >= should not be > ?

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #4) > Comment on attachment 8480047 [details] > Allow small pages not to scroll in system browser > > Mostly nits but before r+'ing I would like to understand 2 things: > - Does the scrollable class is necessary for apps ? Nope - I guess we can attach the listener only if it's a browser. > - Does >= should not be > ? No, >= is fine - strictly speaking, it'd be > windowHeight (i.e. browser iframe height, rather than container height, which includes the toolbar), but we found in fennec that due to people using -webkit-box-sizing and no unprefixed/moz-prefixed version, and just general mistakes, that too many sites were ending up 1 or 2 pixels higher than that height, so we just changed it to >= (windowHeight + toolbarHeight), which is kind of saying "the page is large enough that the toolbar can be entirely hidden". This means that some pages will be able to scroll a small amount without hiding the toolbar, but this is ok.

(In reply to Chris Lord [:cwiiis] from comment #5) > > - Does >= should not be > ? > > No, >= is fine - strictly speaking, it'd be > windowHeight (i.e. browser > iframe height, rather than container height, which includes the toolbar), > but we found in fennec that due to people using -webkit-box-sizing and no > unprefixed/moz-prefixed version, and just general mistakes, that too many > sites were ending up 1 or 2 pixels higher than that height, so we just > changed it to >= (windowHeight + toolbarHeight), which is kind of saying > "the page is large enough that the toolbar can be entirely hidden". This > means that some pages will be able to scroll a small amount without hiding > the toolbar, but this is ok. Sounds fine to me then, but it would be nice to add a comment explaining that. Otherwise it seems a mistake.

Comment on attachment 8480047 [details] Allow small pages not to scroll in system browser r+ with nits then.

Changes made and merged: https://github.com/mozilla-b2g/gaia/commit/8d2ff2ee95b3768c0eaa28174e57413d88b80b38 This relies on bug 757859, so I won't request approval until that gets merged. To reduce confusion, I'm leaving this open until then.

Comment on attachment 8480047 [details] Allow small pages not to scroll in system browser [Approval Request Comment] [Bug caused by] (feature/regressing bug #): [User impact] if declined: Error pages and short pages such as Google Maps will appear cut off, and possibly have inaccessible areas [Testing completed]: Been on master for a while now [Risk to taking this patch] (and alternatives if risky): Low risk [String changes made]: None

Dependent bug has hit aurora, marking this as fixed (as it's on master) and requesting approval.

v2.1: https://github.com/mozilla-b2g/gaia/commit/4cc95f54c07d42604e1c69e808eff549556e3846

Issue is verified fixed in Flame 2.2, 2.1 builds (Full Flash, nightly). Actual Results: Non-scrollable web pages behave correctly. Device: Flame Master Build ID: 20141017040208 Gaia: abef62c0623e5504a97b4fd411e879a67b285b52 Gecko: ae1dfa192faf Version: 36.0a1 (Master) Firmware Version: v180 User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0 Device: Flame 2.1 Build ID: 20141017001201 Gaia: 1ea74943cfe525c76a074ca1d7de8e51a70f6b98 Gecko: 2befa902ff5c Version: 34.0 (2.1) Firmware Version: v180 User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

https://github.com/Cwiiis/gaia/tree/bug1058637-marionette-test So, here's a marionette test for this bug, except it doesn't work because the rocketbar doesn't ever collapse on b2g-desktop. I'm guessing it's because of a lack of 'scroll' events from the browser iframe in this mode? n?ehsan to see if this is a reasonable theory - do we not get all the usual browser iframe events when running b2g via the desktop simulator, and is this something we can fix?

Thanks for the test Chris. Shame it doesn't collapse, that's going to make testing some things difficult. Also ni? on kats as he may be more familiar with scrolling code. Kats - please see comment 16, any idea what we need to do for better scrolling support in b2g desktop?

Last time I checked (which was a long time ago) the b2g-desktop code used the underlying platform widget code rather than the gonk widget code, so from the APZ point of view it's not at all like the emulator/device environment. That said, you'll likely still get the scroll event - but probably not the mozbrowserasynscroll event, since that's sent from the APZ code directly.

So I realised the reason this isn't working is probably because scrollgrab isn't working, not a lack of signals. I assume that scrollgrab is only implemented in apzc, is that correct? If so, how tricky would implementing scrollgrab in b2g-desktop be?

(In reply to Chris Lord [:cwiiis] from comment #19) > I assume that scrollgrab is only implemented in apzc, is that correct? Correct, scrollgrab is only available on APZ platforms. > If so, how tricky would implementing scrollgrab in b2g-desktop be? I'm not very familiar with b2g-desktop. Does it use BrowserElementPanning.js [1] for scrolling? (That's what actual B2G uses when APZ is disabled). Assuming that's the case, implementing scrollgrab would mean porting the scrollgrab logic in APZ to BEP.js. It shouldn't be too difficult in principle, but note that scrollgrab builds on other APZ features that are not available in BEP.js, either, notably regular scroll handoff from a child scrollable element to a parent scrollable element. Before scrollgrab could be ported, these features would need to be ported as well. The general direction that I'd encourage is to spend time on porting more platforms to APZ to take advantage features such as scroll handoff and scrollgrab, rather than porting these features to older scrolling implementations. [1] http://mxr.mozilla.org/mozilla-central/source/dom/browser-element/BrowserElementPanning.js?rev=687318d464a5