The main accesskey issue is that content elements can prevent access to a chrome element with the same accesskey (bug 128452). A additional issue is that it's not even possible to change generalAccessKey to something other than either accelKey or menuAccessKey (at least on systems without [Meta] - see bug 179816). One way to improve the situation and fix both issues would be to replace the pref ui.key.generalAccessKey (value is a VK-code) with two preferences ui.key.contentAccessKey and ui.key.chromeAccessKey where both are numbers indicating a modifier state mask (where Shift=1, Ctrl=2, Alt=4, Meta=8; values from nsMenuBarListener.cpp). The general idea for implementing the bug would be to pass the modifier state mask as an additional argument to nsEventStateManager::HandleAccessKey and then for each PresContext check first whether the modifier state is equal to what's expected for its DocShell's type (typeChrome or typeContent). This should nicely make the difference between content and chrome, allow to keep the situation as is, but just as well allow to set the content accesskey modifier to Shift+Alt or Ctrl+Shift or disable it separately. Should this be alright, I'll see whether I can assemble a patch.

This patch gets rid of ui.key.generalAccessKey completely. Instead there are two new pres ui.key.chromeAccess and ui.key.contentAccess which can be set independently to any modifier combination (the new default is an additional Shift modifier for content access keys). Note: This patch compiles and works on the current branch, but should apply without major (if any) modifications to the trunk as well. Reviewers: Please estimate how much this patch is still lacking and whether it's got a fair chance to make the Firefox 2 train. Thanks.

Comment on attachment 225149 [details] [diff] [review] branch patch Not a super reviewer.

Comment on attachment 225149 [details] [diff] [review] branch patch I'm not the right reviewer for this code; I have no idea what most of it does. That said, this will break existing installs where the user has set the "ui.key.generalAccessKey" pref, won't it? I'm not sure that's acceptable, especially on branch. This will also change what the accesskey accelerator is for web pages, which is a Gecko web api change I'm not convinced we should be making on the branch.

