913536 - Opening findbar only assumes previous value the first time

Status: NEW

(Toolkit :: Find Toolbar, defect)

Description

[Luís Miguel [:quicksaver] (not reading bugmail)]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0 (Beta/Release)
Build ID: 20130814063812

Steps to reproduce:

I just noticed something.

For example:
1) Open a tab. Open the find bar and perform any search.
2) Open another tab. Open the find bar, it will automatically assume the search value used in the first tab.
3) Erase the search value and close the find bar.
4) Go back to the first tab. Search for something else.
5) Go to the second tab, open the find bar, it will be empty.

Even if you change the page location on the second tab, it will never again assume the last search value used when you open it.


Actual results:

Since bug 537013, when the find bar is created for the first time, it always assumes the last find value used. Shouldn't this happen every time the find bar is opened when its search query is empty?


Expected results:

I would have expected that, on step 5, the find bar would once again assume the last search value used.

To the user, it will seem like "Ok, so if I open the find bar without having run any search on it before, it will assume the last search." then he erases the search, closes it and opens it again later expecting the same thing to happen. Or maybe I'm wrong, but at least that's what I would expect as an user. Or, in alternative, never assume the last find value, only to make it more coherent behavior IMO.

Comment 1

[Drew Willcoxon :adw]

Confirming, but I don't think this is a bug.  The per-tab find bar seems to remember whatever you typed per tab.  That includes deleting the text.  As a special case when you're opening a tab's find bar for the first time, it uses the text from the last find bar you used as a (dubious) convenience.  So for example, if step 3 said "type another search" instead of "erase the search value," you'd see that new search when you revisit the tab in step 5.

Comment 2

[Luís Miguel [:quicksaver] (not reading bugmail)]

Yes, that's true, and it's good that the find bar remembers what happens on a per-tab basis (that was one of the objectives of implementing a per-tab find bar after all).

However, imagine that you run a search on a tab for something. You find what you want, and you erase the value and close the find bar because you don't need it anymore. Then you resume your browsing without ever closing that tab. At some point, hours later, that "remember that you erased the find query" part loses its value, because it will hardly be relevant at that point anymore.

Comment 3

[Luís Miguel [:quicksaver] (not reading bugmail)]

(I was not finished writing... it submitted a comment on its own?)

When you close the find bar with an empty query and re-open it, it will stay empty. If the objective is to always remember that there hasn't been a search run on it, then shouldn't that be the case as well for when opening it for the first time? Technically, when you open it for the first time, there hasn't been any search run on that tab either, shouldn't it "remember" that as well? The way it is now is nothing more than an assumption by firefox that the user would want to re-use the search value on that specific case only. That was either done deliberately, following a discussion to make it so, or is an unforeseen effect from the implementation of bug 537013.

I don't mean that it is a good or bad thing that it is happening this way, I'd just want to clarify the situation and make sure that it really is that way that firefox should act. From reading the comments in that bug, I get the feeling this was initially done to emulate in part what happened previously (well, what happens now with the "old" find bar, where the search value is shared from the previous tab to the next), but this specific case didn't seem to be discussed. In short: to emulate "assume last find value used when starting a find action from fresh", but personally I consider "opening the find bar with an empty query" also to fall into that category, regardless of how many times it is done in each tab. Hence my doubt.

Comment 4

[u490389]

This is the break of a killer feature for me. It immediately stood out after the automatic upgrade that I did not ask for (FF is ignoring the "Check for updates, but let me choose" pref). Was this really something that we users were clamoring for? It's a deep, fundamental feature and I'm really disappointed that it's been broken for my workflow.

Comment 5

[Adam]

Attachment: bug913536_20140220.patch

Dão, when you have a chance to look I'm curious whether you think this is worth pursuing (either as-implemented, the "recently used finds" thing, both, or something else). Should I ask for UX opinion?

What this patch does is a little more than this bug:

1. If an existing findbar is empty (has |_findField.value == ""|), then we give it the last-used value. Note that you only have to clear it and then use another tab's find; you do not have to close the findbar. (I didn't add a test for this yet.)
2. Stores the last-used instead of the last-seen value (by listening for "find" and "findagain" events at the |window| level). This part is desired by e.g., bug 920031 and bug 920972. (I did update the tests from bug 537013 for this.)

[1] has a bit of a weird workflow, needing to explicitly clear the value, then use another findbar. But it's a fairly simple case, and I don't see harm in it.

With [2] we lose the ability to look around for the right value (by switching tabs) and then use it, but that's not great UI anyway. The gain is that it includes quickfind values and may be the more expected/consistent behavior users want. Downside is the use of extra event handling.

We probably actually want "recently used finds" (e.g., a popup (and probably autocomplete) which is populated with more than one of the recent find values so the user can select the right one). That is a hairier and heavier change to implement. If that is implemented later, this patch would be compatible with that approach (and we'll still want to prefill with the most plausible value). I can file a bug for that and work on it if wanted.

Comment 6

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Adam [:hobophobe] from comment #5)
> Created attachment 8379400 [details] [diff] [review]
> bug913536_20140220.patch
Just a couple of things I noticed about the patch.

1) Not all find events should update lastFindValue, as they can be preventDefault()'d, thus those don't actually carry a "last used" value, only a sort of "last input" value; PDF.JS comes to mind as an example that calls preventDefault() on them (although the value is still carried over). I'm mentioning this more from the principle of existing a possibility that these values can be wrong, rather than actual cases.

Maybe an onFindResult listener for the Finder.jsm module would be better suited?

