Intent to Deprecate and Remove: keyboard event filtering in fullscreen

[Philip Jägenstedt]

Primary eng (and PM) emails

phi...@opera.com

Summary

Keyboard event are currently filtered in fullscreen for Element.webkitRequestFullScreen() with no argument, HTMLVideoElement.webkitEnterFullscreen() and webkitEnterFullScreen(). Changes:

• Remove the filtering itself from EventHandler.
• Remove Document.webkitFullScreenKeyboardInputAllowed.
• Remove Element.ALLOW_KEYBOARD_INPUT.
• Make Element.webkitRequestFullScreen() and alias of webkitRequestFullscreen(), dropping the optional flags where ALLOW_KEYBOARD_INPUT could be used.

Motivation

This filtering is an odd remnant from a time when it was thought that we would prevent keyboard use in fullscreen to prevent spoofing, and that keyboard access in fullscreen would have a different permissions UI. This model did not end up in the spec and we have the same UI regardless.

Getting rid of this will simplify the fullscreen implementation a little bit, and more importantly remove a difference with the (not yet shipped) unprefixed API that one could accidentally depend on.

Compatibility Risk

Low. This could break pages that depend on keyboard events being filtered and have event handlers that break the page if run, which is likely a minority of the 0.001% (see below) of page loads where the filtering happens.

The removal of Document.webkitFullScreenKeyboardInputAllowed seems like the greater risk, as making it undefined where it was previously true could cause some fullscreen Web app to pick a different code path.

Alternative implementation suggestion for web developers

Don't handle key events in fullscreen. Don't check Document.webkitFullScreenKeyboardInputAllowed.

Usage information from UseCounter

Document.webkitFullScreenKeyboardInputAllowed is accessed in ~0.01% of page loads:

https://www.chromestatus.com/metrics/feature/timeline/popularity/319

The event filtering has an effect on ~0.001% of page loads:

https://www.chromestatus.com/metrics/feature/timeline/popularity/474

Element.ALLOW_KEYBOARD_INPUT does not have a counter as it was only recently that UseCounter started working for constants. Given that this constant only makes sense with Element.webkitRequestFullScreen() and that Element.webkitRequestFullScreen(Element.ALLOW_KEYBOARD_INPUT) will continue to do the same thing, I'm not worried.

Entry on chromestatus.com, crbug.com, or MDN

None.

Requesting approval to remove too?

Yes.

[PhistucK]

I cannot find a single instance of document.webkitFullScreenKeyboardInputAllowed on GitHub. Weird.

I wonder where it gets used so much.

☆PhistucK

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Dimitri Glazkov]

I say we give it a go.

:DG<

[Philip Jägenstedt]

Maybe ~0.01% of page loads enumerates all properties on Document for some reason? webkitFullScreenKeyboardInputAllowed is enumerable...

Philip

[Darin Fisher]

+1

[Eric Seidel]

lgtm

[Vincent Scheib]

Having worked in the fullscreen area previously, I'm supportive of removing the legacy API, but would suggest we deprecate first, and then remove.

[Philip Jägenstedt]

I'm not so sure deprecation is meaningful in this instance. Cases
where pages actually depend on the filtering are likely a fair bit
lower than 0.001%. The 0.01% for webkitFullScreenKeyboardInputAllowed
also seems inflated, since searching for it doesn't yield any examples
of actual usage, only things like Apple's documentation, this very
thread and a blog post I wrote about fullscreen.

API OWNERS, go/no go for immediate removal?

Philip

[Chris Harrelson]

Given the 0.01% figure, I wonder whether we should remove the filtering but make webkitFullScreenKeyboardInputAllowed a constant.

Vincent, do you have examples in mind where we might break content that lead you to prefer deprecation first?

Chris

[Philip Jägenstedt]

I don't think that removing one part of the API and deprecating the other is a good idea. I did it by accident with webkitConvertPointFromPageToNode() and WebKitPoint and it wasn't much fun:

https://groups.google.com/a/chromium.org/d/msg/blink-dev/KS5GAM7JXtg/3jx_wEo5_LoJ

I've search Google and GitHub for uses of webkitFullScreenKeyboardInputAllowed and can only find these:

http://netkoder.dk/netkoder/fuldskerm.php

https://github.com/subhamsaha1004/FullScreenAPI-sample/blob/master/app.js

To me, it does not seem plausible that actual usage that could break is anywhere near 0.01%. Accidental hits by "for (prop in document) bla(document[prop])" seem more likely.

Philip

[Vincent Scheib]

My request for deprecation is based on the principal of giving developers a deprecation period before removing a feature. Even if the number of developers is small, this isn't much work for us but can be quite helpful for any developers using a feature, giving them an explanation of what is happening instead of things suddenly breaking. Unless there's a pressing reason to not use deprecation, I advocate for it.

Ideally we would be able to remove functionality and still emit a deprecation error. Developers attention would be raised due to the functionality change, and a quick look in the console would explicitly explain why.

For example, there were many, many issues that come into the chromium issue tracker for broken apps after the Audio API changed. They were difficult to debug, and first-responder triagers rarely detected the root cause being the intended removal of Audio API endpoints. It just wasted a lot of various engineer's time as people tried to diagnose regressions.

[Chris Harrelson]

LGTM then.

[Philip Jägenstedt]

Part of the reason that I ask to skip the deprecation step when usage is very low is that it actually is a bit of extra work:

• Update test expectations with the deprecation warning. May require trybots to find all failures, and NeedsRebaseline to fix some.
• If the feature is completely untested, like webkitFullScreenKeyboardInputAllowed is, write a new test to verify that the deprecation warning works.
• If the existing counter is conservative in matching only observable usage, add a new one for any potential usage. For this removal, it would make sense to warn when a fullscreen request could potentially cause events to be dropped, but the existing counter is for when they actually are.

I should have mentioned these details up-front, but didn't suppose anyone would care :)

It's a judgement call. For once-standard features, features with non-trivial usage and features where the migration is tricky I agree that a deprecation period is reasonable.

Philip

[Vincent Scheib]

My opinion is not blocking. Depending on how you read Dmitry and Darin's responses you have LGTM for deprecate & remove. You're also right that this is likely a rarely used feature and has low chance of causing developer pain, so I wouldn't hold you back. But, I still stand by us not taking shortcuts, the web is chaotic enough to develop on as it is, and as I stated API removals sometimes have an indirect engineering time cost due to diagnosing failures.

[Philip Jägenstedt]

Does the deprecation period help mitigate the problems with diagnosing regression after the removal? I've done many removals with and without deprecation, and it's always been after the removal that the bugs come in. The idea of warning even after removal is interesting, but how? Maybe a non-enumerable attribute that returns undefined, or is otherwise indistinguishable from an attribute that isn't there?

Philip

[Vincent Scheib]

Moving "The idea of warning even after removal is interesting, but how?" to new thread "Warning after API removal".

[Vincent Scheib]

Good point, the Blink engineering cost is after removal, and we don't have a solution yet for that.  I weaken my request for deprecation for fullscreen keyboard event filtering and agree it's not likely worth the implementation cost.