1383900 - Investigate a web solution for key event listeners with Keyboard APZ

Status: NEW

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

Description

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: scroll-target-change-1.html

I'm going to write up some things that were scattered around in other bugs into one central place. My hope is for this to be useful as a semi-complete description of why we can or cannot use keyboard APZ, for those new to these issues.

Skip the background if you're familiar with why keyboard APZ works the way it does.

# Background

We disable async keyboard scrolling in the presence of any keydown, keypress, keyup listener (passive or not) on the event target chain to the focused element.

There are two reasons for this, the first is `preventDefault()`, and the second is `element.focus()` and `window.getSelection().*`.

The problem with preventDefault is the same problem that other APZ input methods have. A solution for this is to use the `passive` event listener flag which promises the listener won't call preventDefault.

The problem with `element.focus()` and `window.getSelection()` is specific to keyboard APZ.

Keyboard scrolls are targeted to the focused element if there is one, or else the focusNode() of the current selection. These are properties of the DOM and so they can change at any moment on the main thread.

The problem is that we need to maintain a copy of this information in APZ so we know at any moment for new input whether to scroll and if so what to scroll.

Changing the focus or selection on the page can happen at basically anytime:

1. In response to key input (e.g. onkeydown, onkeypress, onkeyup)

  - For example, a page with a key listener that sets the focus of the page
    to a <textarea>.
  - If the user were to hit space twice, they would expect the first space
    to scroll the page, and the second to be input into the <textarea>. But
    we cannot guarantee this to happen if these inputs are handled by APZ,
    because the focus change from the first input will most likely not
    reach before the second input is processed.

2. In response to other input (e.g. onclick)

  - For example, a page with a button the changes the focus of the page to
    a scrollable <div>.
  - If the user were to click the button and then immediately key scroll,
    they would expect to scroll the <div>. But we can't guarantee the focus
    change would be synced in time.

3. In callbacks (e.g. setTimeout(), setInterval(), rAf).

  - For example, a page with a timeout of 5 seconds that will change the
    focus a <textarea>.
  - If the user were key scrolling and they had a stopwatch, they may notice
    that they scroll longer than 5 seconds before the <textarea> started to
    receive input.

I've attached some examples demonstrating these problematic pages.

The choice we made for the current implementation was to guarantee synchronous behavior for #1 and #2 but not #3.

Preserving synchronous behavior for #3 would essentially implie no keyboard APZ.

We preserve the behavior for #1 by disabling keyboard APZ when there are key listeners on the path to the focused element. We preserve behavior for #2 by temporarily disabling keyboard APZ using sequence numbers in some cases and waiting for the key scroll target to be reconfirmed.

# Problem

So in the current implementation we disabled keyboard APZ when there are key listeners on the path to the focused element.

The problem with this is that key listeners are very useful for web pages and in most cases won't actually interfere with keyboard APZ.

We should have a solution for pages to listen to key events but still allow keyboard APZ.

# Keyboard listener prevalence

Bug 1357880 added a telemetry probe for discovering how prevalent the use of key listeners were.

Unfortunately it tracked the presence of non-passive keydown and keypress event listeners, while we ended up blocking any keydown, keypress, keyup listener. The percentage of page loads for the telemetry probe was ~13.89%.

That seems low to me. From my browsing, most popular sites seem to not get keyboard APZ.

For example, the following sites have key listeners on the body of the page:

1. google.com
2. yahoo.com
3. bing.com
4. twitter.com
5. amazon.com
6. facebook.com
7. youtube.com
8. github.com
9. cnn.com
10. bbc.com
11. imgur.com

This seems significant enough that a solution would be nice.

# Key listeners that need to disable keyboard APZ

As a reference, I believe any key listener that doesn't do these two things should be safe to exist with keyboard APZ.

1. Non-passive key listener for a key that triggers scrolling
  - The listener could preventDefault and we'd still scroll the page
2. It may change the focus or selection of the page
  - We could scroll the wrong target or scroll when we shouldn't
  - This could happen either on the current input or following inputs

# Solutions

1. Don't care about synchronous scroll target changes for key event listeners

  - Only care about `passive` flag
  - If a key listener changes the focus, it could be missed as in examples above

2. Add a new Web API for listening to key events

  - Maybe an `async-key-passive` flag on event listeners?
  - Web authors would need to understand this problem

3. Use the current implementation

  - It works on pages without key event listeners

I don't know what the best solution for this problem would be. It'd be ideal for us to not have to care about determinism here, but I don't know if it's relied upon or not. Adding a Web API could be a good long term solution, but I'm not sure if the need for it is that high.

Comment 1

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

BenWa, do you have any thoughts on this from the web-author side of things? Do you know what facebook registers key listeners for? My preference is towards option #2 - add a new thing in EventListenerOptions that allows sites to effectively say "this keyboard listener is safe, don't worry about it" much like passive does for preventDefault.

