1316516 - Findbar: Please back out the dangerous "New findbar" code at least in ESR 52, as it causes undetectable bugs (because of hidden regression bug 384458)

Status: RESOLVED INVALID

(Toolkit :: Find Toolbar, defect)

Description

[arni2033]

Intro:
When it became clear that the "New findbar" is too buggy (probably even by-design) to have it in
release, all patches in bug 384458 were backed out.
Unfortunately, that wasn't a real back out: that functionality was simply hidden behind a pref. It was
done w/ a bug/mistake, so a lot of work related to the "New findbar" affects the good old findbar.
IMO breaking a feature for several releases is the worst way to introduce an update of that feature.

Main part:
What I'm suggesting is EITHER to completely remove "New findbar" code from Firefox 51+ (assuming
there's not enough time for Firefox 50), OR create a new separation between good and new code that
would guarantee that good old findbar will stay untouched no matter what you do with the "New findbar"

Arguments:
I also made 5 assumptions (some may be wrong)
1) There's not enough resources to test good old findbar on Nightly and fix it
2) There may be more bugs in good old findbar that I haven't detected, triggered by bug 384458 & Co
3) Replacing the code to its old (already known) version is easier than messing with new bugs
4) There's no real reason to break it in ESR 52, and I'm pretty sure it wasn't according to the plan
5) Developer(s) won't be able to "quickly" finish the "New findbar" and enable it in ESR 52

The final decision is up to developers, who have a better understanding.

(Unrelated:   I personally wish Mozilla killed "New findbar" with the same passion it killed Panorama)

Comment 1

[Mike de Boer [:mikedeboer]]

(In reply to arni2033 from comment #0)
> Intro:
> When it became clear that the "New findbar" is too buggy (probably even
> by-design) to have it in
> release, all patches in bug 384458 were backed out.
> Unfortunately, that wasn't a real back out: that functionality was simply
> hidden behind a pref. It was
> done w/ a bug/mistake, so a lot of work related to the "New findbar" affects
> the good old findbar.
> IMO breaking a feature for several releases is the worst way to introduce an
> update of that feature.

The bugs you filed are all about the 'Highlight All' feature. I'm not going to debate whether they're bugs; they certainly are! But please consider the fact that the 'Highlight All' feature was _very_ buggy to start with.
There was a good reason _why_ it wasn't toggled ON by default (in no particular order);
a. unidentified, unspecified, untested behavior
b. slow and inconsistent results.

Even though it may seem that all the changes under the hood were only good for the new highlighting mode, the opposite is in fact the case: I wanted the feature to finally become good enough to have turned on by default!
The new look and feel is not even the primary motivation to me.

As the current owner of this component, I'm very grateful to have been granted the resources to work on this task and remove/ replace a lot of rotting code from the findbar. I'm also grateful that you are testing, using and because of that finding bugs that we've over-looked.
(As an aside: because of community members like you we have a unique opportunity to reach a quality level that's higher than our possible competitors!)

> Main part:
> What I'm suggesting is EITHER to completely remove "New findbar" code from
> Firefox 51+ (assuming
> there's not enough time for Firefox 50), OR create a new separation between
> good and new code that
> would guarantee that good old findbar will stay untouched no matter what you
> do with the "New findbar"
> 
> Arguments:
> I also made 5 assumptions (some may be wrong)
> 1) There's not enough resources to test good old findbar on Nightly and fix
> it

There are, but since we don't have enough test coverage for a component as old as the findbar and the excellent folks at Softvision (part of our QE team that has been doing manual testing for this feature) are only human :)
In other words: unfortunately, the status quo is that there are cracks and stuff falls through. It's my job to fix the things that we couldn't find ourselves but someone else could.

> 2) There may be more bugs in good old findbar that I haven't detected,
> triggered by bug 384458 & Co

Highly unlikely - it's more likely that the 'Highlight All' feature is in a better shape now than it was before.

> 3) Replacing the code to its old (already known) version is easier than
> messing with new bugs

There have been so many follow-ups that a backout is not straightforward. The only way we can back things out is a hard reset of the component to its state in the 49 branch, but than we might not catch all the tests that were changed. It's definitely not something I'm looking forward to and I think we can avoid since the fixes for the bugs you filed seem good candidates for uplift, since they're quite small.

> 4) There's no real reason to break it in ESR 52, and I'm pretty sure it
> wasn't according to the plan

Breakage is perhaps over-stating it a wee-bit. I agree with you that there are bugs and, like I mentioned above, I'm not going to debate that whatsoever. Let's see where we are when I fixed, landed & uplifted the bugs you filed.

> 5) Developer(s) won't be able to "quickly" finish the "New findbar" and
> enable it in ESR 52

It's just silly old me :) Well, not really true: I depend on my colleagues Gijs and jaws a lot to help me out.

> The final decision is up to developers, who have a better understanding.
> 
> (Unrelated:   I personally wish Mozilla killed "New findbar" with the same
> passion it killed Panorama)

It's a matter of taste, perhaps. I've heard more opinions that are of the opposite kind. We'll see. Since it's a toolkit component - meaning it has to work reliably for products other than Firefox too - there will always be two modes of highlighting. And the way we have it now, it's not a high maintenance burden.

Comment 2

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

I'd like to add that the severity of regressions in "Highlight all" feature of "Find toolbar" varies based on the web control you highlight and search on. Two examples below:

STR1:
1. Open https://bugzilla.mozilla.org/attachment.cgi?id=8806186&action=edit
2. Ctrl + F
3. Search for "diff --" and highlight all
4. Search for "cpp".
Notice it highlights all instances of "diff --" and "cpp" on that attachment web control.

STR2:
1. Open https://bugzilla.mozilla.org/show_bug.cgi?id=1313740
2. Ctrl + F
3. Search for "status" and highlight all
4. Search for "affected".
This time the only string that is highlighted is "affected" and not both "status" and "affected".

As I said, the severity of the find toolbar -> highlight all regression varies depending on the web control being used.

Comment 3

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Andrew Overholt and I have reviewed this new regression in 50 and also Mike's detailed assessment in comment 1. I do not believe this is so severe as to block 50. I hope we can get some fixes ready soon, stabilized on Beta51 channel and uplift to the planned 50.1.0 release that ships mid-December.

Comment 4

[Ricardo]

This:
- Highlight vanishes when find bar is closed. No older version I remember did that.

And this:
1. Enable "Highlight All" and type something to find. Good we have things highlighted.
2. Close find bar. Highlights vanish.
3a. F3 will go next word without highlights.
3b. Ctrl+F to open find bar. No highlight. We need to disable and re-enable "Highlight All".

Using "Dim the page" (findbar.modalHighlight) we have new issues:
- Darker background on black text reduces visibility of context. Usability issue.
- Zoomed text overlaps texts around. Then, if we search part of a word, the word could be entirely tapped.
- Possibly much more usability issues.

Comment 5

[Mike de Boer [:mikedeboer]]

Hi Ricardo, please file separate bugs for the (new) issues you found! They might already be tracked by bug 1291278, but filing bugs is cheap :)

We will not be backing out the code at this point.

Comment 6

[Ricardo]

Sorry, I'm not a bug filler, and don't feel comfortable using this system. Feel free to fill them for me if you think they need bugs filled. Thanks.

[Imo, proper testing and design shouldn't allow these things pass unnoticed.]