1052569 - [meta] Prevent web pages from overriding core tab/window-management shortcuts

Status: RESOLVED FIXED

(Firefox :: Keyboard Navigation, defect)

Description

[Matt Brubeck (:mbrubeck)]

Attachment: test case

Steps to reproduce:
1. Open the attached test case.
2. Press control-W to close the tab.

Expected results: The tab closes.
Actual results: Nothing happens.

Proposed solution:  Designate a small subset of Firefox keyboard shortcuts as "reserved" and do not allow web pages to override them.  In particular, reserve the most-used shortcuts for switching to a different tab/window/page, such as accel+T, accel+W, accel+N, and a few others.  (Note that the keys used for these shortcuts may vary by UI locale.)

(This is similar to the suggestions by Firefox UX team members in bug 380637 comment 40 and bug 380637 comment 43.  Steven Horlander also indicated on Twitter/IRC that he was in favor of some solution along these lines.)

Rationale:

- Allowing pages to "trap" the user in a tab breaks expected browser behavior, reduces accessibility for users who cannot use a mouse, and may help to enable UI spoofing attacks.

- Other browsers like Chrome do not allow web pages to override these shortcuts.  A web page tested mainly in Chrome may accidentally break these shortcuts in Firefox without the developer realizing it.  This seems to be happening regularly in the wild.  (See https://code.google.com/p/chromium/issues/detail?id=5496 for some more details.)

Why not all shortcuts?

- Reserving all browser shortcuts would break functionality on many existing sites.  Sometimes those sites provide useful features by overriding browser shortcuts, like a document editor that implements an undo/redo feature.  It's true that some sites override shortcuts in ways that annoy users, but as long as it only breaks things within that tab (and doesn't preventing users from getting to their other sites/tabs), it may be more feasible to address this through feedback to those sites' authors.

- If we choose a set of reserved shortcuts that fully or mostly overlaps with Chrome, then we will minimize breakage (since any web page that depends on overriding them is already broken in Chrome).

- Because the set of browser shortcuts changes with locale, configuration, and add-ons, reserving *all* of them would mean that basically every shortcut is disabled for some Firefox users somewhere, making all shortcuts potentially unavailable for web pages.

- An add-on can provide additional control for users who do want to prevent web pages from overriding other browser shortcuts.

What about web apps?

- Since the proposed solution is Firefox-only, it would not affect web apps running in the WebRT environment.  The behavior for these "web apps" would be unchanged.

- Bug 380637 included some suggestions that app tabs should be allowed to override reserved shortcuts.  However, I'm not sure that's still appropriate.  The UI vision for app tabs has changed dramatically since then (they have become more like regular tabs), and many of their originally-envisioned use cases are now addressed by WebRT instead.

What about overrides/prefs/permissions?

- Methods for users to disable these restrictions in general or for specific web sites in Firefox are outside of the scope of this bug.  It's possible to add them, but let's address that separately if possible.

This is an attempt to address bug 380637.  I'm filing it as a new bug at the request of the Firefox engineering and UX people I spoke to.  Unlike bug 380637, this bug is about a specific Firefox UX change (not a change to core event handling).  Since the solution is Firefox-only, fx-team can decide to either put on their backlog or not.  I hope that having a separate, more focused bug in the Firefox product will help us make progress on this issue.

NOTE: If you want to advocate a different solution, please open a different bug, or comment in bug 380637 if your idea is not already mentioned there.  Please help keep this bug focused on the UX issues and technical work related to this specific proposal.

Comment 1

[Dave Garrett]

The short list would probably be:
(shift+)accel+T/N  -  new/old tab/window
(shift+)accel+W    -  close tab/window
shift+accel+P      -  new private browsing window
accel+Q            -  exit application
(accel+)F5         -  reload page
accel+L/K          -  focus address/search bar

Some that already appear to be safe, but should nonetheless be considered:
(shift+)accel+Tab
(shift+)alt+Tab
alt+F4

I'd also add basic find shortcuts (accel+F & (shift+)F3) to the list for the sake of accessibility. Likewise, the zoom keys (accel+0/+/-) might warrant attention, though that's likely outside the scope of this bug.

Comment 2

[philippe (part-time)]

You could add [parity: Safari]

With the attached test case, the following keyboard shortcuts work fine (Safari 7, 10.9):
Cmd+W, Cmd+T, Cmd+L, Cmd+N, Cmd+R
Control+Tab, Shift-control-Tab (prev/next tab)
(possibly a few more, but those are the ones I tested…)

Back in the day with Camino we had mostly the same shortcuts reserved and never had a complain from users.

