<browser remote> doesn't properly handle scrolling on its own

RESOLVED FIXED

Status

()

Core
Layout
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: cjones, Assigned: cjones)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [e10s])

Attachments

(2 attachments, 2 obsolete attachments)

STR
 (1) Load
firefox -no-remote -chrome chrome://global/content/test-ipcbrowser.xul
 (2) Enter http://people.mozilla.com/~cjones/testanchor.html#anchor into the URL bar

Only a gray screen shows up, not the text as expected.  The platform has all the right rendered pixels, they're just not being drawn at the right place.

mbrubeck was kind enough to point me at bug 598391, which AFAICT implemented this for fennec's setup.

The fundamental problem is, our shadow-layer code was designed with fennec in mind, which uses asynchronous scrolling and re-rendering.  This ends up meaning that the "real" scroll offset drawn to screen in the chrome process is usually different than the scroll offset web content sees.  This is never the case in desktop firefox, which synchronously scrolls content.  (Well, depending on how one defines "the scroll offset web content sees".  Let's pretend that this is true.)

So, long story short, for <browser remote> the frontend must manually tell the platform what the frontend wants the scroll offset to be (http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsFrameLoader.cpp#1507).  It doesn't have to be that way; the platform knows what the scroll offset is.  This test would actually be working if it weren't for the code to support fennec.

This has two implications
 (1) (minor) What should the default be?  Async or sync scrolling?  I lean towards default-sync, and having fennec set some flag that tells the platform it wants async scrolling.
 (2) (major) What scrolling behavior do we want in FF5?  Stay with the current model, or adopt a model like fennec's?  (Probably sans checkerboarding; that is, don't allow panning into garbage space.)

Fixing (1) is what this bug will cover.  It's actually somewhat nontrivial, with the interface freeze and impact on fennec.

I'm bringing up (2) so that we firefox and fennec folk can start finding a solution that makes everyone happy.
Created attachment 496773 [details] [diff] [review]
Proof-of-concept patch

This is the effect we want for a real fix for this bug.
Assignee: nobody → jones.chris.g
(This bug also affects scrollTo/scrollBy.)
Summary: <browser remote> doesn't load anchored URLs properly on its own → <browser remote> properly handle scrolling on its own
Is this related/same problem as bug 615395?
Yes!

Mind if I dup that to this, since we've got more discussion+patch here?
Summary: <browser remote> properly handle scrolling on its own → <browser remote> doesn't properly handle scrolling on its own
Duplicate of this bug: 615395
not at all, already dup'ed
Whiteboard: [e10s]
Created attachment 497035 [details] [diff] [review]
Default remote-browser to synchronous scrolling, and let users change that

See comment 0.
Attachment #496773 - Attachment is obsolete: true
Attachment #497035 - Flags: superreview?(roc)
Attachment #497035 - Flags: review?(tnikkel)
Created attachment 497038 [details] [diff] [review]
Enable async scrolling for fennec (BROKEN!)

None of the approaches here seem to be able to get at the browser's nsIFrameLoader.  Pointers very much appreciated.
Attachment #497038 - Flags: feedback?(mark.finkle)
Comment on attachment 497038 [details] [diff] [review]
Enable async scrolling for fennec (BROKEN!)

I would expect this one to work:

let fl = browser.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader;

but I think you need to do that _after_ we add it to the document, a few lines below. How about trying this after the | browser.stop() | call?

This looks like the right general area to set the flag though
Attachment #497038 - Flags: feedback?(mark.finkle) → feedback+
Created attachment 497047 [details] [diff] [review]
Enable async scrolling for fennec

Works!
Attachment #497038 - Attachment is obsolete: true
Attachment #497047 - Flags: review?(mark.finkle)
Attachment #497047 - Flags: review?(mark.finkle) → review+
Attachment #497035 - Flags: superreview?(roc) → superreview+
Attachment #497035 - Flags: review?(tnikkel) → review+
Comment on attachment 497035 [details] [diff] [review]
Default remote-browser to synchronous scrolling, and let users change that

Would like to land this so that we can turn on reftest-ipc.
Attachment #497035 - Flags: approval2.0?
Comment on attachment 497047 [details] [diff] [review]
Enable async scrolling for fennec

Would like to land this so that we can turn on reftest-ipc.
Attachment #497047 - Flags: approval2.0?
No longer blocks: 615386
Approval ping.
Depends on: 625985
You need to log in before you can comment on or make changes to this bug.