Closed Bug 1358500 Opened 6 years ago Closed 6 years ago

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

Categories

(Toolkit :: Find Toolbar, defect)

Product:
Toolkit
Component:
Find Toolbar
Version:
53 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox53 + wontfix
firefox54 + verified
firefox55 + verified

People

(Reporter: drmoose94, Assigned: mikedeboer)

References

(
URL
)

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1358500 - Don't disable the findbar buttons when the search string is not found.
59 bytes, text/x-review-board-request
Details
Bug 1358500 - fix find bar being disabled all the time,
59 bytes, text/x-review-board-request
mikedeboer
: review+
jcristau
: approval-mozilla-beta+
Details 
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.
Component: Untriaged → Find Toolbar
Product: Firefox → Toolkit
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.
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.
Blocks: 935521
Status: UNCONFIRMED → NEW
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
status-firefox-esr52: --- → unaffected
Ever confirmed: true
Flags: needinfo?(mdeboer)
Keywords: regression
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"
URL: data:text/html;charset=utf-8,%3Cdiv%3...
[Tracking Requested - why for this release]:
Serious UX regression in find bar behaviour.
tracking-firefox54: --- → ?
tracking for 54 and 55.
tracking-firefox54: ? → +
tracking-firefox55: --- → +
I'll make this work.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
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.
Attachment #8861950 - Flags: review?(gijskruitbosch+bugs)
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.
tracking-firefox53: --- → +
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. :-)
Attachment #8861950 - Flags: review?(gijskruitbosch+bugs)
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. :/
Too late for 53 at this point. Mike, Gijs, any chance to land a fix here in time for 54 uplift?
status-firefox53: affected → wontfix
Flags: needinfo?(mdeboer)
Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
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.
Attachment #8872377 - Flags: review?(mdeboer) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b89704b3bc19
fix find bar being disabled all the time, r=mikedeboer
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...
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...
Flags: needinfo?(mdeboer) → needinfo?(gijskruitbosch+bugs)
(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.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mdeboer)
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.
Flags: needinfo?(mdeboer)
Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b94437de6db9
fix find bar being disabled all the time, r=mikedeboer
https://hg.mozilla.org/mozilla-central/rev/b94437de6db9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Attachment #8872377 - Flags: approval-mozilla-beta?
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.
Attachment #8872377 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Needs rebasing for Beta.
Flags: needinfo?(gijskruitbosch+bugs)
(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
status-firefox54: affected → fixed
Flags: needinfo?(gijskruitbosch+bugs)
Flags: qe-verify+
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
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
status-firefox55: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.