530629 - Ctrl+F when composing should just show find bar as usual (with "Find and Replace..." button)

Status: VERIFIED FIXED

(Thunderbird :: Message Compose Window, enhancement)

Description

[Thomas D. (:thomas8)]

STR

1 in msg compose, you just want to find a certain word
2 press Ctrl+F (which is the shortcut for finding things in almost any other app, and will normally bring up the find bar, as they have ctrl+H for replacing)

actual

- we show bulky "Find and replace" dialogue
- it's very complex if you just want to find something (many buttons, many checkboxes and inputs)
- it covers the text / the composition window
- it is modal (i.e. you can't select and copy / drag and drop anything from the underlying compose text)
- it can't do useful stuff like "highlight all"

expected

- just show find bar as usual: unobtrusive / non-covering, dead-simple, efficient, versatile (e.g. highlight all)
- optionally implement a "Find & Replace" button on the find bar, which will hide find bar and show the "Find and replace" dialogue instead (obviously, only show this button on the find bar in compose mode)
- implement bug 530626: Ctrl+H keyboard shortcut for "Find and Replace" (as in other applications)

Comment 1

[Thomas D. (:thomas8)]

I'd like some feedback on this proposal.

Also: How hard would it be to hook up existing "Find-in-text" bar (as used in message reader Ctrl+F, or Firefox Ctrl+F) to composition? Is it just a few lines to call the bar, and then it will work as expected in the context of composition?

Comment 2

[:aceman]

There are already at least the 2 findbars mentioned by you. 
http://mxr.mozilla.org/comm-central/search?string=findbar&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Maybe it is easy to add a third one, maybe not. The first thing I wonder is whether the find bar can operate on <editor> element instead of <browser> as the existing ones do.
Neil could know that detail.

Comment 3

[[e-mail]]

It looks as if it works on designmode documents, so I can't see any reason why it wouldn't work on editor documents either. (Obviously you can't automatically find as you type on a designmode document.)

Comment 4

[:aceman]

Suyash, can you look at this?

Comment 5

[:aceman]

Attachment: patch to enable fastFind on <editor>

Suyash, try to apply this onto mozilla-central. Then the findbar should also work on <editor> and you can build the patch in compose window in the same way as there is one for the message preview.

Comment 6

[:aceman]

Comment on attachment 769513 [details] [diff] [review]
patch to enable fastFind on <editor>

We will request review on this after your patch is attached too and works with this fine.

Comment 7

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch for findbar

Sir,

I cannot find any special use of "Find Again" and "Find Previous" menu items under the Edit menu, so what are they required for?

Comment 8

[:aceman]

Comment on attachment 769596 [details] [diff] [review]
Patch for findbar

Review of attachment 769596 [details] [diff] [review]:
-----------------------------------------------------------------

Please first wait for reply from SM guys if they want this too, or we need to implement this as TB specific.

::: editor/ui/composer/content/ComposerCommands.js
@@ +83,5 @@
>      return;
>  
>    //dump("Registering plain text editor commands\n");
>  
> +  commandTable.registerCommand("cmd_find",nsFindCommand);

What is this change for? Also, can you redefine nsFindCommand to pop up the find bar? But that would probably affect Seamonkey, which does not have the find bar so we will need advice from SM guys.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1340,5 @@
>  
> +function invokeFindbar()
> +{
> +  document.getElementById("FindToolbar").onFindCommand();
> +}

This code should go into the implementation of cmd_find in ComposerCommands.js . However if SM guys say this should not be in the common /editor code, then we will need to keep this TB specific code here.

::: mail/components/compose/content/messengercompose.xul
@@ +220,5 @@
>    <key id="key_delete" keycode="VK_DELETE" command="cmd_delete"/>
>    <key id="key_renameAttachment" keycode="VK_F2" oncommand="goDoCommand('cmd_renameAttachment');"/>
>  #endif
>    <key id="key_selectAll" key="&selectAllCmd.key;" modifiers="accel"/>
> +  <key id="key_find" key="&findCmd.key;" command="cmd_findbar" modifiers="accel"/>

Keep cmd_find and reimplement it.

@@ +494,5 @@
>                      key="key_renameAttachment" command="cmd_renameAttachment"/>
>            <menuseparator/>
>            <menuitem id="menu_selectAll" label="&selectAllCmd.label;" key="key_selectAll" accesskey="&selectAllCmd.accesskey;" command="cmd_selectAll"/>
>            <menuseparator/>
> +          <menuitem label="&findCmd.label;" key="key_find" accesskey="&findCmd.accesskey;" command="cmd_findbar"/>

cmd_find.

@@ +495,5 @@
>            <menuseparator/>
>            <menuitem id="menu_selectAll" label="&selectAllCmd.label;" key="key_selectAll" accesskey="&selectAllCmd.accesskey;" command="cmd_selectAll"/>
>            <menuseparator/>
> +          <menuitem label="&findCmd.label;" key="key_find" accesskey="&findCmd.accesskey;" command="cmd_findbar"/>
> +          <menuitem label="&findReplaceCmd.label;" key="key_findReplace" accesskey="&findReplaceCmd.accesskey;" oncommand="goDoCommand('cmd_find')"/>

Create a new command cmd_replace in ComposerCommands.js and make it do what cmd_find was doing till now.

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +83,5 @@
>  <!ENTITY renameAttachmentCmd.accesskey "e">
>  <!ENTITY selectAllCmd.label "Select All">
>  <!ENTITY selectAllCmd.key "A">
>  <!ENTITY selectAllCmd.accesskey "a">
> +<!ENTITY findCmd.label "Find">

You will need to change this stringID now... Probably find2Cmd.*

@@ +90,2 @@
>  <!ENTITY findCmd.key "F">
>  <!ENTITY findCmd.accesskey "F">

Move up and rename to the find2Cmd.*

Comment 9

[[e-mail]]

(In reply to aceman from comment #8)
> Please first wait for reply from SM guys if they want this too, or we need
> to implement this as TB specific.
I don't see anything stopping you from implementing this specifically for Thunderbird.
If you want cmd_find to display the find bar then you would first (separately) want to rename all of the editor find code to (find and) replace.

Comment 10

[[e-mail]]

(In reply to Thomas D.)
> - it is modal (i.e. you can't select and copy / drag and drop anything from
> the underlying compose text)
This would seem to be a bug even when you do want the find and replace dialog.

Comment 11

[[e-mail]]

Comment on attachment 769596 [details] [diff] [review]
Patch for findbar

(This should have been needinfo?)

Comment 12

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v2

Comment 13

[:aceman]

Comment on attachment 778505 [details] [diff] [review]
Patch v2

Review of attachment 778505 [details] [diff] [review]:
-----------------------------------------------------------------

So this patch looks very nice and works beautifully on TB (except the Find Next/Prev menu items).
Any chance you could do the Seamonkey changes too so that we can keep the implementation in /editor shared and not split it into TB/SM versions?

::: editor/ui/composer/content/ComposerCommands.js
@@ +88,3 @@
>    commandTable.registerCommand("cmd_find",       nsFindCommand);
>    commandTable.registerCommand("cmd_findNext",   nsFindAgainCommand);
>    commandTable.registerCommand("cmd_findPrev",   nsFindAgainCommand);

It seems you also need to change the implementation of the nsFindAgainCommand to 
 document.getElementById("FindToolbar").onFindAgainCommand(true/false) otherwise the existing menucommands and hotkeys work strangely.

::: editor/ui/composer/content/editorOverlay.xul
@@ +177,1 @@
>      <command id="cmd_find"          oncommand="goDoCommand('cmd_find')"/>

In the menu the "Find" is above "Find & Replace" so let's keep that order here too. But I wonder where is this file used (TB has its own copy). Maybe this is for SM?

::: mail/components/compose/content/editorOverlay.xul
@@ +97,5 @@
>    >
>      <command id="cmd_pasteNoFormatting" oncommand="goDoCommand('cmd_pasteNoFormatting')"
>               label="&pasteNoFormatting.label;" accesskey="&pasteNoFormatting.accesskey;"/>
>      <command id="cmd_preferences"   oncommand="goDoCommand('cmd_preferences')"/>
> +    <command id="cmd_findReplace"   oncommand="goDoCommand('cmd_findReplace')"/>

In the menu the "Find" is above "Find & Replace" so let's keep that order here too.

::: mail/components/compose/content/messengercompose.xul
@@ +494,5 @@
>            <menuseparator/>
>            <menuitem id="menu_selectAll" label="&selectAllCmd.label;" key="key_selectAll" accesskey="&selectAllCmd.accesskey;" command="cmd_selectAll"/>
>            <menuseparator/>
>            <menuitem label="&findCmd.label;"      key="key_find"      accesskey="&findCmd.accesskey;"      command="cmd_find"/>
> +          <menuitem label="&findReplaceCmd.label;" key="key_findReplace" accesskey="&findReplaceCmd.accesskey;" command="cmd_findReplace"/>

Good, but the key_findReplace is not defined on Mac (it is ifndef above). So probably make 2 variants of this line dependent on OS.

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +88,2 @@
>  <!ENTITY findCmd.key "F">
>  <!ENTITY findCmd.accesskey "F">

Unfortunately you need to change the string ID here. Try findBarCmd.Label and findBarCmd.key and findBarCmd.accesskey .

Comment 14

[Suyash Agarwal (:sshagarwal)]

(In reply to :aceman from comment #13)
> Comment on attachment 778505 [details] [diff] [review]
> Patch v2
> Any chance you could do the Seamonkey changes too so that we can keep the
> implementation in /editor shared and not split it into TB/SM versions?
>
I think this will work for Seamonkey as well.
 
> ::: editor/ui/composer/content/ComposerCommands.js
> @@ +88,3 @@
> >    commandTable.registerCommand("cmd_find",       nsFindCommand);
> >    commandTable.registerCommand("cmd_findNext",   nsFindAgainCommand);
> >    commandTable.registerCommand("cmd_findPrev",   nsFindAgainCommand);
> 
> It seems you also need to change the implementation of the
> nsFindAgainCommand to 
>  document.getElementById("FindToolbar").onFindAgainCommand(true/false)
> otherwise the existing menucommands and hotkeys work strangely.
>
But I had tested this the last time as well, they work perfectly fine.
 
> Good, but the key_findReplace is not defined on Mac (it is ifndef above). So
> probably make 2 variants of this line dependent on OS.
>
I think I have messed up with this now.
 
> ::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
> @@ +88,2 @@
> >  <!ENTITY findCmd.key "F">
> >  <!ENTITY findCmd.accesskey "F">
> 
> Unfortunately you need to change the string ID here. Try findBarCmd.Label
> and findBarCmd.key and findBarCmd.accesskey .
Made the change, but why is this required? We are not changing the strings.

Comment 15

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Comment 16

[:aceman]

(In reply to Suyash Agarwal (:sshagarwal) from comment #14)
> > It seems you also need to change the implementation of the
> > nsFindAgainCommand to 
> >  document.getElementById("FindToolbar").onFindAgainCommand(true/false)
> > otherwise the existing menucommands and hotkeys work strangely.
> >
> But I had tested this the last time as well, they work perfectly fine.
For me it didn't work reliably. It sometimes worked sometimes not. Clicking the arrows on the findbar worked reliably. I had the feeling the menuitems were calling another functionality and it only worked by accident.

> > ::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
> > @@ +88,2 @@
> > >  <!ENTITY findCmd.key "F">
> > >  <!ENTITY findCmd.accesskey "F">
> > 
> > Unfortunately you need to change the string ID here. Try findBarCmd.Label
> > and findBarCmd.key and findBarCmd.accesskey .
> Made the change, but why is this required? We are not changing the strings.
Sorry, the review tool put few context in my comment. In the patch v2 you change the string:
-<!ENTITY findCmd.label "Find and Replace…">
+<!ENTITY findCmd.label "Find">

Comment 17

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3

Comment 18

[:aceman]

Comment on attachment 780550 [details] [diff] [review]
Patch v3

Review of attachment 780550 [details] [diff] [review]:
-----------------------------------------------------------------

This works great now. Ctrl-G cooperates with the findbar. E.g. it shows the "reached end of page" on the find bar when the last occurrence is hit. And also the highlight color is right. The Find & Replace seems to work too. It has its own highlight.

::: mail/components/compose/content/messengercompose.xul
@@ +496,5 @@
>            <menuseparator/>
> +          <menuitem label="&findBarCmd.label;"      key="key_find"      accesskey="&findBarCmd.accesskey;"      command="cmd_find"/>
> +#ifndef XP_MACOSX
> +          <menuitem label="&findReplaceCmd.label;" key="key_findReplace" accesskey="&findReplaceCmd.accesskey;" command="cmd_findReplace"/>
> +#endif

But we want the menuitem on Mac, just without a key. So either remove the ifndef and make someone on Mac try what it does if it references and undefined key (key_findReplace) or make it:
#ifndef XP_MACOSX
		<menuitem label="&findReplaceCmd.label;" key="key_findReplace" accesskey="&findReplaceCmd.accesskey;" command="cmd_findReplace"/>
#else
<menuitem label="&findReplaceCmd.label;" accesskey="&findReplaceCmd.accesskey;" command="cmd_findReplace"/>
#endif

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +91,2 @@
>  <!ENTITY findReplaceCmd.key "H">
> +<!ENTITY findReplaceCmd.accesskey "H">

This should not be "H" but some char found in "Find and Replace". Find something that is not used in the edit menu already.

Comment 19

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v3 (revised)

okay, made the changes.

Comment 20

[Suyash Agarwal (:sshagarwal)]

Should I also make this change for SeaMonkey as well? As I have already touched the editor/

Comment 21

[Suyash Agarwal (:sshagarwal)]

please omit "also" (not sure why I wrote it) :(

Comment 22

[Suyash Agarwal (:sshagarwal)]

Should I also make this change for SeaMonkey as well? As I have already touched the editor/

Comment 23

[[e-mail]]

(In reply to Suyash Agarwal from comment #22)
> Should I also make this change for SeaMonkey as well? As I have already
> touched the editor/

Well you'll need to make some sort of change for SeaMonkey as your code completely breaks find and replace in SeaMonkey.

Comment 24

[Suyash Agarwal (:sshagarwal)]

Okay thanks.

Comment 25

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 781209 [details] [diff] [review]
Patch v3 (revised)

Cancelling all the review requests as the patch is breaking suite/
Will fix it soon.

Thanks.

Comment 26

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4

I have tried to make the changes for Seamonkey as well. Not sure if these are correct or not. If not, please let me know what needs to be altered.

Thanks.

Comment 27

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4.1

A slight change in #ifndef.

Thanks to :stefanh for pointing that out.

Comment 28

[[e-mail]]

Comment on attachment 811912 [details] [diff] [review]
Patch v4.1

Not tried this out yet, but a few nits while I'm looking.

> };
>+//-----------------------------------------------------------------------------------
Nit: blank line after };

>+      document.getElementById("FindToolbar").onFindCommand();
>     }
>     catch(ex) {
>       dump("*** Exception: couldn't open Replace Dialog\n");
Nit: exception message is wrong

>-    <command id="cmd_find"          oncommand="goDoCommand('cmd_find')"/>
>+    <command id="cmd_find"          oncomamnd="goDoCommand('cmd_find')"/>
What changed here?

>+    <command id="cmd_findReplace"   oncommand="goDoCommand('cmd_findReplace')"/>
Nit: goes before cmd_find to be consistent with the other places

>-         key="&findCmd.key;"
>+         key="&findBarCmd.key;"
Don't bother renaming this entity, you aren't changing the text.

>+#ifndef XP_MACOSX
>+  <menuitem id="menu_findReplace"
>+            key="key_findReplace"
>+            accesskey="&findReplaceCmd.accesskey;"
>+            command="cmd_findReplace"/>
>+#else
>+  <menuitem id="menu_findReplace"
>+            accesskey="&findReplaceCmd.accesskey;"
>+            command="cmd_findReplace"/>
>+#endif
stefanh, why wouldn't the replace menutitem have a key?
(This won't work as written because we don't do preprocessing, instead we put keys in platform overlays.)

>   <!-- The mail message body frame -->
>   <vbox id="appcontent" flex="1">
>     <editor type="content-primary" id="content-frame" src="about:blank" name="browser.message.body" flex="1"
>             ondblclick="EditorDblClick(event);"
>             context="contentAreaContextMenu"/>
>+    <findbar id="FindToolbar" browserid="content-frame"/>
>   </vbox>
Nit: I think SeaMonkey puts the find bar above the content frame

Comment 29

[[e-mail]]

Oh, and editor.xul will need a findbar too. (I've no idea how this is going to work with source mode...)

Comment 30

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4.2

Fixed the suggested nits.

Comment 31

[[e-mail]]

Comment on attachment 819434 [details] [diff] [review]
Patch v4.2

>diff --git a/suite/locales/en-US/chrome/common/utilityOverlay.dtd b/suite/locales/en-US/chrome/common/utilityOverlay.dtd
>--- a/suite/locales/en-US/chrome/common/utilityOverlay.dtd
>+++ b/suite/locales/en-US/chrome/common/utilityOverlay.dtd
>@@ -68,25 +68,29 @@
> <!ENTITY showSuggestionsCmd.accesskey		"S"> 
> <!ENTITY preferencesCmd.label				"Preferencesâ¦">
> <!ENTITY preferencesCmd.key					"E">  
> <!ENTITY preferencesCmd.accesskey			"e"> 
> <!ENTITY findCmd.key "F">
> <!-- LOCALIZATION NOTE (findCmd.accesskey): This accesskey should be within
>      findCmd.label found in messengercompose.dtd, messenger.dtd and
>      editorOverlay.dtd and findOnCmd.label found in navigatorOverlay.dtd -->
>-<!ENTITY findCmd.accesskey "F">
>+<!ENTITY findBarCmd.accesskey "F">
>+<!ENTITY findBarCmd.label "Find">
This change seems to be wrong, but even when I removed it I couldn't get the find bar to open - there were a number of errors in the console though, perhaps some new editor.xml changes are needed?

Comment 32

[:aceman]

Attachment: patch to enable fastFind on <editor> v2

So this at least allows the findbar to show up in the compose window.
But there are still some rough edges, I get warnings in the Error console like this:
Warning: ReferenceError: reference to undefined property this.browser._lastSearchHighlight
Source file: chrome://global/content/bindings/findbar.xml
Line: 263

Warning: ReferenceError: reference to undefined property this._caseSensitiveStr
Source file: chrome://global/content/bindings/findbar.xml
Line: 469

Error: this.browser.finder is undefined
Source file: chrome://global/content/bindings/findbar.xml
Line: 479

Maybe we need to wait for solution of bug 918270.

Comment 33

[Suyash Agarwal (:sshagarwal)]

Neil sir, Can you please look at this patch now?
attachment 819905 [details] [diff] [review] is successful as a workaround for the findbar.

Thanks.

Comment 34

[[e-mail]]

(In reply to aceman from comment #32)
> Warning: ReferenceError: reference to undefined property
> this.browser._lastSearchHighlight
> Source file: chrome://global/content/bindings/findbar.xml
> Line: 263
This one I know, but is readily fixed by porting bug 932969. (As you can't readily tell from the patch, I added the two missing fields after the fastFind property).

Comment 35

[[e-mail]]

Comment on attachment 819434 [details] [diff] [review]
Patch v4.2

With both patches, things almost work!

>       <!-- KLUDGE:  Temporary fix for bug 34414:
>            The current editor tag doesn't have a view, 
>            which breaks deck frame-hiding mechanism 
>        -->
>       <stack>
>+        <findbar id="FindToolbar" browserid="content-frame"/>
>         <editor editortype="html"
Seems the kludge is no longer necessary, but you need to change the <stack>...</stack> into a <vbox>...</vbox> otherwise the find bar is invisible.

>+    <command id="cmd_find"          oncomamnd="goDoCommand('cmd_find')"/>
oncommand (you won't believe how long it took me to find that typo...)

>+<!ENTITY findBarCmd.label "Find">
This belongs in editorOverlay.dtd not utilityOverlay.dtd and also editorOverlay.xul needs to have the find_Replace menuitem added to it.

>+    <key id="key_findReplace" key="&findReplaceCmd.key;" command="cmd_findReplace" modifiers="accel"/>
>+    <menuitem id="menu_findReplace" key="key_findReplace"/>
You put these in platformCommunicatorOverlay.xul instead of utilityOverlay.xul for some reason.

Well, now I know that Find and Replace is Ctrl+H!

Comment 36

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v4.8

(In reply to [e-mail] from comment #35)
> Comment on attachment 819434 [details] [diff] [review]

> >+    <key id="key_findReplace" key="&findReplaceCmd.key;" command="cmd_findReplace" modifiers="accel"/>
> >+    <menuitem id="menu_findReplace" key="key_findReplace"/>
> You put these in platformCommunicatorOverlay.xul instead of
> utilityOverlay.xul for some reason.
:stefanh told me to do that on the channel, saying that we don't use #ifdefs in suite/
Comment 27.

Comment 37

[Suyash Agarwal (:sshagarwal)]

Sorry for the incomplete comment.
I wanted to use #ifdefs because we don't have Cmd+H on mac as that is used for hiding I think. So, we need it only for Windows and Unix, then as :stefanh said that we don't use #ifdefs in suite/ xul files and I had to move it in their individual platformCommunicatorOverlay files.

Moreover, when I start composing mail and bring up the find bar with an empty body, "undefined" appears in the findbar textbox. Is that a concern?

Comment 38

[[e-mail]]

(In reply to Suyash Agarwal from comment #37)
> we don't have Cmd+H on mac
Bah, why does Mac have to be different...

> Moreover, when I start composing mail and bring up the find bar with an
> empty body, "undefined" appears in the findbar textbox. Is that a concern?
That's related to comment #32.

Comment 39

[[e-mail]]

Comment on attachment 8337378 [details] [diff] [review]
Patch v4.8

Looks like we're nearly there.

>       <menuitem id="menu_find" label="&findCmd.label;"/>
>+      <menuitem id="menu_findReplace" label="&findReplaceCmd.label;"/>
Because you had to move the strings around you renamed the entities in the DTD which is correct, however you forgot to fix up the findCmd entity in editorOverlay.xul.

>+    <key id="key_findReplace" key="&findReplaceCmd.key;" command="cmd_findReplace" modifiers="accel"/>
>+    <menuitem id="menu_findReplace" key="key_findReplace"/>
> 
>     <!-- Select All Key -->
>     <key id="key_selectAll" key="&selectAllCmd.key;" modifiers="alt"/>
> 	
> 	<!-- Delete Key -->
>   <key id="key_delete" keycode="VK_DELETE" command="cmd_delete"/>
> 
>   <keyset id="findKeys">
key_findReplace belongs in the findKeys keyset. (Note that you'll have to add one for Windows, as its overlay didn't need it before.)

> <!-- LOCALIZATION NOTE (findCmd.accesskey): This accesskey should be within
>      findCmd.label found in messengercompose.dtd, messenger.dtd and
>      editorOverlay.dtd and findOnCmd.label found in navigatorOverlay.dtd -->
This is now findBarCmd.label, right?

>-<!ENTITY findCmd.accesskey "F">
>+<!ENTITY findBarCmd.accesskey "F">
If you want to go with this change, you'll need to update utilityOverlay.xul, and probably rename findCmd.key everywhere too for consistency.

> <!ENTITY findAgainCmd.label "Find Again">
> <!ENTITY findAgainCmd.key "G">
> <!ENTITY findAgainCmd.accesskey "g">
> <!ENTITY findAgainCmd.key2 "VK_F3">
> <!ENTITY findPrevCmd.label "Find Previous">
> <!ENTITY findPrevCmd.key "G">
> <!ENTITY findPrevCmd.key2 "VK_F3">
> <!ENTITY findPrevCmd.accesskey "v">
>+<!ENTITY findReplaceCmd.key "H">
>+<!ENTITY findReplaceCmd.accesskey "i">
Nit: findReplaceCmd belongs before findAgainCmd.

Comment 40

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5

Made the suggested corrections/changes. Thanks.

And, shall we include the searched history part we discussed on the channel in this patch itself?

Comment 41

[:aceman]

Attachment: patch to enable fastFind on <editor> v3

Updated the toolkit part according to comment 34. I still see the other 2 errors from comment 32 (e.g. when clicking "Match case") but those appear also when using the findbar in message view mode. So it does not seem related to pushing it into <editor>.
E.g. the _caseSensitiveStr property is strange, it looks like never properly defined (via a <field> or <property>)
http://mxr.mozilla.org/comm-central/search?string=_caseSensitiveStr&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Comment 42

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) will be away from 15th December from comment #40)
> Created attachment 8340966 [details] [diff] [review]
> Patch v5
> 
> Made the suggested corrections/changes. Thanks.
> 
> And, shall we include the searched history part we discussed on the channel
> in this patch itself?

If "search history" is a new feature, than please let's do that in another bug, not here.

Comment 43

[Thomas D. (:thomas8)]

Suyash, could you add a screenshot of the new findbar in composition, and of the Edit Menu?

Comment 44

[Suyash Agarwal (:sshagarwal)]

Attachment: Screenshot 1: New findbar in the compose window

Comment 45

[Suyash Agarwal (:sshagarwal)]

Attachment: Screenshot 2: Edit menu with new find option.

Comment 46

[Suyash Agarwal (:sshagarwal)]

(In reply to Thomas D. from comment #42)
> If "search history" is a new feature, than please let's do that in another
> bug, not here.

Sure :) I was just asking him if we need that implemented or not.

Comment 47

[[e-mail]]

Comment on attachment 8340966 [details] [diff] [review]
Patch v5

So close!

(unix/platformCommunicatorOverlay)
>-    <key keycode="&findCmd.key2;"
>+    <key keycode="&findBarCmd.key;"
This change (and the dtd) is wrong; findCmd.key2 is completely different from findCmd.key!

>+    <key keycode="&findReplaceCmd.key;"
>+         command="cmd_findReplace"/>
See below for what's wrong with this.

(win/platformCommunicatorOverlay)
>+  <key id="key_findReplace" key="&findReplaceCmd.key;" command="cmd_findReplace" modifiers="accel"/>
This would be correct, except that it's not in the keyset, so it gets ignored.

>+    <key keycode="&findBarCmd.key;"
>+         command="cmd_find"/>
This one is wrong, we already have a key for this.

>+    <key keycode="&findReplaceCmd.key;"
>+         command="cmd_findReplace"/>
See above for what's wrong with this.

Comment 48

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5.2

Is this correct?

Comment 49

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8344485 [details] [diff] [review]
Patch v5.2

Review of attachment 8344485 [details] [diff] [review]:
-----------------------------------------------------------------

I didn't understand the review satisfactorily. So there are a few things I want to ask.

And, while I was marking places in the patch, I saw a few whitespaces added, mostly are not from my patch. Should I remove them?

::: suite/common/unix/platformCommunicatorOverlay.xul
@@ +30,5 @@
>      </menupopup>
>      <key id="key_quit"  key="&quitApplicationCmd.key;" command="cmd_quit" modifiers="accel"/>
>    
>      <!-- Edit Menu -->
>      <key id="key_redo"  key="&redoCmd.key;" command="cmd_redo" modifiers="accel"/>

Why do we need to put our find keys in a keyset? Will this key not get ignored because it isn't in the keyset?

@@ +31,5 @@
>      <key id="key_quit"  key="&quitApplicationCmd.key;" command="cmd_quit" modifiers="accel"/>
>    
>      <!-- Edit Menu -->
>      <key id="key_redo"  key="&redoCmd.key;" command="cmd_redo" modifiers="accel"/>
> +    <menuitem id="menu_findReplace" key="key_findReplace"/>

I hope this is correct. I saw this same sort of treatment in calendar/

Comment 50

[[e-mail]]

OK, so the basic point is that a <key> doesn't do anything if it's not in a <keyset>.

Sometimes a <key> has a different keycode for each platform. In this case the <key> itself is put in a shared <keyset> without a keycode, then the platform overlay gives the <key> a keycode.

Sometimes a <key> only exists on some platforms. In this case the <key> has to be put in the <keyset> in the platform overlay.

Comment 51

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8344485 [details] [diff] [review]
Patch v5.2

Review of attachment 8344485 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to [e-mail] from comment #50)
> OK, so the basic point is that a <key> doesn't do anything if it's not in a
> <keyset>.

::: suite/common/unix/platformCommunicatorOverlay.xul
@@ +39,5 @@
>  	
>  	<!-- Delete Key -->
>    <key id="key_delete" keycode="VK_DELETE" command="cmd_delete"/>
>  
>    <keyset id="findKeys">

So, does that mean that the keys with ids "key_selectAll" and "key_delete" will be ignored since they aren't in the keyset?

If yes, then why do we have them?

Comment 52

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v5.5

Please let me know if this duplication is correct. As the key_find in utilityOverlay.xul's keyset wasn't bringing findbar on pressing Ctrl+F on Unix so I made this change.

Thanks.

(P.S. I hope patch v6 will end this bug).

Comment 53

[Suyash Agarwal (:sshagarwal)]

Clearing needinfo as the issue was resolved on the channel.

Comment 54

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch pending final review

Refreshed patch as last patch was failing to apply on the updated tree.
Ctrl+F brings up find bar both on TB and SM.
Ctrl+H brings up Find and replace dialog box both on TB and SM.

Comment 55

[[e-mail]]

Comment on attachment 8390347 [details] [diff] [review]
Patch pending final review

utilityOverlay already has key_find, no need to copy it to any of the platform overlays.

>+    <key id="key_findReplace"
>+         keycode="&findReplaceCmd.key;"
Needs to be key= (keycode= is for nonprinting keys)

>-<!-- LOCALIZATION NOTE (findCmd.accesskey): This accesskey should be within
>-     findCmd.label found in messengercompose.dtd, messenger.dtd and
>-     editorOverlay.dtd and findOnCmd.label found in navigatorOverlay.dtd -->
>+<!-- LOCALIZATION NOTE (findBarCmd.accesskey): This accesskey should be within
>+     findBarCmd.label found in messengercompose.dtd, findCmd.label in messenger.dtd
>+     and editorOverlay.dtd and findOnCmd.label found in navigatorOverlay.dtd -->
Nit: it's editorOverlay.dtd that uses findBarCmd.label, not messengercompose.dtd

>+<!ENTITY findReplaceCmd.accesskey "i">
(This is a bad access key, but the only other option I can find is "l" which is almost as bad (it's not a vowel, but that's all that's going for it).)

Comment 56

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v6

Made the suggested changes.
(In reply to [e-mail] from comment #55)
> 
> >+<!ENTITY findReplaceCmd.accesskey "i">
> (This is a bad access key, but the only other option I can find is "l" which
> is almost as bad (it's not a vowel, but that's all that's going for it).)
I changed it to "R". Is that fine? It wasn't assigned to any option in the edit menu so I took it. Please let me know if this needs to be changed.

Thanks :)

Comment 57

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #56)
> Created attachment 8398341 [details] [diff] [review]
> Patch v6
> 
> Made the suggested changes.
> (In reply to [e-mail] from comment #55)
> > 
> > >+<!ENTITY findReplaceCmd.accesskey "i">
> > (This is a bad access key, but the only other option I can find is "l" which
> > is almost as bad (it's not a vowel, but that's all that's going for it).)
> I changed it to "R". Is that fine? It wasn't assigned to any option in the
> edit menu so I took it. Please let me know if this needs to be changed.

R is assigned as access key for "Redo", so it's taken and can't be used here.

I think "i" or "l" are the only possible choices, both less than ideal, but perhaps "l" is better because at least it's part of "Replace" which is the significant command word here.

Comment 58

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #57)
> I think "i" or "l" are the only possible choices, both less than ideal, but
> perhaps "l" is better because at least it's part of "Replace" which is the
> significant command word here.

Again in favour of "l", it seems as if the same strategy was employed to get access keys for "Find a&gain" and "Find pre&vious" which also use consonants from the distinguishing second word of the command label, so "l" seems consistent with that. It's (pseudo-)mnemonic in that between the various flavors of "Find xyz...", "l" only occurs in "Find and Replace" whereas "i" occurs in all 4 of the "Find xyz..." commands so it's harder to remember which is which.

Comment 59

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v6.1

Okay, so changed accesskey to "l".

Comment 60

[[e-mail]]

Comment on attachment 8398445 [details] [diff] [review]
Patch v6.1

Sorry for the delay, my PC caught a virus (my fault for opening IE8) and I was too busy over the weekend to be able to clean it up.

>-<!-- LOCALIZATION NOTE (findCmd.accesskey): This accesskey should be within
>-     findCmd.label found in messengercompose.dtd, messenger.dtd and
>-     editorOverlay.dtd and findOnCmd.label found in navigatorOverlay.dtd -->
>+<!-- LOCALIZATION NOTE (findBarCmd.accesskey): This accesskey should be within
>+     findBarCmd.label found in editorOverlay.dtd, findCmd.label in messenger.dtd
>+     and editorOverlay.dtd and findOnCmd.label found in navigatorOverlay.dtd -->
Now you managed to forget messengercompose altogether while editorOverlay is now listed twice. r=me with that fixed.

Comment 61

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v6.2

Thanks :)

Do we need a ui-review too or can I move for check-in? I think I forgot that.

Comment 62

[Thomas D. (:thomas8)]

Comment on attachment 8399780 [details] [diff] [review]
Patch v6.2

Hi Jim,

could you have quick glance to confirm the UI of this nice cooperative patch where we introduce the usability comfort of Quick Find Bar (Ctrl+F) to composition, effecting a considerable increase in ux-efficiency and ux-consistency (with message reader which already has this feature). "Find & Replace" is unaffected and works and looks as before (Ctrl+H on Windows & Linux). Patch has review+ by Neil who helpfully assisted, so this has been approved for SeaMonkey, too.

- Short summary of benefits, see comment 0.
- New Quick Find Bar in composition: see screenshot attachment 8344352 [details].
- New "Find" menu in composition: see screenshot attachment 8344353 [details].

Tia.

Comment 63

[:aceman]

Comment on attachment 8399780 [details] [diff] [review]
Patch v6.2

squib is an unusual choice for ui-r. We use bwinton and Josiah these days :)

Comment 64

[Jim Porter (:squib)]

I am an actual ui-reviewer, blessed by bwinton. :)

But maybe it's good that you picked someone else, since I was about to minus this, since I think we need to overlay a "Find and Replace" button onto the find toolbar. Otherwise, people are going to be sad and confused when Ctrl+F doesn't let them replace text.

Comment 65

[Jim Porter (:squib)]

(Sorry for the weird rambling grammar in the above comment!)

Comment 66

[Josiah Bruner [:jsbruner]]

Yes, we're going to want to add a Replace function. Similar to this:

Not checked - http://cl.ly/UlAv

Checked - http://cl.ly/UlOB

And yes, Jim is a ui-reviewer now! For reference, the UX Team / UI-review choices are laid out here: https://wiki.mozilla.org/Thunderbird:UX/Team

Comment 67

[Thomas D. (:thomas8)]

Comment on attachment 8399780 [details] [diff] [review]
Patch v6.2

In reply to Thomas D. (via pm to syshagarwal):

> I'd also love to see at least a "Find & Replace" button on composition's
> find bar to link up the two functionalities ... (new bug).
> (I was even contemplating to add "Replace-with" text input on find bar, but
> that needs more UI/behaviour planning and is more complex to code).

:JosiahOne (comment 66): Need "Find & Replace" on Find Bar (inline)
:squib (comment 64):     Need "Find & Replace" on Find Bar (button overlay)
:ThomasD (this comment): Wish "Find & Replace" on Find Bar (button overlay; consider inline)

The common denominator being that we all want to make "Find & Replace" functionality accessible from the Find Bar.

As Jim pointed out, we should avoid breaking people's muscle memory for Ctrl+F to "Find & Replace" (albeit we might have trained them wrong all the way because Find & Replace is usually Ctrl+H, on Windows at least). Providing at least a button which links to the existing "Find & Replace" dialogue prevents such users from ending up in a cul-de-sac, and even for other types of users it's a convenient bridge start out from Find (Ctrl+F) and continue with Replace when needed (MS Word has the same kind of setup where Ctrl+F = Find Dialogue, which comes with a Replace tab (bridge), and Ctrl+H = Replace tab straight away; seen on Word 2003).

As a more than active community member with a good public track record in TB UX and hence eligible for ui-reviews (per https://wiki.mozilla.org/Thunderbird:UX/Team), I'll set ui- on this patch because it should have a "Replace" button at least to close find bar call the existing "Find and Replace" dialogue.

Comment 68

[Thomas D. (:thomas8)]

Attachment: Screenshot 3: New findbar with Replace button

I think after all the cooperative effort spent on this bug, we'd really like to this land sooner rather than later.

Notwithstanding the potential of Comment 66 where :JosiahOne proposes an inline variant of "Find & Replace" dialog (Find & Replace *bar* so to speak), imo for this bug it's sufficient to just add a "Replace" *button* at the right side of the Find bar so that we link up the existing "Find & Replace" dialog per general consensus summarized in comment 67 (see attached mockup in Screenshot 3; feedback welcome). Then we can follow up on Josiah's interesting and innovative idea in a new bug and cooperatively fine-tune the details and do all the required coding without further delaying the delivery of the useful basic improvements offered here in bug 530629.

The "Replace" button on Find bar proposed here works straightforward: Hides the bar and opens the existing "Find & Replace" dialogue.

Comment 69

[Thomas D. (:thomas8)]

(In reply to Josiah Bruner [:JosiahOne] from comment #66)
> Yes, we're going to want to add a Replace function. Similar to this:
> Not checked - http://cl.ly/UlAv
> Checked - http://cl.ly/UlOB
> And yes, Jim is a ui-reviewer now! For reference, the UX Team / UI-review
> choices are laid out here: https://wiki.mozilla.org/Thunderbird:UX/Team

FTR (until we have a new bug for inline "find & Replace bar"):
- proposal of comment 66 omits "highlight all", "match case" on find bar (certainly wanted, even FF didn't remove them although they recently redesigned the bar).
- also omits "wrap around" checkbox (not needed for find, but needed for replace) and "search backwards" checkbox (probably still useful); same for "Replace and Find" (not sure about that one, I remember filing a bug in that corner).
- folding out the "Replace" UI by expanding the bar is quite nice; but we need to examine if the Replace *checkbox* sends the right signals - what happens if I click on up/down arrows with replace checkbox checked - will it replace? (suppose it won't and shouldn't, but users might be uncertain about this). Perhaps Replace checkbox could be an opener toggle button instead (like next to the tags field in FF bookmark popup):
[Replace v] (Show replace bar)
[Replace ^] (Hide replace bar)

Comment 70

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #67)
> I'll set ui- on this patch (attachment 8399780 [details] [diff] [review])
> because it should have a "Replace" button at least to close find bar call
> the existing "Find and Replace" dialogue.

Which means that it's actually ui+ for all of that great patch with that little "Replace" button added :)

Comment 71

[Thomas D. (:thomas8)]

OT:

(In reply to Thomas D. from comment #69)
> (In reply to Josiah Bruner [:JosiahOne] from comment #66)
> - also omits "wrap around" checkbox (not needed for find, but needed for
> replace) and "search backwards" checkbox (probably still useful); same for
> "Replace and Find" (not sure about that one, I remember filing a bug in that
> corner).

That's bug 530621. A bit clumsy in wording, but both "Replace" and "Replace and Find" have their benefits, while confusingly, "Replace and Find" is what other apps like MS Word just call "Replace"; and TB's "(Find and) Replace (without advancing)" is a nice feature which such apps don't have, and it's broken/badly implemented in TB.

Anyway, the poor little creature "Find and Replace" dialogue certainly deserves redemption:
Even after my cleanup, still 3 bugs and 2 RFE's including this bug. Comment 66 in it's own bug might solve them all.

https://bugzilla.mozilla.org/buglist.cgi?quicksearch=%3Athun%2Cmailn%20find%20replac

Comment 72

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v6.7

Added the "Replace" button on the findbar that brings up the find and replace dialog.

Now, we have two choices, after the "Find and replace" dialog closes, should we get the focus back to the findbar? or while bringing up the "find and replace" dialog, should we close the findbar?

Thanks.

Comment 73

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #72)
> Created attachment 8400829 [details] [diff] [review]
> Patch v6.7
> 
> Added the "Replace" button on the findbar that brings up the find and
> replace dialog.
> 
> Now, we have two choices, after the "Find and replace" dialog closes, should
> we get the focus back to the findbar? or while bringing up the "find and
> replace" dialog, should we close the findbar?

Wow, that was quick (wrt "Replace" button). Good questions about the behaviour, too. Thanks.

So it's about how volatile we want our nice new Find bar to behave, and where to put focus after "Find & Replace". Some points for consideration:

1) Existing find bars in message reader and FF will remain visible until user deliberately closes them with ESC (focus on findbar) or clicking find bar's [x] button. So that's the behaviour users are familiar with, which somewhat favors keeping the same behaviour for composition find bar too (ux-consistency), i.e. to not close find bar programatically after use.

2) Message reader and composition are different in purpose. "Find" might be less frequently used in composition (over message reader), depending on scenarios. But for checking or navigating long compositions, or to search within quoted parts of composition, or to verify specific words and phrases, etc., Find bar can be helpful. Users using "Find" are somewhat likely to use it again, so no need for us to close it programatically.

3) Users who only use "Find" will either easily close the bar with ESC anyway to refocus into body or deliberately keep Find bar open. So for that scenario, no need for us to close the bar programatically.

4) Users who proceed with "Replace" button from find bar might be somewhat likely to use "Find and Replace" functionality again. So for that scenario, it seems helpful again to have the Find bar present after they close the "Find & Replace" dialog (i.e. to not close the bar programatically). 

5) Should Find bar be visible while "Find & Replace" dialog is shown? I think not.
- It will be potentially irritating to users to have two different "Find" input boxes on screen
- You can't use both Find bar and "Find & Replace" simultaneously, so what's the point?
- So when you can't use it at this time, Find bar is just clutter and wasting screen real estate (which is already reduced by "find & replace" dialog!).
So find bar should be hidden while "Find & Replace" dialog is shown.

6) Conclusion *******:
I suggest that we think of the "Find & Replace" dialogue as a popup (or pop-out): When clicking "Replace" button, Find bar expands into a full-fledged "Find & Replace" dialog. When you're done, that dialog collapses back into it's original position (Find bar at the bottom of screen). We just keep the Find bar around because that's the usual behaviour of Find bars.

7) As for focus, I think when users click "Close" on "Find & Replace" dialog, we can expect them to be done with both finding and replacing. So I think it's better to re-focus on message body, not into the find bar which might be annoying if users feel we're too sticky about it.

8) Furthermore, we could consider hiding Find bar when user presses ESC with focus in msg body, as we already have the same behaviour for closing attachment reminder bar. Btw, can you please verify that Find bar and Attachment reminder bar interact correctly (probably Find bar should be stacked on top of attachment reminder bar, i.e. they should not hide each other no matter in which order they are triggered; could you add a screenshot of that? Tia.).

Comment 74

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v6.9

Okay so this is how it behaves now:

->You bring up the findbar. Do whatever you feel like.
->Click on "Replace".
    Focus shifts from findbar to body (otherwise find and replace remains disabled).
    Findbar is hidden.
    Find and Replace dialog is up and available for use.
->You do whatever you feel like. Close the find and replace dialog.
    Focus remains in the body.
    Findbar comes up again.
->You can either use the findbar or
->can close it with Esc key.

Since, now escape key handles two bars. attachmentNotificationBox and findbar,
it is firstly consumed by the element that has the focus. And, if the body has the focus, firstly attachmentReminderBar consumes it.

Please let me know if there are any usability issues in the proposed and implemented behavior.

Thanks.

Comment 75

[Suyash Agarwal (:sshagarwal)]

Attachment: Screenshot 4: Findbar and attachment reminder notification bar, stacked.

(In reply to Thomas D. from comment #73)
> Btw, can you please verify that Find bar and
> Attachment reminder bar interact correctly (probably Find bar should be
> stacked on top of attachment reminder bar, i.e. they should not hide each
> other no matter in which order they are triggered; could you add a
> screenshot of that? Tia.).

Sure, this is how it looks.

Comment 76

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #74)
> Created attachment 8401336 [details] [diff] [review]
> Patch v6.9
> Okay so this is how it behaves now:
[snip]

awesome! :)

> Since, now escape key handles two bars. attachmentNotificationBox and
> findbar,
> it is firstly consumed by the element that has the focus. And, if the body
> has the focus, firstly attachmentReminderBar consumes it.

Hmmm. I think that detail feels wrong. I suggest to hide them the other way round:
When focus is in body and both find bar and attachment reminder bar are shown, and user presses ESC, first hide find bar, then reminder bar.

Reasons:

1) find bar is on top of reminder bar, i.e. closer to the body: pressing ESC should start clearing bars from top to bottom (= going from near-to-focus to far-from-focus = going from center to outside)

2) find bar is less important than reminder bar: In terms of ux-error-prevention, we need to avoid that users who just want to close inconsequential find bar unintendedly close much more important attachment reminder bar against their will. We also risk spoiling aggressive automatical reminder feature which fires when you try to send without attachments while you've ignored reminder bar (i.e. it's still shown when sending).

3) it's easily possible and entirely inconsequential to restore find bar with Ctrl+F, whereas hiding reminder bar even changes the app behaviour because we won't show reminder again for same keyword, so it's much harder to get that bar back again and you'll never get into the same state as before.

4) Given 1), 2) and 3), it's much more likely that users want to just gain real estate by hiding the temporary find bar, whereas there's a good chance they might want to keep reminder bar until they are ready to act on it (or even to let it trigger automatically at send time when they forget). Even when they want to hide reminder bar, it will be easier and safer to decide on when it's the only bar left on the screen, compared to seeing two bars and suddenly the more important bottom bar gets taken away.

Comment 77

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v7

(In reply to Thomas D. from comment #76)
> Hmmm. I think that detail feels wrong. I suggest to hide them the other way
> round:
> When focus is in body and both find bar and attachment reminder bar are
> shown, and user presses ESC, first hide find bar, then reminder bar.
Done.

Comment 78

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #74)
> Created attachment 8401336 [details] [diff] [review]
> Patch v6.9
> 
> Okay so this is how it behaves now:
> 
> ->You bring up the findbar. Do whatever you feel like.
> ->Click on "Replace".
>     Focus shifts from findbar to body (otherwise find and replace remains
> disabled).

For the avoidance of doubt, that shift of focus is a technical detail which will be covert to the user. The UX is that focus goes straight from the [Replace] button into the "Find & Replace" dialog as expected. Checking edge cases: What happens if user hits [Replace] (even via keyboard) and immediately starts typing very fast? Especially on slow machines, will some characters end up in body (because dialog is still coming up) although they are intended for the dialog input? If yes, can we avoid that?

>     Findbar is hidden.
>     Find and Replace dialog is up and available for use.

i.e. having focus of course.

Comment 79

[Thomas D. (:thomas8)]

Does this work?

Ctrl+F, type "foo" into find bar search box
Enable "Match Case" on the right (I'm quite failing to see why they are so far away, but that's what FF decided)
Click Replace

In Find & Replace dialog, Find text should be prefilled with "foo", and "Match exact case" should be checked. Iow, all inputs from find bar should be copied into the dialog as far as applicable.

Comment 80

[Thomas D. (:thomas8)]

OT: And, wow wow wow, could we add "Highlight all" to the "Find & Replace" dialogue (in a new bug)? I'd love to see all instances of find text highlighted before I decide to "Replace All"...

Comment 81

[Thomas D. (:thomas8)]

Another question:

Find bar hidden (!)
Ctrl+H to get "Find & replace" dialog straight away (bypassing find bar)
Close dialog

Will that show the find bar? (I hope not).

Comment 82

[Suyash Agarwal (:sshagarwal)]

(In reply to Thomas D. from comment #79)
> Does this work?
> 
> Ctrl+F, type "foo" into find bar search box
> Enable "Match Case" on the right (I'm quite failing to see why they are so
> far away, but that's what FF decided)
> Click Replace
> 
> In Find & Replace dialog, Find text should be prefilled with "foo", and
> "Match exact case" should be checked. Iow, all inputs from find bar should
> be copied into the dialog as far as applicable.

Sometimes and sometimes not.
[1] and [2] suggest it should. But many a times it doesn't work. Mostly in the cases when the text isn't found in the body, only the first character is copied in the find field of F&R. Match case status isn't reflected in F&R. So, we can look into this if needed (I'll propose to add a OnLoad() or constructor method with parameters where these values can be provided that'll fill up the things when F&R is brought up) though I didn't get into the details as of now.

> Another question:
> 
> Find bar hidden (!)
> Ctrl+H to get "Find & replace" dialog straight away (bypassing find bar)
> Close dialog
> 
> Will that show the find bar? (I hope not).

No :)

Comment 83

[Suyash Agarwal (:sshagarwal)]

(Quoting Suyash Agarwal (:sshagarwal) from comment #82)
> Sometimes and sometimes not.
> [1] and [2] suggest it should. But many a times it doesn't work. Mostly in
> the cases when the text isn't found in the body, only the first character is
> copied in the find field of F&R. Match case status isn't reflected in F&R.
> So, we can look into this if needed (I'll propose to add a OnLoad() or
> constructor method with parameters where these values can be provided
> that'll fill up the things when F&R is brought up) though I didn't get into
> the details as of now.

[1] http://mxr.mozilla.org/comm-central/source/editor/ui/dialogs/content/EdReplace.js
[2] http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/finddialog.js

Comment 84

[Josiah Bruner [:jsbruner]]

Comment on attachment 8401362 [details] [diff] [review]
Patch v7

Review of attachment 8401362 [details] [diff] [review]:
-----------------------------------------------------------------

So this works quite well and I am generally happy with the UI here except for one thing. At least on OS X, the "Replace" button is shown as text-only, which gives the impression that the field next to it is for replacing text, not finding text. Please make "Replace" use a style similar to "Highlight All" (minus the pushed in state).

Thanks.

Comment 85

[:aceman]

That also happens on Linux (no border around the button).

Comment 86

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v7.2

Comment 87

[Suyash Agarwal (:sshagarwal)]

Attachment: Replace button on Findbar

This is what I see.

Comment 88

[[e-mail]]

Comment on attachment 8402259 [details] [diff] [review]
Patch v7.2

All changes between 6.2 and 7.2 are in /mail so nothing for me to review.

Comment 89

[Thomas D. (:thomas8)]

I think there are two things here:

#1: OS specific styling of buttons (here: Replace button)

On Windows (XP at least) and Linux (I think) the default style for button is flat (without borders), they only "buttonize" on mouseover. This matches the OS default style and is good design which avoids a lot of visual clutter - we're in 2014 and users have had decades of training that they need to click on what they want, so showing permanent button borders isn't needed nor helpful because it reduces the salience of the actual button contents. Following the same idea, Josiah visually flattened the composition header (by removing all borders) to create a more calm and smooth UI for elements that are not currently in use. So flat button design looks alright to me (depending on OS), and we shouldn't create exceptions here.

#2: Location of Replace button near Find input field

This is the design per current patch:

---------------------------------------------------------------------------------------------------
[Find input     ][^][v] [Replace] {Find Feedback}                     Highlight All  Match Case  x
---------------------------------------------------------------------------------------------------

Undesired UX-Effects of design per current patch

As Josiah correctly points out, Replace button might be misunderstood as belonging to the Find input, either as a caption, or as the main action button, which might confuse users who just want to use Find. And the current design also breaks some of the intentions of the current FF find bar design. In detail:

a) Replace button is directly adjacent to the find input, wrongly suggesting they belong together

b) Replace button is over-salient (standing out) compared to the relevant action buttons [^] and [v] (Find previous, Find next), while the main purpose of both the Find input and the entire Find bar is Find, not Replace. (Fyi, labels of Find previous & Find next were intentionally removed by FF because the expected use pattern is: Type search words, then press [Shift+]Enter to navigate results, so using mouse for that scenario is inefficient, unlikely, and discouraged by making the buttons iconic/small/less salient). 

c) Replace button separates Find input and Find options (Highlight All, Match Case), because we now have a mix and the inherent connection is lost:
Patch:    Find input <-> Replace <-> Find options
Intended: Find input       ->        Find options

d) Replace button separates the {Find Feedback} from the Find input, which makes it harder to visually associate the feedback with current search input. {Find Feedback} is one of "Reached end of page, continued from top" or "Phrase not found".
Patch:    [Find in page   ][^][v] [Replace] {Find Feedback}
Intended: [Find in page   ][^][v] {Find Feedback}
For an extreme example of dissociated {Find Feedback}, try Find bar in TB24 message reader and ensure getting some feedback, esp. "Reached end of page..."

Towards finding a better design: What's the main purpose of Find bar?

1) Find vs. Find & Replace are currently two different animals with separate UI and separate menu entries. (We might want to unify the UI lateron along Comment 66, but that's a new story for another bug)
2) On Windows and Linux, we even offer separate keyboard shortcuts for Find vs. Find & Replace:
- Find:           Ctrl+F
- Find & Replace: Ctrl+H
So there's a good chance that users who actually/frequently want Find & Replace will use Ctrl+H directly (or the respective menu), thus bypassing Find bar.
3) Conclusion: In view of 1) and 2), the main purpose of Find bar (per this bug) is Find, not Replace. Replace button on Find bar is more like a bonus which conveniently links to Find & Replace functionality.

