1478776 - Implement event handlers of the Visual Viewport API

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P3)

Description

[Tanushree Podder [:tanushree]]

Implement the event handlers onresize and onscroll of the Visual Viewport API[1]. Bug1357785 tracks the implementation of the non-event handler attributes of the Visual Viewport API. 

[1] https://wicg.github.io/visual-viewport/#the-visualviewport-interface

Comment 1

[Botond Ballo [:botond]]

Here is an outline of how to implement VisualViewport.onscroll:

 * Post the event in nsIPresShell::SetVisualViewportOffset [1]

 * Use a mechanism similar to PostScrollEvent/FireScrollEvent
   to rate-limit the event to once per refresh driver tick

 * On the DOM side of things, we can follow the pattern of
   existing event handlers. For example, consider
   Screen.onmozorientationchange:

    * Declaring the handler: [2]
    * Implementing the handler: [3]
    * Firing the event: [4]

[1] https://searchfox.org/mozilla-central/rev/26b40a44691e0710838130b614c2f2662bc91eec/layout/base/nsIPresShell.h#1684
[2] https://searchfox.org/mozilla-central/rev/26b40a44691e0710838130b614c2f2662bc91eec/dom/webidl/Screen.webidl#40
[3] https://searchfox.org/mozilla-central/rev/26b40a44691e0710838130b614c2f2662bc91eec/dom/base/nsScreen.h#138
[4] https://searchfox.org/mozilla-central/rev/26b40a44691e0710838130b614c2f2662bc91eec/dom/base/ScreenOrientation.cpp#537

Comment 2

[Jan Henning [:JanH]]

The patches still need some tidying up, but I've got a working implementation.

I've noticed one potential problem, though: As per the spec, the events don't bubble, and with the default DOMEventTargetHelper implementation, they don't escape the VisualViewport during capturing, either. This means that any event listener must be added directly on the VisualViewport itself in order to capture any events. This might have been intended because the events use the same names as the normal "scroll"/"resize" events, and as such you cannot specify separate event listeners for VisualViewport and non-VisualViewport "scroll" events if both events end up being dispatched to the same element (you can only try to filter after the fact by looking at the originalTarget of the event).

At the same time, the VisualViewport is part of the (inner) Window, and so each time you navigate, you also get a different VisualViewport object.

All of this might be totally fine from the perspective of a page script, because in that case you won't care what happens when the current page goes away, anyway.

From the session store perspective on the other hand this is rather unfortunate, because I don't want to have to keep registering event listeners
a) manually for each subframe
b) each time the page navigates

One solution would be to make scroll events escape the VisualViewport during the capturing phase, but this would mean that any scroll listener attached to a window/browser/... that uses capturing will now catch both layout and visual viewport scroll events. In some cases this might even be beneficial, but in others (e.g. bug 1498812 comment 21) I'd like to specifically decide which kind of scroll event to capture. Having to look at event.originalTarget to distinguish the two kinds might be defensible in test code, but in case this distinction would be needed in production code as well, given the existence of [1], JS-based scroll event filtering might be a bad idea.

So at the moment I'd tend to have a second, internal-only (using mOnlySystemGroupDispatchInContent if I'm not mistaken?) event that does bubble and more importantly, has a different name, so I can register specific event listeners for it.

[1] https://dxr.mozilla.org/mozilla-central/rev/c2593a3058afdfeaac5c990e18794ee8257afe99/toolkit/components/sessionstore/nsSessionStoreUtils.cpp#122

Comment 3

[Jan Henning [:JanH]]

