398531 - Find and Replace dialogue: Pressing Enter key with focus outside buttons unexpectedly closes the dialogue: can't continue to "Find next"

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[prasad]

User-Agent:       Mozilla/4.0 (compatible; MSIE 6.0; Windows NT 5.1; SV1)
Build Identifier: Version 2.0.0.4(20070828)

Find and Replace window gets closed with user input Enter key,i was expecting the dialog to be on to search for other occurances

Reproducible: Always

Steps to Reproduce:
1.launch thunderbird
2.write a new mail - click on File->New->Message
3.Input some repetitive text in the Mail body(eg: test test test)
4.Place the Cursor at the beginning of the text
5.Click Edit->Find and Replace or ctr+f
6.Find and Replace dialog: input "test" in Find box
7.Press Enter key
Actual Results:  
Only first occurance is shown and the Find and Replace dialog gets closed

Expected Results:  
Find and Replace dialog should not get closed
Workaround noticed:
this can be seen using the Tab button to bring the focus on Find button and pressing Enter key will run through all the search string occurances

Pressing Enter key as described in Steps above is expected to show the first occurance and the focus is expected on "Find Next" command button instead of closure of the dialog

Comment 1

[Magnus Melin [:mkmelin]]

Confirmed on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre) Gecko/2007100303 Thunderbird/3.0a1pre ID:2007100303

Comment 2

[prasad]

(In reply to comment #1)
> Confirmed on Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9a9pre)
> Gecko/2007100303 Thunderbird/3.0a1pre ID:2007100303

Reproducible on solaris nevada - Mozilla Thunderbird 
Version 2.0.0.4(20070828)

Comment 3

[Wayne Mery (:wsmwk)]

still see this on trunk.
Find button works repetitively if you tab to it first

Comment 4

[Thomas D. (:thomas8)]

...and on the branch, too
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.5) Gecko/20091116 Lightning/1.0pre Shredder/3.0.1pre

Comment 5

[Thomas D. (:thomas8)]

This might be a simple fix.

Comment 6

[osource12345]

Hi, I would like to fix this bug, I think I have already fixed it.

Comment 7

[Magnus Melin [:mkmelin]]

Hello Andreas, please attach your patch! 
https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed

Comment 8

[osource12345]

Attachment: patch file

I wasn't sure if I need to write tests as well (e.g. mozmill) ?

Comment 9

[Magnus Melin [:mkmelin]]

Comment on attachment 8517477 [details] [diff] [review]
patch file

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

Thx for the patch! I didn't yet try it, but I have some nits and questions:

::: editor/ui/dialogs/content/EdReplace.js
@@ +126,5 @@
>      gReplaceDialog.findInput.select();
>      gReplaceDialog.findInput.focus();
>      return false;
>    } 
> +  

please remove the added trailing whitespace here

@@ +128,5 @@
>      return false;
>    } 
> +  
> +  SetTextboxFocus(gReplaceDialog.findNext);
> +  gReplaceDialog.findNext.focus();

these both do the same thing, more or less. why both?

@@ +222,5 @@
>  
>    return true;
>  }
>  
> +function onReplaceAndFind()

I know the IDs are replaceAndFind, but onFindAndReplace would be a nicer name IMO

