Last Comment Bug 770847 - [BrowserAPI] mozbrowserscroll event to inform embedder when the content has scrolled
: [BrowserAPI] mozbrowserscroll event to inform embedder when the content has s...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla17
Assigned To: Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: browser-api
  Show dependency treegraph
 
Reported: 2012-07-04 05:13 PDT by Ben Francis [:benfrancis]
Modified: 2013-04-04 13:53 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (7.83 KB, patch)
2012-07-07 06:40 PDT, Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)
justin.lebar+bug: review+
Details | Diff | Splinter Review

Description Ben Francis [:benfrancis] 2012-07-04 05:13:19 PDT
We need a new event on mozbrowser iframes which fires when the user scrolls the page, which reports scrollTop (and Dale suggests maybe scrollLeft as well for completeness).

The first use case for this event is to allow the Gaia browser's address bar to be scrolled on/off the screen as the user scrolls the content.
Comment 1 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-07 06:40:52 PDT
Created attachment 639954 [details] [diff] [review]
Patch

Justin do you know if reading window.scrollX/scrollY force a reflow? I think it doesn't but I want to be sure.

Also I'm not sure this feature alone resolve the use case described by Ben.

To be able to show/hide the titlebar of the browser application you need to have this information *and* the scrolling values not consumed by the content.
Comment 2 Justin Lebar (not reading bugmail) 2012-07-07 09:37:59 PDT
> Justin do you know if reading window.scrollX/scrollY force a reflow? I think it doesn't but I want 
> to be sure.

I don't, but bz should.

> To be able to show/hide the titlebar of the browser application you need to have this information 
> *and* the scrolling values not consumed by the content.

IOW, you need this and bug 757859, right?
Comment 3 Justin Lebar (not reading bugmail) 2012-07-07 09:45:20 PDT
Comment on attachment 639954 [details] [diff] [review]
Patch

It looks like reading scroll{X,Y} can cause us to flush invalidations on the parent window (nsGlobalWindow::EnsureSizeUpToDate()).  I don't know if that's the same thing as reflow.  But in any case, OOP browser tabs won't have a parent window.

It also /may/ cause us to flush invalidations on the window itself, but it tries not to do so.

So I /think/ we're OK, but I'd like bz to confirm.  Code looks great, thanks.
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-07-07 10:01:17 PDT
Currently in Gecko reading window.scrollX/scrollY will always flush reflow on the parent, if there is a parent.  This is because changing the size of a window can affect its scrollX/scrollY, so we have to make sure that anyone who can both change our size and read our scroll position gets consistent answers.

In the common case, I would expect a reflow flush on the parent of a mozbrowser to be cheap.

But somewhat worse for your situation is that any time the window is not scrolled all the way to the top-left corner of the document getting scrollX/Y will _also_ flush layout on the window itself.  The reason is documented in nsGlobalWindow::GetScrollXY, but basically the scroll position depends on the layout of the content in the window.

