1358500 - "Phrase not found" locks out Find buttons when a new page is loaded

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect)

Description

[drmoose94]

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170413192749

Steps to reproduce:

1. Navigate to any given page.
2. Press Ctrl+F and enter a string that is not present in the page.
3. Navigate to a new page by clicking a link or using the back button.


Actual results:

The "phrase not found" message from the previous Find remains into the new page, even if the phrase does exist in the new page. The up/down find buttons are also locked out, preventing the search being repeated on the new page.


Expected results:

The "phrase not found" message should at least disappear, reflecting the fact that the new page has not yet been searched, and the search buttons should be available.

Comment 1

[Loic]

Yes, it's really a bad regression because up/down arrows (for next/previous occurrence) of the Find Toolbar stay inactive when you browse from a 1st page where the string is missing to a 2nd page where the string is present. Therefore the user can't find the string and needs to restart the Find Toolbar to find it on the 2nd page.

Comment 2

[Loic]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5e76768327660437bf3486554ad318e4b70276e1&tochange=85a9d908e91a7071bbb1f554b275fb1b4e796e34

Mike de Boer — Bug 935521 - properly disable the find next/ previous buttons on the find toolbar and update their appearance when disabled. r=Gijs

Mike, what do you think? IMHO this new behaviour is a bad design, because the user needs to close/reopen the Find Toolbar in the same tab to find the occurrence.

Comment 3

[Loic]

Simple testcase:

1) Load data:text/html;charset=utf-8,%3Cdiv%3E%3Ca%20href%3D%22data%3Atext%2Fhtml%2C%253Cdiv%253Esomething%253C%252Fdiv%253E%22%3Enothing%3C%2Fa%3E%3C%2Fdiv%3E
2) Open the Find Toolbar and search for "something" (no result)
3) Click on the link "nothing"

Comment 5

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
Serious UX regression in find bar behaviour.

Comment 6

[Julien Cristau [:jcristau]]

tracking for 54 and 55.

Comment 7

[Mike de Boer [:mikedeboer]]

I'll make this work.

Comment 8

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1358500 - Don't disable the findbar buttons when the search string is not found.

Since bug 935521 introduced a disabled state for the findbar buttons, we also
overhauled and fine-tuned their behavior. The case we missed there was that when
the findbar is open on a page, but the with its state NOT_FOUND thus the buttons
disabled, and the user navigates to another page, the buttons would not re-enable.

Review commit: https://reviewboard.mozilla.org/r/133954/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/133954/

Comment 9

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=bd11561eb861c449b6a61cbdf458d7766f31fb97

Comment 10

[:Gijs (he/him)]

Comment on attachment 8861950 [details]
Bug 1358500 - Don't disable the findbar buttons when the search string is not found.

https://reviewboard.mozilla.org/r/133954/#review136862

If we do do this, I'd like us not to have to invent our own messaging here.

I'm a little confused by the same document check. Do we have other code that catches changes in the same document, especially for the history.pushState() case, or just generally if you look for something and then the rest of the document arrives / more DOM gets inserted from a pending XHR request, or something like that?

Ultimately, what I'm starting to wonder is whether disabling those buttons the right solution, if (when not disabled) they are the most direct way a user can force the search to be re-tried after the content on the page changes. Maybe we should just leave them enabled, or style them differently but still allow them to force a restart of the search when clicked, or something?

::: toolkit/modules/RemoteFinder.jsm:259
(Diff revision 1)
> +  onLocationChange() {
> +    this._global.sendAsyncMessage("Finder:LocationChange");
> +  },

Can we avoid adding extra messages for this and reuse the RemoveWebNavigation and/or browser binding handlers instead? We already propagate this event from the content to the parent process, we shouldn't need to reinvent that.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Once this lands let's verify that and at least uplift to 54. 
I would consider it as a ridealong fix if we have a 53 desktop dot release.

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8861950 [details]
Bug 1358500 - Don't disable the findbar buttons when the search string is not found.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/133954/diff/1-2/

Comment 14

[:Gijs (he/him)]

Comment on attachment 8861950 [details]
Bug 1358500 - Don't disable the findbar buttons when the search string is not found.

https://reviewboard.mozilla.org/r/133954/#review143048

There's a bunch of orange, so I think this patch isn't quite finished yet.