(In reply to comment #3) > I'm not the right reviewer for this code; I have no idea what most of it does. I'd appreciate if you could hint me into the direction of who would be appropriate to review this. > That said, this will break existing installs where the user has set the > "ui.key.generalAccessKey" pref, won't it? I'm not sure that's acceptable, > especially on branch. If by "break" you mean that the pref isn't respected anymore, then yes. One option to avoid this would be to automatically migrate the pref on startup. But since that pref doesn't have UI, it should normally not have been changed and those who changed it once did so intentionally and simply will have to do so once more. > This will also change what the accesskey accelerator is for web pages, which is > a Gecko web api change I'm not convinced we should be making on the branch. I'll revert the access modifiers to be identical for chrome and content for the next patch. Changing the defaults should be done in another bug (e.g. bug 128452 or a new bug) anyway.

I'd look at the CVS blame for this code to figure out who should review. That's what I'd have to do to say anything intelligent about who should review, at least. > But since that pref doesn't have UI It does in some Gecko apps. You're changing code that affects _all_ Gecko apps, not just Firefox.

Comment on attachment 225149 [details] [diff] [review] branch patch (In reply to comment #2) > Not a super reviewer. Oops, I meant second-review. Oh well, review then. > It does in some Gecko apps. You're changing code that affects _all_ Gecko > apps, not just Firefox. I don't see any apps in Mozilla's CVS expose this pref. Anyway, inserting a few lines for pref migration at startup could be done at the point where the prefs are read.

Comment on attachment 225149 [details] [diff] [review] branch patch Sorry, I don't mean to be difficult but I think Mats Palmgren is the best guy for keyboard stuff going forward. He's working on Mozilla foll time now so it shouldn't take long.

Comment on attachment 225149 [details] [diff] [review] branch patch Index: MOZILLA_1_8/content/events/src/nsEventStateManager.cpp +// mask values for ui.key.chromeAccess and ui.key.contentAccess +#define MODIFIER_SHIFT 1 Add a NS_ prefix at least. + if (keyEvent->isShift) + modifierMask = modifierMask | MODIFIER_SHIFT; Personally, I find |= more readable. Index: MOZILLA_1_8/accessible/src/base/nsAccessible.cpp + // XXXzeniko what happens when the prefs change afterwards? You could add an observer, as ESM does, but I don't think that's necessary here - just call GetIntPref() every time and instead of caching the values cache the service. + if (!docShell) return 0; Move the return(s) to a new line. Generally, could you make sure all the lines you change is <= 80 columns. ============== As Boris said, I think we need to migrate an existing non-default "ui.key.generalAccessKey" into "ui.key.contentAccess". I don't know what the best method would be to do that though.

Simon, please also add a patch for the new default values to bug 128452. I think that's the important part of this change.

Fixing all nits from comment #8. (In reply to comment #8) > As Boris said, I think we need to migrate an existing non-default > "ui.key.generalAccessKey" into "ui.key.contentAccess". > I don't know what the best method would be to do that though. IMO that migration is up to the applications exposing that pref. All applications in Mozilla's CVS keep this pref hidden and set it to a reasonable default depending on the platform. Since that default is mirrored in the new prefs, I'd say that those who prefer different behavior and learned about the pref should be able enough to learn about the change and adapt the new prefs in the same way (that concerns individuals as well as distributors). And applications exposing the original pref might want to do more than a simple one-to-one conversion, especially since they could either tie the two new prefs together or expose them seperately. So they know best and they should thus do it themselves. Feel free to differ. (In reply to comment #9) > Simon, please also add a patch for the new default values to bug 128452. > I think that's the important part of this change. I'll do so as soon as this bug has been fixed.

Comment on attachment 226218 [details] [diff] [review] branch patch (take 2) nsAccessible.cpp does not compile for me (even on 1.8 branch). You need to add a #include "nsIDocShellTreeItem.h" at the top and then change: + nsCOMPtr<nsIDocShell> docShell = do_QueryInterface(container); + if (!docShell) ... + docShell->GetItemType(&itemType); to nsCOMPtr<nsIDocShellTreeItem> treeItem(do_QueryInterface(container)); if (!treeItem) ... treeItem->GetItemType(&itemType); The code looks good otherwise, r=mats with the above changes.

(In reply to comment #10) > ... those who prefer different behavior and learned about the pref > should be able enough to learn about the change and adapt the > new prefs in the same way ... I agree that they should be *able* to do that, I just think it would be nice of us to not force them to do that, if we can avoid it. I don't feel strongly about it though, I'll leave the pref migration issue for the superreviewer (and others) to comment on.

Interestingly it did compile for me, don't know why. Thanks Mats. Boris, would you mind having a look as a super-reviewer? Or would dbaron or roc be more appropriate?

Comment on attachment 226301 [details] [diff] [review] branch patch (nits fixed) I'll take it

I'd actually like to see a trunk patch land before putting this on the branch. Furthermore, if we aren't going to change the key defaults, then landing this on the branch adds unnecessary risk; on the other hand, it's not clear that changing the key defaults on branch is a good idea. So I think this should be discussed before any branch landing. I think there should be pref migration code.

I personally think that caching these prefs is a waste of time. They don't get used very often (at most once per user event), so it would be simpler and not perceptibly slower to just get them every time we need them. This change means that XUL in content documents uses different accelerators from the same XUL in chrome documents. I understand that that could be seen as a win, but it could also be very confusing. We need wider feedback on that. I think that ui.key.accelKey should be removed because it's redundant with the new prefs. Code that uses it (e.g. http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/nsMenuFrame.cpp#1527 ) should look at the new prefs instead.

(In reply to comment #15) > I'd actually like to see a trunk patch land before putting this on the branch. Of course. I just preferred testing on the more stable branch first for myself. > Furthermore, if we aren't going to change the key defaults, then landing this > on the branch adds unnecessary risk; on the other hand, it's not clear that > changing the key defaults on branch is a good idea. So I think this should be > discussed before any branch landing. I don't intend to change any defaults in this bug, that's up to bug 128452. > I think there should be pref migration code. For all Gecko apps or only for those exposing the pref? If for all, I'd have added a check for nsIPrefBranch::PrefHasUserValue for "ui.key.generalAccessKey" to nsEventStateManager::Init and - if it returned true - adjusted the two new prefs accordingly before resetting the old one. (In reply to comment #16) > I personally think that caching these prefs is a waste of time. They don't get > used very often (at most once per user event), so it would be simpler and not > perceptibly slower to just get them every time we need them. They actually get used at least three times for each event (usually about a dozen times), so caching them in nsEventStateManager makes IMHO sense - otherwise I'd have to pass them through HandleAccesskey or fetch them again for every presShell. Caching sPrefBranch in nsAccessible might however be unnecessary (or did you mean that in the first place?). > This change means that XUL in content documents uses different accelerators > from the same XUL in chrome documents. I understand that that could be seen as > a win, but it could also be very confusing. We need wider feedback on that. As I see it, that's outside of the scope of this bug and should thus not be relevant. > I think that ui.key.accelKey should be removed because it's redundant with the > new prefs. Code that uses it (e.g. > http://lxr.mozilla.org/mozilla/source/layout/xul/base/src/nsMenuFrame.cpp#1527 > ) should look at the new prefs instead. Am I missing something? The two new prefs just split ui.key.generalAccessKey. ui.key.accelKey is on all platforms a different key. Or do you mean menuAccessKey (which usually equals generalAccessKey, except on the Mac where it's disabled)?

Or should/can pref migration go somewhere else?

(In reply to comment #17) > > Furthermore, if we aren't going to change the key defaults, then landing this > > on the branch adds unnecessary risk; on the other hand, it's not clear that > > changing the key defaults on branch is a good idea. So I think this should be > > discussed before any branch landing. > > I don't intend to change any defaults in this bug, that's up to bug 128452. Nevertheless, if we decide not to change the defaults, then there is no point in landing this on branch, and we should not land this on branch.

(In reply to comment #19) > > I don't intend to change any defaults in this bug, that's up to bug 128452. > > Nevertheless, if we decide not to change the defaults, then there is no point > in landing this on branch, and we should not land this on branch. I'm not sure I can follow you on this. Fixing this on the branch even with the same defaults will allow those most affected by bug 128452 to improve/fix the situation without having to recompile Firefox. But if I can't fix this bug without also fixing bug 128452, then I'll fix both. I'll see whether I can get some feedback on m.d.a.firefox.

relatively speaking, the number of users who will discover and change hidden prefs is very small and not worth addressing for their own sake.

In order to lessen the impact of this bug, I've reverted this patch to always use ui.key.generalAccessKey, but allow the new two prefs (ui.key.contentAccess and ui.key.chromeAccess) to override the original pref. This allows to fix bug 128452 and completely fixes bug 179816 without enforcing a change.

Comment on attachment 226621 [details] [diff] [review] trunk patch (take 3) I think we all agree that we want to change the default value of "ui.key.contentAccess" to be SHIFT+Access as soon as possible. But this patch does not support having default values for the new prefs at all (since they would override a user-assigned value for the old pref). I do appreciate being able to manually override the "contentAccess" key which this patch provides, but I think the more important thing to fix is to have new default values. So, I have a suggestion to support that: 1. change the default value for "ui.key.generalAccessKey" to -1 on all platforms. 2. add default values for "ui.key.chromeAccess" and "ui.key.contentAccess" 3. make the lookup code first look at "generalAccessKey" and if it's != -1 then use it (it was set by the user); else use the new prefs. This would support having new default values on trunk, but also keeping the old default values on branch if that's what we want but still allow users to override the default also on branch... What do you think? (FWIW, I think we should change the default value on branch as well, I agree completely with bug 128452 comment 31)

*** Bug 282222 has been marked as a duplicate of this bug. ***

(In reply to comment #23) > I think we all agree that we want to change the default value of > "ui.key.contentAccess" to be SHIFT+Access as soon as possible. I hope we do, although I'm not sure that I have heard a clear statement in favor of this yet. > This would support having new default values on trunk, but also keeping > the old default values on branch if that's what we want but still allow > users to override the default also on branch... That indeed sounds better than my previous proposal. I've updated the patch accordingly. One remaining question: in case we really change the default accesskey modifier for Gecko 1.8.1/Firefox 2/etc., how do we communicate that change? Will an entry in the release notes be enough (since the help files don't seem to mention accesskeys at all)?

Comment on attachment 227036 [details] [diff] [review] trunk patch (take 4) Looks good! r=mats

We'll also change the chrome accesskey on Mac to fix bug 207510, won't we?

I think bug 179816 should actually be a dupe of this. Either way, once this gets in please close that bug as fixed if it is.

Any chance of getting the reviewed patch checked in on the trunk? Without proper baking and some feedback during the next two weeks this will never make Firefox 2 (might be too late even with baking, but I'd like to try anyway).

Comment on attachment 227036 [details] [diff] [review] trunk patch (take 4) + case nsIDocShellTreeItem::typeChrome: + return nsContentUtils::GetIntPref("ui.key.chromeAccess", + sChromeAccessModifier); + case nsIDocShellTreeItem::typeContent: + return nsContentUtils::GetIntPref("ui.key.contentAccess", + sContentAccessModifier); This is confusing. Just pass some constant as the default here, say zero. Other than that it looks OK. I'm a bit concerned about the duplicated code in nsAccessible.cpp but I guess sharing more with nsEventStateManager.cpp would not be easy. I'm suspicious that the pref caching in nsEventStateManager is not really worthwhile but I'll save that for another day.

(In reply to comment #30) > This is confusing. Just pass some constant as the default here, say zero. Done. > Other than that it looks OK. I'm a bit concerned about the duplicated code in > nsAccessible.cpp but I guess sharing more with nsEventStateManager.cpp would > not be easy. We could at a later point at least get the NS_MODIFIER_* constants to some more central place (don't know where though). Other than that I'd consider the duplicated code bits not valuable enough to share them through larger parts of Gecko (unless .accelKey and .menuAccessKey are converted to the new constants as well).

I'll announce this change to m.d.platform and MZ once the patch is checked in.

mozilla/content/events/src/nsEventStateManager.h 1.149 mozilla/content/events/src/nsEventStateManager.cpp 1.660 mozilla/accessible/src/base/nsAccessible.cpp 1.199 mozilla/modules/libpref/src/init/all.js 3.649

This is the branch version of the landed patch. The only difference is that the default content access modifier for Mac has been changed back to Ctrl (see bug 207510 comment #8).

Drivers: This patch introduces two new prefs which allow to specify different modifier combinations for content and chrome accesskey. The content pref is furthermore set to Alt+Shift on Windows and Ctrl+Shift on Unix, which greatly alleviates the long-standing issue of web pages taking over chrome keyboard shortcuts (such as Alt+D on this page) and of incorrectly taking over modifier combinations (such as Alt+Shift+D on this page). For backwards compatibility, the old single pref is still recognized and will overwrite the new prefs if set to a non-default value. This patch has been baking for about a week and there haven't been any negative issues mentioned in any of the places this change was advertised. Since all changes are limited to the respective files, the risk of this patch is quite low.

Not going to block on this, but will consider the approval request shortly.

mozilla/embedding/minimo/all.js 1.16.2.1 mozilla/modules/libpref/src/init/all.js 3.585.2.43 mozilla/content/events/src/nsEventStateManager.h 1.137.4.5 mozilla/content/events/src/nsEventStateManager.cpp 1.595.2.23 mozilla/accessible/src/base/nsAccessible.cpp 1.165.2.9

*** Bug 128452 has been marked as a duplicate of this bug. ***

*** Bug 264204 has been marked as a duplicate of this bug. ***

*** Bug 360693 has been marked as a duplicate of this bug. ***

(In reply to comment #40) > *** Bug 360693 has been marked as a duplicate of this bug. *** > shortcuts are executed by websites instead of the browser While the cursor is blinking in the input-mask Ctrl+Prior/Next as shortcut to go to the tab to the left/right is ignored and executed as navigate throw the form-history on pages like dict.leo.org or google.de instead. Reproducible: Always Expected Results: A webpage should not be able to *change* shortcuts. (It can provide additional shortcuts.)