Criteria for better design

* avoid UX problems listed in a), b), c) and d)
* prioritize the main purpose of Find bar, which is Find

I'll propose a simple solution in my next comment.

Comment 90

[Thomas D. (:thomas8)]

Attachment: Screenshot 5 (accepted proposal): Findbar with Replace button on the right

I'd propose to simply move the Replace button to the right end of the Find bar (see mockup screenshot attached here).

Benefits:

1) Solves all the UX problems a) b) c) d) mentioned in comment 89:

a)b) Replace button deliberately dissociated from Find input, now less salient (de-emphasized) on the far right

c) Find vs. Replace now in coherent order and clearly separated (by location, and by | separator before Replace):
Find input     ->     Find options | Replace

d) {Find Feedback} now inplace/adjacent again (as intended by FF design):
[Find input  ][^][v] {Find Feedback}

2) In view of the current clear separation between Find vs. Find & Replace, the design and behaviour of this proposal prioritizes the main purpose of Find bar (which is finding), while still providing convenient in-place access to Find & Replace dialog.

Feedback welcome :)

Comment 91

[Josiah Bruner [:jsbruner]]

I like that.

No further comments.

:)

Comment 92

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #86)
> Created attachment 8402259 [details] [diff] [review]
> Patch v7.2

Nit:

