Ctrl-Shift-K bound to 2 functions in Compose

User-Agent: Mozilla/5.0 (X11; U; Linux i686; cs-CZ; rv:1.9.0.2) Gecko/2008100707 Fedora/3.0.2-1.fc10 Firefox/3.0.2
Build Identifier: thunderbird-2.0.0.17-1.fc10.x86_64

(originally filed as https://bugzilla.redhat.com/show_bug.cgi?id=468130)

In composer or html-mail-composer, the Format>Discontinue Link menu item
advertises Control-Shift-k as a valid hotkey, however pressing this key brings
up the "Check Spelling" window instead.

Reproducible: Always

Steps to Reproduce:
1.look at Format/Discontinue Link and Options/Spellchecking Options in HTML Composer
2.
3.
Actual Results:
same key shortcut

Expected Results:
shouldn't be

Bryan, I wonder if unlink shouldn't be ctrl+shift+L instead of K, because _link_ is ctrl+K, under Insert. I don't know the history of why these don't match up.

Regardless, ctrl+shift+K is WFM, does not invoke spell using trunk.
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b2pre) Gecko/20081104 Shredder/3.0b1pre

It keeps checking spelling on Mac. Confirming bug.

Created an attachment (id=347226)
patch v1

This seems to fix the issue by switching the access key for discontinue link to cmd-shift-l though I'm having reservations about my approach.

Check spelling is Ctrl+K, not Ctrl+Shift+K. (Is that not the case on mac?)

Re comment 1, I imagine it's not "L" since you don't remove the link, just end it.

Magnus, have you gone to the dark side? http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#232 and my builds say it's accel-shift-k on Linux and Mac, and only accel-k (or F7) on Windows.

Only momentarily. Back on the light side now;)

I was wrong about Ctrl+Shift+L too, looking more closely I think it's currently not "discontinue link"/"remove links" - which are the same menu - because Ctrl+Shift+L is Open Web Location in seamonkey composer... and then we just copied it.

I'm not sure why it's different on different platforms now.
CVS blame only says
  1.56 <email address hidden> 2004-02-04 21:10
  spell check on unix should be ctrl-shift-k like seamonkey.

  Thanks to Stephen Walker for the patch

Might be because of the old gtk limitation, which isn't an issue on trunk - see bug 311756 comment 35. I don't see a reason we can't use the same combination for all platforms now.

And at least it would be very logical to use ctrl-shift-L for that, to match insert link (ctrl-L).

we're going to be using ctrl-k for the thunderbar (bug 452281) so I'd recommend we go with a ctrl-L combination as magnus suggests in comment 6

(From update of attachment 347226)
Minusing, let's make it the same for all platforms, and preferably "L" as per previous discussion.

I'm not sure it actually affects thunderbar though as this is only the composition window hotkeys.

Created an attachment (id=347801)
patch v1.1

I'm suspecting I'm missing something else, do I need to change the accelerators or something to keep them identical cross-platform?

Not quite ready for review until this is sorted out.

(From update of attachment 347801)
wrong wrong..

Created an attachment (id=347803)
patch v1.2

This should taste better. :)

Access key for removing links changed to L, and ifdef removed for the case of checking spelling, standardizing on (ctrl or cmd)-shift-l.

(From update of attachment 347803)
Wrong editorOverlay.xul, and I think in these cases you should bump the entity so localizers can keep up.

(Unless seamonkey wants the same change... but that would need more work.)

> I'm not sure it actually affects thunderbar though as this is only the
> composition window hotkeys.

True in the current separate window it's not likely to affect it. I guess I meant to say I wanted to keep it open for a future where we might also have the compose area inside a tab.

