209989 - After Find Again (Ctrl+G / F3), text highlighting color becomes green

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P4)

Description

[Caspy7]

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)

Comment 1

[Caspy7]

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.

Comment 2

[Christopher Curzio]

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. 

Comment 3

[Justin Kerk]

Looks like a dupe of bug 193910

Comment 4

[Daniel Wang]

this does not only happen to framed page

Comment 5

[Daniel Wang]

*** Bug 295489 has been marked as a duplicate of this bug. ***

Comment 6

[Daniel Wang]

*** Bug 269945 has been marked as a duplicate of this bug. ***

Comment 7

[Daniel Wang]

Firefox only. Doesn't happen in Mozilla/Seamonkey -> Firefox

Comment 8

[Adam Guthrie]

*** Bug 267971 has been marked as a duplicate of this bug. ***

Comment 9

[Kevin Brosnan]

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

Comment 10

[Adam Guthrie]

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.

Comment 11

[Andrés Delfino]

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).

Comment 12

[Andrés Delfino]

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b5) Gecko/20051020 Firefox/1.5

Sorry, :P

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

taking.

Comment 14

[Uri Bernstein]

*** Bug 316013 has been marked as a duplicate of this bug. ***

Comment 15

[Adam Guthrie]

*** Bug 326907 has been marked as a duplicate of this bug. ***

Comment 16

[Peter Kasting]

*** Bug 195623 has been marked as a duplicate of this bug. ***

Comment 17

[Peter Kasting]

Taking, I have a patch for this.

Comment 18

[Peter Kasting]

OK, apparently "accept bug" doesn't also assign it to me...

Comment 19

[Peter Kasting]

Attachment: 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.

Comment 20

[Peter Kasting]

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 21

[Mike Connor [:mconnor]]

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 22

[Peter Kasting]

Comment on attachment 224644 [details] [diff] [review]
patch v1

OK, switching sr? to bz.

Comment 23

[Boris Zbarsky [:bzbarsky]]

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 24

[Peter Kasting]

Comment on attachment 224644 [details] [diff] [review]
patch v1

Hmmm... OK, how about dbaron then?  Since much of this patch is in layout/.

Comment 25

[Mike Connor [:mconnor]]

nice to fix, but not a blocker

Comment 26

[Peter Kasting]

Comment on attachment 224644 [details] [diff] [review]
patch v1

Preemptively adding branch approval request

Comment 27

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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).

Comment 28

[Peter Kasting]

Attachment: sr comments addressed, updated to apply cleanly

Here's the final version to go on trunk + branch

Comment 29

[Mark Mentovai]

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

Comment 30

[Peter Kasting]

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.

Comment 31

[Peter Kasting]

Attachment: branch version (v1)

This is a branch version of the trunk patch, with the same caveats about functionality.

Comment 32

[Peter Kasting]

Attachment: trunk tweak

This tweak should fix the problem completely on trunk.  Updated branch patch coming up in a second.

Comment 33

[Peter Kasting]

Attachment: branch version (final)

Here's the branch version of the entire patch, including the above tweak.

Comment 34

[Olli Pettay [:smaug][[e-mail]]]

Is it possible that this caused some new balsa trace_malloc_leaks.

Comment 35

[Peter Kasting]

I sure don't think so... reviewing my code I can't see how it could leak where the old code didn't.

Comment 36

[Adam Guthrie]

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).

Comment 37

[Peter Kasting]

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 38

[Peter Kasting]

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).

Comment 39

[Mark Mentovai]

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 3.199.2.12
toolkit/components/typeaheadfind/content/findBar.js 1.21.2.17

Comment 40

[pal-moz]

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

Comment 41

[pal-moz]

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

Comment 42

[Peter Kasting]

"sometimes green, sometimes blue" doesn't tell me much.  Can you give me some steps as to what you're doing?

Comment 43

[Peter Kasting]

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.

Comment 44

[Peter Kasting]

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.

Comment 45

[Peter Kasting]

At Gavin Sharp's recommendation, I'm changing this back to FIXED (since it is, indeed, fixed) and letting bug 342741 track the regression.

Comment 46

[Wayne Mery (:wsmwk)]

*** Bug 256801 has been marked as a duplicate of this bug. ***