> +replaceButton.tooltip=Bring up the Find and Replace dialog

Please change that tooltip to "Show the Find and Replace dialog".

Unfortunately, "bring up" doesn't work because it's too colloquial, slightly different from the intended meaning* and worse, one of its meanings in English is "vomit"...

*: to bring forward, to bring (in)to the foreground, or even just "to foreground" as a verb

Comment 93

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v7.5

Addressing comments from 89 to 92.

Comment 94

[Suyash Agarwal (:sshagarwal)]

Attachment: Screenshot 6: Find bar with Replace button on the right (Patch v6 addressing comments 89-92)

Comment 95

[Thomas D. (:thomas8)]

(In reply to Josiah Bruner [:JosiahOne] from comment #91)
> I like that.
> 
> No further comments.
> 
> :)

Thanks for the comment :)

(In reply to Suyash Agarwal (:sshagarwal) from comment #94)
> Created attachment 8402382 [details]
> Screenshot v6 addressing comments 89-92

Thanks again for the rapid patch & screenshot :)

Comment 96

[Thomas D. (:thomas8)]

Another nit:

+replaceButton.label=Replace

Pls change this to

+replaceButton.label=Replace...

While looking at screenshot attachment 8402382 [details], I was wondering why we can't use "Find and Replace" as a tooltip. I realized we can't because it would suggest an immediate action using the search word from find bar. So in the tooltip, we make it explicit that this command button will call another dialog without immediate action. However, we still have the same UX problem on the button label:

From just looking at "Replace" button (without trailing "..." in the label), users might easily be misled to assume that this will be an immediate action (leaving them insecure what is going to be replaced).

* There's actually a design rule handed down through the ages which demands that labels of buttons which trigger a dialogue requiring further user input (rather than an immediate action) must have trailing "..." as an indication of that
* internal ux-consistency: TB still follows that design rule in almost all menus and even toolbars. E.g., compare:

File > Open saved message...  (dialog) vs.
File > Compact Folders (immediate action)

Attachment reminder notification bar:
Add attachment... (dialog) vs.
Remind me later (immediate action)

* external ux-consistency: I'm seeing the same design principle on other Windows apps like Winword, Notepad etc.
* Using "Replace..." button label will also nicely increase distinction against Find option buttons, and avoid users to wrongly associate [Replace] with the subseqent [x] button. Compare:
Replace x 
Replace... x (it's clearer here that Replace... and x are two different buttons)

Comment 97

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #96)
> Another nit:
> +replaceButton.label=Replace
> Pls change this to
> +replaceButton.label=Replace...

And if possible, we probably want to use the actual ellipsis character (single character having three dots), which is also used in menus I think.

Comment 98

[Suyash Agarwal (:sshagarwal)]

This time I'll wait for further improvement comments before working on the patch again.
Please let me know when we're ready :)

Thanks.

Comment 99

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #82)
> (In reply to Thomas D. from comment #79)
> > In Find & Replace dialog, Find text should be prefilled with "foo", and
> > "Match exact case" should be checked. Iow, all inputs from find bar should
> > be copied into the dialog as far as applicable.
> 
> Sometimes and sometimes not.
> [1] and [2] suggest it should. But many a times it doesn't work. Mostly in
> the cases when the text isn't found in the body, only the first character is
> copied in the find field of F&R.

That might be intended as it's actually pretty clever design:

Suppose your text has "foobar" and using quick findbar, you search for "foobaz".
These are the incremental searches performed (at least if type slow enough, i.e. with sufficient pauses after each character):

f -> found
fo -> found
...
fooba -> found
foobaz -> not found

For each of these searches, search phrase will be memorized for later use in Find & Replace dialogue but only IF it has actually been found. Obviously, it would not be easy to find and replace a word which is not even found in your text... ;) Not sure if same helpful behaviour applies to *fast* typing of same search word.

> Match case status isn't reflected in F&R.
> So, we can look into this if needed

I think passing of "Match case" status of findbar into find & replace dialog would be nice, but not a requirement for this bug.

Comment 100

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #98)
> This time I'll wait for further improvement comments before working on the
> patch again. Please let me know when we're ready :)
> Thanks.

Sorry for the incremental procedure, but such is the nature of refined cooperative quality coding... :|

Here's another question:

In message reader, TB (like FF) also offers this hidden functionality, as documented in FF keyboard shortcuts [1]:

Quick Find within link-text only:  '
Quick Find:                        /

Both of these are actually cool features although they are crippled as they get stuck on the first occurence, so it's only useful if you want to find first occurence or unique terms (To be more useful, I think [Shift+]Enter should proceed to next/previous occurence).

How does this behave in TB composition? (make sure to have a link in your composition body when searching for link text). Again, having the functionality (crippled or not) would be nice, but not a must for this bug.

[1] https://support.mozilla.org/en-US/kb/keyboard-shortcuts-perform-firefox-tasks-quickly?redirectlocale=en-US&redirectslug=Keyboard+shortcuts#w_search

Comment 101

[Thomas D. (:thomas8)]

The toolkit search prompt is:

Find in page

That might make sense for browser (find in this web page), does it make sense for us as a mail/news reader? I think not. A message is not a page, so page could even misleadingly suggest we're just searching the current page of the message (pages as in print preview).

Perhaps one of these?

1) Find in message text
2) Find text in message
2b) Find text in this message
3) Find in this message

