Closed
Bug 621944
Opened 14 years ago
Closed 12 years ago
"Find and Replace" wrap around in compose screen does not work
Categories
(Core :: XUL, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: raoul, Assigned: enndeakin)
References
Details
(Keywords: regression)
Attachments
(2 files, 2 obsolete files)
216 bytes,
patch
|
Details | Diff | Splinter Review | |
4.85 KB,
patch
|
neil
:
review+
|
Details | Diff | Splinter Review |
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•14 years ago
|
||
Raoul did this use to work before ?
Reporter | ||
Comment 2•14 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
Comment 4•13 years ago
|
||
confirmed. but perhaps it is core related?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment 5•12 years ago
|
||
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
Comment 6•12 years ago
|
||
This is not a core bug.
Updated•12 years ago
|
Version: unspecified → 3.1
Comment 8•12 years ago
|
||
I can reproduce in Tb3.1.20 - Daily18.0a1, but not Tb3.0.11.
Comment 9•12 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•12 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•12 years ago
|
Component: Message Compose Window → XUL
Product: Thunderbird → Core
Version: 3.1 → 1.9.2 Branch
Reporter | ||
Comment 11•12 years ago
|
||
thanks for tracking it down!
Assignee | ||
Comment 12•12 years ago
|
||
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.
Assignee | ||
Comment 13•12 years ago
|
||
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.
Assignee | ||
Comment 14•12 years ago
|
||
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 15•12 years ago
|
||
Comment on attachment 661307 [details] [diff] [review] patch Seems reasonable from code inspection.
Attachment #661307 -
Flags: feedback?(neil) → feedback+
Assignee | ||
Comment 16•12 years ago
|
||
Comment on attachment 661307 [details] [diff] [review] patch OK, let's try this then.
Attachment #661307 -
Flags: review?(neil)
Comment 17•12 years ago
|
||
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-
Assignee | ||
Comment 18•12 years ago
|
||
That doesn't look to be true for the testcase though. The search root is the content page.
Assignee | ||
Comment 19•12 years ago
|
||
Assignee: nobody → enndeakin
Attachment #661307 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #663445 -
Flags: review?(neil)
Comment 20•12 years ago
|
||
Bah, the // static annotation confused me, I thought GetFocusedDescendant was static to nsFocusManager.cpp :-(
Comment 21•12 years ago
|
||
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.
Assignee | ||
Comment 22•12 years ago
|
||
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.
Assignee | ||
Updated•12 years ago
|
Attachment #664111 -
Flags: review?(neil)
Assignee | ||
Updated•12 years ago
|
Attachment #663445 -
Attachment is obsolete: true
Attachment #663445 -
Flags: review?(neil)
Comment 23•12 years ago
|
||
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+
Comment 24•12 years ago
|
||
Can we get this checked in ?
Comment 25•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/56d0d35f515a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite-
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Comment 26•11 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.
Description
•