How to reproduce: 1. insure "find text as you type" is on 2. go to http://www.money-net.net/welcome.htm note this is a framed page, I don't believe this bug exists in non framed pages 3. type "a" (without quotes" 4. type <ctrl> + g to find the next occurance of "a" 5. click on the webpage to cancel the search. 6. click and drag to select a portion of text. Expected behavior: text is highlighted with system defaulted color (blue for me) Actual behavior: text is hightlighted bright green (current default color for this feature). This behavior can be undone by doing another find as you type search and hitting escape to cancel. This improper behavior does not occur when one hits escape to cancel the search or attempts it on a non-framed webpage (in my experience)
It appears the same problem with green highlighting can be observed in non-framed pages by doing the following: 1. insure "find text as you type" is on 2. insure there are multiple tabs open. 3. type "a" (without quotes) 4. type <ctrl> + g to find the next occurance of "a" 5. click on a different tab 6. type <ctrl> + g to find an occurance of "a" 7. click on the original tab. 8. click and drag to select a portion of text. I hope this helps to point towards the real problem.
I've found that all you need to do is escape out of the find bar once you've located an instance of whatever you're searching for, hit CTRL+G to find another one, then start highlighting. It doesn't seem to matter whether the page has frames or not.
Looks like a dupe of bug 193910
this does not only happen to framed page
*** Bug 295489 has been marked as a duplicate of this bug. ***
*** Bug 269945 has been marked as a duplicate of this bug. ***
Firefox only. Doesn't happen in Mozilla/Seamonkey -> Firefox
*** Bug 267971 has been marked as a duplicate of this bug. ***
I cannot reproduce comment 0 or comment 1 using a current nightly. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051019 Firefox/1.5 worksforme
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051019 Firefox/1.6a1 ID:2005101911 I reproduced this with the following steps: 1. Make sure "begin finding when you begin typing" is on. 2. Type "a" (no quotes) and type <Ctrl>+g to find the next occurance of "a". 3. Switch to another tab and hit <Ctrl>+g in that tab. The find bar is not shown and anything you select in that tab will be green thereafter.
How to reproduce: 1. insure "begin finding when you begin typing" is on 2. type "a" (without quotes) 3. close find bar 4. press F3/Ctrl + G to find the next occurance of "a" 5. click on the webpage to cancel the search. 6. click and drag to select a portion of text. Expected behavior: text is highlighted with system defaulted color (blue for me) Actual behavior: text is hightlighted bright green (current default color for this feature).
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051020 Firefox/1.5 Sorry, :P
*** Bug 316013 has been marked as a duplicate of this bug. ***
*** Bug 326907 has been marked as a duplicate of this bug. ***
*** Bug 195623 has been marked as a duplicate of this bug. ***
Taking, I have a patch for this.
OK, apparently "accept bug" doesn't also assign it to me...
Created attachment 224644 [details] [diff] [review] patch v1 (and of course, assigning the bug to myself meant it went back to NEW... whatever) This fixes this bug and makes some handling of SELECTION_ATTENTION a bit better/more consistent: * When using SELECTION_ATTENTION to select over an image, use the correct background color (the ATTENTION color, not the DISABLED color) * Treat ATTENTION just like ON in the documentViewer blur handler (for whose purposes it is just like ON) * Other miscellaneous code/comment cleanup The actual bug fix could be considered a hack, but there doesn't seem to be a "better" way to do this. The Find code is the only code that uses SELECTION_ATTENTION, and when the Find bar isn't onscreen, nothing about its code can turn the selection "back to normal" when the user starts goofing with the selection. So, the fix here is to make the FrameSelection automatically reset itself to ON when it's cleared in ATTENTION mode. If the Find bar wants to show another ATTENTION selection, it can (and does!) set the selection type back to ATTENTION. Thus, ATTENTION is not a "permanent" state change, so much as a temporary state change "for this selection", which fits with what the Find code is trying to do. I purposefully did not go farther in trying to reset to ON if, say, the selection changes at all (for example, if the user uses shift + arrows to extend the selection). Not only would this be more invasive and unlikely to even be noticed by most users, but I actually consider being able to change the ATTENTION selection a "feature", in the sense that you can use it to quickly draw your eye to where on the page the selection is when you do a Find and can't locate the result. I needed to make some changes in findBar.js to change the color after we find a new match rather than before, since the act of finding a new match will trip my new code :) Since this patch sort of sprawls over a couple different locations, I wasn't sure who the right set of reviewers would be... please reassign as appropriate. I'd like to get this fix into Fx2 if possible.
Nominating for Fx2 B1. Not only is this a mildly annoying bug, it may turn out to be a blocker for bug 189039 (find in textareas), since it seems more aggravating with that fixed. While the change is a little creepy-looking, it only affects a selection type used by Find.
Comment on attachment 224644 [details] [diff] [review] patch v1 r=me for the toolkit bits, jst is out for this week, maybe try bz instead?
Comment on attachment 224644 [details] [diff] [review] patch v1 OK, switching sr? to bz.
I'm actually out of town Thurs -> Monday, then again soon after... I might be able to get to this next week, but if not then it won't happen till sometime in mid July.
Comment on attachment 224644 [details] [diff] [review] patch v1 Hmmm... OK, how about dbaron then? Since much of this patch is in layout/.
nice to fix, but not a blocker
Comment on attachment 224644 [details] [diff] [review] patch v1 Preemptively adding branch approval request
Comment on attachment 224644 [details] [diff] [review] patch v1 I'd love to see us figure out what the model really is here. Clearly find has to display one (or preferably more) things highlighted. The question is what influenced find has on the normal selection and whether one of those highlighted things is the normal selection just displayed differently. And I'd also love to see the selection code provide a better API for things like this. One substantial comment in GetImage: container = mDisabledContainer; does not do what you think. You can't rebind a reference in C++, but that's what you wanted to do here. This is rebinding what the reference refers to. Although it's ugly, you need to make container a pointer to an nsCOMPtr. (Or you could start with a pointer and then assign *ptr into a reference.) Also, a few nits: You can remove the if (!container) return NS_ERROR_OUT_OF_MEMORY; since do_CreateInstance won't return null and success. (That really should be documented better in the code, including the interfaces all of whose implementations have to follow the rules to make that true. But we really do rely on it, and most C++ objects have factories implemented by the same macros that do do it right reliably.) Likewise for the result of do_GetService and: if (NS_SUCCEEDED(result) && look) although in that case you're probably better off with only the null-check, since you're not propagating the error, and you can save passing the &result to do_GetService. You should also add a space after the comma in: do_CreateInstance("@mozilla.org/image/container;1",&result); And in nsDisplaySelectionOverlay::Paint nsILookAndFeel::nsColorID lookandFeel; This was already here, but could you rename the variable to |colorID| instead of |lookandFeel| (the half-camel-case name for what usually refers to a service.) You already used |colorID| elsewhere, and I wrote this comment before reading that part of the patch (because I had to use the HTML view of the diff to read it).
Created attachment 225941 [details] [diff] [review] sr comments addressed, updated to apply cleanly Here's the final version to go on trunk + branch
Patch didn't apply on the branch, checked in on the trunk: layout/base/nsDocumentViewer.cpp 1.491 layout/generic/nsFrame.cpp 3.656 layout/generic/nsSelection.cpp 3.242 toolkit/components/typeaheadfind/content/findBar.js 1.40
My patch does not, in fact, fix this bug in all cases. Specifically, I fix a case where, sayu, you find a match in an anchor and then highlight some text outside the anchor; but not the case where you find, and then highlight, something in the same block of (non-anchor) text. I'm going to attach a branch version of the trunk patch above, but it shouldn't be applied. Mark and I aren't going to back out the trunk patch for now since it's not wrong, just incomplete.
Created attachment 225955 [details] [diff] [review] branch version (v1) This is a branch version of the trunk patch, with the same caveats about functionality.
Created attachment 225957 [details] [diff] [review] trunk tweak This tweak should fix the problem completely on trunk. Updated branch patch coming up in a second.
Created attachment 225958 [details] [diff] [review] branch version (final) Here's the branch version of the entire patch, including the above tweak.
Is it possible that this caused some new balsa trace_malloc_leaks.
I sure don't think so... reviewing my code I can't see how it could leak where the old code didn't.
Checkins for the leakage increase: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006%3A06%3A16%3A17%3A37%3A00&maxdate=2006%3A06%3A16%3A18%3A48%3A00&cvsroot=%2Fcvsroot It's leaking an nsRunnable, and nsSelection does have a class in it that implements (?) nsRunnable (please excuse my lack of C++ knowledge).
I'm not creating any new nsSelections anywhere. I suppose tomorrow I can try and see if I can use a local tinderbox to test backing out my patch to see if it changes leak numbers, but I really must have missed something if this patch can possibly leak.
Comment on attachment 225957 [details] [diff] [review] trunk tweak Didn't get a random reviewer over the weekend, so I'm sticking this quick one in dbaron's queue. (I'm assuming in a case like this all I really need is the r+ on the trunk tweak to get the tweaked versions landed on trunk + branch).
Tweak checked in on trunk: layout/generic/nsSelection.cpp 3.243 Patch checked in on MOZILLA_1_8_BRANCH for 1.8.1b1: layout/base/nsDocumentViewer.cpp 1.442.4.14 layout/generic/nsFrame.cpp 3.574.2.10 layout/generic/nsSelection.cpp 22.214.171.124 toolkit/components/typeaheadfind/content/findBar.js 126.96.36.199
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1a3) Gecko/20060620 BonEcho/2.0a3 ID:2006062004 landed on Bon Echo ? sometimes green, sometimes blue. screenshot : http://img223.imageshack.us/img223/373/fayt4oh.jpg
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060620 Minefield/3.0a1 ID:2006062004 [cairo] same with Minefield. screenshot : http://img210.imageshack.us/img210/7484/fayt16fi.jpg
"sometimes green, sometimes blue" doesn't tell me much. Can you give me some steps as to what you're doing?
Ah, I know what he means. The highlight is cleared to SELECTION_ON when we scroll the selection into view. It doesn't break if you don't need to scroll. Reopening.
This seems to be broken because the selection controller that findBar.js sets SELECTION_ATTENTION on isn't the same as the selection controller the matched text lives on (!?). Or at least that's what it looks like in gdb. Not sure why that's the case. Happily, this is fixed by my patch in bug 189039, because it moves selection controller-related stuff almost completely into the C++ side of things. (The rest of the fix that was checked in for this bug is still helpful and necessary alongside that patch.) For now, my plan is to sit tight and wait for that patch to make it in. If it becomes clear that patch isn't going to make it, then I need to come up with a different fix.
At Gavin Sharp's recommendation, I'm changing this back to FIXED (since it is, indeed, fixed) and letting bug 342741 track the regression.
*** Bug 256801 has been marked as a duplicate of this bug. ***