898075 - Remove the mozbrowserasyncscroll event

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

In the world prior to multi-apzc, the mozbrowserasyncscroll event was only dispatched on the top-level window (i.e. the one associated with the APZC). In the world with multi-apzc, the event should be updated to indicate which window or subframe is actually scrolling.

Filing this as a placeholder bug. For now my plan is that in bug 866232 each APZC will be calling the GeckoContentController::SendAsyncScrollDOMEvent callback with the scroll id, and the RenderFrameParent will only dispatch as events the ones corresponding to ROOT_SCROLL_ID. This maintains backwards compatibility with the gaia browser which uses these events to show/hide the toolbar.

Comment 1

[Kartikaya Gupta (email:[e-mail])]

Actually once the new dynamic toolbar for b2g is in place we shouldn't need the mozbrowserasyncscroll event at all.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

Since bug 943849 changed the scroll events to fire every 40ms I think we should change the browser app to also use the regular scroll event instead of mozbrowserasyncscroll. That is the only user of this event, and then we can get rid of the event entirely. At this point I'm not convinced that the mozbrowserasyncscroll event fires any more often than the scroll event so it shouldn't make a noticeable difference in the browser addressbar hiding. Ben, any thoughts?

Comment 3

[Ben Francis [:benfrancis]]

I didn't think we added the asyncscroll event because it fired more often, it was because it fired in sync with the kinetic scrolling or something. There were bugs when using the normal scroll events. I could be wrong though!

We shouldn't need this from Firefox OS 1.4 as the UI design is changing, so could we perhaps keep this around until 1.4 has reached code complete and we are sure?

Comment 4

[Kartikaya Gupta (email:[e-mail])]

(In reply to Ben Francis [:benfrancis] from comment #3)
> I didn't think we added the asyncscroll event because it fired more often,
> it was because it fired in sync with the kinetic scrolling or something.
> There were bugs when using the normal scroll events. I could be wrong though!

There's no way it can actually be in sync because the scrolling happens on the compositor thread and the asynsc-scroll event (by definition) is read in the content thread so there is inherently a disconnect there.

> We shouldn't need this from Firefox OS 1.4 as the UI design is changing, so
> could we perhaps keep this around until 1.4 has reached code complete and we
> are sure?

I suppose so. Is there a meta-bug or something for the 1.4 changes that can I make this depend on?

Comment 5

[Ben Francis [:benfrancis]]

Yes, bug 945259 is the system browser MVP meta bug which will determine whether we can ship the system browser and turn off the current app.

Comment 6

[GitHub Autolander Bot]

Attachment: [gaia] staktrace:noasync > mozilla-b2g:master

Comment 7

[Kartikaya Gupta (email:[e-mail])]

Attachment: Gecko patch

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8683701 [details] [diff] [review]
Gecko patch

This is all just straightforward code deletion. r?sicking because I touched a .webidl file.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5595ca752578

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8683701 [details] [diff] [review]
Gecko patch

Putting this on hold for a bit, discussion on IRC with ben francis indicates this might still be needed.

Comment 11

[Ben Francis [:benfrancis]]

Comment on attachment 8683677 [details] [review]
[gaia] staktrace:noasync > mozilla-b2g:master

The scroll event is still used by AppChrome, please don't remove it.

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_chrome.js#L278
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_chrome.js#L625
https://github.com/mozilla-b2g/gaia/blame/master/apps/system/js/app_window.js#L1192

Comment 12

[Chris Lord [:cwiiis]]

(In reply to Ben Francis [:benfrancis] from comment #11)
> Comment on attachment 8683677 [details] [review]
> [gaia] staktrace:noasync > mozilla-b2g:master
> 
> The scroll event is still used by AppChrome, please don't remove it.
> 
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_chrome.
> js#L278
> https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_chrome.
> js#L625
> https://github.com/mozilla-b2g/gaia/blame/master/apps/system/js/app_window.
> js#L1192

I don't think that scroll handler is attached to that scroll event - Pretty sure that handler is attached to the scroll events of the scrollgrab container.

Comment 13

[Chris Lord [:cwiiis]]

Just to confirm: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_chrome.js#L710

I think this is safe to remove. n?benfrancis+kats to confirm.

Comment 14

[Kartikaya Gupta (email:[e-mail])]

I also tested the scenario that Ben pointed out on IRC:

"Pinned sites have pinned chrome which doesn't expand/collapse on scroll. It will collapse after being manually expanded but not expand again. So load a website in a browser window, pin the site (via the browser menu) and observed collapsed & "pinned" chrome no longer expands/collapses on scroll. Manually expand by tapping the collapsed chrome, then scroll to collapse it again."

and this works fine both with and without my gaia patch.

Comment 15

[Ben Francis [:benfrancis]]

Comment on attachment 8683677 [details] [review]
[gaia] staktrace:noasync > mozilla-b2g:master

Yep, my bad.

We use the same event name but it seems the event being listened to here is actually a "scroll" DOM event from the "scrollable" div, not the "scroll" event we publish on AppWindow. It used to listen to the Browser API scroll events but it doesn't any more :)

https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_chrome.js#L710

Sorry for the confusion.

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Thanks. Flagging as checkin-needed for the gaia PR; leave-open for the gecko side

Comment 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8683701 [details] [diff] [review]
Gecko patch

Kan-Ru, do you see any problem with removing the mozbrowserasyncscroll event? Ben said on IRC that you were the module owner for the browser API. This event is not used in Gaia anymore and I would like to remove it because it's not really a reliable indicator of scroll position; it is no more "async" than regular scroll events.

Comment 18

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8683701 [details] [diff] [review]
Gecko patch

Review of attachment 8683701 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this!

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8683701 [details] [diff] [review]
Gecko patch

Thanks! Restoring original review request as in comment 8.

Comment 20

[Carsten Book [:Tomcat]]

https://github.com/mozilla-b2g/gaia/commit/5eecb6baccb31ccbf0a384451c3270b8da1814a6 for the gaia PR

Comment 21

[Botond Ballo [:botond]]

Comment on attachment 8683701 [details] [diff] [review]
Gecko patch

Review of attachment 8683701 [details] [diff] [review]:
-----------------------------------------------------------------

Burn it with fire :)

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8683701 [details] [diff] [review]
Gecko patch

Review of attachment 8683701 [details] [diff] [review]:
-----------------------------------------------------------------

r=me on the webidl change. Didn't look at any other parts.

Comment 23

[Kan-Ru Chen [:kanru] (UTC+9)]

Comment on attachment 8683701 [details] [diff] [review]
Gecko patch

Review of attachment 8683701 [details] [diff] [review]:
-----------------------------------------------------------------

Actually I could r+ the browser-element part.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

Thanks all!

Comment 25

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0d3a6fcb8687

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0d3a6fcb8687

Comment 27

[Chris Mills [:cmills]]

I've updated the mozbrowserasyncscroll event page as obsolete:

https://developer.mozilla.org/en-US/docs/Web/Events/mozbrowserasyncscroll

and added a note to the 2.5 release notes:

https://developer.mozilla.org/en-US/docs/Mozilla/Firefox_OS/Releases/2.5#Web_API_changes

Comment 28

[Kartikaya Gupta (email:[e-mail])]

Thanks Chris!