Comment 3

[silverwind]

I'd advise against adding find shortcuts to this list. There are editors like CodeMirror which only render a part of a file's content on the DOM, causing the built-in find to become useless, creating the need to override it.

Comment 4

[:Gijs (he/him)]

AFAICT bug 1008772 aimed to address exactly this...

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to :Gijs Kruitbosch (intermittently here 14-15 August; then away until 19th) from comment #4)
> AFAICT bug 1008772 aimed to address exactly this...

FYI: content can handle the keyboard shortcuts, but they cannot prevent the default actions. It is what bug 1008772 changes the shortcuts as.

Comment 6

[bobbuun]

This report is a special case of 380637 and should be marked as a duplicate of it, should it not?

Comment 7

[:Gijs (he/him)]

(In reply to bobbuun from comment #6)
> This report is a special case of 380637 and should be marked as a duplicate
> of it, should it not?

No, as comment #0 points out, we want to address a subset of all shortcuts, without trying to solve the complete problem here (as people are unhappy no matter what we do, and so a middle road is to only fix some shortcuts). So this isn't a duplicate, the other bug will remain valid even if we fix this one (and at the moment we are interested in fixing this, and less so in fixing bug 380637).

Comment 8

[Fabian]

The subset of shortcuts that we won't allow a page to eat will likely be quite small and debated, as web apps are starting to utilize numerous shortcuts with an arguably useful behaviour.

Might I therefore additionally suggest:
When a user presses ALT and releases it without hitting another key (so not hitting a direct shortcut), always open the browser menu.

This will continue to guarantee access to a wide range of browser functions. I'm not aware (yet) of apps hijacking this particular 'shortcut' and it is parity-IE and Opera(Presto).

Comment 10

[:Gijs (he/him)]

Google do this for the clipboard as well. This means that in the html5 player on youtube, for instance, you can't right click and say 'get video url', and ctrl/cmd-c when the text in the input is selected in Firefox - but it 'just works' in chrome.

Philipp, can we make a UX decision here about which shortcuts (humble suggestion: accel-w, accel-t, accel-(shift-)tab, accel-n, accel-x/c/v - although I guess we can also look at what chrome does) to ignore preventDefault for?

(In reply to Fabian from comment #8)
> Might I therefore additionally suggest:
> When a user presses ALT and releases it without hitting another key (so not
> hitting a direct shortcut), always open the browser menu.

Not all windows have a menubar, and this is not discoverable. I'd also fully expect games to try using those keys in some cases. I therefore don't think this is a very good idea, but UX may disagree.

Comment 11

[Fabian]

(In reply to :Gijs Kruitbosch from comment #10)

> Not all windows have a menubar

Then a fix would start making it work for those windows that do, and not change anything for those that don't (and therefore don't have anything to which access is blocked in the first place).

> and this is not discoverable.

For Windows at least, ALT is the most basic starting point for keyboard based menu access since the '90s and the paradigm will be known to most keyboard users. How else is the Firefox menu currently accessible for keyboard users by the way? If you worry about discoverability, the general hidden state is more of an accessibility issue than the odd page that intercepts ALT key presses.

> I expect games to try using those keys in some cases.

Hitting and releasing ALT (not ALT+character simultaneously) and doing something with that? I'm curious for use cases and whether they work in other browsers then, they already seem to safeguard the user here.

Comment 12

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Would that mean that websites can't do anything on these commands, or just that they won't be able to prevent the default action?

Anyway, the following seem like a good start:

accel-t
accel-w
accel-q
accel-n
ctrl-tab, ctrl-shift-tab
alt key single press

I'm a little hesitant with accel-c/x/v, since some sites (like Google Sheets) do some custom stuff with these commands and would probably break.

In any case, if we land those changes, we need to watch out for new web-compat bugs that might arise from these changes and reconsider depending on the feedback we get.

Comment 13

[bobbuun]

It will be impossible to come up with a list of "special keys" that will satisfy everyone (if anyone). As discussed previously, it needs to be in the hands of the user and will likely vary by platform.

For instance: for me, even the arrow keys should not be able to be overwritten without consent.

Comment 14

[Fabian]

Indeed. Use case against intercepting arrow keys: due to poor eyesight I might zoom certain pages, which upon pressing left/right don't scroll sideways but enter a slideshow mode, go back/forward or do nothing. Major image hosting sites and Twitter do this, making content inaccessible.

Yet even the small set proposed above will already 'break', e.g. on Hotmail/Outlook.com which uses ctrl+N for Compose New Message. All fixed sets of safeguarded shortcuts will always be both too small and too big.

Comment 15

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] Currently behind on reviews & needinfos from comment #12)
> Would that mean that websites can't do anything on these commands, or just
> that they won't be able to prevent the default action?

The latter.

> Anyway, the following seem like a good start:
> 
> accel-t
> accel-w
> accel-q
> accel-n
> ctrl-tab, ctrl-shift-tab
> alt key single press
> 
> I'm a little hesitant with accel-c/x/v, since some sites (like Google
> Sheets) do some custom stuff with these commands and would probably break.

