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)
Thunderbird
Message Compose Window
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+
neil
:
superreview+
|
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
Reporter | ||
Updated•16 years ago
|
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
Comment 1•16 years ago
|
||
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
Updated•16 years ago
|
Component: General → Message Compose Window
QA Contact: general → message-compose
Assignee | ||
Comment 2•16 years ago
|
||
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
Assignee | ||
Comment 3•16 years ago
|
||
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 | ||
Updated•16 years ago
|
Assignee: nobody → nth10sd
Comment 4•16 years ago
|
||
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.
Comment 5•16 years ago
|
||
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.
Comment 6•16 years ago
|
||
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).
Comment 7•16 years ago
|
||
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 8•16 years ago
|
||
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-
Assignee | ||
Comment 9•16 years ago
|
||
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)
Assignee | ||
Comment 10•16 years ago
|
||
Comment on attachment 347801 [details] [diff] [review] patch v1.1 wrong wrong..
Attachment #347801 -
Attachment is obsolete: true
Assignee | ||
Comment 11•16 years ago
|
||
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)
Updated•16 years ago
|
Attachment #347803 -
Flags: review?(mkmelin+mozilla) → review-
Comment 12•16 years ago
|
||
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.)
Comment 13•16 years ago
|
||
> 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.
Assignee | ||
Comment 14•16 years ago
|
||
(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?
Assignee | ||
Comment 15•16 years ago
|
||
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)
Comment 16•16 years ago
|
||
(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?
Assignee | ||
Comment 17•16 years ago
|
||
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)
Comment 18•16 years ago
|
||
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.
Assignee | ||
Updated•16 years ago
|
Attachment #348312 -
Attachment is obsolete: true
Attachment #348312 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Updated•16 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 19•16 years ago
|
||
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 20•16 years ago
|
||
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 21•16 years ago
|
||
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-
Comment 22•16 years ago
|
||
I'm still not used to the new order of the flags in the dropdown :-(
Assignee | ||
Comment 23•16 years ago
|
||
(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.
Comment 24•16 years ago
|
||
Neil is our official "UI tsar" for SeaMonkey, and he already commented here :)
Comment 25•16 years ago
|
||
(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.
Comment 26•16 years ago
|
||
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.
Assignee | ||
Comment 27•16 years ago
|
||
(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
Comment 28•16 years ago
|
||
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?)
Comment 29•16 years ago
|
||
(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
Assignee | ||
Comment 30•16 years ago
|
||
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.
Assignee | ||
Comment 31•16 years ago
|
||
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
Comment 32•15 years ago
|
||
How about Ctrl+Shift+I for discontinue link?
Comment 33•15 years ago
|
||
Sorry, but that's DOM Inspector's hot key.
Comment 34•15 years ago
|
||
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.
Comment 35•15 years ago
|
||
http://hg.mozilla.org/dom-inspector/file/9efde0dd93e7/resources/content/tasksOverlay.xul#l16
Comment 36•15 years ago
|
||
Doesn't work for me. Anyway, change the Check Spelling... to Ctrl+Shift+P then?
Comment 37•15 years ago
|
||
Yeah, that's fine by me.
Assignee | ||
Comment 38•15 years ago
|
||
(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?
Comment 39•15 years ago
|
||
Yes
Assignee | ||
Comment 40•15 years ago
|
||
Attachment #348585 -
Attachment is obsolete: true
Attachment #356532 -
Flags: superreview?(neil)
Attachment #356532 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 41•15 years ago
|
||
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)
Updated•15 years ago
|
Attachment #356535 -
Flags: superreview?(neil) → superreview+
Comment 42•15 years ago
|
||
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.
Assignee | ||
Comment 43•15 years ago
|
||
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 44•15 years ago
|
||
Comment on attachment 356546 [details] [diff] [review] patch 2.2 Looks good, Gary!
Attachment #356546 -
Flags: review?(mkmelin+mozilla) → review+
Assignee | ||
Updated•15 years ago
|
Keywords: checkin-needed
Comment 45•15 years ago
|
||
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
Comment 46•15 years ago
|
||
(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
Comment 47•15 years ago
|
||
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 → ---
Comment 48•15 years ago
|
||
(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?
Comment 49•15 years ago
|
||
(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.
Assignee | ||
Comment 50•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Status: REOPENED → ASSIGNED
Assignee | ||
Updated•15 years ago
|
Attachment #357145 -
Attachment is obsolete: true
Attachment #357145 -
Flags: superreview?(neil)
Attachment #357145 -
Flags: review?(mkmelin+mozilla)
Assignee | ||
Comment 51•15 years ago
|
||
Incorporates KaiRo's suggestions off IRC, fixes my |modifiers="accel,shift"| error.
Attachment #357147 -
Flags: superreview?(neil)
Attachment #357147 -
Flags: review?(mkmelin+mozilla)
Updated•15 years ago
|
Attachment #357147 -
Flags: superreview?(neil) → superreview+
Comment 52•15 years ago
|
||
Any chance we can get this checked in before the next suite nightlies?
Comment 53•15 years ago
|
||
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+
Comment 54•15 years ago
|
||
http://hg.mozilla.org/comm-central/rev/f197c0bd1296
Status: ASSIGNED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
See Also: → https://launchpad.net/bugs/68456
You need to log in
before you can comment on or make changes to this bug.
Description
•