@@ +226,5 @@
> +function onReplaceAndFind()
> +{
> +  onReplace();
> +  var result = onFindNext();
> +  if(result) {

space after if please

Comment 10

[osource12345]

Attachment: 398531.patch

Hello Magnus, thx for your comments!

I have corrected the formatting (removed whitespace, added space).

I think the name "onReplaceAndFind" is more intuitive, because thats what is written on the button and what the function does, it first replaces then finds the next match. However, if you insist I will change it to onFindAndReplace.

In the function onFindNext I am using the two similar calls SetTextBoxFocus(...) and gReplaceDialog.findNext.focus(), because I think the latter does not work as expected, c.f. the comment in SetTextBoxFocus (http://mxr.mozilla.org/comm-central/source/editor/ui/dialogs/content/EdDialogCommon.js#173) and related Bug 103197. Then again once this is fixed, the second call (...focus()) should be the right one to use, so I still included it. Also just above the same is done for gReplaceDialog.findInput and I simply adopted this.

Comment 11

[Magnus Melin [:mkmelin]]

Comment on attachment 8518487 [details] [diff] [review]
398531.patch

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

Well, I can confirm it works, but I do get this in the console
JavaScript error: chrome://editor/content/EdDialogCommon.js, line 179: TypeError: textbox.select is not a function

(the textbox is a XULElement FWIW.)

Bouncing this review to Neil. (I'm not an /editor peer)

Comment 12

[[e-mail]]

Comment on attachment 8518487 [details] [diff] [review]
398531.patch

>-   ondialogaccept="return onFindNext();">
>+   ondialogaccept="onFindNext(); return false">
I understand this change because it fixes the bug, but what I don't understand is what the point of the rest of the changes are.

Comment 13

[osource12345]

From the description:

> Pressing Enter key as described in Steps above is expected to show the first
> occurance and the focus is expected on "Find Next" command button instead of
> closure of the dialog

The "rest of the changes" you mention, are needed to set the focus to the "Find Next" button.
Simply setting this focus in "onFindNext()" breaks the correct focus for the "Replace and Find" button, hence the new function "onReplaceAndFind()", which keeps the focus on the "Replace and Find" button as long as further matches are found. If there is no further match, the focus always returns to the findInput Textfield.

Comment 14

[osource12345]

> Well, I can confirm it works, but I do get this in the console
> JavaScript error: chrome://editor/content/EdDialogCommon.js, line 179:
> TypeError: textbox.select is not a function

The reason for this is simply that .focus() is called on the button element in the function setTextBoxFocus, which expects a TextBox. I think one could either write a similar function to setTextBoxFocus or another fix is needed, because the .focus() function does not work as expected right now, as I already explained.

Comment 15

[[e-mail]]

(In reply to osource12345 from comment #13)
> setting this focus in "onFindNext()"
But why?

Comment 16

[osource12345]

> > setting this focus in "onFindNext()"
> But why?

Well, in the description it was asked that the focus should be on the "Find Next" Button after hitting enter, this was not my idea. However, I might agree with you that this does not provide any functionality since hitting enter again (after the first match) calls "onFindNext()" anyway.

Comment 17

[[e-mail]]

(In reply to osource12345 from comment #16)
> > > setting this focus in "onFindNext()"
> > But why?
> Well, in the description it was asked that the focus should be on the "Find
> Next" Button after hitting enter
That's not how I read it; the description just says that a workaround is to manually focus the find next button before hitting enter.

Comment 18

[osource12345]

The last sentence in the description reads:

> Pressing Enter key as described in Steps above is expected to show the first
> occurance and the focus is expected on "Find Next" command button instead of
> closure of the dialog

To me this is sounds pretty clear. However, I can also leave the focus as it is and simply not close the dialogue. Please let me know what I should do.

Comment 19

[Thomas D. (:thomas8)]

Hi osource, thanks for fixing this small yet highly annoying bug.

(In reply to osource12345 from comment #18)
> The last sentence in the description reads:
> 
> > Pressing Enter key as described in Steps above is expected to show the first
> > occurance and the focus is expected on "Find Next" command button instead of
> > closure of the dialog
> 
> To me this is sounds pretty clear. However, I can also leave the focus as it
> is and simply not close the dialogue. Please let me know what I should do.

Definitely pls leave the focus in the inputboxes or whereever it happens to be when pressing enter.
So when Enter is pressed with focus in Find (or Replace) input box, focus remains there.

Reasons:

1) Find button is default button (and indicated as such), and I believe the customary ux principles for that scenario are exactly that the focus can stay outside the button but per default button setting you can still press it "remotely" by pressing Enter without leaving your current focus.
2) Which is useful behaviour, because it makes changing the search/replace text much easier as the focus is still there, e.g. when you've mistyped the search word, which is a likely scenario.
3) Compared to that, adding explicit focus ring to a default button is much less useful if it's useful at all.
4) UX-consistency with Quick Find Bar, where pressing enter after typing search word also keeps focus in input box

(Fyi, I'm one of TB's key UX contributors so you can just go ahead per my advice ;)

Comment 20

[[e-mail]]

(In reply to osource12345 from comment #18)
> The last sentence in the description reads:
> 
> > Pressing Enter key as described in Steps above is expected to show the first
> > occurance and the focus is expected on "Find Next" command button instead of
> > closure of the dialog

Huh,I missed that completely.

Comment 21

[[e-mail]]

Comment on attachment 8518487 [details] [diff] [review]
398531.patch

OK, so f+ for the actual fix, without messing with focus.

Comment 22

[Thomas D. (:thomas8)]

osource, as comment 21 confirms my comment 19, can you provide a new patch which doesn't change focus when Enter is pressed on most elements of the dialogue, e.g. in the text input boxes?

Comment 23

[osource12345]

hello, yes I will as soon as I have time

Comment 24

[osource12345]

also thx for your help and opinion

Comment 25

[osource12345]

Attachment: 398531.patch

Comment 26

[Thomas D. (:thomas8)]

Comment on attachment 8527281 [details] [diff] [review]
398531.patch

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

Yeah, this does one-liner does the trick in my tests.

r=Neil per his comment 21 (and comment 12).
ui-r=ThomasD (per comment 19).

Comment 27

[Thomas D. (:thomas8)]

r=Neil,ui-r=ThomasD

(In reply to Thomas D. from comment #26)
> r=Neil per his comment 21 (and comment 12).
> ui-r=ThomasD (per comment 19).

Comment 28

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/6d2e86b2e141