459744 - Javascript can consume all keystrokes in content area, including tab and app menu shortcuts

Status: RESOLVED DUPLICATE of bug 380637

(Core :: DOM: Navigation, defect, P2)

Description

[Chris Lawson (gone)]

STR:

1) Visit http://forums.mozillazine.org/viewforum.php?f=12
2) Using FAYT (either auto-activated or with the / key), search for something like "fa" that isn't found.
3) Cmd-anything (or Tab).

ER:

Command-shortcut works as normal. Gecko should NEVER be able to eat Command shortcuts like this. Tab key should cycle through focusable elements on page.

AR:

Command-shortcut is eaten by page and results in looping through text size control. Tab key does the same.

A reduced testcase would be VERY helpful here. See also this forum thread, where the issue is discussed further:

http://forums.mozillazine.org/viewtopic.php?t=834415

On a scale of 1 to "certain", I'd say this is pretty likely to have been caused by the Gecko key-handling rewrite on trunk. CCing Josh, since he'll at least know who else might be able to help out here.

Comment 1

[Chris Lawson (gone)]

Attachment: Reduced testcase

Here's a thoroughly reduced testcase. New STR:

1) Use FAYT to find any of the characters appearing in the phrase "Eat all keystrokes!"
2) Observe all subsequent keystrokes are eaten by the script.

This definitely should NOT be possible. Yes, it's bad coding, but Gecko shouldn't be allowing it to happen in the first place.

Comment 2

[Stuart Morgan]

Moving to core. Note that this affect Firefox 3 as well, but the STR for the attached test case in Firefox would have "click on the link" for step 2.

This makes it possible for a page to accidentally or deliberately completely block a non-mouse-using user from doing anything at all with the browser--including closing the page--which isn't acceptable.

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Better STR for Firefox (works in Camino as well):

1) Start FAYT in links mode (')
2) Esc, or wait for the Find bar to time out
3) Try to do anything with the keyboard: enter, arrow keys, Cmd-Q

ER: At least some keyboard shortcuts work
AR: No keyboard shortcuts work, as the page consumes every single keypress (although for Cmd-Q, the 'Firefox' menu flashes like it knows it wants to quit).

This seems like a MAJOR accessibility regression (Cmd-key shortcuts were not eaten in 1.8 Cocoa Widgets, so a keyboard user could at least close or navigate away from the page after getting stuck).

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Er, I forgot one obvious step:

1a) Type the letter "e"

Comment 5

[philippe (part-time)]

Note: you don't need FAYT to repro the issue. If FKA and 'tab to form controls and links' is enabled, just tab to the link. You can't do anything from the keyboard anymore.

And yeah, Gecko 1.8.1 does allow the use of command+xxx (keyboard shortcuts) to work correctly.

'onKeyPress' on links as long been known as a hell in accessibility circles.

Comment 6

[Daniel Veditz [:dveditz]]

Nominating for blocking 1.9.1, and if it gets fixed on trunk we can take it on branch. I don't have a big enough stick to make people fix this on branch if they wouldn't do it for trunk. It does sound like an accessibility problem that should be fixed.

Comment 7

[Stuart Morgan]

Attachment: Cocoa embedding fix

In bug 428047 comment 6, Josh appeared to be saying that the current behavior is considered correct by at least some people. I disagree very strongly, but I really don't want a stop-ship regression for Camino to get bogged down in whether Firefox wants this and if it's safe for the branch, so this patch has the following effects:
 - For embedders (Camino): Always give the menu first crack at the event. Pass unhandled events to Gecko, and pass handled events to plugins if they are still in focus.
 - For everyone else: No change in behavior.

If Firefox wants the changed behavior too, it can easily be done in a follow-up patch and that can be evaluated for risk and target release separately.

Comment 8

[Josh Aas]

I did some messing around and talked to some people, I believe the current behavior is correct. Safari even behaves the same way we do, you can see that if you take the basic test case here and move the key handler to the body instead of the link. Someone suggested that HTML 5 may even require this behavior, but I'd have to talk to hixie to figure that out.

