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

RESOLVED FIXED in mozilla19

Status

()

Core
XUL
RESOLVED FIXED
7 years ago
4 years ago

People

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

Tracking

({regression})

1.9.2 Branch
mozilla19
x86
Windows 7
regression
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

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 ?
(Reporter)

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.

cheers,
raoul
Duplicate of this bug: 642811
confirmed. 
but perhaps it is core related?
Status: UNCONFIRMED → NEW
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

Updated

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

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

5 years ago
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)
Blocks: 178324

Updated

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

Comment 11

5 years ago
thanks for tracking it down!
Created attachment 660953 [details] [diff] [review]
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.
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]
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.
Attachment #661307 - Flags: feedback?(neil)
Comment on attachment 661307 [details] [diff] [review]
patch

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

OK, let's try this then.
Attachment #661307 - Flags: review?(neil)
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...
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
Status: NEW → ASSIGNED
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 ?
https://hg.mozilla.org/mozilla-central/rev/56d0d35f515a
Status: ASSIGNED → RESOLVED
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 
https://bugzilla.mozilla.org/show_bug.cgi?id=256380

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