(In reply to Jan Henning [:JanH] from comment #2)
> So at the moment I'd tend to have a second, internal-only (using
> mOnlySystemGroupDispatchInContent if I'm not mistaken?) event that does
> bubble and more importantly, has a different name, so I can register
> specific event listeners for it.

mOnlySystemGroupDispatchInContent seems to work (and apparently even Desktop's session store content script is still considered as Chrome, so I don't have to move its scroll event listener to the system group), but I've stumbled across a comment for mOnlySystemGroupDispatchInContent that says:
> // Be aware, if this is true, EventDispatcher needs to check if each event
> // listener is added to chrome node, so, don't set this to true for the
> // events which are fired a lot of times like eMouseMove.

I guess that scroll/resize events fall into that category as well, so I'd need something like mOnlySystemGroupDispatch to skip the expensive isChrome check. Unfortunately this also means changing nsISessionStoreUtil's addDynamicFrameFilteredListener method definition so I can actually register an event listener in the system group...

Comment 4

[Jan Henning [:JanH]]

Attachment: Bug 1478776 - Part 1: Rewrite APZ panning test without callbacks. r?botond

I want to check that panning also triggers the appropriate scroll events, and
that will be easier without callbacks to worry about.
The test has been reorganised after the model given by helper_basic_zoom.html.

Comment 5

[Jan Henning [:JanH]]

Attachment: Bug 1478776 - Part 2: Add utility class for counting events. r?botond

Comment 6

[Jan Henning [:JanH]]

Attachment: Bug 1478776 - Part 3: Forward todo/todo_is to APZ mochitests, too. r?botond

Comment 7

[Jan Henning [:JanH]]

Attachment: Bug 1478776 - Part 4: Add basic tests for visual viewport events. r?botond

This will use the existing APZ basic pan/pinch-zoom tests to check that
scrolling/zooming will also generate the expected visual viewport events.

Because the various scroll-related events are throttled by the refresh driver
and only fire once per tick, merely flushing APZ repaints is no longer enough.
We now have to actually wait for the paints themselves, so we're sure that we've
had an opportunity to receive the corresponding events, too.

Comment 8

[Jan Henning [:JanH]]

Attachment: Bug 1478776 - Part 5: Define Visual Viewport event handlers. r?botond

As per https://wicg.github.io/visual-viewport/#the-visualviewport-interface.

Comment 9

[Jan Henning [:JanH]]

Attachment: Bug 1478776 - Part 6: Initial Visual Viewport event implementation. r?botond

The event rate throttling mechanism is modelled on the logic for "scroll" events
in nsGfxScrollFrame.cpp.

That is
1. When a request to fire an event is posted to the VisualViewport, we create a
   new runnable for this and register it with the RefreshDriver. If we already
   have a pending runnable, calling VisualViewport->Post...Event() becomes a
   no-op.
2. When the RefreshDriver is ready, it executes the runnable, which in turn
   fires the actual event and then cleans itself up.

To keep this patch manageable, we simply fire a scroll event every time the
stored visual viewport offset is changed. Because we are storing the absolute
offset of the viewport relative to the page, this behaviour doesn't match the
spec, which demands that scroll events are fired only when the relative offset
between visual and layout viewport changes. We'll fix this up in the next patch.

Comment 10

[Jan Henning [:JanH]]

Attachment: Bug 1478776 - Part 7: Tune scroll events to only fire when the *relative* offset changes. r?botond

Internally, Gecko stores and updates the *absolute* offset between the visual
viewport and the page, however the spec demands that the scroll event be fired
whenever the *relative* offset between visual and layout viewport changes.

Comment 11

[Jan Henning [:JanH]]

Attachment: Bug 1478776 - Part 8: Add an event flag for dispatching to system group only. r?smaug

The semantics of the VisualViewport resize/scroll events aren't quite what is
needed for internal browser usage, so we need a separate set of events that can
be used e.g. by the session store. To avoid future web compatibility issues,
that event should be kept internal, however none of the existing
options to achieve that are suitable:
- mNoContentDispatch can actually end up being dispatched to content after all
  and as per its comment preferably shouldn't be used any more for new features
- mOnlySystemGroupDispatchInContent would work perfectly, except that it
  shouldn't be used for frequent events, which the resize/scroll events
  arguably are
- mOnlyChromeDispatch doesn't work for the Desktop session store's content
  script, plus it might have the same performance problems as
  mOnlySystemGroupDispatchInContent

Therefore, I propose to introduce a new mOnlySystemGroupDispatch event flag,
which skips the comparatively expensive IsCurrentTargetChrome() check and relies
only on the event listener having been registered in the system group.

Comment 12

[Jan Henning [:JanH]]

Attachment: Bug 1478776 - Part 10: Add internal VisualViewport resize/scroll events. r?botond

The VisualViewport events are all nice and shiny, but unfortunately not quite
what is needed for the session store.

Firstly, the spec wants the "scroll" event to be fired only when the *relative*
offset between visual and layout viewport changes. The session store however
records the absolute offset and as such is interested in when *that* changes.

Secondly, again as per the spec the events don't bubble, and with the default
DOMEventTargetHelper implementation they don't escape the VisualViewport during
capturing, either. This means that any event listener must be added directly on
the VisualViewport itself in order to capture any events.

This might have been intended because the events use the same names as the
normal "scroll"/"resize" events, and as such you cannot specify separate event
listeners for VisualViewport and non-VisualViewport "scroll" events if both
events end up being dispatched to the same element (you can only try to filter
after the fact by looking at the originalTarget of the event).

At the same time, the VisualViewport is attached to the inner Window, and so
each time you navigate, you also get a different VisualViewport object.
All of this might be totally fine from the perspective of a page script, because
in that case you won't care anyway about what happens when the current page goes
away.

From the session store perspective on the other hand this is rather unfortunate,
because we don't want to have to keep registering event listeners
a) manually for each subframe
b) each time the page navigates

The event target chain problem could be solved by letting the scroll events
escape the VisualViewport during the capturing phase (which the spec doesn't say
anything about), but this would mean that any scroll listener attached to a
window/browser/... that uses capturing will now catch both layout and visual
viewport scroll events.

In some cases this might even be beneficial, but in others (e.g. bug 1498812
comment 21) I'd like to specifically decide which kind of scroll event to
capture. Having to look at event.originalTarget to distinguish the two kinds
might be defensible in test code, but in case this distinction would be needed
in production code as well, given the existence of a C++-based filtering helper
in nsSessionStoreUtils for another use case where (scroll) events need to be
filtered, JS-based scroll event filtering might be a bad idea.