(In reply to comment #12)
> (From update of attachment 347803 [details])
> Wrong editorOverlay.xul, and I think in these cases you should bump the entity
> so localizers can keep up.

So |formatRemoveLinks.key| should be changed to |formatRemoveLinks.key2| ? (then the former will be changed throughout)

Another thing is, in both
http://mxr.mozilla.org/comm-central/source/editor/ui/composer/content/editorOverlay.xul#101
and
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/editorOverlay.xul#69

the modifier keys are already "accel, shift" already, so I suppose they don't need to be changed?

Created an attachment (id=348137)
patch v1.3

Trying again. Entity bumped. formatRemoveLinks.key isn't referenced anywhere else with modifier keys of "accel" only -- existing ones are already "accel,shift".

I didn't find any relevant ifdefs to be removed -- am I missing something somewhere?

(In reply to comment #14)
> So |formatRemoveLinks.key| should be changed to |formatRemoveLinks.key2| ?
> (then the former will be changed throughout)

The tools localizers use can do some automatic stuff for certain entity name endings, so it should still end in '.key'. Something like formatRemoveDiscontinueLinks.key

> the modifier keys are already "accel, shift" already, so I suppose they don't
> need to be changed?

No, but remove the platform ifdef for the spelling one.

Also, this patch would give seamonkey composer two functions for ctrl+shift+L, no?

Created an attachment (id=348312)
patch v1.4

Incorporates all of Magnus' comments so far, but without the SM "conflicting-shortcut" fix.

What should the conflicting SM shortcuts be changed to then? or should the SM fix be in a separate bug?

Well, seems at least Ctrl+K should be unused, so perhaps that?
It should all happen in the same patch. Note that you'll have to get neil's reviews as well.

Created an attachment (id=348585)
now with SM fixes

Now shortcut for openRemoteCmd.key is ctrl-k as suggested by Magnus. Bumped entity to openRemoteCmd2.key, TB changes remain identical to v1.4.

Requesting review from Neil for this SM change first before I re-request for the TB changes.

(From update of attachment 348585)
Ctrl+K doesn't make sense for two reasons:
1. The same reason that we didn't use it for spellcheck in the first place: some Linux keyboard layouts map it to delete to end of line.
2. The shortcut for open web location in SeaMonkey's browser is also Ctrl+Shift+L.

I'm still not used to the new order of the flags in the dropdown :-(

(In reply to comment #21)
> (From update of attachment 348585 [details])
> Ctrl+K doesn't make sense for two reasons:
> 1. The same reason that we didn't use it for spellcheck in the first place:
> some Linux keyboard layouts map it to delete to end of line.
> 2. The shortcut for open web location in SeaMonkey's browser is also
> Ctrl+Shift+L.

If Ctrl+K isn't suitable, what should be the correct mappings?

Now there's Ctrl+Shift+L being currently used in 3 places, open web location, open remote cmd, discontinue link.

cc KaiRo as I don't know who does UI for SM.

Neil is our official "UI tsar" for SeaMonkey, and he already commented here :)

(In reply to comment #23)
> If Ctrl+K isn't suitable, what should be the correct mappings?
How about Ctrl+Shift+C for check spelling?

> Now there's Ctrl+Shift+L being currently used in 3 places, open web location,
> open remote cmd, discontinue link.
I don't think "currently" applies to discontinue link, that's still only a suggestion (from as far back as comment #1); open remote cmd is basically editor's version of open web location.

Ctrl+Shift+C = mark all read in thread pane, which is kind of destructive - bad enough it's close to ctrl+c(copy).

If there is no other alternative I'd sooner see Ctrl+Shift+L used in 3 places for 3 different reasons.

(In reply to comment #25)
> (In reply to comment #23)
> > If Ctrl+K isn't suitable, what should be the correct mappings?
> How about Ctrl+Shift+C for check spelling?

Sigh~ sure this is ok from what Wayne said in comment #26?

>
> > Now there's Ctrl+Shift+L being currently used in 3 places, open web location,
> > open remote cmd, discontinue link.
> I don't think "currently" applies to discontinue link, that's still only a
> suggestion (from as far back as comment #1); open remote cmd is basically
> editor's version of open web location.

Yes, my bad, I should have said s/currently/currently suggested to be

Ugh. So, tb could of course use Ctrl+Shift+L anyways... but I suppose seamonkey needs a fix too. (No hotkey for it, or then perhaps the suggested Ctrl+Shift+C?)

(In reply to comment #28)
> Ugh. So, tb could of course use Ctrl+Shift+L anyways... but I suppose seamonkey
> needs a fix too. (No hotkey for it, or then perhaps the suggested
> Ctrl+Shift+C?)
Sadly the key is in editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd

Neil, Magnus, I'd like to push forward on this, should there be a consensus on the hotkeys for TB/SM?

Basically patch 1.4 shifts the hotkey for DiscontinueLink from Ctrl-Shift-K to Ctrl-Shift-L, but the concept cannot be applied as-is to SM since Ctrl-Shift-L is used for other functions in SM.

If I recall my IRC logs correctly in a conversation with Neil, here's the current SM situation:

NeilAway> nth10sd: so, the free non-shifted keys seems to be d, e, h, j and r, while the used shifted keys seem to be a, g, k, l, n, s, v, y

How about Ctrl+Shift+I for discontinue link?

Sorry, but that's DOM Inspector's hot key.

Huh? Ctrl+Shift+I doesn't seem to open DOMi in either composer or mail compose window. (And it's not listed to do it in the menus either.)

I suggested "I" to get at char that's in the "Link" word, to match the similar menu hot keys around it.

http://hg.mozilla.org/dom-inspector/file/9efde0dd93e7/resources/content/tasksOverlay.xul#l16

Doesn't work for me.

Anyway, change the Check Spelling... to Ctrl+Shift+P then?

Yeah, that's fine by me.

(In reply to comment #36)
> Doesn't work for me.
>
> Anyway, change the Check Spelling... to Ctrl+Shift+P then?

So should I abandon the patch that changes mainly Format/Discontinue Link and change Options/Check Spelling to Ctrl+Shift+P instead?

Yes

Created an attachment (id=356532)
patch that changes Options/Check Spelling to Ctrl-Shift-P instead

Created an attachment (id=356535)
oops, prev patch didn't bump an entity

(From update of attachment 356535)
> <key id="checkspellingkb"
>- key="&checkSpellingCmd.key;"
>+ key="&checkSpellingCmd.key3;"
> observes="cmd_spelling"
> modifiers="&accel.emacs_conflict;"
Change this to "accel,shift" please.

> <!ENTITY checkSpellingCmd.label "Check Spelling">
> <!ENTITY checkSpellingCmd.accesskey "h">
>-<!ENTITY checkSpellingCmd.key "K">
>+<!ENTITY checkSpellingCmd.key3 "P">
Nit: "p" to match "Spelling"

> <key id="key_checkspelling"
>- key="&checkSpellingCmd.key;"
>+ key="&checkSpellingCmd.key3;"
> command="cmd_spelling"
> modifiers="&accel.emacs_conflict;"
Ditto please.

> <!ENTITY checkSpellingCmd.label "Check Spelling…">
>-<!ENTITY checkSpellingCmd.key "K">
>+<!ENTITY checkSpellingCmd.key3 "P">
Ditto please.

Created an attachment (id=356546)
patch 2.2

Patch 2.2 that incorporates Neil's comments, bringing over sr+, requesting review from mkmelin.

(From update of attachment 356546)
Looks good, Gary!

Checked in: http://hg.mozilla.org/comm-central/rev/c263cff9191d

(In reply to comment #45)
> Checked in: http://hg.mozilla.org/comm-central/rev/c263cff9191d

That should have been: http://hg.mozilla.org/comm-central/rev/3e31d68c0edb

From a localizers point of view, please don't rename "xxxx.key" to "xxxx.key3". Please rename the key again to "xxxx3.key".

(In reply to comment #47)
> From a localizers point of view, please don't rename "xxxx.key" to "xxxx.key3".
> Please rename the key again to "xxxx3.key".

Mhh, maybe this wouldn't be a good idea, too. "xxxx3.key" wouldn't match the labels name. So rename the label and the key?

(In reply to comment #42)
>(From update of attachment 356535)
>> <key id="checkspellingkb"
>>- key="&checkSpellingCmd.key;"
>>+ key="&checkSpellingCmd.key3;"
>> observes="cmd_spelling"
>> modifiers="&accel.emacs_conflict;"
>Change this to "accel,shift" please.
Bah, I thought the quotes would make it clear that I wanted the entire attribute changed...

(In reply to comment #48)
>(In reply to comment #47)
>>From a localizers point of view, please don't rename "xxxx.key" to "xxxx.key3".
>>Please rename the key again to "xxxx3.key".
>Mhh, maybe this wouldn't be a good idea, too. "xxxx3.key" wouldn't match the
>labels name. So rename the label and the key?
Yes, that makes sense, we'll call it checkSpellingCmd2.key etc.

Created an attachment (id=357145)
add on patch

Sorry about the "accel,shift" error, I mis-interpreted it.

Created an attachment (id=357147)
add on patch v2

Incorporates KaiRo's suggestions off IRC, fixes my |modifiers="accel,shift"| error.

Any chance we can get this checked in before the next suite nightlies?

(From update of attachment 357147)
r=mkmelin. I'll check this in in a moment.

http://hg.mozilla.org/comm-central/rev/f197c0bd1296

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

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

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