Do you feel differently considering that they will still work, just not be able to prevent the default action?

> In any case, if we land those changes, we need to watch out for new
> web-compat bugs that might arise from these changes and reconsider depending
> on the feedback we get.

For sure.

Comment 16

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #15)
> (In reply to Philipp Sackl [:phlsa] Currently behind on reviews & needinfos
> from comment #12)
> > Would that mean that websites can't do anything on these commands, or just
> > that they won't be able to prevent the default action?
> 
> The latter.
> 
> > Anyway, the following seem like a good start:
> > 
> > accel-t
> > accel-w
> > accel-q
> > accel-n
> > ctrl-tab, ctrl-shift-tab
> > alt key single press
> > 
> > I'm a little hesitant with accel-c/x/v, since some sites (like Google
> > Sheets) do some custom stuff with these commands and would probably break.
> 
> Do you feel differently considering that they will still work, just not be
> able to prevent the default action?

+needinfo

Comment 17

[:Gijs (he/him)]

Note also that for clipboard handling, web pages have other (clipboard) events that they can/do work with, which work differently and also capture e.g. context/main menu cut/copy/paste events - so all the more reason to not let pages prevent the shortcut from doing something.

Comment 18

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #17)
> Note also that for clipboard handling, web pages have other (clipboard)
> events that they can/do work with, which work differently and also capture
> e.g. context/main menu cut/copy/paste events - so all the more reason to not
> let pages prevent the shortcut from doing something.

If it's just the preventDefault that gets blocked, I'm OK with including accel-c/x/v into the starting set.
In general, I'd err on the side of including too few shortcuts rather than too many.

Comment 21

[Jesse Ruderman]

Please file a new bug (also blocking bug 380637) for the clipboard pieces. The reasons for restricting or not are different. The implementation will also be different, because of interaction with custom clipboard handling and with bug 363132.

Comment 23

[arni2033]

If you're make Alt not cancellable in chrome context, then bug 1203103 couldn't be fixed. Delete this comment and "See also" if this has already taken into account.

Comment 24

[:Gijs (he/him)]

