575018 - Wrong commandkey label Ctrl+G in Edit -> Find -> Find in this message vs. Find Again

Status: VERIFIED FIXED

(Thunderbird :: Mail Window Front End, defect)

Comment 1

[RickieES aka Ricardo Palomares]

Sorry for the blank initial report (shouldn't BugZilla require a non-empty comment #0?)

In Thunderbird 3.1 main mail window, the following two menu options share the same commandkey / shortcut (Ctrl + G):

- Edit -> Find -> Find in this message...
- Edit -> Find -> Find again

I don't know whether this is by design or not, but I'd say this sharing prevents the user from doing this sequence of actions: search for a string, (optionally) search again and then search for a different string in the same message.

Since it is not common that several actions share a unique commandkey, localizers are likely to get reports of duplicated commandkeys in their respective translations, as users will assume it has been a mistake during localization process.

Comment 2

[rsx11m]

Actually, the Ctrl+F shortcut works as intended, only the label appears to be mixed up in the menu. I'll have a patch shortly.

Comment 3

[rsx11m]

Attachment: Proposed patch

This resolves the dual use of key_findAgain which was apparently introduced with the Quick-Filter redesign in bug 545955. There is no indication that this was done intentionally, thus I assume it's a typo. The patch would also hold if the keyboard shortcut is modified by bug 564328, so no dependency here.

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 454323 [details] [diff] [review]
Proposed patch

I'm pretty sure it was intentional, but that doesn't mean it's right, of course :).

I presume my goal in changing the label was so that people who wanted to directly go to find-in-message could learn a hotkey that would take them there.  The supposition was that it is probably not obvious to people that find-again-in-message will always go directly to the find-in-message box, so this was an attempt to help them out.

I'm not entirely sure why I didn't change the "command" at the same time as the key.  Probably either an oversight or concern that there might be some logic that might disable the findagain command in all its incarnations.

Clearing review request pending ui-review.  The feedback flag is probably what you want to use in cases like this where my/developer input is desired/useful but review is not appropriate because no ui-review or general ux approval of the direction has happened.

Comment 5

[rsx11m]

Thanks for the feedback, I actually meant "review" rather than "feedback" as it affects searching. It's a bit fuzzy these days who can review which part...

> (comment #4) Probably either an oversight or concern that there might be some logic
> that might disable the findagain command in all its incarnations.

I've just tried using Ctrl+G only for Find in Message - it opens nicely the bar the first time to enter the search term and will then continue finding that string. However, after closing the search bar with ESC, further Ctrl+G will continue searching the same string in that message and any other message I go to. Thus, apparently two different shortcuts will remain required for this.

As shortcuts and menu labels should match, I'll stick with this patch for now.

Comment 6

[rsx11m]

(In reply to comment #4)
> but review is not appropriate because no ui-review or general ux approval of
> the direction has happened.

Ok, guess I've misunderstood that part the first time. You didn't object against picking you as a reviewer as such but doing so before ui-review was granted, thus I'll reinstate r? if and when the patch gets ui-r+.

Comment 8

[rsx11m]

Ping for ui-review. Bryan, this should take less than 10 minutes of your time.

Comment 9

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 454323 [details] [diff] [review]
Proposed patch

After trying out what you were talking about in comment 5 this seems like the right thing.

Comment 10

[rsx11m]

Comment on attachment 454323 [details] [diff] [review]
Proposed patch

Thanks for the ui-review. Based on comment #6, I'm asking Andrew for the technical review again (this time for real).

Comment 11

[Andrew Sutherland [:asuth] (he/him)]

Do the mozmill tests pass with the change?

Comment 12

[rsx11m]

Yes, this shouldn't have any impact on the tests. It only changes labels.

There are test_control_f_triggers_display() on testing that the qfb is shown on Ctrl+F and test_control_f_toggles_between_textboxes() for the interaction with the search-in-message function. No test verifies the labels in the menus (and since those are static in the XUL, this would certainly overdo it...).

Comment 13

[rsx11m]

Andrew, do you see any problems with this patch? Trunk is open for a change,
so this would be a good opportunity to get it in.  :-)

Comment 14

[Andrew Sutherland [:asuth] (he/him)]

Fell off my radar somewhat.  I wanted to know that the mozmill tests passed as the change seems more than just a label and I didn't want to have to delve into the XUL code to verify.  I was hoping mozmill tests on try would happen to reduce the hassle for all involved.

Comment 15

[Andrew Sutherland [:asuth] (he/him)]

As an update, bug 497353 is tracking the Thunderbird try-server running unit tests and mozmill tests and is, per recent update, allegedly going to happen in the very near future.  Once that happens I'll push the patch to the try server.  Of course, if someone is able to run the mozmill tests on multiple platforms before then, that also works.

Comment 16

[Michael A. Pasek]

This should be dependent on the outcome of 564328.  Two problems here: Label in "Find->Find in this message" menu item says "Cmd-G" (or Ctl-G), but menu selection operates like "Cmd-F" (or Ctl-F) and does "Find->Search Messages" menu function, and "Cmd-F" is -- as noted in 564328 -- "overloaded".  Key shortcut "Cmd-G" in 3.1.1 operates as "Cmd-F" did in 3.1.

Comment 17

[rsx11m]

Michael, as stated in comment #2, that menu entry still has Ctrl+F as the keyboard shortcut for the underlying cmd_find function. Its label is wrong,
that's all what the patch here is addressing. If bug 564328 untangles the cmd_find operation and decides to define a different keyboard shortcut for
it, the patch here would actually recognize it as it only depends on the key_find association (currently the menu is using key_findAgain = Ctrl+G).

Thus, what you mention is actually bug 564328, which may need to separate
the two operations from the common cmd_find function. Also, as figured out
in comment #5, Ctrl+G is not a sufficient replacement for Ctrl+F alone.

Comment 18

[Michael A. Pasek]

(In reply to comment #17)
> Michael, as stated in comment #2, that menu entry still has Ctrl+F as the
> keyboard shortcut [...]

It wasn't clear to me whether Comment #2 (and clarification in 17) is describing the contents of the menu item as seen by the user -- which, on my 3.1.1 is "Find in this message [Cmd]G" (i.e., correct, sort of) -- and what I would refer to as the "menu entry", or the code (or table, etc.) that controls what happens when that menu item is selected.

Hate to see you "fix" this, only to have it revert if/when "ctrl-F" returns to its previous mode of operation as a result of Bug 564328.

Comment 19

[rsx11m]

If cmd_find is reverted to its previous Find in This Message function and some other cmd_qfbFocus or similar is defined in result of the other bug, the patch here is doing just the right thing. Also, this here is intended for 3.1.x to ensure consistency of the keyboard shortcut, whereas bug 564328 may need to wait for 3.2 to allow any necessary string changes. So, this has its purpose.

Comment 20

[Blake Winton (:bwinton) (:☕️)]

FWIW, On Mac OS X 10.6, using comm-central with this patch (and a couple from Standard8 to get the tests to build and run), I get:
INFO | (runtestlist.py) | Directories Run: 18, Passed: 456, Failed: 0

Later,
Blake.

Comment 21

[rsx11m]

(In reply to comment #15)
> As an update, bug 497353 is tracking the Thunderbird try-server running unit
> tests and mozmill tests and is, per recent update, allegedly going to happen in
> the very near future.  Once that happens I'll push the patch to the try server.

Judging from the new tinderboxes showing up on ThunderbirdTry and the updates
on the Wiki, this seems to be close to done. Unless you are happy with Blake's confirmation that the patch doesn't break anything, please push it to the try server as soon as standard8 and gozer give the go-ahead.

Comment 22

[rsx11m]

Thanks to gozer for running the try-server builds and tests last night.

http://hg.mozilla.org/try-comm-central/rev/42e7fab1650a

Linux 32-bit 
http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1281934107.1281934746.24228.gz&fulltext=1
INFO | (runtestlist.py) | quick-filter-bar: 54 passed, 0 failed

Linux 64-bit
http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1281935998.1281936549.31747.gz&fulltext=1
INFO | (runtestlist.py) | quick-filter-bar: 54 passed, 0 failed

Mac OSX
http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1281944946.1281945897.5142.gz&fulltext=1
INFO | (runtestlist.py) | quick-filter-bar: 54 passed, 0 failed

Windows
http://tinderbox.mozilla.org/showlog.cgi?log=ThunderbirdTry/1281938703.1281939097.10508.gz&fulltext=1
INFO | (runtestlist.py) | quick-filter-bar: 54 passed, 0 failed

Now that it has been established that the patch indeed won't break anything, I'd appreciate a quick approval within the next few days, also to provide the opportunity for standard8 to take this label change for 3.1.3 if desired.

Comment 23

[rsx11m]

Thanks Andrew, push on trunk please.

Comment 24

[Mark Banner (:standard8)]

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

Comment 25

[rsx11m]

Comment on attachment 454323 [details] [diff] [review]
Proposed patch

Requesting branch approval for 3.1.3. This is a minimum-risk patch which restores the previous keyboard-shortcut label to avoid user confusion.
Note that there is no l10n impact, only the association is corrected in the respective menu, no changes to the shortcut key itself.

Comment 26

[Mark Banner (:standard8)]

Checked into 1.9.2: http://hg.mozilla.org/releases/comm-1.9.2/rev/80c311ad99ad

Comment 27

[Ludovic Hirlimann [:Usul]]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2.9) Gecko/20100825 Thunderbird/3.1.3