621944 - "Find and Replace" wrap around in compose screen does not work

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[raoul bhatia]

User-Agent:       Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:2.0b9pre) Gecko/20101228 Firefox/4.0b9pre
Build Identifier: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b9pre) Gecko/20101229 Thunderbird/3.3a2pre

wrap around find/replace seems to be broken for the compose mail windows

Reproducible: Always

Steps to Reproduce:
1. create a new message and c/p the following rows into the body
> ipax
> raoul bhatia
> ipax

2. position your curor at the beginning of line 1 (ctrl + pos1)

3. press ctrl +f to open "Find and Replace". Enter ipax and make sure that "Wrap around" is checked

4. press Find Next a couple of times
Actual Results:  
no wrap around happens

Expected Results:  
expected: search wraps around

Comment 1

[Ludovic Hirlimann [:Usul]]

Raoul did this use to work before ?

Comment 2

[raoul bhatia]

actually, i do not know if this has worked before.
i just tested it:

does *not* work for:
* thunderbird 3.1.7+build3+nobinonly-0ubuntu0.10.04.1 (ubuntu linux)
* thunderbird 3.1.8~hg20101223r5918+nobinonly-0ubuntu1~umd1~maverick (ubuntu linux)


i cannot tell for tb 3.0 because i haven't got an installation anymore.

cheers,
raoul

Comment 4

[Wayne Mery (:wsmwk)]

confirmed. 
but perhaps it is core related?

Comment 5

[Wayne Mery (:wsmwk)]

works fine both plain text and html in version 2 = regression. 

current version fails both plain text and html

didn't test tbird 3.0
no report of this found on getsatisfaction

Comment 6

[(no longer active)]

This is not a core bug.

Comment 8

[Alice0775 White]

I can reproduce in Tb3.1.20 - Daily18.0a1, but not Tb3.0.11.

Comment 9

[Alice0775 White]

Regression window

Good:
http://hg.mozilla.org/mozilla-central/rev/90d3e6d2cbb9
http://hg.mozilla.org/comm-central/rev/f1fcff5eb061
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090610 Thunderbird/3.1a1pre ID:20090610031946

Bad:
http://hg.mozilla.org/mozilla-central/rev/4430cae50dad
http://hg.mozilla.org/comm-central/rev/6c5a94b9bba2
Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090611 Thunderbird/3.1a1pre ID:20090611045744

Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=90d3e6d2cbb9&tochange=4430cae50dad
http://hg.mozilla.org/comm-central/pushloghtml?fromchange=f1fcff5eb061&tochange=6c5a94b9bba2

Comment 10

[Alice0775 White]

In local build:
Last Good:
http://hg.mozilla.org/mozilla-central/rev/771509552a2e
http://hg.mozilla.org/comm-central/rev/6c5a94b9bba2
First Bad:
http://hg.mozilla.org/mozilla-central/rev/cabb8925dcd3
http://hg.mozilla.org/comm-central/rev/6c5a94b9bba2

Regressed by:
http://hg.mozilla.org/mozilla-central/rev/cabb8925dcd3	Neil Deakin -— Bug 178324, refactor focus by moving all focus handling into one place and simplifying it, add many tests, fixes many other bugs too numerous to mention in this small checkin comment, r=josh,smichaud,ere,dbaron,marco,neil,gavin,smaug,sr=smaug (CLOSED TREE)

Comment 11

[raoul bhatia]

thanks for tracking it down!

Comment 12

[Neil Deakin]

Attachment: testcase

Debugging this shows that the find code is iterating into some other frame, but that frame has no presshell so an early return happens here:

http://mxr.mozilla.org/mozilla-central/source/embedding/components/find/src/nsWebBrowserFind.cpp#797

Thunderbird likely has some hidden iframe appearing after the editing window.

The attached testcase can be used to reproduce in firefox.

Comment 13

[Neil Deakin]

The issue here is that find after reaching the end of the page moves on and starts searching the chrome, in this case failing when reaching the hidden customize toolbar frame.

It should just be constrained to the content page.

Comment 14

[Neil Deakin]

Attachment: patch

