340902 - Split ui.key.generalAccessKey into prefs for content and chrome

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Simon Bünzli]

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.

Comment 1

[Simon Bünzli]

Attachment: branch 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 2

[Aaron Leventhal]

Comment on attachment 225149 [details] [diff] [review]
branch patch

Not a super reviewer.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Simon Bünzli]

(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.

Comment 5

[Boris Zbarsky [:bzbarsky]]

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 6

[Simon Bünzli]

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 7

[Aaron Leventhal]

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 8

[Mats Palmgren (inactive)]

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.

Comment 9

[Mats Palmgren (inactive)]

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

Comment 10

[Simon Bünzli]

Attachment: branch patch (take 2)

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 11

[Mats Palmgren (inactive)]

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.

Comment 12

[Mats Palmgren (inactive)]

(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.

Comment 13

[Simon Bünzli]

Attachment: branch patch (nits fixed)

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 14

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

Comment on attachment 226301 [details] [diff] [review]
branch patch (nits fixed)

I'll take it

Comment 15

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

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.

Comment 16

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

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.

Comment 17

[Simon Bünzli]

Attachment: trunk patch

(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)?

Comment 18

[Simon Bünzli]

Attachment: trunk patch (with pref migration and without prefBranch caching)

Or should/can pref migration go somewhere else?

Comment 19

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

(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.

Comment 20

[Simon Bünzli]

(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.

Comment 21

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

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

Comment 22

[Simon Bünzli]

Attachment: trunk patch (take 3)

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 23

[Mats Palmgren (inactive)]

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)

Comment 24

[Matthew T (active 1999-2002)]

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

Comment 25

[Simon Bünzli]

Attachment: trunk patch (take 4)

(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 26

[Mats Palmgren (inactive)]

Comment on attachment 227036 [details] [diff] [review]
trunk patch (take 4)

Looks good! r=mats

Comment 27

[Greg K.]

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

Comment 28

[Aaron Leventhal]

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.

Comment 29

[Simon Bünzli]

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 30

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

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.

Comment 31

[Simon Bünzli]

Attachment: trunk patch (take 4; nits fixed and unbitrotted)

(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).

Comment 32

[Simon Bünzli]

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

Comment 33

[:Gavin Sharp [email: [e-mail]]]

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

Comment 34

[Simon Bünzli]

Attachment: branch patch

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).

Comment 35

[Simon Bünzli]

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.

Comment 36

[Mike Connor [:mconnor]]

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

Comment 37

[:Gavin Sharp [email: [e-mail]]]

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

Comment 38

[Mats Palmgren (inactive)]

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

Comment 39

[Mats Palmgren (inactive)]

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

Comment 40

[Ria Klaassen (not reading all bugmail)]

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

Comment 41

[mey.wer]

(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.)