For your use case, would you want scroll positions that are in an inconsistent state because there are pending reflows that just haven't happened yet?
Comment 5 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-09 04:40:49 PDT
(In reply to Justin Lebar [:jlebar] from comment #2)
> > To be able to show/hide the titlebar of the browser application you need to have this information 
> > *and* the scrolling values not consumed by the content.
> 
> IOW, you need this and bug 757859, right?

I think something else is needed in order to achieve what they want.

Since you don't have access to the mouse events of the inner window you don't know what is the distance of the movement, or even you don't know if there is a pan event.

This lead to a few weird things like:
 - if you listen scroll event, a webpage will be able to hide the urlbar by scrolling it's content while you likely want to hide the url bar only on user gesture.

 - If your content is at 0,0 and the user pan 100px to the top the url bar should go way before panning anything but with this solution the content will be scrolled first.

 - since you don't know the distance of the pan gesture this is hard to determine how many pixels of your urlbar you want to scroll.
Comment 6 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-09 04:48:27 PDT
(In reply to Boris Zbarsky (:bz) from comment #4)
> 
> For your use case, would you want scroll positions that are in an
> inconsistent state because there are pending reflows that just haven't
> happened yet?

I don't think they want. One could argue that it will be possible to rectify the values by listening window sizes changes (bug 757859) but this seems a workaround.


So I wonder if the solution to the described use case is not something completely different. For example if one embedder want to add gestures support it should be an API that help us for that. And imho what they want to do if a bit similar.

If what I said is correct I think the timeline is too short for the release so I will postpone the feature if that's possible?
Comment 7 Justin Lebar (not reading bugmail) 2012-07-09 07:33:30 PDT
> If what I said is correct I think the timeline is too short for the release so I will postpone the 
> feature if that's possible?

This sounds pretty hard to get right, yes!
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-07-10 14:52:33 PDT
Comment on attachment 639954 [details] [diff] [review]
Patch

Anyway, feedback is above.  ;)
Comment 9 Justin Lebar (not reading bugmail) 2012-07-16 07:23:17 PDT
Vivien, it sounds like this is blocking some browser front-end work.  Do you want to push this patch through (and/or figure out what needs to be done), or do you want to hand it off to someone else?
Comment 10 Dale Harvey (:daleharvey) 2012-07-16 09:41:22 PDT
> This lead to a few weird things like:
> - if you listen scroll event, a webpage will be able to hide the urlbar by 
> scrolling it's content while you likely want to hide the url bar only 
> on user gesture.

I just wanted to mention that a lot of web content will scroll automatically just to get rid of the url bar (on ios / android), hiding the url bar is a really annoying interaction problem

http://remysharp.com/2010/08/05/doing-it-right-skipping-the-iphone-url-bar/
Comment 11 Mounir Lamouri (:mounir) 2012-07-16 15:09:11 PDT
This is actually a security issue because web pages would be able to spoof the URL bar.
Comment 12 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-17 07:15:51 PDT
(In reply to Justin Lebar [:jlebar] from comment #9)
> Vivien, it sounds like this is blocking some browser front-end work.  Do you
> want to push this patch through (and/or figure out what needs to be done),
> or do you want to hand it off to someone else?

I'm just not sure of how useful is this patch. I can land it if needed but I don't think it will resolve anything :/
Comment 13 Justin Lebar (not reading bugmail) 2012-07-17 07:44:26 PDT
> I'm just not sure of how useful is this patch.

Based on comment 5, or something else?  Here was my suggestion in github: https://github.com/mozilla-b2g/gaia/issues/1222#issuecomment-6849060

We need to do /something/ about hiding the URL bar in the browser (where "something" might include "deciding we're not going to do anything")...
Comment 14 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-20 08:44:08 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd4e6a53400d
Comment 15 Justin Lebar (not reading bugmail) 2012-07-20 08:45:37 PDT
I thought we'd decided this wasn't useful?  What are you going to use this for, given bug 774809?
Comment 16 Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) 2012-07-20 08:59:35 PDT
(In reply to Justin Lebar [:jlebar] from comment #15)
> I thought we'd decided this wasn't useful?  What are you going to use this
> for, given bug 774809?

I though it is still needed to know the current scrolling position of the browser. If it is at 0,0 then the urlbar can be scrolled if there is a gesture. Otherwise it should not. Or maybe I've missed a way to get this information with a different API ?
Comment 17 Justin Lebar (not reading bugmail) 2012-07-20 09:38:43 PDT
Ah, we realized on IRC that you need to know the current scroll position so that when you're scrolling up, we unhide the URL bar only once you've scrolled to the top of the page.
Comment 18 Ryan VanderMeulen [:RyanVM] 2012-07-20 21:06:00 PDT
https://hg.mozilla.org/mozilla-central/rev/cd4e6a53400d

Note You need to log in before you can comment on or make changes to this bug.