240627 - Add debug warning in Mozilla for Ctrl+Shift+[A-F] shortcuts

Status: RESOLVED FIXED

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

Description

[Philip K. Warren]

In order to prevent problems like in Bug 186789, we should probably add an
assertion somewhere in Mozilla when a shortcut using Ctrl+Shift+[A-F] is
created. We could even ifdef out the assertion for non GTK2 builds.

This would make it easier to track down and reassign keyboard shortcuts which
are impossible to enter on GTK2 builds.

Comment 1

[Aaron Leventhal]

Hmm, how would the assertion help if someone is building a XUL app using a
release build?

Comment 2

[[e-mail]]

I'd log a console message, except it would make xbl depend on the console
service; I don't know if that's allowed.

Comment 3

[Aaron Leventhal]

We also should add a note here:
http://www.mozilla.org/projects/ui/accessibility/accessible-xul-authoring.html

and here:
http://www.mozilla.org/projects/ui/accessibility/mozkeyintro.html
This doc should be retitled so people realize it has a FAQ on how to choose
keystrokes for features.

Comment 4

[Aaron Leventhal]

Neil said on IRC that he was looking into this.

Comment 5

[[e-mail]]

OK, so when do we want to issue this warning? Since Windows and Mac define
Ctrl+Shift+D for Add Bookmark Immediately we don't want that combo generating
warnings if we can help it... perhaps we could output to JS console only on Unix
and output to stdout on other platforms?

Comment 6

[Aaron Leventhal]

Neil, can it be done when the Window opens or when the assignment is registered?
Also, can the warning give some information about the key and location in the
source where the problem is?

Comment 7

[[e-mail]]

The way I've done it it happens when the assignment is registered, which for
<key> appears to be the first time you press a key in the given window, but for
<handler> is when the binding is loaded.

As for the message, I should be able to locate a URL, for <key> that would be
the owner document (overlay information will have been lost by then), while for
<handler> that would be the xbl document.

But that wasn't actually my question... I meant, in which combinations of builds
and platforms? For instance, we don't want release Windows builds warning of the
Ctrl+Shift+D shortcut that adds a bookmark immediately.

Comment 8

[Aaron Leventhal]

Here's my philosophy on this -- I think in the future release builds will be
used to create XUL apps. Why should everyone have to build their own Mozilla
from source just because they want to use XP Toolkit. They may be building an
app that they want to run on many platforms, so just because they're building it
on Windows doesn't mean that the app won't run on Linux.

Don't mean to give a bunch of irrelevant discussion on this, but I think it's
good to agree on the possible scenarios. There may be other situation where this
comes up. Because of the need to develop cross platform XUL apps using something
other than debug builds, I think good error messages for XUL and XBL source are
useful even in release builds. In general, do we need a way to tell if the
current environment is being used as a tool to build apps with?

Back to this bug. It's probably hard, but can your code emit the warning on
Windows or Mac only when the binding is not from a platform-specific file?

Comment 9

[[e-mail]]