Also, in bug 935521 we moved the enabling/disabling of the find buttons to the find call finishing, instead of immediately when the find bar value changes (e.g. https://hg.mozilla.org/mozilla-central/rev/89101092aea1#l2.64 ) and I expect that that should now be reverted, given that the disabled state only changes based on the text field input. Is there another reason not to do that?

In fact, is there a good reason not to just revert the non-styling changes in the patches from 935521 ?

::: toolkit/content/tests/browser/browser_findbar.js:214
(Diff revision 2)
> +    let promise = new Promise(resolve => {
> +      let listener = () => {
> +        browser.messageManager.removeMessageListener("Content:LocationChange", listener);
> +        resolve();
> +      };
> +      browser.messageManager.addMessageListener("Content:LocationChange", listener);
> +    });

BrowserTestUtils.waitForLocationChange ?

::: toolkit/content/tests/browser/browser_findbar.js:221
(Diff revision 2)
> +    await ContentTask.spawn(browser, null, async function() {
> +      const {document} = content;
> +      document.getElementsByTagName("a")[0].click();
> +    });

```js
await BrowserTestUtils.synthesizeMouse("a");
```

or whatever that's called. :-)

Comment 15

[Loic]

This bug is super annoying when using regularly the findbar on various pages with the same keyword, really bad to not have it in 53.0.x. :/

Comment 16

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 53 at this point. Mike, Gijs, any chance to land a fix here in time for 54 uplift?

Comment 17

[:Gijs (he/him)]

Attachment: Bug 1358500 - fix find bar being disabled all the time,

Review commit: https://reviewboard.mozilla.org/r/143878/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/143878/

Comment 18

[Mike de Boer [:mikedeboer]]

Comment on attachment 8872377 [details]
Bug 1358500 - fix find bar being disabled all the time,

https://reviewboard.mozilla.org/r/143878/#review147610

backout is fine.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b89704b3bc19
fix find bar being disabled all the time, r=mikedeboer

Comment 20

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/d594a107a7ad for failures like https://treeherder.mozilla.org/logviewer.html#?job_id=102858293&repo=autoland

Comment 21

[:Gijs (he/him)]

Comment on attachment 8872377 [details]
Bug 1358500 - fix find bar being disabled all the time,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/143878/diff/1-2/

Comment 22

[:Gijs (he/him)]

Mike, I fixed the issue in comment #20 by reverting the reverts to that test. Can you check that that actually makes sense and/or doesn't point to something that's actually broken? I'm a little confused why there's any change in behaviour at all - the test assertions in question, AFAICT, are for whether the highlight button is checked, and I didn't think changing the disabled state of the next/prev find buttons would affect it...

Comment 23

[Mike de Boer [:mikedeboer]]

But at this point, wouldn't it just make sense to go with my patch on top of the current revision? I'm not super thrilled that our other cleanups and changes have to be reverted as well...

Comment 24

[:Gijs (he/him)]

(In reply to Mike de Boer [:mikedeboer] from comment #23)
> But at this point, wouldn't it just make sense to go with my patch on top of
> the current revision? I'm not super thrilled that our other cleanups and
> changes have to be reverted as well...

Well, don't the other changes mean that it will take until the find operation completes for the buttons to become enabled/disabled, because we don't call updateControlState() until that point? Whereas previously the button state was updated immediately when the input field contents changed. The latter seems more correct, given that the state of the buttons now won't depend on the result of the find operation.

Comment 25

[Mike de Boer [:mikedeboer]]

I guess, but ISTR that shorlander liked it better that way. Anyways, I'm not in favor of punting on this, so let's just land your patch. Your changes looks reasonable to me.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b94437de6db9
fix find bar being disabled all the time, r=mikedeboer

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b94437de6db9

Comment 28

[:Gijs (he/him)]

Comment on attachment 8872377 [details]
Bug 1358500 - fix find bar being disabled all the time,

Approval Request Comment
[Feature/Bug causing the regression]: bug 935521
[User impact if declined]: find bar buttons stay disabled too long in some circumstances
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: not yet
[Needs manual test from QE? If yes, steps to reproduce]: yes, see comment #0
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: a little bit
[Why is the change risky/not risky?]: it's basically mostly a backout of the logic parts (but not the styling parts) of bug 935521, and in that sense it is relatively safe.
[String changes made/needed]: no.

Comment 29

[Julien Cristau [:jcristau]]

Comment on attachment 8872377 [details]
Bug 1358500 - fix find bar being disabled all the time,

fix a regression in find bar, beta54+

Should be in 54.0b13.  We should get some testing around that area once this lands to uncover any fallout.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing for Beta.

Comment 31

[:Gijs (he/him)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #30)
> Needs rebasing for Beta.

I hate this yield/await switch some days...

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/86339b41e6e98b5e0580ae19ffbe6c85a54bfc9f

Comment 32

[Emil Ghitta, QA [:emilghitta]]

I have reproduced the issue mentioned in comment 0 using Firefox 55.0a1 (Build Id:20170421030241).

This issue is verified fixed on Firefox 55.0a1 (Build Id:20170606030207) and Firefox 54.0 (Build Id:20170605134926)on Windows 10 64bit, macOS 10.11.6 and Ubuntu 14.04 32bit