Additionally, in any case this wouldn't solve the fundamental conflict between
the spec and the session store about *when* the "scroll" event should be fired
in the first place.

Hence I'd like to introduce a separate set of events with distinct event names,
which will be dispatched according to the requirements of our internal users
(i.e. currently the session store). To avoid potential web compatibility issues
down the road, for now these events will be dispatched only to event listeners
registered in the system group (allowing *all* Chrome event listeners cannot be
done because checking the Chrome status of each event target might be too
expensive for frequently dispatched events).

Comment 13

[Botond Ballo [:botond]]

Jan, thanks very much for working on this!

Comment 14

[Jan Henning [:JanH]]

Attachment: Bug 1478776 - Part 9: Helper function for layout viewport scroll position in PresShell. r?botond

This changes the semantics of the relative visual viewport offset calculation in
the PresShell slightly, in that a missing root scroll frame will no longer
force the relative offset to zero, even if the visual viewport itself has a non-
zero scroll position [1].
On the other hand, the visual viewport's own relative offset calculations
already work that way today, in that layout and visual viewport scroll positions
are retrieved separately and then subtracted from one another regardless of
whether those values are actually valid or merely a fallback because the
PresShell/scroll frame weren't available.

[1] Though I'm not sure under what circumstances this could really be relevant.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72a39ded10f8
Part 1: Rewrite APZ panning test without callbacks. r=botond
https://hg.mozilla.org/integration/autoland/rev/93a3c7fc4ace
Part 2: Add utility class for counting events. r=botond,masayuki
https://hg.mozilla.org/integration/autoland/rev/e6b186bffed7
Part 3: Forward todo/todo_is to APZ mochitests, too. r=botond
https://hg.mozilla.org/integration/autoland/rev/b056dade814d
Part 4: Add basic tests for visual viewport events. r=botond
https://hg.mozilla.org/integration/autoland/rev/24951c9d732d
Part 5: Define Visual Viewport event handlers. r=botond,Ehsan
https://hg.mozilla.org/integration/autoland/rev/63749e66f266
Part 6: Initial Visual Viewport event implementation. r=botond
https://hg.mozilla.org/integration/autoland/rev/6f2e2faa3321
Part 7: Tune scroll events to only fire when the *relative* offset changes. r=botond
https://hg.mozilla.org/integration/autoland/rev/767281b6922e
Part 8: Add an event flag for dispatching to system group only. r=smaug
https://hg.mozilla.org/integration/autoland/rev/4d4e9b811045
Part 9: Helper function for layout viewport scroll position in PresShell. r=botond
https://hg.mozilla.org/integration/autoland/rev/1f867de12312
Part 10: Add internal VisualViewport resize/scroll events. r=botond,nika

Comment 16

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/72a39ded10f8
https://hg.mozilla.org/mozilla-central/rev/93a3c7fc4ace
https://hg.mozilla.org/mozilla-central/rev/e6b186bffed7
https://hg.mozilla.org/mozilla-central/rev/b056dade814d
https://hg.mozilla.org/mozilla-central/rev/24951c9d732d
https://hg.mozilla.org/mozilla-central/rev/63749e66f266
https://hg.mozilla.org/mozilla-central/rev/6f2e2faa3321
https://hg.mozilla.org/mozilla-central/rev/767281b6922e
https://hg.mozilla.org/mozilla-central/rev/4d4e9b811045
https://hg.mozilla.org/mozilla-central/rev/1f867de12312

Comment 17

[Chris Mills [:cmills]]

I've documented these additions.

1. Added an events section to the main VisualViewport page: https://developer.mozilla.org/en-US/docs/Web/API/VisualViewport#Events

2. Updated the on... handler pages:
   https://developer.mozilla.org/en-US/docs/Web/API/VisualViewport/onresize
   https://developer.mozilla.org/en-US/docs/Web/API/VisualViewport/onscroll

3. Added pages for the actual events:
   https://developer.mozilla.org/en-US/docs/Web/API/VisualViewport/resize_event
   https://developer.mozilla.org/en-US/docs/Web/API/VisualViewport/scroll_event

4. Added compat data for the documented features: https://github.com/mdn/browser-compat-data/pull/3562

I've not added a note to the Fx66 rel notes, as the features are still behind a pref.

Let me know if that looks OK. Thanks!

Comment 18

[Botond Ballo [:botond]]

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #17)

Let me know if that looks OK.

Thanks, Chris, the additions look good.

I'd still like to revise the example at some point, as mentioned in bug 1357785 comment 33, but I haven't had a chance to think of a better example. I made myself a note to do that before enabling the API by default.

Comment 19

[Chris Mills [:cmills]]

(In reply to Botond Ballo [:botond] from comment #18)

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #17)

Let me know if that looks OK.

Thanks, Chris, the additions look good.

I'd still like to revise the example at some point, as mentioned in bug 1357785 comment 33, but I haven't had a chance to think of a better example. I made myself a note to do that before enabling the API by default.

Thanks for the review Botond, appreciate it! Yes, a better example would be good; let me know if you need any help with getting it published.