Comment 2

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: scroll-target-change-2.html

Comment 3

[Ryan Hunt [:rhunt] (on leave until early May)]

Attachment: scroll-target-change-3.html

Comment 4

[Benoit Girard (:BenWa)]

I had a quick look and here's what I was told:
- We have have a lot of listeners that don't preventDefault at all. Some of them are just installed to monitor performance and randomly log to our profiling datasets.
- I'm told we preventDefault in some cases dealing with unicode checks, URL highlights and various interaction in our status composer/editor. I'll get back to you with specific examples.

If we could re-purpose the passive event flag I think it would be the most natural for web developers to hunt them down everywhere. Right now it's only valuable to hunt them down and remove them for wheel/mousewheel/touch events so we didn't do any work to use passive key event listeners. But if there's a performance benefits then it would be easy to educate web developers to care about these event types as well and use passive in more places. It would just be a minor update to the passive guidance.

Facebook would start using #1 or #2 as much as possible to allow for async key scrolling. So if the 'passive' spec is compatible with this I think #1 would be highly preferable since it's only updating the web dev guidance. We could rope in Rick Byers for feedback in using the passive flag.

Also there's the interaction with the cancelable flag to consider (See bug 1356293 for dependency in Firefox). With non-passive/passive the cancelable flag in the event should be set accordingly. I think #1 would fall in naturally with this. This allows website to easily tell and detect regressions in scroll blocking event listeners and that's an easy objective metric that can be used to set quarterly goals to get good async scrolling. Right now there's no other way for website to confirm that they're getting good async scrolling in practice.

Sites like Facebook are very unlikely to achieve zero key listeners so #3 would shut us out in a lot of use cases.

I blocked off time to investigate the exact code that's using it so leaving the ni? for now and I'll have concrete examples to share.

Comment 5

[Ryan Hunt [:rhunt] (on leave until early May)]

(In reply to Ryan Hunt [:rhunt] from comment #0)
> 
> # Solutions
> 

I feel like I worded this section poorly. Here are the possible approaches for handling key listeners:

1. Don't block for any key listeners
   - Not an option because of the preventDefault() problem
2. Block for key listeners that are not passive
   - This effectively expands the definition of passive for key event listeners to mean
     they also don't change the focus or selection of the page
3. Add a new event listener flag and block for key listeners that are not passive and not 'focus-passive'
   - This is like the last option but preserves the definition of passive
   - This flag could potentially be reused for other event listeners (e.g. to mark a mousemove listener
     so we know that it doesn't change focus, and allow us to not wait to reconfirm the focus information
     in APZ after a mousemove. We could also use passive for this purpose too.
4. Block for all key listeners (the current implementation)
   - Works, but not for major sites

So I'd agree with kats and BenWa that 2. and 3. make the most sense.

Comment 6

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

/cc Rick also

Comment 7

[Ryan Hunt [:rhunt] (on leave until early May)]

Note: with bug 1387566 in nightly, we allow keyboard event listeners that are marked passive.

We're testing to see if there is any significant breakage.

Comment 8

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

(In reply to Benoit Girard (:BenWa) from comment #4)
> I blocked off time to investigate the exact code that's using it so leaving
> the ni? for now and I'll have concrete examples to share.

BenWa, did you end up getting a chance to investigate this?

Comment 9

[Ryan Hunt [:rhunt] (on leave until early May)]

(In reply to Ryan Hunt [:rhunt] from comment #7)
> Note: with bug 1387566 in nightly, we allow keyboard event listeners that
> are marked passive.
> 
> We're testing to see if there is any significant breakage.

Just an update, keyboard APZ is riding the trains for 57 with `apz.keyboard.passive-listeners` enabled (from bug 1387566). We haven't gotten any reports of significant breakage.

So the easiest way to get keyboard APZ right now is to mark key listeners as passive as appropriate.

Comment 10

[Benoit Girard (:BenWa)]

Here's a few examples:
* Carousel will preventDefault on left/right arrows to trigger custom behavior.
* Some lists will preventDefault on Enter.
* Dialogs will preventDefault on ESC and Enter.
* Several examples that don't call preventDefault that are fixable.

I'm seeing a lot of references left in the code base that I can't all go through but there's many.

I'm not sure how many of these would be hit during a typical keyboard scroll in practice however because of focus. Bug 1356293 would let us track better data in Firefox and prioritize event listeners that are blocking scrolling by frequency. We used this with Chrome to clean up offending 'wheel' scrolling listeners and it also lets us have regression triggers.

It's a shame that, unlike the 'wheel' event which is more isolated, sites have to register a listener for keys just to prevent default on Enter/ESC. Updating event listeners registration to be more specific would help but it may not be 'clean' enough.