Closed Bug 621944 Opened 10 years ago Closed 9 years ago

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


(Core :: XUL, defect)

1.9.2 Branch
Windows 7
Not set





(Reporter: raoul, Assigned: enndeakin)



(Keywords: regression)


(2 files, 2 obsolete files)

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
Raoul did this use to work before ?
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.

Duplicate of this bug: 642811
but perhaps it is core related?
Ever confirmed: true
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
Keywords: regression
This is not a core bug.
Duplicate of this bug: 613791
Version: unspecified → 3.1
I can reproduce in Tb3.1.20 - Daily18.0a1, but not Tb3.0.11.
Regression window

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090610 Thunderbird/3.1a1pre ID:20090610031946

Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.2a1pre) Gecko/20090611 Thunderbird/3.1a1pre ID:20090611045744

In local build:
Last Good:
First Bad:

Regressed by:	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)
Blocks: 178324
Component: Message Compose Window → XUL
Product: Thunderbird → Core
Version: 3.1 → 1.9.2 Branch
thanks for tracking it down!
Attached patch testcaseSplinter Review
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:

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

The attached testcase can be used to reproduce in firefox.
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.
Attached patch patch (obsolete) — Splinter Review
This patches fixes the thunderbird issue I think, but not finding in general, since frames aren't really handled very well by this code.
Attachment #661307 - Flags: feedback?(neil)
Comment on attachment 661307 [details] [diff] [review]

Seems reasonable from code inspection.
Attachment #661307 - Flags: feedback?(neil) → feedback+
Comment on attachment 661307 [details] [diff] [review]

OK, let's try this then.
Attachment #661307 - Flags: review?(neil)
Comment on attachment 661307 [details] [diff] [review]

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...
Attachment #661307 - Flags: review?(neil) → review-
That doesn't look to be true for the testcase though. The search root is the content page.
Attached patch patch, v2 (obsolete) — Splinter Review
Assignee: nobody → enndeakin
Attachment #661307 - Attachment is obsolete: true
Attachment #663445 - Flags: review?(neil)
Blocks: 707599
Bah, the // static annotation confused me, I thought GetFocusedDescendant was static to nsFocusManager.cpp :-(
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.
Attached patch patch, v3Splinter Review
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.
Attachment #664111 - Flags: review?(neil)
Attachment #663445 - Attachment is obsolete: true
Attachment #663445 - Flags: review?(neil)
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(); ?
Attachment #664111 - Flags: review?(neil) → review+
Can we get this checked in ?
Closed: 9 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Seems never really fixed or resolved.  Also related to

Tested version: Thunderbird 17.0.8 WinXP
You need to log in before you can comment on or make changes to this bug.