(In reply to comment #8)
>Back to this bug. It's probably hard, but can your code emit the warning on
>Windows or Mac only when the binding is not from a platform-specific file?
Yes, it's hard. The code is only triggered when you press a key.
It might be possible to add a flag to the modifiers to disable the warning,
e.g. modifiers="ctrl, shift, platform" or something...

Comment 10

[Aaron Leventhal]

(In reply to comment #9)
> (In reply to comment #8)
> >Back to this bug. It's probably hard, but can your code emit the warning on
> >Windows or Mac only when the binding is not from a platform-specific file?
> Yes, it's hard. The code is only triggered when you press a key.
> It might be possible to add a flag to the modifiers to disable the warning,
> e.g. modifiers="ctrl, shift, platform" or something...

Or modifiers = "ctrl, shift, nowarning"
I don't understand what "platform" would mean it that context. It's not
intuitive to me.

But I like the idea, I think it's good to make people notice that they're using
a keybinding that won't work well on Linux. Half the reason someone would build
a XUL app is that it's cross platform.

Comment 11

[[e-mail]]

Attachment: Proposed patch

Comment 12

[[e-mail]]

Attachment: Fix shortcut key display

The previous patch affected the display of shortcut keys, so for this version I
have had to change the logic to check for a leading comma instead.

Comment 13

[Aaron Leventhal]

Comment on attachment 151243 [details] [diff] [review]
Fix shortcut key display

Neil, I have no idea on this area of the code. Shouldn't you look for a
reviewer who knows about the xbl code a little more?

Comment 14

[Brian Ryner (not reading)]

Comment on attachment 151243 [details] [diff] [review]
Fix shortcut key display

My only objection is that the message should be localizable.  There's already
an xbl.properties file, can you put this message in there?

Comment 15

[[e-mail]]

Attachment: Localization

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 151717 [details] [diff] [review]
Localization

Looks ok, but I'd suggest giving ReportGTK2Conflict a more generic name
(ReportOSKeyConflict, perhaps), and changing the text appropriately.  Also, a
comment as to what's going on would be a good idea.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 151717 [details] [diff] [review]
Localization

>Index: nsXBLPrototypeBinding.h
>+  nsXBLProtoImpl* GetImplementation() { return mImplementation; }

Make this a const method, and have it return a |const nsXBLProtoImpl*|, please.

>Index: nsXBLPrototypeHandler.cpp

>+#include "nsIStringBundle.h"
>+#include "nsIConsoleService.h"

Please don't include these; more below.

>+nsXBLPrototypeHandler::ReportGTK2Conflict(const PRUnichar* aKey, const PRUnichar* aModifiers, nsIContent* aKeyElement)

>+      nsCString spec;
>+      aURI->GetSpec(spec);

nsCAutoString?	Or was there a good reason to use nsCString?

>+  nsCOMPtr<nsIStringBundleService> stringBundleService =
>+    do_GetService(NS_STRINGBUNDLE_CONTRACTID);

Please use nsContentUtils::ReportToConsole instead of doing all this manually. 
That will have pleasant side-effects like properly linkifying the URI in the
error message, saving codesize, being faster, etc.

Also, it may allow you to pass in a useful aSourceLine if you can produce one
somehow... can you?

Comment 18

[[e-mail]]

Attachment: Updated for new features in the source ;-)

I also added a Windows warning but I don't know the Mac key combo reservations.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 198384 [details] [diff] [review]
Updated for new features in the source ;-)

>Index: content/xbl/src/nsXBLPrototypeHandler.cpp
>+nsXBLPrototypeHandler::ReportKeyConflict(const PRUnichar*

>+  if (mPrototypeBinding)
>+    NS_NewURI(getter_AddRefs(uri), mPrototypeBinding->GetImplementation()->mClassName);

Wouldn't using mPrototypeBinding->DocURI() make more sense?  I think that's
guaranteed to be the XBL document URI, whereas mClassName can be affected by
the XBL itself (and may not even be a URI, I think).

nsContentUtils::ReportToConsole(nsContentUtils::eXBL_PROPERTIES,
...
>+                                  uri, EmptyString(), 0, 0,

Why not mLineNumber for the line number?  That's why we have the member...

sr=bzbarsky with those fixed.

Comment 20

[[e-mail]]

Attachment: Updated for review comments

Ah, that's nice, I don't have to include nsNetUtil.h either :-)

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 198795 [details] [diff] [review]
Updated for review comments

>Index: content/xbl/src/nsXBLPrototypeHandler.cpp
>+                   aKeyElement ? aKeyElement->GetBaseURI() : nsnull;

Argh.  I find new stuff every time I look at this.  :(

You want aKeyElement->GetOwnerDoc()->GetDocumentURI(), unless you want this URI
to be affected by xml:base (and I don't think you do).

sr=bzbarsky with that; no need for more review requests; I've checked it over
with a fine-toothed comb this time.  ;)

Comment 22

[[e-mail]]

Attachment: Updated for crash fix

I found out the hard way that I'm mixing my references :-[

Comment 23

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 198804 [details] [diff] [review]
Updated for crash fix

Oh, yikes.  The comment about using the GetOwnerDoc() URI stands, though (and
then the URI doesn't have to be an nsCOMPtr).

Comment 24

[[e-mail]]

I checked in the fix based on attachment 198795 [details] [diff] [review], but with the change to
aKeyElement->GetOwnerDoc()->GetDocumentURI() (which also fixes the crash).

Comment 25

[[e-mail]]

Comment on attachment 198795 [details] [diff] [review]
Updated for review comments

Oh, and I also changed the nsCOMPtr to an nsIURI* too.

Comment 26

[Marek Stępień [:marcoos, inactive]]

Comment on attachment 198804 [details] [diff] [review]
Updated for crash fix

+# LOCALIZATION NOTE: do not localize key="%S" modifiers="%S"
+GTK2Conflict=Key event not available on GTK2: key="%S" modifiers="%S"
+WinConflict=Key event not available on some keyboard layouts: key="%S" modifiers="%S"

Just a question from l10n teams: why shouldn't we translate 'key' and 'modifier' here?

Comment 27

[[e-mail]]

(In reply to comment #26)
>Just a question from l10n teams: why shouldn't we translate 'key' and
>'modifier' here?
Because they are XBL keywords rather than English words.