Note: There are a few cases of duplicates in user autocompletion which are being worked on.

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

RESOLVED FIXED in mozilla19



7 years ago
4 years ago


(Reporter: raoul bhatia, Assigned: Neil Deakin (not available until Aug 9))



1.9.2 Branch
Windows 7
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)



(2 attachments, 2 obsolete attachments)



7 years ago
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 ?

Comment 2

7 years ago
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


5 years ago
Version: unspecified → 3.1

Comment 8

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

Comment 9

5 years ago
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


Comment 10

5 years ago
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


5 years ago
Component: Message Compose Window → XUL
Product: Thunderbird → Core
Version: 3.1 → 1.9.2 Branch

Comment 11

5 years ago
thanks for tracking it down!
Created attachment 660953 [details] [diff] [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.
Created attachment 661307 [details] [diff] [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.
Created attachment 663445 [details] [diff] [review]
patch, v2
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.
Created attachment 664111 [details] [diff] [review]
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.
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 ?
Last Resolved: 5 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19

Comment 26

4 years ago
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.