At this time, I am not inclined to take a patch in our widget code. If Camino wants to change behavior we consider to be correct, you'll have to do it in Camino code, probably by tapping into sendEvent: somewhere.

Comment 9

[Josh Aas]

Attachment: simpler test case, works in Safari

Comment 10

[Mike Pinkerton (not reading bugmail)]

I fail to understand how a browser (embedding or not) would ever voluntarily allow a website to make keystrokes stop working for the user. That's just a bad user experience all around. If this makes it to the standard, I would be very very sad as clearly they only care about content developers and not the people who have to use the internet. The user should always be in control, that's a fundamental rule of user experience. 

Who are these "some people" to which you refer? I'm gobsmacked that this is "correct behavior".

Comment 11

[Stuart Morgan]

Safari at least allows tab, providing a way for keyboard users to reach a portion of the UI where keystrokes work.

The current Gecko code *completely* eliminates keyboard control. How is that correct behavior?

Comment 12

[Josh Aas]

Personally I don't necessarily disagree, so lets call it the "intended" behavior instead of "correct". Doesn't change anything as far as taking that patch goes though.

I really am not well versed in the specifications or history involved here, it would be best to ask those who are. At the widget level, I just need to be sure that the patch fits intended behavior and right now I don't think it does.

Comment 13

[Stuart Morgan]

Josh wanted me to clarify my last comment:
If it's correct for all other keystrokes to be eaten, then tab absolutely needs to work, as it does in Safrai. Without absolutely no way for keyboard users to escape, this is an accessibility nightmare.

Comment 14

[Josh Aas]

