When I switch tabs, I don't want the find bar to follow me. When you try to find something you are doing it on the current page. Different tabs usually represent different activities. Same goes for different windows. Here's a quick summary of how I think it should work: * Open a new tab - no find bar * Switch tabs (after activating find bar) - no find bar in other tab (unless it was open there too) * Switching tabs - the find bar should remember what I searched for in that tab (not be global per window) * Activating find bar - prefilled with what I last searched for (in last active find bar), maybe do search right away... * Reactivating find bar - remember what I searched for before in that tab * Open a new window - no find bar I know the new windows problem has to do with persisting attributes/toolbars, but if the find bar is per tab, that becomes easy to deal with. This would help with our lingering "too much chrome" issue. I often find myself wondering why I see less web page and realize I left the find bar open 20 minutes ago in a different tab. FWIW, how some other browsers handle it: Safari/Chrome (I guess it's part of Webkit?): Shown per tab, remembers search across all though (so editing one edits them all) Opera: No bar, not something to look at IE: Unknown

Thanks for opening this, Paul - my first instinct is just like yours, that it should be per tab. In fact, I just noticed that my own find bar is open from twenty minutes ago. Before signing on, we should think hard about if there's a use case to keep it open. A possible one is opening many tabs, to do the same search in each of them. Maybe the key shortcut to repeat a search should work even if the search bar has been closed, so for people that use key commands in search they wouldn't need to reopen search. "Reactivating find bar - remember what I searched for before in that tab" - yes, but only in this tab - we wouldn't activating the search bar to reveal what was searched for tabs, minutes, hours, days ago

(In reply to comment #1) > Before signing on, we should think hard about if there's a use case to keep it > open. A possible one is opening many tabs, to do the same search in each of > them. Maybe the key shortcut to repeat a search should work even if the search > bar has been closed, so for people that use key commands in search they > wouldn't need to reopen search. I had thought about the legitimate use case that exists now for keeping it open (there really is only one use case), but that seems like a less frequent use case than not wanting the find bar open. For clarity: that one use case being that you want to perform the same search across multiple tabs (I know I did that when studying for exams or writing papers). I guess an issue does come up when you want to then change the search across tabs. Chrome/Safari handle this by changing the search term across all tabs/windows. My suggestions don't cover a good way to deal with this though. > "Reactivating find bar - remember what I searched for before in that tab" - > yes, but only in this tab - we wouldn't activating the search bar to reveal > what was searched for tabs, minutes, hours, days ago Right. I think we'd want to have some sort of timeout. Perhaps after X minutes/hours of no use (and it being hidden) we clear it out or treat it as though it were blank. Then if the find bar was reactivated it would be like just activating it (and so maybe prefilled) This was just a brain dump on my part. There are obviously details to figure out and caveats not considered yet.

See Bug 248955. There is also a quickfind way of searching with a timeout of a few seconds for the bar. When you focus another tab, the bar disappears, even if you have set a timeout of hours (Bug 331443).

(In reply to comment #3) > See Bug 248955. That's a similar discussion but about the search box (ctrl-k), not the find bar (ctrl-f) > There is also a quickfind way of searching with a timeout of a few seconds for > the bar. When you focus another tab, the bar disappears, even if you have set a > timeout of hours (Bug 331443). Sure, but that's a different feature (very related and it has it's own issues, such as clearing my find bar). I'm pretty sure it reuses the same XUL elements though so maybe we could fix that other bug at the same time. I guess this belongs in Toolkit|Find Toolbar? I'm not really thinking cross-product details here. Thunderbird/Seamonkey/Others could have very different use cases.

I think this can be implemented cleanly mostly in Firefox-specific code (perhaps with some minor tweaks to the findbar widget), so this component's fine, I think.

Implementing the behaviour as summarized by Paul in this bug's description would solve Bug 478903. I concur with Jennifer (comment #1) that I often find the find bar opened, claiming unnecessary screen space. The use case mentioned by Paul (comment #3) is one I execute on a regular basis, while searching for certain keywords in documents (opened in tabs). I think that immediately repeating the last search in a different tab by pressing ctrl-f if the previous search was done in the last n seconds could solve this problem, but I have no idea if this interferes with other usage and/or how difficult that would be to implement. Bottom line: although I love the current find functionality, I would really like to see it adopt somewhat less persistent behaviour, as described in Bug 478903.

More often than not, I use the same find string in multiple tabs, so I wouldn't want that behavior broken, even if it fixes other bugs previously mentioned. Can per-tab provide an easy method to repeat/recall the last find from a previous tab? I think my sympathies are also with bug 478903.

(In reply to comment #7) > More often than not, I use the same find string in multiple tabs, so I wouldn't > want that behavior broken, even if it fixes other bugs previously mentioned. > Can per-tab provide an easy method to repeat/recall the last find from a > previous tab? > > I think my sympathies are also with bug 478903. Wayne, I'm assuming the use case you are describing is when you're temporarily doing a search for a particular string. For instance, maybe you're searching for the same name across a few articles you have open in abs. In an hour, when your task is complete, you are unlikely to need that same search string again. More than likely, that same search string in an hour would be completely irrelevant to you. At worst, it could be embarrassing if your next search is with someone watching. To handle the case of a temporarily often-repeated search (same string) and performing a new search later (different string), we could assign the key command for Find Again (Cmd-G) to use the last string used, while the key command for Find (Cmd-F) always starts with a blank search box.

> To handle the case of a temporarily often-repeated search (same string) and > performing a new search later (different string), we could assign the key > command for Find Again (Cmd-G) to use the last string used, while the key > command for Find (Cmd-F) always starts with a blank search box. Wouldn’t such an inconsistency in the behaviour of the two commands lead to some confusion? Alternatively, copy over the most recent Find text when a Find (or Find Again) is attempted in a tab that did not already have a Find text. Additionally, I would suggest copying over the Find text when a new tab is spawned. This would partially (though not fully) make the previous suggestion unnecessary.

(In reply to comment #0) > When I switch tabs, I don't want the find bar to follow me. When you try to > find something you are doing it on the current page. Different tabs usually > represent different activities. This really depends on your workflow. if your workflow is monolithic and tightly focused, the answer might be no. I'm not suggesting this is the norm. However, this is the mode I frequently find myself in. Not for hours necessarily, but certainly 15-60+ minutes. And I might walk away from that task and come back to it. >Same goes for different windows. again, this really depends on your workflow. but probably less likely "the same" compared to using multiple tabs. > Here's a quick summary of how I think it should work: > > * Open a new tab - no find bar > * Switch tabs (after activating find bar) - no find bar in other tab (unless it > was open there too) > * Switching tabs - the find bar should remember what I searched for in that tab > (not be global per window) remember from which tab? A (the first), or B (the one switched to)? > * Activating find bar - prefilled with what I last searched for (in last active > find bar), maybe do search right away... > * Reactivating find bar - remember what I searched for before in that tab > * Open a new window - no find bar > > I know the new windows problem has to do with persisting attributes/toolbars, > but if the find bar is per tab, that becomes easy to deal with. I think I like all that you describe, as long as there is a way to continue finding the same text from the last tab where I invoked find. Perhaps you are describing the same thing or something close to that. But in case not, I'm looking for ... tab1 - find for xyz open tab2 - find for nothing open tab3 - cmd-F (or some other key) I want what I searched for in tab1 which is what I can do today. (In reply to comment #8) > (In reply to comment #7) > > More often than not, I use the same find string in multiple tabs, so I wouldn't > > want that behavior broken, even if it fixes other bugs previously mentioned. > > Can per-tab provide an easy method to repeat/recall the last find from a > > previous tab? > > > > I think my sympathies are also with bug 478903. > > Wayne, I'm assuming the use case you are describing is when you're temporarily > doing a search for a particular string. For instance, maybe you're searching > for the same name across a few articles you have open in abs. In an hour, when > your task is complete, you are unlikely to need that same search string again. > More than likely, that same search string in an hour would be completely > irrelevant to you. not really, I sometimes return to a task days later. (but days later is worst case) > At worst, it could be embarrassing if your next search is > with someone watching. isn't what you are suggesting security via obscurity? anyone with such worst case scenario of sensitive issues/concerns should immediately clear the area upon completion, and have the means to also clear find again. But even then, undo brings it back. I don't find the example use case compelling. An edge case worthy of an add-on > To handle the case of a temporarily often-repeated search (same string) and > performing a new search later (different string), we could assign the key > command for Find Again (Cmd-G) to use the last string used, while the key > command for Find (Cmd-F) always starts with a blank search box. a) we shouldn't steal cmd-G which already widely regarded as "find again" in strict context of, do again what I just did with cmd-F. But perhaps modifier of shift-cmg-G to execute last search from last open tab (but not from this tab)? b) fwiw clearing upon cmd-F is unnecessary - cmd-F already selects existing previous text, and all one need do to erase the previous contents is start typing, so no extra key strokes. However, removing the previous text upon cmd-F precludes modifying a previous search, which is bad IMO. Unless there is a clear recall function. But that adds keystrokes. Regarding clearing the field after timeouts - this will certainly confuse some users (one can imagine questions of "where did my stuff go?"), affects revising the previous text, and is not particularly discoverable. I suggest this is gallant but trying to be "too smart". But if this is disproved by empirical data, I'll stand corrected. Lastly, I'm sympathetic to the idea of hiding the find bar after X period of time - from a space standpoint it does sound nice. On the other hand ... a) hitting escape to close the find bar is trivial (although not everyone knows, because the UI lacks hints) b) if I leave a tab/page to go do something else, and come back hours or days later then I'd prefer my page state exactly the way I left it. I don't want to have to guess or remember what I was doing, including the hint that ... "I was searching for something"

Forward duping, as there's a partly-reviewed patch in the other bug, though it doesn't do everything mentioned in comment #0.

(In reply to comment #12) > Forward duping, as there's a partly-reviewed patch in the other bug, though it > doesn't do everything mentioned in comment #0. And therefore this isn't really a dupe. I think there are ideas and discussion here that validate keeping this open even if a simplified version of the idea was implemented.

This makes the find bar per-tab, except for highlight button. The highlight button should go away as part of bug 595552, so I don't see a reason to make it tab-specific first.

This duplicates a part of bug 628179.

Adam, are you ready to request review for the attached patch here?

This is ready for review unless we want to have this manage the highlight button too. I'm guessing the highlight-all performance (bug 251862) is why that's currently sticking around, so I'm pushing for landing bug 769791 to see how close that gets us.

I don't think this patch will need to manage the highlight button. Let's try to split that out to a separate bug.

Use case for change is getting the right hand-me-down value for a tab you haven't used to find before. With the change, you can "shop around" for the tab with the existing find value you want. Example, you open the find bar on a tab you didn't use it before, but it's populated with the find from a tab that you don't want, but know (or want to find) the tab with the right value, you can switch there and back. Chromium's behavior is to blank the find value if you switch away/back without performing a find. If that behavior is preferred, I can implement that instead.

The previous change broke the test for ReplaceTabWithWindow. If the tab's find value is a hand-me-down, the find value should be empty when it lands as a window. Otherwise, it should be filled. Test now covers both cases.

Earlier patch was half-baked. This should be closer to the real deal. Includes private browsing support, coverage of status, and better case sensitivity coverage. One outstanding issue is whether merely toggling case sensitivity should constitute a full user interaction, or whether it should be handled separately. That is: 1. Opens find on a new tab 2. Toggles case sensitivity 3. Switches away 4. Switches back At [4], the case sensitivity could either follow the tab at [3], or the toggled value from [2]. Same goes for the find value. Leaning toward [2] causing the values to be fixed on both counts.

Adam, are you able to pick this bug back up? There is some other find toolbar-related bugs that are moving forward so it would be nice to get this in at the same time :)

(In reply to Jared Wein [:jaws] from comment #23) > Adam, are you able to pick this bug back up? There is some other find > toolbar-related bugs that are moving forward so it would be nice to get this > in at the same time :) Thanks Jared! Yes, this is long overdue. * Drops bits formerly needed to save/restore when entering/leaving Private Browsing. * Adds a small test case for case sensitivity persistence. * Minor cleanup to tests. * Renames the findbar properties |_modified| and |_userModified| to |_stateChanged| and |_hasUserData| for clarity/disambiguation. * Renames findbar method |hasData| to |hasNewData| for clarity. I'll look at this more tomorrow.

Working on dealing with the case sensitivity preference (accessibility.typeaheadfind.casesensitive). It currently has three states: off (0), on (1), and auto (any other integer; behavior is determined by whether the find string is/isn't lowercase). Any ideas for the best way to handle this? My thoughts/current direction follows. 1. We shouldn't have some tabs in automatic mode and others in manual mode. If a user changes between manual and automatic (rare), all the findbars should be changed to reflect that. 2. In manual mode, we shouldn't toggle the preference itself. It should be the default for new sessions, and not subject to how the user left the browser last time (especially when this means which findbar they changed last or which findbar they were looking at last). My guess is most users rarely use case sensitive find, and even fewer want it as a default. But I don't have any data to back that up.

Can we leave the behavior of how we handle accessibility.typeaheadfind.casesensitive for another bug? It seems like something we could leave as-is and handle in a follow-up so this work doesn't get sidetracked.

This defers worrying about the preference for now. The 'automatic' mode doesn't work with this patch (the preference gets set to insensitive every session). Calls to |_updateCaseSensitivity| already set the checkbox state, so this no longer does so directly.

Have we considered lazily creating a separate <findbar> for the selected browser on demand? E.g. if you'd open the find bar in two tabs, there would be two coexisting <findbar>s. This way we wouldn't need manually to keep track of the per-tab data and I think the toolkit findbar widget would need no modification either.

(In reply to Dão Gottwald [:dao] from comment #28) > Have we considered lazily creating a separate <findbar> for the selected > browser on demand? E.g. if you'd open the find bar in two tabs, there would > be two coexisting <findbar>s. This way we wouldn't need manually to keep > track of the per-tab data and I think the toolkit findbar widget would need > no modification either. I think that would definitely simplify the code, at a small memory cost since it would only be incurred if the browser accessed the <findbar>. I would prefer this type of solution.

I'm pursuing this approach. It will be more versatile, especially in light of the other refinements down the road. As for needing manual data migration, the only place that will need this so far is when moving a tab to another window.

Also, it should be possibly to do this while maintaining gFindBar and gFindBarInitialized as primary APIs that automatically refer to the selected browser. unrelated note: you'll need to revert bug 869919

This is shaping up, but some work still needed: 1. Tests (this bug's and other findbar tests including two that currently fail with this patch for bug 409624). 2. Currently saving the hidden/shown state per-tab, but may drop that so as to partially implement bug 628179 (hide findbar on tab switch). 3. No case sensitivity saving here, but we may pick that up in a follow-up. 4. Populating the find field correctly on creation: first taking the page-selected value, otherwise the last-seen value from another tab. 5. The gFindBar{,Initialized} setters/getters. The setters just return whatever you passed in.

This would conflict with fastfind, basically the first tab to use fastfind would have its findbar reused on other tabs. This fixes that behavior. To do so this has to override the findbar.handleEvent so it won't handle |keypress|, but will still handle |mouseup| (for hiding the fastfind bar). Also avoids state management for fastfind mode. This also fixes the tests mentioned and undoes bug 869919.

This is mostly code cleanup, but also clears the tabbrowser._lastFindValue in sanitize.js so that opening a new findbar on a new tab can't leak an old value. The cleanup removes the 'active' attribute from the current tab's findbar as this doesn't need it. If that's desirable in its own right, it can be added back with minimal code.

Review ping?

Comment on attachment 749067 [details] [diff] [review] Cleanup >+this.__defineGetter__("gFindBar", function() { >+ return window.gBrowser.getActiveFindBar(); >+}); >+this.__defineSetter__("gFindBar", function (val) { >+ return val; >+}); >+ >+this.__defineGetter__("gFindBarInitialized", function() { >+ return window.gBrowser.mCurrentTab._findBar != null; >+}); >+this.__defineSetter__("gFindBarInitialized", function (val) { >+ return val; > }); The setters aren't needed. >+ // Clear any saved find value >+ currentWindow.gBrowser.tabContainer.tabbrowser._lastFindValue = ""; currentWindow.gBrowser.tabContainer.tabbrowser == currentWindow.gBrowser >+ <method name="initFindBarForCurrentTab"> >+ <body><![CDATA[ >+ let findBar = document.createElementNS(this.namespaceURI, "findbar"); >+ >+ let browserBottomBox = document.getElementById("browser-bottombox"); >+ browserBottomBox.insertBefore(findBar, browserBottomBox.firstChild); Why is this appended to the global browser-bottombox rather than e.g. browserContainer that exists on a per-tab basis? >+ findBar.browser = gBrowser; gBrowser == this >+ // Tab moved to new window: add a findbar and restore data >+ if (this.mCurrentTab._findData) { >+ gFindBar.hidden = this.mCurrentTab._findData.hidden; >+ gFindBar._findField.value = this.mCurrentTab._findData.value; >+ } >+ delete this.mCurrentTab._findData; This looks like it belongs in swapBrowsersAndCloseOther rather than updateCurrentBrowser.

Thanks Dão. In moving the |findbar| elements to be appended to their |browserContainer|, also had to modify the sanitize.js |querySelectorAll| calls, as the |findbar|s weren't being found from the document.

Comment on attachment 768696 [details] [diff] [review] Move findbars to their browserContainers >@@ -965,19 +1048,25 @@ > true, false); > } > > if (!this._previewMode) { > this.mCurrentTab.removeAttribute("unread"); > this.selectedTab.lastAccessed = Date.now(); > > // Bug 666816 - TypeAheadFind support for e10s >- if (!gMultiProcessBrowser) >+ if (!gMultiProcessBrowser) { > this._fastFind.setDocShell(this.mCurrentBrowser.docShell); > >+ this.deactivateFindBarForTab(oldTab); >+ if (gFindBarInitialized && >+ gFindBar.findMode == gFindBar.FIND_NORMAL) >+ gFindBar.hidden = this.mCurrentTab._findHidden; >+ } What is this chunk good for? Can we not have multiple open findbars in background tabs?

This removes the code (|updateCurrentBrowser|) for deactivating the last tab's findbar, as it can now stay open in a background tab without interfering. We still remember the last-seen find value if that tab had an open findbar, though.

Comment on attachment 769243 [details] [diff] [review] Remove unnecessary deactivation code >+ <method name="getActiveFindBar"> >+ <body><![CDATA[ >+ return this.mCurrentTab._findBar || this.initFindBarForCurrentTab(); nit: use selectedTab rather than mCurrentTab >+ ]]></body> >+ </method> >+ >+ <method name="initFindBarForCurrentTab"> Can this be merged into getActiveFindBar? getActiveFindBar basically does nothing other than calling this method... >+ <body><![CDATA[ >+ let findBar = document.createElementNS(this.namespaceURI, "findbar"); >+ >+ let browserContainer = this.mCurrentBrowser.parentNode.parentNode; nit: use this.getBrowserContainer(); >+ browserContainer.appendChild(findBar); >+ >+ findBar.browser = this; Should we set findBar.browser to this.selectedBrowser? I'm not sure there's a point in tabbrowser's fastFind property any longer. >+ // We handle the keyboard events for the findbar to prevent the wrong >+ // findbar from handling them. >+ findBar.handleEvent = function (aEvent) { >+ switch (aEvent.type) { >+ case "mouseup": >+ if (gFindBarInitialized && !gFindBar.hidden && >+ gFindBar.findMode != gFindBar.FIND_NORMAL) >+ gFindBar.close(); >+ break; >+ } >+ } If you set findBar.browser to the right browser rather than the tabbrowser, this shouldn't be needed anymore. >@@ -2027,16 +2097,23 @@ > this._tabAttrModified(aOurTab); > if (aOurTab.selected) > this.mIsBusy = true; > } > > this._swapBrowserDocShells(aOurTab, otherBrowser); > } > >+ // Handle findbar data (if any), removing its reference >+ let findData = this.deleteFindBarForTab(aOtherTab); >+ if (findData) { >+ gFindBar.hidden = findData.hidden; >+ gFindBar._findField.value = findData.value; >+ } >+ > // Finish tearing down the tab that's going away. > remoteBrowser._endRemoveTab(aOtherTab); Why do you need to delete aOtherTab's findbar? That tab is going away anyway.

> >+ // We handle the keyboard events for the findbar to prevent the wrong > >+ // findbar from handling them. > >+ findBar.handleEvent = function (aEvent) { > > If you set findBar.browser to the right browser rather than the tabbrowser, > this shouldn't be needed anymore. The other event handling change (had removed |!gFindBarInitialized| check) is not needed now, too. This does remove |tabbrowser.fastFind|. Note that |browser.fastFind| (toolkit/content/widgets/browser.xml) was piggybacking on |tabbrowser.fastFind| when possible. This removes the piggybacking.

Comment on attachment 770562 [details] [diff] [review] Findbar browser points to selectedBrowser >+this.__defineGetter__("gFindBarInitialized", function() { >+ return window.gBrowser.mCurrentTab._findBar != null; > }); nit: use gBrowser.selectedTab > // Bug 666816 - TypeAheadFind support for e10s >- if (!gMultiProcessBrowser) >- this._fastFind.setDocShell(this.mCurrentBrowser.docShell); >+ if (!gMultiProcessBrowser) { >+ let oldFindBar = oldTab._findBar; >+ if (oldFindBar && >+ oldFindBar.findMode == oldFindBar.FIND_NORMAL && >+ !oldFindBar.hidden) >+ this._lastFindValue = oldFindBar._findField.value; >+ } The !gMultiProcessBrowser check isn't useful anymore here, as you're not dealing with docShells. >+++ b/browser/base/content/test/browser_bug537013.js >+function addTabWithText(aText, aCallback) { >+ let newTab = gBrowser.addTab(); >+ tabs.push(newTab); >+ gBrowser.selectedTab = newTab; >+ content.location = "data:text/html,<h1 id='h1'>" + aText + "</h1>"; >+} use gBrowser.addTab("data:text... >+function setFindString(aString) { >+ gFindBar.open(); >+ gFindBar._findField.select(); >+ gFindBar._findField.focus(); you can drop focus(), since select() already implies it >+ ok(gFindBar.hasAttribute("hidden"), "Second tab doesn't show find bar!"); can you just use gFindBar.hidden here? >+ // wait for gBrowser to come along >+ newWindow.addEventListener("load", function onLoad() { >+ newWindow.removeEventListener("load", onLoad, false); >+ executeSoon(checkNewWindow); >+ }, false); whenDelayedStartupFinished(newWindow, checkNewWindow); >+ cleanUp(); Please use registerCleanupFunction to remove the tabs and just call finish() here. >--- a/browser/base/content/test/browser_zbug569342.js >+++ b/browser/base/content/test/browser_zbug569342.js Why did you need to modify this test? >--- a/toolkit/content/widgets/browser.xml >+++ b/toolkit/content/widgets/browser.xml >- var tabBrowser = this.getTabBrowser(); >- if (tabBrowser) >- return this._fastFind = tabBrowser.fastFind; >- >- var tabBrowser = this.getTabBrowser(); >- if (!tabBrowser || tabBrowser.mCurrentBrowser == this) >- this.fastFind.setDocShell(this.docShell); >+ this.fastFind.setDocShell(this.docShell); Please check for ("fastFind" in tabBrowser) since other tabbrowsers such as SeaMonkey's may still use it. Overall this looks pretty good, hopefully just needs one more iteration.

> >--- a/browser/base/content/test/browser_zbug569342.js > >+++ b/browser/base/content/test/browser_zbug569342.js > > Why did you need to modify this test? Bug 569342 dealt with hiding the findbar for in-content chrome like about:addons. The tests expected a global findbar. Earlier versions of this modified its tests for compatibility, but still used a global findbar. This version loads the various URIs on a single tab to test the behavior of a single findbar. This also leaves intact the code that hides a tab's findbar temporarily when loading a chrome document that has attribute disablefastfind="true"; earlier patches removed it, but the multi-findbar approach doesn't need to.

Comment on attachment 771113 [details] [diff] [review] Leave the temporary hiding for disablefastfind documents, rework related tests (In reply to Adam [:hobophobe] from comment #43) > This also leaves intact the code that hides a tab's findbar temporarily when > loading a chrome document that has attribute disablefastfind="true"; earlier > patches removed it, but the multi-findbar approach doesn't need to. But this code also runs when switching tabs, which doesn't seem to make sense anymore with one findbar per tab. Maybe it should move from XULBrowserWindow.onLocationChange to TabsProgressListener.onLocationChange? >+ <method name="getActiveFindBar"> >+ <body><![CDATA[ >+ if (this.selectedTab._findBar) >+ return this.selectedTab._findBar; >+ >+ let findBar = document.createElementNS(this.namespaceURI, "findbar"); >+ >+ let browserContainer = this.getBrowserContainer(); >+ browserContainer.appendChild(findBar); >+ >+ findBar.browser = this.selectedBrowser; >+ >+ // Force a style flush to ensure that our binding is attached. >+ findBar.clientTop; >+ findBar._findField.value = this._lastFindValue; >+ this.selectedTab._findBar = findBar; >+ return findBar; >+ ]]></body> >+ </method> Can you rename this to getFindBar and let it optionally take a tab (defaulting to selectedTab)? Then I think you can use it instead of gFindBar to activate the findbar in tabs that aren't selected (see below). >+ // Handle findbar data (if any) >+ if (aOtherTab._findBar && >+ aOtherTab._findBar.findMode == aOtherTab._findBar.FIND_NORMAL) { >+ gFindBar.hidden = aOtherTab._findBar.hidden; >+ gFindBar._findField.value = aOtherTab._findBar._findField.value; >+ } The swapBrowsersAndCloseOther API doesn't require that aOurTab is selected. In case it's not selected, gFindBar is the wrong findbar. >--- a/toolkit/content/widgets/browser.xml >+++ b/toolkit/content/widgets/browser.xml >@@ -560,17 +560,18 @@ > } > // Delete the feeds cache if we're hiding the topmost page > // (as opposed to one of its iframes). > if (this.feeds && aEvent.target == this.contentDocument) > this.feeds = null; > if (!this.docShell || !this.fastFind) > return; > var tabBrowser = this.getTabBrowser(); >- if (!tabBrowser || tabBrowser.mCurrentBrowser == this) >+ if (!tabBrowser || !"fastFind" in tabBrowser || >+ tabBrowser.mCurrentBrowser == this) > this.fastFind.setDocShell(this.docShell); - !("fastFind" in tabBrowser) - Please replace mCurrentBrowser with selectedBrowser while you're touching this

(In reply to Dão Gottwald [:dao] from comment #44) > (In reply to Adam [:hobophobe] from comment #43) > > This also leaves intact the code that hides a tab's findbar temporarily when > > loading a chrome document that has attribute disablefastfind="true"; earlier > > patches removed it, but the multi-findbar approach doesn't need to. > > But this code also runs when switching tabs, which doesn't seem to make > sense anymore with one findbar per tab. Maybe it should move from > XULBrowserWindow.onLocationChange to TabsProgressListener.onLocationChange? I'd still be fine with just removing this code, since the "if the user opens the find bar and then switches tabs" part of bug 869919 comment 0 doesn't apply anymore. The user manually loading e.g. about:addons in a tab where the find bar is open seems like an edge case not worth worrying about.

This removes the findbar hiding on location change. Still modifies the test for bug 569342 slightly, as it expects the findbar to be re-shown in |testFindEnabled()|, but that's not the case now.

Comment on attachment 771505 [details] [diff] [review] getFindBar with optional parameter >+ let findBars = currentWindow.gBrowser.mTabBox.querySelectorAll( >+ "findbar"); >+ Array.forEach(findBars, function (aFindBar) { aFindBar.clear(); }); How about adding an isFindBarInitialized method to tabbrowser and using that together with gBrowser.tabs and getFindBar instead of querySelectorAll? >+ <method name="getFindBar"> >+ <parameter name="aTab"/> >+ <body><![CDATA[ >+ if (!aTab) >+ aTab = this.selectedTab; >+ if (aTab._findBar) >+ return aTab._findBar; >+ >+ let findBar = document.createElementNS(this.namespaceURI, "findbar"); >+ >+ let browserContainer = this.getBrowserContainer(); this.getBrowserContainer(aTab) >+ findBar.browser = this.selectedBrowser; this.getBrowserForTab(aTab) >+ // Handle findbar data (if any) >+ if (aOtherTab._findBar && >+ aOtherTab._findBar.findMode == aOtherTab._findBar.FIND_NORMAL) { >+ let ourFindBar = this.getFindBar(aOurTab); >+ ourFindBar.hidden = aOtherTab._findBar.hidden; >+ ourFindBar._findField.value = aOtherTab._findBar._findField.value; >+ } please start by assigning aOtherTab._findBar to a variable and then use that >+ is(gFindBar, gBrowser.selectedTab._findBar, "Find bar is right one!"); use getFindBar here since that's the public API

Comment on attachment 771545 [details] [diff] [review] Add isFindBarInitialized and use for santize.js >+ Array.forEach(gBrowser.tabs, function (aTab) { >+ if (gBrowser.isFindBarInitialized(aTab)) >+ gBrowser.getFindBar(aTab).clear(); >+ }); This seems simpler: for (let tab of gBrowser.tabs) { if (gBrowser.isFindBarInitialized(tab)) gBrowser.getFindBar(tab).clear(); } And gBrowser needs to be currentWindow.gBrowser, right? >+ let findBarCanClear = Array.some(gBrowser.tabs, function (aTab) { >+ return gBrowser.isFindBarInitialized(aTab) && >+ gBrowser.getFindBar(aTab).canClear; nit: indent the last line with another five spaces Again, it doesn't look like gBrowser should be available here. You need to assign windows.getNext() to a variable and use it here like you did in the 'clear' method. >+ <method name="isFindBarInitialized"> >+ <parameter name="aTab"/> >+ <body><![CDATA[ >+ if (!aTab) >+ aTab = this.selectedTab; >+ return aTab._findBar != null; >+ ]]></body> >+ </method> Doesn't really matter, but I'd prefer: return (aTab || this.selectedTab)._findBar != undefined; >+ findBar.browser = browser; >+ >+ // Force a style flush to ensure that our binding is attached. >+ findBar.clientTop; >+ findBar._findField.value = this._lastFindValue; setting findBar.browser requires that the binding is attached, so please move it after findBar.clientTop r=me with these changes. Thanks!

Thanks Dão! Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=7e5a6af3e99c

I'm sorry to join so late this discussion, but I didn't know about this bug. I think I should say add-on FindBar Tweak already does everything this bug is intended to address (if I understand correctly at least): - New tabs are opened with the find bar hidden - The search values and results are when switching tabs - Quick find bar more easily accessible (and customizable) All this can be optionally toggled through the add-ons preferences dialog. Of course, my implementation of this is what I guess what was the first approach by this bug, which was to save values on a per-tab basis and just update the findbar accordingly. So I don't know how much of a good substitute in that aspect my add-on would actually be. Functionally speaking, it should be exactly the same. However, judging from bug https://bugzilla.mozilla.org/show_bug.cgi?id=890584, it seems like this patch will break a lot of add-ons, while also not giving the user a choice in the matter (I believe some users like to keep the findbar between tabs, I used to actually, and only don't anymore because of my own add-ons functionality which improves on it). So, I guess all I'm saying is FindBar Tweak already does this without breaking other findbar related add-ons, at least none that I know of. Without having checked the codes changes, I do have a question though, if/when this bug lands, in your opinion do you think it would be possible/easy to simulate the old behavior through an add-on?

(In reply to Quicksaver from comment #52) > Without having checked the codes changes, I do have a question though, > if/when this bug lands, in your opinion do you think it would be > possible/easy to simulate the old behavior through an add-on? Absolutely. You could either add back a global find bar or keep track of the selected tab's find bar state and update other tabs' find bars when switching tabs. Letting users restore the old behavior without an add-on isn't a goal.

Sorry, correcting my Comment 52 to make it more clear on one point: "- The search values and results are kept when switching tabs and then switching back again" And thank you for your answer, I wouldn't think that was a goal here but it probably is a good idea not to make it very difficult for an add-on to do it either. But my doubt with keeping track of the find bar in the last tab and "copying" its state to the new tab's find bar is, since it's added lazily upon command, wouldn't it cause a performance slowdown when switching between tabs with the tab-bar open? Or is it a completely aSync' process as I would hope it is? Or did I completely misunderstand the discussion?

Nevermind my last question in Comment 54, of course it will be as aSync as I would make it to be. It was a bad question.

Not sure if this change should be advertised in the release notes. Flagging for relnote to be on the safe side.

If I want the old behavior per whole window should I file a bug for option near for example "Match case" etc. or should I wait for someones addon?

(In reply to Virtual_ManPL [:Virtual] from comment #58) > If I want the old behavior per whole window should I file a bug for option > near for example "Match case" etc. or should I wait for someones addon? There are no plans to add such an options. It's add-on fodder.

(In reply to Dão Gottwald [:dao] from comment #59) > (In reply to Virtual_ManPL [:Virtual] from comment #58) > > If I want the old behavior per whole window should I file a bug for option > > near for example "Match case" etc. or should I wait for someones addon? > > There are no plans to add such an options. It's add-on fodder. I will definitely add this to FindBar Tweak.

(In reply to Dão Gottwald [:dao] from comment #57) > Not sure if this change should be advertised in the release notes. Flagging > for relnote to be on the safe side. Note that this bug, bug 869543 and bug 776708 all target the same release.

Desktop only under tha change section on relase notes

Adding the feature keyword so that this bug is properly picked up by the Release Tracking wiki page.

With this bug fixed, ALT+N and ALT+P hotkeys no longer work in the find bar. This makes me very sad about Firefox - it is like hit to the back when features are removed from the software that you liked.

Actually, that's not because of this bug but because of bug 776708, and you can still use F3 and Shift+F3 for that.

(In reply to Luís Miguel [:Quicksaver] from comment #65) > Actually, that's not because of this bug but because of bug 776708, and you > can still use F3 and Shift+F3 for that. As well as Enter and Shift+Enter.

(In reply to Dão Gottwald [:dao] from comment #66) > (In reply to Luís Miguel [:Quicksaver] from comment #65) > > Actually, that's not because of this bug but because of bug 776708, and you > > can still use F3 and Shift+F3 for that. > > As well as Enter and Shift+Enter. Thanks for the tips guys - but how was I supposed to find this out on my own? Previously Next\Previous buttons had underlined letters in them. Anyway, alt+p and alt+n were the most convenient way, because I don't have to twist my fingers (shift+f3) or move my arms (shift+enter) right after typing. I have already posted a bug 933011 on this. I'll re-post this comment into bug 776708 also.

There is nothing wrong with changing the default Firefox behavior to anything the developers choose but since there are a large number of people who more often than not, use the same find string in multiple tabs, is it now impossible for there to be an option, just an option, to restore both CTRL+F displaying 'Next' and 'Previous' just like it used to be displayed as well as to have the Quick Find Bar be displayed in all TABs just like it used to be through userChrome.css ? More and more people are only now realizing this has been taken away and are hitting this thread only now. Could someone just post if old behavior can or cannot be restored in Firefox 25, by individuals choosing to do so?

The old behavior (global find bar) can be emulated through my add-on FindBar Tweak, by disabling preference "Findbar starts closed in new tabs"

Your Add-on appears to be able to extend the search to all TABs. And instead of Next and Previous, it offers colors on the scroll bar. I admit this is more useful and will start using your add-on from now on. As a matter of information, can you confirm that there is no way to customize existing Firefox default Quick Find Bar back to its old behavior? I understand that you are saying that no code can be typed into userChrome.css or similar to restore CTRL+F displaying *words* 'Next' and 'Previous' instead of small up and down arrows. Or to restore the same search term across all TABs.

The rest of the conversation I don't believe belongs in here, so if you'd like to discuss my add-on further, e-mailing would be better. And about the new look of the findbar, those issues are better going in bug 776708. But I'll humor you this once. Quick Find Bar is also global in my add-on with that preference disabled, it just closes when you switch tabs and that's expected. But if you press F3, it will still use that same search term as in the previous tab. This code in userChrome.css should restore the text in those buttons, but won't add back the Alt+N and Alt+P functionality: > findbar toolbarbutton[anonid="find-next"] label:after { content: 'Next' !important; } > findbar toolbarbutton[anonid="find-previous"] label:after { content: 'Previous' !important; } > findbar toolbarbutton[anonid="find-next"] label:after, > findbar toolbarbutton[anonid="find-previous"] label:after { margin-left: 2px !important; }

Remove the ">"s. Sorry.

One of the bad ideas. I want the old behavior back. I often write on different pages where i have to search for the same phrase over and over again.

(In reply to c.moeller from comment #73) > One of the bad ideas. I want the old behavior back. I often write on > different pages where i have to search for the same phrase over and over > again. The previous behaviour is not ideal. The new behaviour is designed to fix some really annoying usability issues with the old one. A more ideal fix for the new usability issues is to copy the most recent Find text when you start a Find on a page with an empty find bad.

(In reply to c.moeller from comment #73) > One of the bad ideas. I want the old behavior back. I often write on > different pages where i have to search for the same phrase over and over > again. (In reply to David Regev from comment #74) > (In reply to c.moeller from comment #73) > > One of the bad ideas. I want the old behavior back. I often write on > > different pages where i have to search for the same phrase over and over > > again. > > The previous behaviour is not ideal. The new behaviour is designed to fix > some really annoying usability issues with the old one. A more ideal fix for > the new usability issues is to copy the most recent Find text when you start > a Find on a page with an empty find bad. This is already present today. For example, on Windows you can do a find in one tab for the text "me" and then switch to a different tab (that doesn't have the findbar open), and press F3 and Firefox will search for "me" within the newly selected tab.

(In reply to Jared Wein [:jaws] from comment #75) > This is already present today. For example, on Windows you can do a find in > one tab for the text "me" and then switch to a different tab (that doesn't > have the findbar open), and press F3 and Firefox will search for "me" within > the newly selected tab. This doesn’t work consistently. Comment 73 wouldn’t have been written if it did, and I can verify that it almost never works for me either. There are already a few bugs that this one depends one dealing with some of these issues.

(In reply to David Regev from comment #76) > (In reply to Jared Wein [:jaws] from comment #75) > > This is already present today. For example, on Windows you can do a find in > > one tab for the text "me" and then switch to a different tab (that doesn't > > have the findbar open), and press F3 and Firefox will search for "me" within > > the newly selected tab. > > This doesn’t work consistently. Comment 73 wouldn’t have been written if it > did, and I can verify that it almost never works for me either. There are > already a few bugs that this one depends one dealing with some of these > issues. The add-on FindBar Tweak already has a few behavior fixes for some of these issues out-of-the-box, making the multiple find bars across tabs more consistent. Also, comment 69.

I understand that old behavior can be restored with an addon, however I didn't really like the idea to add a bloated addon (sorry Quicksaver), but I would prefer something really simple just to restore old global findbar behavior... also relying on addons to "fix" core features it's no sense to me, not to talk that addons frequently go unsupported with time, get problems with future versions, etc, etc... for such a simple thing IMO there should be **at least** a native about:config option to control it, if not an option on options window. Very sad to see Mozilla imposing a feature (again), doesn't matter if some users like it or not, just to mimic other browsers :( On good old days Firefox was fully customizable on these simple things on about:config options, but nowadays this is not a reality anymore... seeing the path that things go I would not be surprised if about:config actually get's removed on a future version.

I do not know ... But for people who want to have findbar in permanence, would it be possible to do the same? : http://www.webm6.com/images/capture9.png

Comment 69.

As usual (so many times in the last years that it becomes annoying) you are removing something that is a _feature_ and not a bug without giving the possibility to choose. Firefox has tens of useless preference but when it comes the moment to add something useful preference Mozilla always chooses to not listen to the users, the result is a nonsense from usability and look POV. Other user have already talked about usability, but what is the reason to make worse even the look? Why you move two buttons to the right and leave two button and the textbox on the left? Why the previous and next button are so small and so close? All that room in the center of the bar is needed for... ? Were the icons in the button so ugly? As usual we have to install the umpteenth addon, making FF heavier and slower to load, only to restore a preexisting feature and we have to hope in the good will of a developer for an update every time that a new FF version comes out (here, again, uselessly frequent) and we also have to hope that God will maintain that developer in good health and his life won't become too complicated (work, wife, kids need much time and a Mozilla addon is the last thing in order of importance).

Yet again a few close-minded devs bunched up and “fixed” up the product some more. Even here, on the bug tracker, the place that an absolute majority of your users likely never even heard of, you've had actual user voices indicating they prefer the old behavior, and instead of giving your users a choice, which is the reason most of us started using ffx in the first place, you screwed us over. Here's a workflow for you: I research something, I open multiple tabs, then I search for some — same — text in them. That accounts for about 90% of my use of the on-page search. So thanks for nothing. As for the add-ons, I already have quite a few to fix your “improvements”: image style, menu editor, status, UI fixer. Sooner or later those add-ons are likely to stop being maintained, and then I really am screwed. If unwarranted and unneeded changes continue, we may as well give up on the whole thing and switch to Chrome, or, god forbid, IE, since they give us just as little choice as you do recently.

If this cannot be reverted, I propose that an option exist to get back old functionality. https://bugzilla.mozilla.org/show_bug.cgi?id=953259

I understand some people may want a per tab behaviour, but by no way does it mean the other behaviour is wrong, and you can't even say such a change is approved by a majority of users — come on, this report has *8 votes*, how does it compare to the number of Firefox users ? As usual, an option was the way to go. Please stop implementing new features by removing already existing features.

It is time to calm down for both sides :) I offer Bug 961916 to resolve this situation, and even to advance FF a bit. (otherwise I would vote for restoration of the previous behavior)

Is there any good reason that a pref hasn't been added to get back the old behavior? This change is a strict loss of functionality, with no gains.

If this change broke your workflow like it did mine, vote for bug 920031.

Anybody not happy with this change can revert to the old behavior with this addon: https://addons.mozilla.org/en-US/firefox/addon/globalfindbar/

STRONGLY agree re: this having been yanked out from under us. There are many other bugs related to this one, e.g., https://bugzilla.mozilla.org/show_bug.cgi?id=920031 , so I'm confused re: status of restoring this critical feature.

I concur with Skippy le Grand Gourou on this, and left a note on the <a href="https://bugzilla.mozilla.org/show_bug.cgi">Bug 961916</a> page.

Lol, there has to be a better way to deal with these functionality changes based on nothing but personal opinion other than treating them as bugs. The solution to this one has reduced the usefulness of the product unnecessarily. What a waste of time and effort this shaped up to be. For starters, the assertion is untrue: "When you try to find something you are doing it on the current page. Different tabs usually represent different activities. Same goes for different windows.". Essentially, that would mean that when trying to find information on a topic, users tend to only visit one site at a time. Preposterous. I usually try to keep one "activity" in a single window (anymore than 2 or three activities going on and I start to lose track of what I'm doing). Sometimes I get as many as 30 tabs open related to a single activity. Sometimes I pop open a second window so I can compare related information side by side. Am I really an edge case? The two unrelated issues cited are: 1. doesn't like to see the same search term on other tabs 2. it was taking up screen realestate Number one is about as invalid as you can get. Number 2, well someone should have just found a solution that doesn't break existing behavior and functionality (time out after 5 mins and hide it for instance). I also don't think discussing technical limitations of the product in the bug report to try and sort of negotiate a solution to any perceived problem makes for a quality bug report. I can mostly live with the new behavior, inconvenient as it is. However, hitting F3 when the find bar is missing provides me with no feedback as to what I'm searching for, if anything. Has anyone logged a bug for that? Going back to tabs where the search term is no longer what I'm interested in finding, I have no idea really what F3 is doing unless it finds a hit.