Closed Bug 461317 Opened 16 years ago Closed 15 years ago

Ctrl-Shift-K is a hotkey for two functions -- Options/Check Spelling and Format/Discontinue Link

Categories

(Thunderbird :: Message Compose Window, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b2

People

(Reporter: mcepl, Assigned: gkw)

References

Details

Attachments

(1 file, 10 obsolete files)

11.59 KB, patch
mkmelin
: review+
Details | Diff | Splinter Review
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
Summary: The "Check Spelling" window is falsely launched by Ctrl-Shitf-K hotkey → Ctrl-Shitf-K is a hotkey for two functions -- Options/Check Spelling and Format/Discontinue Link
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
Component: General → Message Compose Window
QA Contact: general → message-compose
It keeps checking spelling on Mac. Confirming bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → All
Hardware: PC → All
Summary: Ctrl-Shitf-K is a hotkey for two functions -- Options/Check Spelling and Format/Discontinue Link → Ctrl-Shift-K is a hotkey for two functions -- Options/Check Spelling and Format/Discontinue Link
Version: unspecified → Trunk
Attached patch patch v1 (obsolete) — Splinter Review
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.
Attachment #347226 - Flags: ui-review?(clarkbw)
Attachment #347226 - Flags: review?(mkmelin+mozilla)
Assignee: nobody → nth10sd
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 <scott@scott-macgregor.org> 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
Comment on attachment 347226 [details] [diff] [review]
patch v1

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.
Attachment #347226 - Flags: review?(mkmelin+mozilla) → review-
Attached patch patch v1.1 (obsolete) — Splinter Review
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.
Attachment #347226 - Attachment is obsolete: true
Attachment #347226 - Flags: ui-review?(clarkbw)
Comment on attachment 347801 [details] [diff] [review]
patch v1.1

wrong wrong..
Attachment #347801 - Attachment is obsolete: true
Attached patch patch v1.2 (obsolete) — Splinter Review
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.
Attachment #347803 - Flags: review?(mkmelin+mozilla)
Attachment #347803 - Flags: review?(mkmelin+mozilla) → review-
Comment on attachment 347803 [details] [diff] [review]
patch v1.2

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] [diff] [review])
> 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?
Attached patch patch v1.3 (obsolete) — Splinter Review
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?
Attachment #347803 - Attachment is obsolete: true
Attachment #348137 - Flags: review?(mkmelin+mozilla)
(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?
Attached patch patch v1.4 (obsolete) — Splinter Review
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?
Attachment #348137 - Attachment is obsolete: true
Attachment #348312 - Flags: review?(mkmelin+mozilla)
Attachment #348137 - Flags: review?(mkmelin+mozilla)
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.
Attachment #348312 - Attachment is obsolete: true
Attachment #348312 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Attached patch now with SM fixes (obsolete) — Splinter Review
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.
Attachment #348585 - Flags: review?(neil)
Comment on attachment 348585 [details] [diff] [review]
now with SM fixes

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.
Attachment #348585 - Flags: review?(neil) → review+
Comment on attachment 348585 [details] [diff] [review]
now with SM fixes

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.
Attachment #348585 - Flags: review+ → review-
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] [diff] [review])
> 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.
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?
Attachment #348585 - Attachment is obsolete: true
Attachment #356532 - Flags: superreview?(neil)
Attachment #356532 - Flags: review?(mkmelin+mozilla)
Attachment #356532 - Attachment is obsolete: true
Attachment #356535 - Flags: superreview?(neil)
Attachment #356535 - Flags: review?(mkmelin+mozilla)
Attachment #356532 - Flags: superreview?(neil)
Attachment #356532 - Flags: review?(mkmelin+mozilla)
Attachment #356535 - Flags: superreview?(neil) → superreview+
Comment on attachment 356535 [details] [diff] [review]
oops, prev patch didn't bump an entity

>     <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.
Attached patch patch 2.2 (obsolete) — Splinter Review
Patch 2.2 that incorporates Neil's comments, bringing over sr+, requesting review from mkmelin.
Attachment #356535 - Attachment is obsolete: true
Attachment #356546 - Flags: superreview+
Attachment #356546 - Flags: review?(mkmelin+mozilla)
Attachment #356535 - Flags: review?(mkmelin+mozilla)
Comment on attachment 356546 [details] [diff] [review]
patch 2.2

Looks good, Gary!
Attachment #356546 - Flags: review?(mkmelin+mozilla) → review+
Checked in: http://hg.mozilla.org/comm-central/rev/c263cff9191d
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 3.0b2
From a localizers point of view, please don't rename "xxxx.key" to "xxxx.key3". Please rename the key again to "xxxx3.key".
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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 [details] [diff] [review])
>>     <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.
Attached patch add on patch (obsolete) — Splinter Review
Sorry about the "accel,shift" error, I mis-interpreted it.
Attachment #356546 - Attachment is obsolete: true
Attachment #357145 - Flags: superreview?(neil)
Attachment #357145 - Flags: review?(mkmelin+mozilla)
Status: REOPENED → ASSIGNED
Attachment #357145 - Attachment is obsolete: true
Attachment #357145 - Flags: superreview?(neil)
Attachment #357145 - Flags: review?(mkmelin+mozilla)
Attached patch add on patch v2Splinter Review
Incorporates KaiRo's suggestions off IRC, fixes my |modifiers="accel,shift"| error.
Attachment #357147 - Flags: superreview?(neil)
Attachment #357147 - Flags: review?(mkmelin+mozilla)
Attachment #357147 - Flags: superreview?(neil) → superreview+
Any chance we can get this checked in before the next suite nightlies?
Comment on attachment 357147 [details] [diff] [review]
add on patch v2

r=mkmelin. I'll check this in in a moment.
Attachment #357147 - Flags: review?(mkmelin+mozilla) → review+
http://hg.mozilla.org/comm-central/rev/f197c0bd1296
Status: ASSIGNED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.