2) I don't think the find field should be filled with the lastFindValue on every |gFindBar| call, like it would do in the patch. For example, gFindBar.hidden is a common check for the find bar state, it would be useless filling it here (and maybe counter-productive even?); or when calling gFindBar.close(), more than useless here, I think it'd look weird when you hit the x button in an empty find bar, and it suddenly has text during the closing animation. And these are just from the top of my head, and again, more of an "in principle" thing, I don't think either of these cases would actually break anything usage-wise.

Comment 7

[Adam]

Attachment: Address feedback from Quicksaver

Thanks for the input, Quicksaver! I was mostly adding the previous patch as a proof-of-concept, but I've rolled an improved version anyway based on your feedback.

What this version of the patch does:

1. (On switch to tab) If a findbar is inited, and |_findField.value == ""|, populate it with |_lastFindValue| and set a new field |_findValueSpeculative|. (Added a test for this.)
2. (On "find{,again}" if |!aEvent.defaultPrevented|) We set a new field |_setLastFindOnSwitch|.
3. (On switch away from tab) If |_setLastFindOnSwitch && _findField.value != ""|, update |_lastFindValue|. If |!_setLastFindOnSwitch && _findValueSpeculative|, clear |_findField.value|.

Note that this means you can now "shop around" for find values on a cleared findbar (provided you actually use the find on the other tab). This may be confusing, though:

1. Clear tab A's open findbar
2. Switch to tab B
3. Use tab B's findbar ("foo")
4. Switch back to A (shows "foo")
5. Switch to tab C
6. Use tab C's findbar ("bar")
7. Switch back to tab A (shows "bar")

Your thoughts, Quicksaver? Again, I feel like this won't be as bad if we do add "recently used finds", but as-is it will still have some cases where we miss what the user wants/confuse them.

I'm going to forgo the feedback flag and leave this needinfo for Dão to avoid flag churn if I make further changes.

Dão, I'm just wondering if this or some other approach (e.g., the "recently used finds" idea) should be pursued.

Comment 8

[Luís Miguel [:quicksaver] (not reading bugmail)]

(In reply to Adam [:hobophobe] from comment #7)
> Created attachment 8380021 [details] [diff] [review]
> Address feedback from Quicksaver
I like it, especially how findValueSpeculative resets the find bar back to blank if it's not used, so it can be re-filled with an updated value when switching back to it again. I've got nothing against the behavior of the current patch, even as still only a proof-of-concept thing, it's not raising any red flags for me, behavior-wise!

I still think the "find(again)" listeners activity would be better done by an onFindResult listener for Finder.jsm, but that technicality can be left for a final patch of course.

> I feel like this won't be as bad if we do
> add "recently used finds", but as-is it will still have some cases where we
> miss what the user wants/confuse them.
Because any "automatic filling" of the find bar only happens when it is blank, I think users might not be as confused as you think. I think that's the part that's most important: only assume a lastFindValue when the find bar would be opened/used blank. This way it doesn't sacrifice its per-tab characteristic, and it can still be used across tabs if the user cleans it. That's my opinion at least, but perhaps there's a more elegant way to go about it?

The exception might be quick find, where the bar closes automatically without clearing the find query, and the user might assume (when switching back and forth between tabs) its value is reset (because it's closed) when he uses another tab's find. Honestly... I've got nothing for this. :)

Side-note: personally, I would welcome the "recently used finds" feature; even if this ends up being decided against, I will probably just code it myself into FindBar Tweak, I was already planning to do that actually. But these behavioral changes shouldn't be held back in detriment of adding this feature. You mentioned before the previous patch would work with such a feature, and I think the both would go best together anyway. But I think that's your intention, so cool.

Comment 9

[Greg]

This behavior was instituted on purpose by Firefox some time ago. Only God knows why.

I HATE this "Find in page" behavior. the "Find in page" search box should remember the last search as long as the window stays open. It should remember it across tabs. Each new tab should remember the last search done. Mozilla, PLEASE change the "Find in page" behavior back to something sensible. Using Find in firefox has been a miserable experience since you made this change.


(In reply to Luís Miguel [:Quicksaver] from comment #0)
> User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101
> Firefox/23.0 (Beta/Release)
> Build ID: 20130814063812
> 
> Steps to reproduce:
> 
> I just noticed something.
> 
> For example:
> 1) Open a tab. Open the find bar and perform any search.
> 2) Open another tab. Open the find bar, it will automatically assume the
> search value used in the first tab.
> 3) Erase the search value and close the find bar.
> 4) Go back to the first tab. Search for something else.
> 5) Go to the second tab, open the find bar, it will be empty.
> 
> Even if you change the page location on the second tab, it will never again
> assume the last search value used when you open it.
> 
> 
> Actual results:
> 
> Since bug 537013, when the find bar is created for the first time, it always
> assumes the last find value used. Shouldn't this happen every time the find
> bar is opened when its search query is empty?
> 
> 
> Expected results:
> 
> I would have expected that, on step 5, the find bar would once again assume
> the last search value used.
> 
> To the user, it will seem like "Ok, so if I open the find bar without having
> run any search on it before, it will assume the last search." then he erases
> the search, closes it and opens it again later expecting the same thing to
> happen. Or maybe I'm wrong, but at least that's what I would expect as an
> user. Or, in alternative, never assume the last find value, only to make it
> more coherent behavior IMO.

Comment 10

[DJiK]

Is this the same as https://bugzilla.mozilla.org/show_bug.cgi?id=920972 ?

This is very annoying when searching text in different tabs. How is it possible this bug is still open after years???

Comment 11

[Tracy Walker [:tracy]]

Manual testcase(s) for this exist in our test repository.  They have been automated in the Iris test suite and are currently disabled due to this bug.