This patches fixes the thunderbird issue I think, but not finding in general, since frames aren't really handled very well by this code.

Comment 15

[[e-mail]]

Comment on attachment 661307 [details] [diff] [review]
patch

Seems reasonable from code inspection.

Comment 16

[Neil Deakin]

Comment on attachment 661307 [details] [diff] [review]
patch

OK, let's try this then.

Comment 17

[[e-mail]]

Comment on attachment 661307 [details] [diff] [review]
patch

So, I wheeled out the debugger, and it turns out that this doesn't appear to be relevant, because the real problem is that bug 178324 accidentally changed the search root from the content window to the chrome window. Excluding chrome windows from the search won't stop us from searching into la la land...

Comment 18

[Neil Deakin]

That doesn't look to be true for the testcase though. The search root is the content page.

Comment 19

[Neil Deakin]

Attachment: patch, v2

Comment 20

[[e-mail]]

Bah, the // static annotation confused me, I thought GetFocusedDescendant was static to nsFocusManager.cpp :-(

Comment 21

[[e-mail]]

Comment on attachment 663445 [details] [diff] [review]
patch, v2

>+    nsCOMPtr<nsPIDOMWindow> ourWindow = do_QueryInterface(scriptGO);
>+    nsCOMPtr<nsIDOMWindow> windowToSearch(do_QueryInterface(scriptGO));
>+
>+    nsCOMPtr<nsPIDOMWindow> focusedWindow;
>+    nsFocusManager::GetFocusedDescendant(ourWindow->GetPrivateRoot(), true, getter_AddRefs(focusedWindow));
>+    if (focusedWindow) {
>+      windowToSearch = do_QueryInterface(focusedWindow);
>     }
Not sure why you're trying to get the focused descendant of the private root here. Also, nsPIDOMWindow inherits from nsIDOMWindow, and I can't see how GetFocusedDescendant can return a null window, so something like this should suffice:
nsCOMPtr<nsPIDOMWindow> ourWindow = do_QueryInterface(scriptGO);
nsCOMPtr<nsPIDOMWindow> windowToSearch;
nsFocusManager::GetFocusedDescendant(ourWindow, true, getter_AddRefs(windowToSearch));

>+    nsCOMPtr<nsPIDOMWindow> window(do_QueryInterface(aWindow));
>+
>+    nsCOMPtr<nsPIDOMWindow> focusedWindow;
>+    nsCOMPtr<nsIContent> focusedContent =
>+      nsFocusManager::GetFocusedDescendant(window, false, getter_AddRefs(focusedWindow));
>+    if (!focusedContent) {
>+      focusedContent = nsFocusManager::GetFocusedDescendant(window->GetPrivateRoot(),
>+                         true, getter_AddRefs(focusedWindow));
>+    }
>+
>+    if (focusedContent) {
>+      frame = focusedContent->GetPrimaryFrame();
>+      if (frame && frame->PresContext() != presContext)
>+        frame = nullptr;
As far as I can tell, this ensures that the focused content is within the frame that we're searching, so I'm not sure why you bothered to do a deep search for it.

Comment 22

[Neil Deakin]

Attachment: patch, v3

In the previous patch, I was attempting to recreate more or less what the code originally did before bug 178324. I'm not sure what window the two code blocks above actually want to use, but in this patch, I use what I think is actually desired.

In EnsureFind, the focused window descendant of the 'this' docshell. In GetFrameSelection the focused node in the supplied aWindow argument.

Comment 23

[[e-mail]]

Comment on attachment 664111 [details] [diff] [review]
patch, v3

(Sorry for the delay.)

This works well, but I do have a possibly unrelated question:

Why would you want to use content = nsFocusManager::GetFocusedDescendant(window, false, ...); instead of content = window->GetFocusedNode(); ?

Comment 24

[Ludovic Hirlimann [:Usul]]

Can we get this checked in ?

Comment 25

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/56d0d35f515a

Comment 26

[lihlii]

Seems never really fixed or resolved.  Also related to 
https://bugzilla.mozilla.org/show_bug.cgi?id=256380

Tested version: Thunderbird 17.0.8 WinXP