1601118 - Mana page's Find/Replace feature no longer works in Firefox 63+

Status: VERIFIED FIXED

(Toolkit :: Find Toolbar, defect, P2)

Description

[Chris Peterson [:cpeterson]]

STR

1. Load an editable Mana page, such as https://mana.mozilla.org/wiki/display/Firefox/Fission
2. Click the "Edit" button to edit the page.
3. Press CTRL+F to open the Mana editor's Find/Replace toolbar.
4. Enter a search term (such as "Fission") in the Find field and click the "Next" button.

Expected Result

The first instance of "Fission" should be highlighted in yellow. Pressing "Next" again should highlight the next instance.

Actual Result

Some instance of "Fission" is highlighted in yellow, but not necessarily the first instance on the page. Pressing "Next" does not move the highlight to any other instances of the word "Fission" on the page.

I don't see any exceptions or error messages in the DevTools web or browser consoles.

This is a regression in Firefox 63. The Mana editor's Find/Replace also broke and was fixed back in Firefox 47 (bug 1257444).

@ Emilio, could your Shadow DOM changes in bug 1455891 have broken Mana's Find/Replace?

I bisected this regression to the following pushlog regression range, but mozregression bug 1596201 prevents me from bisecting to narrower regression range:

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=afdeb0288690f0acde2ba6bd008f860e1acdd026&tochange=9849ea3937e2e7c97f79b1d2b601f956181b4629

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Is the mana source available somewhere? Yeah, those changes look the most likely culprit. I wish somebody would've added a test in bug 1257444... :)

Comment 2

[Chris Peterson [:cpeterson]]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #1)

Is the mana source available somewhere? Yeah, those changes look the most likely culprit.

Mana is just Mozilla's instance of Atlassian's Confluence wiki, which is a proprietary product. I asked Mozilla's Mana admins (in Mozilla's #mana-support Slack channel) if our Confluence license gives us access to Atlassian's source code.

https://www.atlassian.com/software/confluence

Comment 3

[Chris Peterson [:cpeterson]]

Emilio, if there is some specific Confluence question or code you need, our Mana admins have contacts with Confluence tech support. Just let me know.

Comment 4

[Pascal Chevrel:pascalc]

Marking as wontfix for 71 as we ship this regression since 71. fix-optional for 72 to remove it from our weekly regression triage, it's unlikely we would take an uplift though. I am leaving 68ESR as affected as Confluence is enterprise software and we might take a safe fix in a future 68ESR release.

Comment 5

[(no longer active)]

test test

Comment 6

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

Mike, can you set a priority or find an owner for this issue? It's not a recent regression, but I agree with Pascal's point that we should support this common enterprise software if we can.

Comment 7

[Mike de Boer [:mikedeboer]]

This has nothing to do with the find backend, but is something local to Atlassian's implementation of that feature - which very much looks like it uses the find backend, but they implemented their own.
Therefore tentatively moving to Web Compat, but could be that Emilio knows more.

Please note that regular find-in-page using Firefox's findbar works as expected.

Comment 8

[Mike Taylor [:miketaylr]]

Emilio, let us know if we can help dig in here.

Comment 9

[Jens Stutte [:jstutte]]

:miketaylor, is it still unclear how to move this forward (tracked as carry-over regression for 74) ?

Comment 10

[Mike Taylor [:miketaylr]]

Sorry, fell off the radar.

Ksenia, can you look into this please?

Comment 11

[Ksenia Berezina [:ksenia] on leave until 09/15]

Attachment: 1601118.html

I've attached a reduced test case. To reproduce the behavior click "Next" button.

The problem here is that window.find(...) doesn't switch to the next item if range.surroundContents(element) is used on a current selection range.

Emilio, would you be able to take a look?

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Oh that test is really awesome. Yeah, will take a look, thank you!

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1601118 - Fix nsFind state initialization logic. r=mats,smaug

We should start seeking from the child-at-start/end-offset, rather than the
selection container.

The logic to select the starting node was backwards too...

If we're finding backwards we want to start looking from the beginning of the
range and vice versa. But I think that doesn't really matter since
nsWebBrowserFind always gives us a start range with start == end.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

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

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9a03944aff45
Fix nsFind state initialization logic. r=smaug

Comment 16

[Alexandru Michis [:malexandru]]

Backed out changeset 9a03944aff45 for causing AssertionError in geckoview.test.FinderTest

Backout link: https://hg.mozilla.org/integration/autoland/rev/5cf34865fa418f444a18e04000a0c0dc89f23823

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=android%2C7.0%2Cx86-64%2Copt%2Ctest-android-em-7.0-x86_64%2Fopt-geckoview-junit-e10s%2C%28gv-junit%29&fromchange=9a03944aff4517c7300c5d1505a69a9b0d4faf92&selectedJob=289899593

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=289899593&repo=autoland&lineNumber=3209

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f1fdc659bdb
Fix nsFind state initialization logic. r=smaug

Comment 18

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/2f1fdc659bdb

Comment 19

[Julien Cristau [:jcristau]]

Is this worth uplifting at this stage? (Tomorrow's the last beta for 74)

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Probably not, I wouldn't say the patch is trivial.

Comment 21

[Julien Cristau [:jcristau]]

Thanks Emilio!

Comment 22

[Chris Peterson [:cpeterson]]

Plus, this bug was a regression in Fx63, so we've been shipping it since 2018-10-23. :)

Comment 23

[Bogdan Maris, Desktop Test Engineering]

Reproduced the initial issue using an old 70.0 RC build. Verified that the issue does not reproduce on Fx 75.0b5 across platforms (Windows 10 64bit, macOS 10.13.6 and Ubuntu 18.04 64bit) using both mana and the testcase attached to this bug.