(In reply to arni2033 from comment #23)
> If you're make Alt not cancellable in chrome context, then bug 1203103
> couldn't be fixed. Delete this comment and "See also" if this has already
> taken into account.

This won't affect chrome contexts. This is about websites.

Comment 25

[arni2033]

(In reply to :Gijs Kruitbosch from comment #24)
> This won't affect chrome contexts. This is about websites.
This bug is reported this way. However, current implementation of Ctrl+Tab can't be cancelled in chrome context: I opened new window, opened Scratchpad and executed in browser context the following line
>   onkeypress = onkeyup = onkeydown = function(e){ e.preventDefault(); }
After that, I couldn't type anything bug Ctrl+Tab/PageUp/PageDown still worked.

So, I just thought that you're going to implement shortcuts in this bug in same way.

Comment 26

[:Gijs (he/him)]

(In reply to arni2033 from comment #25)
> (In reply to :Gijs Kruitbosch from comment #24)
> > This won't affect chrome contexts. This is about websites.
> This bug is reported this way. However, current implementation of Ctrl+Tab
> can't be cancelled in chrome context: I opened new window, opened Scratchpad
> and executed in browser context the following line
> >   onkeypress = onkeyup = onkeydown = function(e){ e.preventDefault(); }
> After that, I couldn't type anything bug Ctrl+Tab/PageUp/PageDown still
> worked.
> 
> So, I just thought that you're going to implement shortcuts in this bug in
> same way.

I'm pretty sure that this has to do with whether/how we check isDefaultPrevented when handling the event.

Comment 27

[:Gijs (he/him)]

I've been looking at this. The current functionality is essentially already working on aurora and trunk if you use e10s, because of bug 1074971.


I'm trying to make the reserved attribute work in non-e10s. However, I've hit a number of issues:

0) It's not as simple as "just" using system event listeners - <key> elements already use system event listeners.
1) they rely on a working charCode property (otherwise they can't match key elements to shortcuts)
2) charCode is 0 for keyup and keydown, as per spec, which means they rely on keypress events
3) if a webpage calls preventDefault() on a keydown event, no keypress event will fire.

There's a hardcoded check for keypress at https://dxr.mozilla.org/mozilla-central/rev/b41b92c09fcf94d077a54297aea1dc675b161a9d/dom/base/nsContentUtils.cpp#4991-5002 . Just changing that to keydown doesn't help because the charCode is still 0 all the time, meaning the code finds no matching key elements at all.

This is a problem because we don't know if a shortcut key is being used at all (nevermind if it is reserved).

I actually suspect this also explains bug 1173061: we get a keydown event, but we can't find a matching <key> element to handle it, and thereby also can't decide not to send that to content. The reason the code there works anyway is because in the Firefox chrome process, the event is not preventDefault()'d and we generate a keypress event either way, which then doesn't get sent to content and does the required thing (opening/closing a tab, whatever).

So it seems like the sensible thing to do is to move the processing to use keydown events and divine the charCode for those separately in nsContentUtils. That should also fix bug 1173061. Felipe/Olli, does that sound sensible to you?

Comment 29

[arni2033]

(In reply to :Gijs Kruitbosch from comment #26)
> I'm pretty sure that this has to do with whether/how we check isDefaultPrevented when handling event
Just in case: it's bug 1008772 that introduced uncancellable (in chrome context) Ctrl+Tab/ PageUp/Down

Comment 30

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, I realized that what this bug tries to fix is exactly same as what I'm working on bug 1154183. For fixing this, we need to refactor all widget's native keyboard event handlers for making them set alternativeCharCodes to eKeyDown event since we need to avoid to reopen "key hell" bugs.

So, can I take this bug? I must be the best person to rewrite all widgets for fixing this.

Comment 31

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> So, can I take this bug?

Oh, I meant that I should take only a part of a fix of this bug:
* Change all widgets (I'm now preparing to fix this bug in blocker bugs of bug 1137560)
* Make  nsXBLWindowKeyHandler handle keydown event instead of keypress event if <command> element is "reserved" (bug 1154183).

They are my goal of Q1, 2016.

Comment 32

[:Gijs (he/him)]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #31)
> > So, can I take this bug?
> 
> Oh, I meant that I should take only a part of a fix of this bug:
> * Change all widgets (I'm now preparing to fix this bug in blocker bugs of
> bug 1137560)
> * Make  nsXBLWindowKeyHandler handle keydown event instead of keypress event
> if <command> element is "reserved" (bug 1154183).
> 
> They are my goal of Q1, 2016.

Sounds good to me. I think it makes sense to keep this bug open so that frontend/UX can evaluate if any more <key> elements need the reserved attribute.

Comment 33

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Thanks. I'd like Firefox team to list up the target while I'm working on them ;-)

Comment 34

[CJ Kucera]

Seems there's been no activity on this in awhile; would be nice to have a way of dealing with this.  An increasing number of sites I frequent have started to prevent Ctrl-W, recently, and it's been driving me bonkers.

Comment 35

[:Gijs (he/him)]

(In reply to CJ Kucera from comment #34)
> Seems there's been no activity on this in awhile; would be nice to have a
> way of dealing with this.  An increasing number of sites I frequent have
> started to prevent Ctrl-W, recently, and it's been driving me bonkers.

Please file a new bug with more details. Without plugins involved, I don't think this should still be possible. Some of the related/dependent bugs have been blocking ctrl-w on (from a quick look) Firefox 48 and later.

Comment 36

[CJ Kucera]

(In reply to :Gijs Kruitbosch from comment #35)
> Please file a new bug with more details. Without plugins involved, I don't
> think this should still be possible. Some of the related/dependent bugs have
> been blocking ctrl-w on (from a quick look) Firefox 48 and later.

Okay, will do.  I've got at least one reproduceable case on Firefox 50 while on Safe Mode.  Thanks!

Comment 37

[CJ Kucera]

I've put that in as bug 1319643, btw.  Thanks again!

Comment 38

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 39

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

This doesn't reproduce for me, is it perhaps fixed?

Comment 40

[:Gijs (he/him)]

(In reply to Tom Ritter [:tjr] (ni for response to sec-[approval|rating|advisories|cve]) from comment #39)

This doesn't reproduce for me, is it perhaps fixed?

I think this is answered in comment #32. We now prevent web content overriding some shortcuts (like ctrl-w), but not others. Does that answer the question?

Comment 41

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

Given that we've closed off the vectors we know of and the lack of input on this, we're going to close this as resolved. If there are future shortcuts that need followup, they can be filed as a new bug.