4) Search message text
5) Search this message (ambiguous?)
5b) Search in this message
6) Search the text of this message (ambiguous?)

If possible, I think we should use the same prompt both for message reader and composition.
I'm not sure but I'd prefer 2b) or 5b), and between the two, perhaps 2b).

If we decide to change this and agree how, it should be changed for message reader and composition.
Not required for this bug, but nice polish.

Comment 102

[Thomas D. (:thomas8)]

That's all I can think of for today, but you never know tomorrow... ;)

Comment 103

[Thomas D. (:thomas8)]

> +replaceButton.accesskey=R

No, that's already taken by F_rom. Remember we had the same problem for "Remind me later".

The most important test is Alt+access key while focus in composition body.
For testing the access key, please make sure it's unique between all other access keys that are accessible on primary UI of composition:

- main menus
- UI elements in composition like From...
- UI elements on any notification bars potentially shown in composition (attachment reminder notification, anything else?)
- UI elements on Contacts Side bar (although these might have their own limited scope, pls try)
- ...

Comment 104

[Suyash Agarwal (:sshagarwal)]

(In reply to Thomas D. from comment #100)
> In message reader, TB (like FF) also offers this hidden functionality, as
> documented in FF keyboard shortcuts [1]:
> 
> Quick Find within link-text only:  '
> Quick Find:                        /
> 

Nice but probably not a part of this bug.

(In reply to Thomas D. from comment #103)
> > +replaceButton.accesskey=R
> 
> No, that's already taken by F_rom. Remember we had the same problem for
> "Remind me later".

So, the only key left I can find out of the word "Replace" is 'l'. Should we use it? Or should we exchange keys? E.g. 'g' on Highlight All is empty, so probably we can assign 'g' as its accesskey and use 'a' for Replace.

Comment 105

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #104)
> (In reply to Thomas D. from comment #100)
> > In message reader, TB (like FF) also offers this hidden functionality, as
> > documented in FF keyboard shortcuts [1]:
> > 
> > Quick Find within link-text only:  '
> > Quick Find:                        /
> Nice but probably not a part of this bug.

Could you answer if these shortcuts do anything at all in composition with current patch?

> (In reply to Thomas D. from comment #103)
> > > +replaceButton.accesskey=R
> > 
> > No, that's already taken by F_rom. Remember we had the same problem for
> > "Remind me later".
> 
> So, the only key left I can find out of the word "Replace" is 'l'. Should we
> use it? Or should we exchange keys? E.g. 'g' on Highlight All is empty, so
> probably we can assign 'g' as its accesskey and use 'a' for Replace.

Suyash, pls check again. You need to test on Trunk, not release, for all the UI elements listed in my comment 103: "l" is also taken (Remind me _Later on reminder bar), and so is "a" (Add attachment on reminder bar). But then, all letters of the word "Replace" are taken. So we have to share one access key. Please do the calculations yourself:

Your comment 59 + ux-consistency = access key...

Having same access key for one command on button and in the menu is very ux-consistent & mnemotic.
Sharing the same access key between two buttons in the same corner (Replace and Remind me Later) also makes it easy for the user to realize they are shared, because he's looking at that corner anyway so he just needs to toggle between the two. Plus, there's a good chance that only one of the bars is shown, which is when the access key will work instantly.

Comment 106

[Suyash Agarwal (:sshagarwal)]

(In reply to Thomas D. from comment #105)
> > > Quick Find within link-text only:  '
> > > Quick Find:                        /
> Could you answer if these shortcuts do anything at all in composition with
> current patch?

No, I couldn't put them to use.

> > (In reply to Thomas D. from comment #103)
> > > > +replaceButton.accesskey=R
> > So, the only key left I can find out of the word "Replace" is 'l'. Should we
> > use it? Or should we exchange keys? E.g. 'g' on Highlight All is empty, so
> > probably we can assign 'g' as its accesskey and use 'a' for Replace.
> 
> Your comment 59 + ux-consistency = access key...
> 
> Having same access key for one command on button and in the menu is very
> ux-consistent & mnemotic.
> Sharing the same access key between two buttons in the same corner (Replace
> and Remind me Later) also makes it easy for the user to realize they are
> shared, because he's looking at that corner anyway so he just needs to
> toggle between the two. Plus, there's a good chance that only one of the
> bars is shown, which is when the access key will work instantly.

Ya, so 'l' is fine isn't it.

Comment 107

[Josiah Bruner [:jsbruner]]

Sadly your patch no longer applies cleanly.

Comment 108

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v7.5 refreshed

Comment 109

[Thomas D. (:thomas8)]

Comment on attachment 8403792 [details] [diff] [review]
Patch v7.5 refreshed

Review of attachment 8403792 [details] [diff] [review]:
-----------------------------------------------------------------

Suyash, you're doing a great job here, thanks a lot also for your patience and rapid delivery of incremental patches. We're all eager to see this land asap... :)

There are some minor nits (some of which already mentioned in comments).

::: editor/ui/composer/content/editorOverlay.xul
@@ +308,5 @@
>        <menuitem id="menu_delete"/>
>        <menuseparator id="sep_selectAll"/>
>        <menuitem id="menu_selectAll"/>
>        <menuseparator id="sep_find"/>
> +      <menuitem id="menu_find" label="&findBarCmd.label;"/>

Pls move label attribute into new line and align with id attribute (as seen in surroundings; imo we should always do that even regardless of the surroundings):

<menuitem id="menu_find"
          label="&findBarCmd.label;"/>

@@ +309,5 @@
>        <menuseparator id="sep_selectAll"/>
>        <menuitem id="menu_selectAll"/>
>        <menuseparator id="sep_find"/>
> +      <menuitem id="menu_find" label="&findBarCmd.label;"/>
> +      <menuitem id="menu_findReplace" label="&findReplaceCmd.label;"/>

dito, attributes alignment

::: editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd
@@ +16,5 @@
>  <!ENTITY pasteAsQuotationCmd.label "Paste As Quotation">
>  <!ENTITY pasteAsQuotationCmd.accesskey "Q">
>  <!ENTITY pasteAsQuotationCmd.key "o">
> +<!ENTITY findBarCmd.label "Find">
> +<!ENTITY findReplaceCmd.label "Find and Replace">

Pls add trailing "…" (single character) to both of these labels (as explained in comment 96)

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1785,5 @@
>   * notifications with close button in the
>   * attachmentNotificationBox.
>   */
>  function handleEsc()
>  {

The findbar part of this function should have a comment:
What does it do, and under which exact conditions? Writing such comments also helps to re-analyse the exact behaviour and find problems like focus issue below.

@@ +1787,5 @@
>   */
>  function handleEsc()
>  {
> +  let findbar = document.getElementById('FindToolbar');
> +  if (!findbar.hidden) {

- Doesn't this require an explicit focus check as we have for hiding the attachment reminder notification (see below)? I think we had our reasons for positively enforcing that focus is either in body or on the bar for ESC to do the trick, not just anywhere. E.g., imo we don't want find bar to be hidden when user presses ESC with focus in contacts side bar or in recipients area.
- Does this work when pressing ESC with focus on findbar? (probably yes)

@@ +2009,5 @@
>  CheckForAttachmentNotification.shouldFire = true;
>  
>  function ComposeStartup(recycled, aParams)
>  {
> +  // Findbar overlay

We're reconstructing the entire button for every composition, which looks resource-eating. I wonder if this button e.g. could be parked somewhere in xul and then we just create a copy or another instance of that button here, or even move the button node? (Sorry I wouldn't know how to do any of that, just a thought).

@@ +2020,5 @@
> +    replaceButton.setAttribute("accesskey", getComposeBundle().getString("replaceButton.accesskey"));
> +    replaceButton.setAttribute("oncommand", "findbarFindReplace();");
> +
> +    let findbar = document.getElementById("FindToolbar");
> +    let lastButton = findbar.getElement('find-case-sensitive');

Here and for subsequent lines, pls use double quotes for consistency

@@ +2022,5 @@
> +
> +    let findbar = document.getElementById("FindToolbar");
> +    let lastButton = findbar.getElement('find-case-sensitive');
> +    let tSeparator = document.createElement('toolbarseparator');
> +    tSeparator.setAttribute("id", 'findbar-separator');

If somebody adds other separators lateron, this id won't scale well. Let's use more informative ID (with double quotes):
"findbar-beforeReplaceSeparator"

::: mail/components/compose/content/messengercompose.xul
@@ +517,5 @@
>                      key="key_renameAttachment" command="cmd_renameAttachment"/>
>            <menuseparator/>
>            <menuitem id="menu_selectAll" label="&selectAllCmd.label;" key="key_selectAll" accesskey="&selectAllCmd.accesskey;" command="cmd_selectAll"/>
>            <menuseparator/>
> +          <menuitem label="&findBarCmd.label;"      key="key_find"      accesskey="&findBarCmd.accesskey;"      command="cmd_find"/>

Unless there's a reason not to (like overlays or whatever), we should add IDs to these menuitems following the pattern seen in the surrounding menuitems. We also want to align attributes in rows; key and accesskey lines need to be swapped so that access key goes with label:

<menuitem id="menu_find"
          label="&findBarCmd.label;"
          accesskey="&findBarCmd.accesskey;"
          key="key_find"
          command="cmd_find"/>

@@ +519,5 @@
>            <menuitem id="menu_selectAll" label="&selectAllCmd.label;" key="key_selectAll" accesskey="&selectAllCmd.accesskey;" command="cmd_selectAll"/>
>            <menuseparator/>
> +          <menuitem label="&findBarCmd.label;"      key="key_find"      accesskey="&findBarCmd.accesskey;"      command="cmd_find"/>
> +#ifndef XP_MACOSX
> +          <menuitem label="&findReplaceCmd.label;" key="key_findReplace" accesskey="&findReplaceCmd.accesskey;" command="cmd_findReplace"/>

dito, id="menu_findReplace", attribute alignment, swap lines of key and accesskey

@@ +521,5 @@
> +          <menuitem label="&findBarCmd.label;"      key="key_find"      accesskey="&findBarCmd.accesskey;"      command="cmd_find"/>
> +#ifndef XP_MACOSX
> +          <menuitem label="&findReplaceCmd.label;" key="key_findReplace" accesskey="&findReplaceCmd.accesskey;" command="cmd_findReplace"/>
> +#else
> +          <menuitem label="&findReplaceCmd.label;" accesskey="&findReplaceCmd.accesskey;" command="cmd_findReplace"/>

dito, id, alignment

@@ +527,1 @@
>            <menuitem label="&findAgainCmd.label;" key="key_findNext" accesskey="&findAgainCmd.accesskey;" command="cmd_findNext"/>

While we're here, let's add id here, too, and do attribute alignment as a courtesy:
id="menu_findNext"

@@ +527,2 @@
>            <menuitem label="&findAgainCmd.label;" key="key_findNext" accesskey="&findAgainCmd.accesskey;" command="cmd_findNext"/>
>            <menuitem label="&findPrevCmd.label;"  key="key_findPrev" accesskey="&findPrevCmd.accesskey;"  command="cmd_findPrev"/>

dito, id="menu_findPrev", attribute alignment (courtesy)

::: mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
@@ +411,5 @@
>  ## upload notification bar to allow the user to dismiss the notification permanently.
>  stopShowingUploadingNotification.accesskey=N
>  stopShowingUploadingNotification.label=Never show this again
> +
> +replaceButton.label=Replace

Per comment 96, this must be
replaceButton.label=Replace…

@@ +413,5 @@
>  stopShowingUploadingNotification.label=Never show this again
> +
> +replaceButton.label=Replace
> +replaceButton.tooltip=Show the Find and Replace dialog
> +replaceButton.accesskey=R

Per comment 103 and comment 105, access key R is already used by From and that's too important to interfere with. In fact, all letters from "Replace" are already taken, so we have to share one access key. As I explained in comment 105, "l" looks like a good choice which will even be unique when attachment reminder bar isn't around.

replaceButton.accesskey=l

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +87,5 @@
>  <!ENTITY renameAttachmentCmd.accesskey "e">
>  <!ENTITY selectAllCmd.label "Select All">
>  <!ENTITY selectAllCmd.key "A">
>  <!ENTITY selectAllCmd.accesskey "a">
> +<!ENTITY findBarCmd.label "Find">

Pls change this to "Find…"

Out of curiosity, why do we define this twice, once here and again in editorOverlay.dtd (above)?

::: mail/themes/linux/mail/compose/messengercompose.css
@@ +845,5 @@
>    visibility: visible;
>  }
> +
> +.findbar-replaceButton {
> +  border-radius: 10000px;

10000px? (just wondering, might be right)

Comment 110

[Thomas D. (:thomas8)]

For better readability, coherence and easier localization, pls ensure that whereever your patch has accesskeys, they should directly follow after the respective label attribute to which that access key will apply. Same also for string files, pls re-arrange them to have .label directly before the respective .accesskey which applies to that label. I did not catch all of these in my last review. Setting needinfo for confirmation.

Comment 111

[Suyash Agarwal (:sshagarwal)]

(In reply to Thomas D. from comment #109)
> Comment on attachment 8403792 [details] [diff] [review]
> Patch v7.5 refreshed

> Pls move label attribute into new line and align with id attribute.
Sure, will do.

> @@ +16,5 @@
> >  <!ENTITY pasteAsQuotationCmd.label "Paste As Quotation">
> >  <!ENTITY pasteAsQuotationCmd.accesskey "Q">
> >  <!ENTITY pasteAsQuotationCmd.key "o">
> > +<!ENTITY findBarCmd.label "Find">
> > +<!ENTITY findReplaceCmd.label "Find and Replace">
> 
> Pls add trailing "…" (single character) to both of these labels (as
> explained in comment 96)
But this was this way already, I haven't changed it.
And, honestly, I don't like the ellipsis we now have on the "Replace" button on the findbar, it looks as if the button is shrink and I need to expand my window to get the full button. For obvious reasons, if the user wants to find out what the button will do, he/she will hover over the button and will get the tooltip, ellipsis doesn't make any sense to me, in fact even if it does, it doesn't look nice :) 
Secondly, fine I don't decide the UI but having the same in menu, doesn't make sense at all. We do have different modules being launched from the menu items but we don't have ellipsis for them in the menus.
Please correct me if I made some wrong assumptions.

> @@ +1785,5 @@
> >   * notifications with close button in the
> >   * attachmentNotificationBox.
> >   */
> >  function handleEsc()
> >  {
> 
> The findbar part of this function should have a comment:
> What does it do, and under which exact conditions? Writing such comments
> also helps to re-analyse the exact behaviour and find problems like focus
> issue below.
Sure, will do.
> @@ +1787,5 @@
> >   */
> >  function handleEsc()
> >  {
> > +  let findbar = document.getElementById('FindToolbar');
> > +  if (!findbar.hidden) {
> 
> - Doesn't this require an explicit focus check as we have for hiding the
> attachment reminder notification (see below)? I think we had our reasons for
> positively enforcing that focus is either in body or on the bar for ESC to
> do the trick, not just anywhere. E.g., imo we don't want find bar to be
> hidden when user presses ESC with focus in contacts side bar or in
> recipients area.
Do we want the esc to work only when the focus is either in the body or on the bar?
Okay, will do that. Otherwise, the ESC is consumed by the findbar only when there are no other consumers. 

> - Does this work when pressing ESC with focus on findbar? (probably yes)
Ya, but that's already a part of findbar (the functionality). 

> @@ +2009,5 @@
> >  CheckForAttachmentNotification.shouldFire = true;
> >  
> >  function ComposeStartup(recycled, aParams)
> >  {
> > +  // Findbar overlay
> 
> We're reconstructing the entire button for every composition, which looks
> resource-eating. I wonder if this button e.g. could be parked somewhere in
> xul and then we just create a copy or another instance of that button here,
> or even move the button node? (Sorry I wouldn't know how to do any of that,
> just a thought).
That would need a binding as Seamonkey has. I found it requires writing another file (a binding file over the xbl findbar component) which I thought wasn't feasible for just a button.
Moreover, we are just constructing it once for a composition. I don't think that's such a big deal. We are also constructing attachment reminder bar for every keyword :) (Being handled in bug 938829).

> @@ +2020,5 @@
> > +    replaceButton.setAttribute("accesskey", getComposeBundle().getString("replaceButton.accesskey"));
> > +    replaceButton.setAttribute("oncommand", "findbarFindReplace();");
> > +
> > +    let findbar = document.getElementById("FindToolbar");
> > +    let lastButton = findbar.getElement('find-case-sensitive');
> 
> Here and for subsequent lines, pls use double quotes for consistency
I am planning to change all of them to single quotes, looks nice isn't it :)
Though, if you want them all to be double quotes, please re-state it, I'll do it that way.

> @@ +2022,5 @@
> > +
> > +    let findbar = document.getElementById("FindToolbar");
> > +    let lastButton = findbar.getElement('find-case-sensitive');
> > +    let tSeparator = document.createElement('toolbarseparator');
> > +    tSeparator.setAttribute("id", 'findbar-separator');
> 
> If somebody adds other separators lateron, this id won't scale well. Let's
> use more informative ID (with double quotes):
> "findbar-beforeReplaceSeparator"

I was once told that we should not think much about future. If someone plans to add something later on to this bar, they should take care of the names not me :)

> ::: mail/components/compose/content/messengercompose.xul
> @@ +517,5 @@
> >                      key="key_renameAttachment" command="cmd_renameAttachment"/>
> >            <menuseparator/>
> >            <menuitem id="menu_selectAll" label="&selectAllCmd.label;" key="key_selectAll" accesskey="&selectAllCmd.accesskey;" command="cmd_selectAll"/>
> >            <menuseparator/>
> > +          <menuitem label="&findBarCmd.label;"      key="key_find"      accesskey="&findBarCmd.accesskey;"      command="cmd_find"/>
> 
> Unless there's a reason not to (like overlays or whatever), we should add
> IDs to these menuitems following the pattern seen in the surrounding
> menuitems. We also want to align attributes in rows; key and accesskey lines
> need to be swapped so that access key goes with label:
Wrapping isn't an issue, it is as the surrounding. I don't remember why I skipped the IDs for them, 
I'll put them if there's any reason I didn't put them for.

> :::
> mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
> @@ +411,5 @@
> > +
> > +replaceButton.label=Replace
> 
> Per comment 96, this must be
> replaceButton.label=Replace…
Mentioned above.

> @@ +413,5 @@
> > +replaceButton.accesskey=R
> 
> As I explained in
> comment 105, "l" looks like a good choice which will even be unique when
> attachment reminder bar isn't around.
> 
I think I forgot to make the change, will do.

> Out of curiosity, why do we define this twice, once here and again in
> editorOverlay.dtd (above)?

Most probably suite/ 

> ::: mail/themes/linux/mail/compose/messengercompose.css
> @@ +845,5 @@
> >    visibility: visible;
> >  }
> > +
> > +.findbar-replaceButton {
> > +  border-radius: 10000px;
> 
> 10000px? (just wondering, might be right)

I don't know about this. The change is still invisible to me. Probably the ui-reviewer will throw some light.

Thanks.

Comment 112

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #111)
> (In reply to Thomas D. from comment #109)
> > Comment on attachment 8403792 [details] [diff] [review]
> > Patch v7.5 refreshed
> 
> > > +<!ENTITY findBarCmd.label "Find">
> > > +<!ENTITY findReplaceCmd.label "Find and Replace">
> > 
> > Pls add trailing "…" (single character) to both of these labels (as
> > explained in comment 96)
> But this was this way already, I haven't changed it.

That's not true, the current label has the ellipsis and you removed it:

http://mxr.mozilla.org/comm-central/source/editor/ui/locales/en-US/chrome/composer/editorOverlay.dtd#19
> <!ENTITY findCmd.label "Find and Replace…">

> And, honestly, I don't like the ellipsis we now have on the "Replace" button
> on the findbar, it looks as if the button is shrink and I need to expand my
> window to get the full button.

Who ever heard of shrinked or truncated buttons? It's not like we have an ellipsis in the middle of the button or a truncated-in-the-middle-of-the-word button label... Why should it be shrinked if theres enough space on the bar, and it's not even the last button (which is the [x] button)?

> For obvious reasons, if the user wants to
> find out what the button will do, he/she will hover over the button and will
> get the tooltip

Tooltips are very volatile and require targeted user interaction, whereas an ellipsis is is a self-explaining by tradition, always visible hint about the behaviour of an UI element, required by design guidelines. You'll be surprised how many users are not actively aware of tooltips in the sense they would use them to find out about UI element actions.

> ellipsis doesn't make any sense to me, in fact even if it
> does, it doesn't look nice :)

I'm sorry that ellipses don't agree with you, but we will not tolerate such intolerance :p

> Secondly, fine I don't decide the UI but having the same in menu, doesn't
> make sense at all. We do have different modules being launched from the menu
> items but we don't have ellipsis for them in the menus.
> Please correct me if I made some wrong assumptions.

!? We have ellipsis on menu and button labels all over the UI *where they are required* by the design guidelines (quick check I only found one prominent case where it's missing).

Please read my Comment 96 where I've already correctly explained the purpose of ellipses citing relevant examples, including Attachment Reminder Bar which is very much the same type of UI as the Find Bar we're implementing here (see your own screnshot for a family photo of the twins in attachment 8401341 [details]...):

> Attachment reminder notification bar:
> Add attachment...  (dialog -> ellipsis) vs.
> Remind me later    (immediate action -> no ellipsis)

But don't take my word for it. Take it from official OS-wide design guidelines (Apple in this case, but Windows and Linux will have similar such):

Quoting Apple Human Interface Guidelines:

https://developer.apple.com/library/mac/documentation/UserExperience/Conceptual/AppleHIGuidelines/Menus/Menus.html#//apple_ref/doc/uid/TP30000356-TPXREF117

> Use an ellipsis to show users that further action is required to complete the command. The ellipsis
> character (…) means that a dialog or a separate window will open in which users need to make
> additional choices or supply additional information in order to complete the action. For details on
> when to use an ellipsis in menu items, see “Using the Ellipsis Character.”

https://developer.apple.com/library/mac/documentation/UserExperience/Conceptual/AppleHIGuidelines/TextStyle/TextStyle.html#//apple_ref/doc/uid/TP30000365-TPXREF126

> Using the Ellipsis Character

> When it appears in the name of a button or a menu item, an ellipsis character (…) indicates to the
> user that additional information is required before the associated operation can be performed.
> Specifically, it prepares the user to expect the appearance of a window or dialog in which to make
> selections or enter information before the command executes.
> 
> Because users expect instant action from buttons and menu items (as described in “Buttons” and “Menu
> Appearance and Behavior”), it's especially important to prepare them for this alternate behavior by
> appropriately displaying the ellipsis character. The following guidelines and examples will help you
> decide when to use an ellipsis in menu item and button names.

If you're interested, there's more information on that page exactly when to use ellipses or not... ;)





> > @@ +1785,5 @@
> > >   * notifications with close button in the
> > >   * attachmentNotificationBox.
> > >   */
> > >  function handleEsc()
> > >  {
> > 
> > The findbar part of this function should have a comment:
> > What does it do, and under which exact conditions? Writing such comments
> > also helps to re-analyse the exact behaviour and find problems like focus
> > issue below.
> Sure, will do.
> > @@ +1787,5 @@
> > >   */
> > >  function handleEsc()
> > >  {
> > > +  let findbar = document.getElementById('FindToolbar');
> > > +  if (!findbar.hidden) {
> > 
> > - Doesn't this require an explicit focus check as we have for hiding the
> > attachment reminder notification (see below)? I think we had our reasons for
> > positively enforcing that focus is either in body or on the bar for ESC to
> > do the trick, not just anywhere. E.g., imo we don't want find bar to be
> > hidden when user presses ESC with focus in contacts side bar or in
> > recipients area.
> Do we want the esc to work only when the focus is either in the body or on
> the bar?
> Okay, will do that. Otherwise, the ESC is consumed by the findbar only when
> there are no other consumers. 
> 
> > - Does this work when pressing ESC with focus on findbar? (probably yes)
> Ya, but that's already a part of findbar (the functionality). 
> 
> > @@ +2009,5 @@
> > >  CheckForAttachmentNotification.shouldFire = true;
> > >  
> > >  function ComposeStartup(recycled, aParams)
> > >  {
> > > +  // Findbar overlay
> > 
> > We're reconstructing the entire button for every composition, 
> That would need a binding as Seamonkey has. I found it requires writing
> another file (a binding file over the xbl findbar component) which I thought
> wasn't feasible for just a button.

That's OK with me.

> I am planning to change all of them to single quotes, looks nice isn't it :)
> Though, if you want them all to be double quotes, please re-state it, I'll
> do it that way.

I don't think it should be random. Afasik double quotes is the norm, and single quotes only used where it's unavoidable, e.g. inside double quotes.

> > If somebody adds other separators lateron, this id won't scale well. Let's
> > use more informative ID (with double quotes):
> > "findbar-beforeReplaceSeparator"
> 
> I was once told that we should not think much about future. If someone plans
> to add something later on to this bar, they should take care of the names
> not me :)

Sorry Sir, since you're implementing new stuff here, it's you who's responsible to make this part of the code as future-proof as possible, which includes adding IDs which have some descriptive value and are consistent in naming with other such IDs. We'll not be perfect, but identified shortcomings should be fixed along my proposal. Descriptive IDs also help with understanding the code, and we try to keep them stable for potential addon usage.

> > ::: mail/components/compose/content/messengercompose.xul
> > @@ +517,5 @@
> > >                      key="key_renameAttachment" command="cmd_renameAttachment"/>
> > >            <menuseparator/>
> > >            <menuitem id="menu_selectAll" label="&selectAllCmd.label;" key="key_selectAll" accesskey="&selectAllCmd.accesskey;" command="cmd_selectAll"/>
> > >            <menuseparator/>
> > > +          <menuitem label="&findBarCmd.label;"      key="key_find"      accesskey="&findBarCmd.accesskey;"      command="cmd_find"/>
> > 
> > Unless there's a reason not to (like overlays or whatever), we should add
> > IDs to these menuitems following the pattern seen in the surrounding
> > menuitems. We also want to align attributes in rows; key and accesskey lines
> > need to be swapped so that access key goes with label:
> Wrapping isn't an issue, it is as the surrounding.

Well it's an issue now because a reviewer made it an issue. The surrounding code has all sorts of attribute wrapping variants, horizontal, vertical, mixture of both, and we're not here to copy bad style. Unless you show me the design document recommending horizontal, it's vertical, which is also what I see in most parts of this file. I've recently been told to wrap even a comment to 80 chars; the horizontal menuitems here already have 127 chars without ID. Horizontal is also harder to maintain as you'll keep changing all items if there's some longer attribute added somewhere. Label and access key should go together for obvious reasons, so pls do all the changes here exactly as proposed in my review including adding IDs for the findnext menuitems etc.

> I don't remember why I
> skipped the IDs for them, 
> I'll put them if there's any reason I didn't put them for.

probably you meant if = unless;
pls do put them.

> > :::
> > mail/locales/en-US/chrome/messenger/messengercompose/composeMsgs.properties
> > @@ +411,5 @@
> > > +
> > > +replaceButton.label=Replace
> > 
> > Per comment 96, this must be
> > replaceButton.label=Replace…
> Mentioned above.

Refuted above :)

> > > +.findbar-replaceButton {
> > > +  border-radius: 10000px;
> > 
> > 10000px? (just wondering, might be right)
> 
> I don't know about this. The change is still invisible to me. Probably the
> ui-reviewer will throw some light.

Let's keep an eye on this, 10000px really looks wrong (and I tried it somewhere online and got something like a 4cm rounded corner, which seems unlikely for our button here).

Comment 113

[Thomas D. (:thomas8)]

FTR, Apple HIG references updated from Ben Bucksch's Bug 412387 Comment 9 where I found them.

Comment 114

[Magnus Melin [:mkmelin]]

Comment on attachment 8403792 [details] [diff] [review]
Patch v7.5 refreshed

Review of attachment 8403792 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/ui/composer/content/ComposerCommands.js
@@ +2260,5 @@
> +        window.openDialog("chrome://editor/content/EdReplace.xul", "_blank",
> +                          "chrome,modal,titlebar", editorElement);
> +    }
> +    catch(ex) {
> +      dump("*** Exception: couldn't open Replace Dialog\n");

why? this try catch is not useful at all. let it blow up if it can't open the dialog

@@ +2284,3 @@
>      }
>      catch(ex) {
> +      dump("*** Exception: couldn't open Find Toolbar\n");

also useless

@@ +2309,2 @@
>      }
>      catch (ex) {}

useless try/catch, yes, it's really common in editor :(

Comment 115

[Suyash Agarwal (:sshagarwal)]

Are we allowed to make changes in editor/ ?

Comment 116

[Magnus Melin [:mkmelin]]

Needs an editor/ peer review. I think that's basically Neil.

Comment 117

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v7.7

(In reply to Thomas D. from comment #112)
Okay done :)

Requesting review from Neil due to changes in editor/ code (Comment #114).

Thanks.

Comment 118

[Thomas D. (:thomas8)]

Comment on attachment 8404396 [details] [diff] [review]
Patch v7.7

Review of attachment 8404396 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you for the new patch! Much neater.

::: editor/ui/composer/content/ComposerCommands.js
@@ -2286,5 @@
> -      var findService = Components.classes["@mozilla.org/find/find_service;1"]
> -                                  .getService(Components.interfaces.nsIFindService);
> -      findInst.findBackwards = findService.findBackwards ^ findPrev;
> -      findInst.findNext();
> -      // reset to what it was in dialog, otherwise dialog setting can get reversed

What was the purpose of this in the old code, and can we really just remove it?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +1789,5 @@
>  {
> +  // If findbar is visible and the focus is in the message body,
> +  // hide it. (Focus on the findbar is handled by findbar itself).
> +  let findbar = document.getElementById('FindToolbar');
> +  if (!findbar.hidden) {

Does this ensure focus is in body? I think not.
Imho, as mentioned before, ESC should NOT close findbar when focus is in contacts sidebar or recipients area.
- contacts sidebar and recipients area have no direct relationship with the main body of composition to which the findbar belongs; I don't think users will expect to close findbar while focus is inside these
- it's much more likely that users press ESC in contacts sidebar or recipients area for other reasons (ESC from address search or autocomplete); if they happen to press ESC once too much, we don't want unrelated findbar to go away (ux-error-prevention)
- we do not accept ESC from contacts sidebar or recipients to close attachment reminder bar (but since we now check subjects, ESC should be allowed just within the subject field to do that)

::: mail/components/compose/content/messengercompose.xul
@@ +517,5 @@
>                      key="key_renameAttachment" command="cmd_renameAttachment"/>
>            <menuseparator/>
>            <menuitem id="menu_selectAll" label="&selectAllCmd.label;" key="key_selectAll" accesskey="&selectAllCmd.accesskey;" command="cmd_selectAll"/>
>            <menuseparator/>
> +          <menuitem id="menu_findBar"

As I proposed, this should be id="menu_find" to be consistent with the command name (as done in all other ids).

@@ +520,5 @@
>            <menuseparator/>
> +          <menuitem id="menu_findBar"
> +                    label="&findBarCmd.label;"
> +                    key="key_find"
> +                    accesskey="&findBarCmd.accesskey;"

For this and all subsequent menuitems, as stated before, pls ensure that accesskey follows on label because they belong together (swap key and accesskey lines):

label=...
accesskey=...
key=...

Comment 119

[[e-mail]]

Comment on attachment 8404396 [details] [diff] [review]
Patch v7.7

Good catch on the dropped ellipsis.

Comment 120

[Suyash Agarwal (:sshagarwal)]

Attachment: Screenshot 7: Odd menu effect when "menu_find" is used as ID

(In reply to Thomas D. from comment #118)
> ::: mail/components/compose/content/messengercompose.xul
> @@ +517,5 @@
> >                      key="key_renameAttachment" command="cmd_renameAttachment"/>
> >            <menuseparator/>
> >            <menuitem id="menu_selectAll" label="&selectAllCmd.label;" key="key_selectAll" accesskey="&selectAllCmd.accesskey;" command="cmd_selectAll"/>
> >            <menuseparator/>
> > +          <menuitem id="menu_findBar"
> 
> As I proposed, this should be id="menu_find" to be consistent with the
> command name (as done in all other ids).

Using "menu_find" introduces another an hbox with class "menu-right".

Comment 121

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #120)
> Created attachment 8405283 [details]
> Screenshot from 2014-04-11 15:24:10.png
> > > +          <menuitem id="menu_findBar"
> > 
> > As I proposed, this should be id="menu_find" to be consistent with the
> > command name (as done in all other ids).
> 
> Using "menu_find" introduces another an hbox with class "menu-right".

That's weird, and I think it shouldn't be like that. I see no reason why Find menu should get any special formatting based on the ID being "menu_find". But well, if the culprit for that cannot be identified (or there's some other reason I'm not aware of), I suppose it's ok then to use id="menu_findBar" to work around this problem.

menu_find on comm-central:
http://mxr.mozilla.org/comm-central/search?string=menu_find&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Could it be this? http://mxr.mozilla.org/comm-central/source/mail/themes/linux/mail/messenger.css#250
> 250 #menu_find {
> 251   -moz-binding: url("chrome://global/content/bindings/menu.xml#menu-iconic");
> 252   list-style-image: url("moz-icon://stock/gtk-find?size=menu");

In general, if there's a reason not to realize a review comment, pls comment immediately with the subsequent patch so that reviewers are aware of it and don't have to nag for the the same thing twice :)

Comment 122

[Magnus Melin [:mkmelin]]

Comment on attachment 8404396 [details] [diff] [review]
Patch v7.7

Review of attachment 8404396 [details] [diff] [review]:
-----------------------------------------------------------------

::: editor/ui/composer/content/editorOverlay.xul
@@ +309,5 @@
>        <menuseparator id="sep_selectAll"/>
>        <menuitem id="menu_selectAll"/>
>        <menuseparator id="sep_find"/>
> +      <menuitem id="menu_find"
> +                label="&findBarCmd.label;"/>

no accesskey?

@@ +311,5 @@
>        <menuseparator id="sep_find"/>
> +      <menuitem id="menu_find"
> +                label="&findBarCmd.label;"/>
> +      <menuitem id="menu_findReplace"
> +                label="&findReplaceCmd.label;"/>

no accesskey?

::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
@@ +95,2 @@
>  <!ENTITY findReplaceCmd.key "H">
> +<!ENTITY findReplaceCmd.accesskey "l">

please put accesskey below label as they belong together.

Comment 123

[Suyash Agarwal (:sshagarwal)]

Comment on attachment 8404396 [details] [diff] [review]
Patch v7.7

(In reply to Magnus Melin from comment #122)
> Comment on attachment 8404396 [details] [diff] [review]
> Patch v7.7
> ::: editor/ui/composer/content/editorOverlay.xul
> @@ +309,5 @@
> > +      <menuitem id="menu_find"
> > +                label="&findBarCmd.label;"/>
> 
> no accesskey?
> 
> @@ +311,5 @@
> > +      <menuitem id="menu_findReplace"
> > +                label="&findReplaceCmd.label;"/>
> 
> no accesskey?
Aren't these both part of composer? and will be used in suite/ ? and suite/ has its accesskeys in overlays?

> 
> ::: mail/locales/en-US/chrome/messenger/messengercompose/messengercompose.dtd
> @@ +95,2 @@
> >  <!ENTITY findReplaceCmd.key "H">
> > +<!ENTITY findReplaceCmd.accesskey "l">
> 
> please put accesskey below label as they belong together.

Will do, thanks.

And, can you please provide the mail/ review too so that I can update the patch in one go?

Comment 124

[Magnus Melin [:mkmelin]]

(In reply to Suyash Agarwal (:sshagarwal) from comment #123)
> And, can you please provide the mail/ review too so that I can update the
> patch in one go?

I don't have any other comments atm, but the patch has unfortunately bitrotted.

Comment 125

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v8

Addressing unattended comments from comment #118.

(In reply to Thomas D. from comment #118)
> Comment on attachment 8404396 [details] [diff] [review]
> Patch v7.7
> @@ -2286,5 @@
> > -      findInst.findBackwards = findService.findBackwards ^ findPrev;
> > -      findInst.findNext();
> > -      // reset to what it was in dialog, otherwise dialog setting can get reversed
> What was the purpose of this in the old code, and can we really just remove
> it?
Not much sure, but it wasn't doing anything and removing it had no adverse effects :)
> @@ +1789,5 @@
> >  {
> > +  // If findbar is visible and the focus is in the message body,
> > +  // hide it. (Focus on the findbar is handled by findbar itself).
> Does this ensure focus is in body? I think not.
Handled.
> @@ +517,5 @@
> > +          <menuitem id="menu_findBar"
> As I proposed, this should be id="menu_find" to be consistent with the
> command name (as done in all other ids).
You seem to be satisfied :)
> @@ +520,5 @@
> For this and all subsequent menuitems, as stated before, pls ensure that
> accesskey follows on label because they belong together (swap key and
> accesskey lines):
Handled.

Requesting ui-r from Paenglab too with the thoughts:
I think we need to verify the look on Windows machines.

Thanks.

Comment 126

[Richard Marti (:Paenglab)]

Comment on attachment 8406670 [details] [diff] [review]
Patch v8

Review of attachment 8406670 [details] [diff] [review]:
-----------------------------------------------------------------

ui-r+ for Windows and Linux, but the findbar-beforeReplaceSeparator shouldn't be 35px tall. This let's grow the findbar unneeded for 9px on Windows and Linux. I propose to use 26px to not let the findbar grow.
Only Windows needs also this rule to make the replace button looking good:

#findbar-replaceButton > .toolbarbutton-icon {
  display: none;
}

Without it the button has a 5px wider margin on the left to the button text.

Comment 127

[Richard Marti (:Paenglab)]

Comment on attachment 8406670 [details] [diff] [review]
Patch v8

Review of attachment 8406670 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, forgot this:

::: mail/themes/linux/mail/compose/messengercompose.css
@@ +844,5 @@
>       menus shouldn't have icons. */
>    visibility: visible;
>  }
> +
> +.findbar-replaceButton {

I see no use of this rule. Please can you remove it. Also in osx and windows.

Comment 128

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch v8.1

Made the suggested changes, thanks.

Comment 129

[Thomas D. (:thomas8)]

Comment on attachment 8407399 [details] [diff] [review]
Patch v8.1

Review of attachment 8407399 [details] [diff] [review]:
-----------------------------------------------------------------

This looks like a checkin-candidate to me. :)

Comment 130

[Thomas D. (:thomas8)]

(In reply to Suyash Agarwal (:sshagarwal) from comment #125)
> Created attachment 8406670 [details] [diff] [review]
> Patch v8
> 
> Addressing unattended comments from comment #118.

Thanks!

> (In reply to Thomas D. from comment #118)
> > Comment on attachment 8404396 [details] [diff] [review]
> > Patch v7.7
> > @@ -2286,5 @@
> > > -      findInst.findBackwards = findService.findBackwards ^ findPrev;
> > > -      findInst.findNext();
> > > -      // reset to what it was in dialog, otherwise dialog setting can get reversed
> > What was the purpose of this in the old code, and can we really just remove
> > it?
> Not much sure, but it wasn't doing anything and removing it had no adverse
> effects :)

Magnus, do you know what this was doing?
I feel uneasy when we remove things without fully understanding their original purpose.

Comment 131

[Thomas D. (:thomas8)]

Suyash, one nit:

The checkin comment needs to be changed because the "Replace..." button isn't optional.

> Bug 530629 - Ctrl+F when composing should just show find bar as usual (optionally with
> "Find and Replace..." button).

Bug 530629 - For composition, implement find bar (Ctrl+F) with "Replace..." button.

Comment 132

[Magnus Melin [:mkmelin]]

Comment on attachment 8407399 [details] [diff] [review]
Patch v8.1

Review of attachment 8407399 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! r=mkmelin

Comment 133

[Thomas D. (:thomas8)]

Last polishing nits:

Pls change checkin comment per comment 131:

> Bug 530629 - For composition, implement find bar (Ctrl+F) with "Replace..." button.

> +    replaceButton.setAttribute("label", getComposeBundle().getString("replaceButton.label"));
> +    replaceButton.setAttribute("tooltiptext", getComposeBundle().getString("replaceButton.tooltip"));
> +    replaceButton.setAttribute("accesskey", getComposeBundle().getString("replaceButton.accesskey"));

Swap last 2 lines so that "accesskey" follows immediately after "label".

> <!ENTITY findPrevCmd.label "Find Previous">
> <!ENTITY findPrevCmd.key "G">
> <!ENTITY findPrevCmd.key2 "VK_F3">
> <!ENTITY findPrevCmd.accesskey "v">

As mentioned before, although you didn't add this code, pls move .accesskey to follow after .label (as you already did with .accesskey for findAgainCmd).

Comment 134

[Thomas D. (:thomas8)]

Josiah, current patch here has all the reviews including paenglab's ui-review+. So this will be ready for landing as soon as you'll add your ui-r+ for MAC OS... :) Tia.

Suyash, pls address last nits of comment 133 before check-in. Tia.

Comment 135

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #130)
> [snip]
> Magnus, do you know what this was doing?
> I feel uneasy when we remove things without fully understanding their
> original purpose.

^^

Comment 136

[Josiah Bruner [:jsbruner]]

Comment on attachment 8407399 [details] [diff] [review]
Patch v8.1

Review of attachment 8407399 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good, so ui-r+ when you address one nit.

::: mail/themes/shared/mail/messenger.css
@@ +89,5 @@
>    to { background-color: transparent; }
>  }
>  
> +#findbar-beforeReplaceSeparator {
> +  height: 26px;

Please make this 16px for now. At least on OS X, it extends the height of the entire bar.

I'll do some trickier theme stuff in a separate bug to extend the height, but you don't need to worry about that now.

Comment 137

[Thomas D. (:thomas8)]

You guys are so gorgeous, I love the cooperation and speed of this...

(In reply to Josiah Bruner [:JosiahOne] from comment #136)
> Review of attachment 8407399 [details] [diff] [review]:
> -----------------------------------------------------------------
> This looks good, so ui-r+ when you address one nit.
> Please make this 16px for now. At least on OS X, it extends the height of
> the entire bar.

Good catch, thanks, even on Screenshot v6 attachment 8402382 [details] the bar looks unnecessarily high, pls let's be as vertical-space-efficient as possible even on Windows and Linux, given that composition already eats such a lot of space for the header in its current design (and I'm so much looking forward to Josiah changing it to single-line headers for each of To, CC, BCC in that other bug; would be great to land that for TB31 but will be a tight race...).

> I'll do some trickier theme stuff in a separate bug to extend the height,
> but you don't need to worry about that now.

Not sure if I understand that plan, but I hope it won't waste vertical pixels of message body for find bar eye-candy. Anyway, I'm curious so pls CC me on that bug and mark dependency here :)

Comment 138

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. from comment #135)
> (In reply to Thomas D. from comment #130)
> > [snip]
> > Magnus, do you know what this was doing?

Apparently keeping track of backward/forward finding, but I don't know for what purpose.

Comment 139

[Thomas D. (:thomas8)]

If possible, screenshot of that 16px variant would be nice, too...

Comment 140

[Suyash Agarwal (:sshagarwal)]

Attachment: Patch ready for check-in

Carrying over review from Neil, mkmelin and ui-r from JosiahOne and Paenglab.

Also requires patch from aceman. To be checked-in m-c.

Thanks.

Comment 141

[Thomas D. (:thomas8)]

Great!

Comment 142

[Thomas D. (:thomas8)]

Oh, and I think we'd want a followup bug to include Find bar into the fast track Ctrl+tab/F6 tab cycle when it's shown (and also contacts side bar, Bug 682424 which has ui-r+ already), in the same way we have already included composition's attachment pane (Bug 473901).

Comment 143

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/fd84c515f4c8

Comment 144

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ce418853a12

Comment 145

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0ce418853a12

Comment 146

[Thomas D. (:thomas8)]

Awesome! Just saw this in action first time.

I'm a bit surprised that this doesn't look as defined here on WinXP. This is what I see:

[find input] [^][v] Highlight all  Match case | Replace... (Find feedback)

Iow, all buttons are left-aligned whereas per our design, starting from "Highlight all" they should be right-aligned. The unfortunate side effect is that the Find feedback text is no longer adjacent to the find input. It's not all that bad, but for the design reasons explained in my Comment 89, I think the design we agreed upon is better.

:paenglab, any idea why this happens and how it could be changed so that those buttons end up on the right side of the toolbar?

Comment 147

[Richard Marti (:Paenglab)]

This is because bug 935519 moved the buttons back to the left again.

Comment 148

[:aceman]

Congrats to Suyash and Thomas for getting this in:) Looks very nice in current trunk.

Comment 149

[Thomas D. (:thomas8)]

Given "268 bugs found" for Toolkit's find toolbar, it works amazingly well...

https://bugzilla.mozilla.org/buglist.cgi?component=Find%20Toolbar&product=Toolkit&bug_status=__open__&list_id=9940176

Suyash, btw, implementing find bar history dropdown with "clear find history" option is bug 256802.

Comment 150

[Thomas D. (:thomas8)]

Composition find bar is a nifty new feature which makes everyday searches in composition so much lighter, more efficient, and hassle-free (and also adds "highlight all") - this definitely deserves mentioning in release notes.