(In reply to comment #11)
"Safari at least allows tab"

Sweet, that sounds like a promising possibility for us.

Comment 15

[Stuart Morgan]

(Re-re-changing title and assignee change, since it keeps getting stomped.)

Comment 16

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

BTW, Opera at least allows Cmd-W, Cmd-Q, and tab switching. It's hard to figure out how the rest of Opera's content keyboard shortcuts work :P but they're very clearly eager to let you leave the page.

(In reply to comment #8)
> I did some messing around and talked to some people, I believe the current
> behavior is correct. 

Which people did you talk to? It would be useful to hear their reasons for why the current behavior is considered correct.

Comment 17

[philippe (part-time)]

(In reply to comment #16)
> BTW, Opera at least allows Cmd-W, Cmd-Q, and tab switching. It's hard to figure
> out how the rest of Opera's content keyboard shortcuts work :P but they're very
> clearly eager to let you leave the page.

* Opera 9.61 allows a subset of keyboard shortcuts to work, but weirdly Cmd-L (focus location bar) is eaten.
'tabbing' through links is done with Cmd up/down arrow key (eaten as well, with onkeypress attached to a link).

* Fwiw, IE 7 winXPsp2 allows all keyboard shorts (Ctrl+xxxx) to work - always (ok, all those that I know, I'm not a user of that browser...). Tabbing through links always work.

* Still confused by the HTML 5 specs about this.
http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#scripting
seems to imply that (non-content) keyboard shortcuts should be captured (accessibility nightmare), but I need to dig deeper.

Comment 18

[Josh Aas]

My impression is that we don't know the right solution here yet, this is cross-platform behavior, and it isn't an immediate regression from Firefox 3. I don't see us blocking now with that being the case.

If someone can show that this problem is specific to the Mac OS X version of Gecko then I think we should block.

Comment 19

[Stuart Morgan]

(In reply to comment #18)
> and it isn't an immediate regression from Firefox 3

But it is an immediate regression for OS X Cocoa widgets; Camino cannot ship a 1.9 product without accepting this as a regression. Gecko isn't just here for Firefox.

Comment 20

[Carsten Book [:Tomcat]]

This Problem can also be reproduced with the Testcase on Mozilla/5.0 (Windows; U; Windows NT 5.2; en-US; rv:1.9.1b2pre) Gecko/20081105 Minefield/3.1b2pre and Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.1b2pre) Gecko/20081105 Minefield/3.1b2pre

Comment 21

[Josh Aas]

Moving to all OS versions as our Mac OS X implementation seems to be in line with decisions made for Gecko so far, we should fix this for Gecko in general.

Comment 22

[Samuel Sidler (old account; do not CC)]

Then re-nominating since the blocking1.9.1- decision was made based on this being Mac-only.

Comment 23

[Mike Beltzner [:beltzner, not reading bugmail]]

Regardless of what the spec says, I don't think we should blindly allow web content to completely remove the ability to control the UI with the keyboard. That's relinquishing too much control.

The Safari solution of at least allowing tab to return you to a locus where keyboard events work sounds reasonable. As do, perhaps, reserving some sort of escape sequence of characters.

I'm not sure that this blocks 1.9.1 at this time; I think I need some more help for someone to explain how we got to this point (was it in order to be spec compliant, or was it just an oversight when doing the re-write?) but that's my initial $0.02.

Comment 24

[Stuart Morgan]

(In reply to comment #23)
> I think I need some more help for someone to explain how we got to this point

We really do, but it has been bafflingly difficult to find out who "someone" should be, which is why I gave up on making progress here. See for example comments 10 and 16, which were never answered.

The opinions of "some people" were enough to block my proposal to allow both Camino and Firefox to keep their previous behaviors, but we can't have any kind of discussion about why because we don't know who "they" are.

Comment 25

[Mike Beltzner [:beltzner, not reading bugmail]]

OK, I think this blocks. It's a regression from Firefox 2, and if we can actually implement that behaviour of just not allowing web pages to eat the tab key (thus allowing the user to tab out to the location bar) then I think we'll be in OK shape.

Comment 26

[Damon Sicore (:damons)]

Neil, can you take a look at this?  If this isn't something for you, can you suggest the right person to take a look?  

Josh, also looking for owner suggestions.

Comment 28

[Stuart Morgan]

Comment on attachment 343508 [details] [diff] [review]
Cocoa embedding fix

Re-requesting review for 1.9.0 at Josh's request. This patch still applies on that branch.

Comment 29

[Josh Aas]

Comment on attachment 343508 [details] [diff] [review]
Cocoa embedding fix

Stuart - does this page break Google Docs in Camino? I suspect this will cause cmd-s in a Google Docs text document to invoke Camino's save page functionality instead of saving the current document.

Comment 30

[Stuart Morgan]

(In reply to comment #29)
> (From update of attachment 343508 [details] [diff] [review])
> Stuart - does this page break Google Docs in Camino? I suspect this will cause
> cmd-s in a Google Docs text document to invoke Camino's save page functionality
> instead of saving the current document.

Yes, "breaking" that kind of thing is the point of the patch; exactly the same type of code can trap Command-Q, Command-W, Command-L, etc.

So long as pages can be made that prevent keyboard-only users from getting to the chrome area through normal means like tabbing, we are unwilling to allow overriding of menu commands as a regression in Camino (comments 11 and 19).

I personally think allowing pages to override menu commands under any circumstances is a mind-bogglingly bad idea in fact (my view is that Google Docs breaks Camino's menus at the moment), but were there a reasonable way for keyboard users to escape I would accept it and move on. That's not the case at the moment, however.

Comment 31

[Josh Aas]

Comment on attachment 343508 [details] [diff] [review]
Cocoa embedding fix

I can't give this patch r+ or otherwise endorse it on technical grounds. It is obvious to me that seriously breaking basic functionality for a large number of top web applications is much worse than potentially suffering focus issues on a poorly coded site. The latter is unlikely to be an issue for many sites with a significant number of visitors and where undesirable behavior can be found on the web it probably isn't nearly as serious as the problems this will certainly cause for many major web applications.

I acknowledge that Gecko has a problem here (in particular, we don't exempt the tab key like WebKit does), but this isn't the right fix - in fact it makes things worse. I'd perhaps take the patch on the grounds that Camino is the only embedding consumer that will ever use the Gecko 1.9.0 branch on Mac OS X and if you really want to make this choice I don't see much harm in allowing you to do it. However, I won't allow this patch on any other branches and if you choose to advocate for it on those grounds then that rationale and the trade-offs involved in taking this patch need to be documented in comments near the code.

Comment 32

[Stuart Morgan]

(In reply to comment #31)
> I can't give this patch r+ or otherwise endorse it on technical grounds.

I'm confused. This patch hasn't changed since we discussed this on Friday when you said you were okay in principle with landing it as a 1.9.0-only embedding-only change and asked me to re-request review for that specific purpose. What was the point of that exercise if you aren't going to review it?

> I'd perhaps take the patch on the grounds that Camino is the only
> embedding consumer that will ever use the Gecko 1.9.0 branch on Mac OS X

Which is exactly what we discussed and is being proposed with this patch. What extra information do you now need in order to make a decision rather than "perhaps" taking it?

Updating the patch with a long explanatory comment before you've decided whether or not to allow the change seems pretty pointless, since that comment won't have any new information in it that could influence the decision.

> [...] if you choose to advocate for it on those grounds [...]

I already have advocated for it on those grounds, in this bug and in direct conversation. I thought the point of revisiting this was that we had agreement on how to move forward for 1.9.0+embedding here (to prevent a very serious regression in Camino's accessibility support in the short term), not to restart the entire discussion from scratch and rehash the same arguments again.

Comment 33

[Josh Aas]

On Friday I said I was OK with it in general if it applied only to Camino, but I still needed to do a normal review, which you then requested and I just did. The result is that I'll take the patch if you update the comments and they look good. I may object to your new comments, accidental changes to the code, anything like that. I'm not out to make more work for you or play games, I'm just not using 100% committal wording about patches I haven't seen.

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #31)
> It is
> obvious to me that seriously breaking basic functionality for a large number of
> top web applications is much worse than potentially suffering focus issues on a
> poorly coded site.

This is far from obvious to me.  I think users are far more likely to be aware of the keyboard shortcuts for the browser as a whole than for particular Web applications, and I think allowing Web applications to break those shortcuts is broken.

Comment 35

[Josh Aas]

I'm thinking of the question in terms of my understanding of web standards, the details of which I'll admit I don't understand well. Please correct me if I'm wrong, but things are set up so that web applications can use whatever keys they want, on purpose, and the problem here is that there are *no* exceptions, not even tab. Whether or not I agree with that from a usability perspective, that is the standard Gecko chose to follow and many major web applications coded against it. Cocoa widgets doesn't seem like the place to stage a coup against those standards.

My guess is that such a standard exists so that, for example, cmd-s can save a document the same way in Google Docs as in MS Word or TextEdit.

Comment 36

[Josh Aas]

(In reply to comment #35)
> Cocoa widgets doesn't seem like the place to stage a coup
> against those standards.

To be clear, I'm all for re-examining this behavior in general, for all platforms, but that is not what the patch here is doing. It does seem obvious to me that Cocoa widgets should not stand alone in behaviorally differing on this point.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to comment #35)
> wrong, but things are set up so that web applications can use whatever keys
> they want, on purpose, and the problem here is that there are *no* exceptions,
> not even tab. Whether or not I agree with that from a usability perspective,

The relevant documents do not define behavior here, since they don't say how (or even whether) browser key handling fits within the processing model.  For that matter, there is no relevant standard for DOM Key Events, since the DOM working group gave up on it; what we implement is somewhat related to a very old working draft from before they gave up.  The Web API group has since picked up the work, but I don't think we've synced with their changes.


Agreed that Cocoa widgets probably isn't the right place to differ, though.

Comment 38

[Chris Lawson (gone)]

As far as I'm concerned, if Google thinks they can override the use of the Command key, they're very mistaken. They should never assume they can override keys reserved for the OS (and I've never seen any documentation that suggests the Command key should be passed on to Web sites; it's historically been reserved for execution entirely within the OS-app space).

Comment 39

[Aaron Leventhal]

IMO the right place for this discussion is something like the whatwg or HTML 5 mailing list. It's an unfortunate issue for all browsers, not just Firefox.

On a Mozilla-related note: if something is running in something like Prism it as opposed to within browser chrome that might have an impact.

Sometimes an app developer needs to do this, in order to provide discoverable keyboard shortcuts. For example, an app that provides a drag and drop UI will want discoverable keys for moving to an item, copying it, and pasting it somewhere else.

But, I'd rather see this all discussed on a more general mailing list for browsers.

Comment 40

[Stuart Morgan]

Attachment: 1.9.0 Cocoa embedding workaround v2

New version with expanded comment; let me know if there's anything else you want it to cover.

> The latter is unlikely to be an issue for many sites with a
> significant number of visitors and where undesirable behavior can be found on
> the web it probably isn't nearly as serious as the problems this will certainly
> cause for many major web applications.

The reason that we are so adamant about preventing this regression for our 2.0 release is that we already have evidence that suggests this isn't true. The complete-black-hole test case here is not invented, it's reduced from MozillaZine. Off the top of my head, we have another bug where whitehouse.gov breaks our tab-switching shortcuts. And that's just what our handful of nightly build users have reported; already we've seen more people affected by these kind of issues than have ever reported that Google Docs shortcuts don't work in our current release version (and that's the only site I remember anyone filing keyboard shortcuts about at all).

If you give people the power to do something with JS, some number of people--including some popular sites--are going to screw it up. (Plus, I don't think it's wise to assume that porn/gambling/fake-antivirus/etc. people won't ever figure out they can toss a bit of JS on their pages and make it hard for Gecko users to close popups or leave pages they are tricked into visiting.)

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 362725 [details] [diff] [review]
1.9.0 Cocoa embedding workaround v2

Impressive train-wreck.

Comment 42

[Josh Aas]

Stuart's patch is a work-around for Camino on the 1.9.0 branch only. It is not a general Gecko solution to this bug and won't be going on the 1.9.1 branch.

Comment 43

[Daniel Veditz [:dveditz]]

Comment on attachment 362725 [details] [diff] [review]
1.9.0 Cocoa embedding workaround v2

Approved for 1.9.0.8, a=dveditz for release-drivers

Comment 44

[Josh Aas]

checked in Stuart's Mac OS X embedding-only patch for 1.9.0.8

Checking in widget/src/cocoa/nsChildView.mm;
/cvsroot/mozilla/widget/src/cocoa/nsChildView.mm,v  <--  nsChildView.mm
new revision: 1.368; previous revision: 1.367

Comment 45

[Samuel Sidler (old account; do not CC)]

Can someone from Camino please verify this bug is fixed on 1.9.0 and change the keyword from fixed1.9.0.8 to verified1.9.0.8?

Comment 46

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

When loading either of the testcases in this bug, I can still trigger menu commands with shortcuts (e.g. Cmd-L for the location bar, Cmd-[ for back, Cmd-Opt-{L|r}arrow to switch tabs).  Verified1.9.0.8 in

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en; rv:1.9.0.8pre) Gecko/2009022700 Camino/2.0b3pre (like Firefox/3.0.8pre)

Comment 47

[Johnny Stenback (:jst)]

Based on the discussion about this in last weeks Firefox meeting we won't be blocking the release on this old bug (or old problem, even if this particular bug is not that old).

Comment 48

[Dão Gottwald [:dao]]

What needs check-in here, and where?

Comment 49

[Nochum Sossonko [:Natch]]

This is really bad ue, a page can block all browser commands including Alt-F4 on Windows. The page can similarly capture all mouse events and block context menus, etc.

Comment 50

[Johnny Stenback (:jst)]

Not holding the release for this, and it's unlikely we'd be able to fix this in a sane way for all platforms anyways in the amount of time we have left (i.e. none) for big changes for 1.9.2.

Comment 51

[Johnny Stenback (:jst)]

Smaug, do the more recent event standardization efforts say anything one way or another about this? Either way, we're not going to fix this for alpha1.

Comment 52

[Michael Kohler [:mkohler]]

This is a mass change. Every comment has "assigned-to-new" in it.

I didn't look through the bugs, so I'm sorry if I change a bug which shouldn't be changed. But I guess these bugs are just bugs that were once assigned and people forgot to change the Status back when unassigning.

Comment 53

[Johnny Stenback (:jst)]

This sounds like a dupe of bug 380637, duping.