626985 - accessibility.typeaheadfind.usefindbar=false broken in View Source window

Status: RESOLVED FIXED

(SeaMonkey :: Find In Page, defect)

Description

[K Chayka]

User-Agent:       Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110117 Firefox/4.0b10pre SeaMonkey/2.1b2pre
Build Identifier: Mozilla/5.0 (Windows NT 5.1; rv:2.0b10pre) Gecko/20110117 Firefox/4.0b10pre SeaMonkey/2.1b2pre

The View Source window does not honor the accessibility.typeaheadfind.usefindbar=false setting on a "repeat find" (F3). In addition, after the findbar is dismissed (by pressing ESC) and a new search text is typed, pressing F3 continues searching on the original search term, not the new one.

Reproducible: Always

Steps to Reproduce:
1. Set prefs to 
   accessibility.typeaheadfind.usefindbar=false and
   accessibility.typeaheadfind.linksonly=false
2. Go go http://www.mozilla.org/
3. View Source
4. Start FAYT by typing "fire" (without quotes). The first instance will be selected as expected.
5. Press F3 for "repeat find". The expectation is the next instance of "fire" would be selected but instead the findbar pops up and gets the focus.
6. Dismiss the findbar by pressing ESC. F3 continues to find "fire" as expected.
7. Start a new FAYT in the same window by typing "fox". The first instance of "fox" is selected as expected.
8. Press F3. The findbar does not pop up as expected, however the next instance of "fire" is selected, not "fox".


Expected Results:  
The 2 typeaheadfind prefs work as expected in normal browser windows--the findbar does not pop up and F3 always continues with the most recent search. The expectation is these prefs work the same way in a View Source window.

Comment 1

[Philip Chee]

Attachment: Proposed fix v1.0

Comment 2

[[e-mail]]

Comment on attachment 505370 [details] [diff] [review]
Proposed fix v1.0

I'm not sure we want large amounts of browser.js in the view source window - we might accidentally break stuff like print preview. Can we move the necessary methods to findUtils.js instead? It seems particularly strange that we declare nsFindInstData there but the method to get it is in browser.js!

Comment 3

[Philip Chee]

Attachment: Patch v1.1 Don't depend on browser.js

> I'm not sure we want large amounts of browser.js in the view source window - we
> might accidentally break stuff like print preview. Can we move the necessary
> methods to findUtils.js instead? It seems particularly strange that we declare
> nsFindInstData there but the method to get it is in browser.js!

CVS Blame says jag. See Bug 127589 circa 2003. :P

+++ b/suite/common/viewSourceOverlay.js
+function getFindInstData()
+    // defaults for rootSearchWindow and currentSearchWindow are fine here

I couldn't move getFindInstData() into findUtils.js because MailNews needs a different rootSearchWindow and currentSearchWindow.

Note to self: We need should implement the findbar in mailnews (like Thunderbird).

Comment 4

[[e-mail]]

Comment on attachment 505752 [details] [diff] [review]
Patch v1.1 Don't depend on browser.js

>diff --git a/suite/common/viewSourceOverlay.xul b/suite/common/viewSourceOverlay.xul
You're missing findBundle. r=me with that fixed.

Comment 5

[jag (Peter Annema)]

I think I used to have a |getFindInstData()| in Navigator, ViewSource (which back then just used navigator.js, I think), Mail/News, Help, and Venkman (inlined).

Doing that seemed cleaner to me than something like:

findUtils.js:

// will set gFindInstData at global scope (YUCK!)
// uses getBrowser() if available, otherwise set in initFindInstData()
// override defaults in initFindInstData()
// 
function getFindInstData2()
{
  if (!gFindInstData) {
    gFindInstData = new nsFindInstData();

    if ("getBrowser" in window)
      gFindInstData.browser = getBrowser();

    if ("initFindInstData" in window)
      initFindInstData();
  }

  return gFindInstData;
}


mailWindowOverlay.js:

function initFindInstData()
{
  gFindInstData.browser = getMessageBrowser();
  gFindInstData.rootSearchWindow = window.top.content;
  gFindInstData.currentSearchWindow = window.top.content;
}


I'm still not a big fan of utilities waltzing over the window object of their users, but...


And we could get even more clever and remove most of the passing around of findInstData:

findUtil.js:

function findInPage(findInstData)
{
  var findbar = document.getElementById("FindToolbar");
  if (findbar && Services.prefs.getBoolPref("browser.findbar.enabled"))
    findbar.onFindCommand();
  else if ("findDialog" in window && window.findDialog) // is the find dialog up already?
    window.findDialog.focus();
  else
  {
    if (!findInstData)
      findInstData = getFindInstData2();
    findInstData.init();
    window.findDialog = window.openDialog("chrome://global/content/finddialog.xul", "_blank", "chrome,resizable=no,dependent=yes", findInstData);
  }
}

function findAgainInPage(findInstData, reverse)
{
  // Support calling with just a |reverse| arg.
  if (!(findInstData instanceof nsFindInstData))
    [reverse, findInstData] = [findInstData, null]

...

    var searchString = findService.searchString;
    if (searchString.length == 0) {
      // no previous find text
      findInPage(findInstData);
      return;
    }

    if (!findInstData)
      findInstData = getFindInstData2();

    findInstData.init();

...

}


And then we can just use |findInPage()| and |findAgainInPage(false)| (or true) in most of our use cases.


On a final note (even ignoring all the above), would you mind moving the comments above findInPage() up a bit to the nsFindInstData prototype, where they belong, as I should have done oh so many years ago? :-)

Comment 6

[Philip Chee]

Attachment: [As checked in] Patch v1.1a r=Neil

Pushed to comm-central
http://hg.mozilla.org/comm-central/rev/3a40bd22fd66

>> [e-mail]      2011-01-21 03:58:01 PST

>> diff --git a/suite/common/viewSourceOverlay.xul b/suite/common/viewSourceOverlay.xul
> You're missing findBundle. r=me with that fixed.
Fixed.

> jag (Peter Annema)      2011-01-21 06:54:37 PST
> 
>> Doing that seemed cleaner to me than something like:

Hmm. I think all that should be in a separate bug.

>>   else if ("findDialog" in window && window.findDialog) // is the find dialog up already?
>>     window.findDialog.focus();

Irritatingly enough Blake, the boy wonder fixed this in aviary when he forked findUtils.js:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/content/findUtils.js&rev=1.1#56

Comment 7

[jag (Peter Annema)]

You'll notice that rev 1.1 is from before the changes I made, which got ported in rev 1.2.