1905331 - Findbar in PDF viewer doesn't play "not found" sound

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, enhancement)

Description

[bootleq]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:127.0) Gecko/20100101 Firefox/127.0

Steps to reproduce:

1. With below about:config settings

• accessibility.typeaheadfind.enablesound => true (default)
• accessibility.typeaheadfind.soundURL => "beep" (default)

2. Open a PDF file, by enter below URL
   https://bootleq.github.io/etc/findbar/sample.pdf
3. Hit Ctrl+F to open the findbar, then type firr (the last r can't be found)

Actual results:

Findbar shows "Phrase not found", and NO sound was played.

Expected results:

The "not found" sound "beep" is played after typing last r.

Comment 1

[bootleq]

Attachment: Bug 1905331 - Move findbar sound handling to new JS module, r=NeilDeakin

The "not found" sound was initially implemented in cpp, then reworked
again in FinderParent (mjs) during fission migration.

Now completely move the implemention to JS as a new module. Make it
easier to be integrated by find controllers, also reduce typeaheadfind's
unnecessary responsibility.

To be clear, the FinderParent.sys.mjs affects the general findbar in
web pages, while Finder.sys.mjs affects non-remote pages e.g., chrome
pages like about:about (previously handled by nsTypeAheadFind.cpp).

Comment 2

[bootleq]

Attachment: Bug 1905331 - Add findbar "not found" sound to PDF view, r=NeilDeakin,#pdfjs-reviewers

Comment 3

[bootleq]

Where to test various Findbars:

(1/3) Most general findbar, handled by FinderParent.sys.mjs

• Find in general web page.
  Below is a sample page with subframes, both cross-origin or not
  https://bootleq.github.io/etc/findbar/frames/

• Find content in "contenteditable" area.
  Also use above sample page to test.

• Find in "View Page Source" page.
  view-source:https://bootleq.github.io/etc/findbar/frames/

• Raw text file
  https://raw.githubusercontent.com/mozilla/gecko-dev/149da4336adfb168f1a7ddcfd106ab746e4268a4/README.txt

• Privileged about: pages
  about:certificate

• Extension page
  Enter about:addons and open preference page of any addon.
  For example below addon has a standalone options page
  https://addons.mozilla.org/firefox/addon/content-farm-terminator/
  The hash in URI is not same for each user, I got:
  moz-extension://ccb0d664-cad2-40a0-9fa2-23a6777f8e95/options.html

(2/3) Non-remote findbar, handled by Finder.sys.mjs

• about pages, for example about:about

  See isRemoteBrowser about what "remote" means.
  https://searchfox.org/mozilla-central/source/dom/interfaces/base/nsIBrowser.idl#52

(3/3) Findbar in PDF view, handled by PdfStreamConverter.sys.mjs

• Open PDF in Firefox
  https://bootleq.github.io/etc/findbar/sample.pdf
  Note there is also PDF.js' built-in findbar, which is out of scope of this change
  https://mozilla.github.io/pdf.js/web/viewer.html

(4). Others situations like specific preferences change

• typeaheadfind related

  • accessibility.typeaheadfind.enablesound (default: true)
  • accessibility.typeaheadfind.soundURL (default: beep)

• findbar.modalHighlight (default: false)
  This feature is going to be removed.
  https://bugzilla.mozilla.org/show_bug.cgi?id=1872349
  I didn't find a way to enable it, so didn't test it.

Comment 4

[bootleq]

Attachment: Bug 1905331 - Add test for findbar sound, r=NeilDeakin,#pdfjs-reviewers

Add MockSound to replace nsISound in test. Allow us to track play
calls by additional exposed played array.

Comment 5

[Neil Deakin]

The test fails when I run it locally:

toolkit/modules/tests/browser/browser_FinderSound.js
FAIL "beep" notfound sound - Structures begin differing at:
got[0] = Does not exist
expected[0] = "beep"

Stack trace:
chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.isDeeply:1983
chrome://mochitests/content/browser/toolkit/modules/tests/browser/browser_FinderSound.js:test_notfound_sound_with_preferences:27
...

Comment 6

[bootleq]

(In reply to Neil Deakin from comment #5)

The test fails when I run it locally:

toolkit/modules/tests/browser/browser_FinderSound.js
FAIL "beep" notfound sound - Structures begin differing at:
got[0] = Does not exist
expected[0] = "beep"

It passes on my side, command ./mach test toolkit/modules/tests/browser/browser_FinderSound.js with code rebased after today's build after commit dee87b3ded.

Do we have remote CI to see test result, or we use email or chat to further investigate?

Comment 7

[Neil Deakin]

I did a test run at https://treeherder.mozilla.org/jobs?repo=try&revision=175e96755cab6938e8bc66b6eded0e5aa56d3439

Here it fails consistently on Linux. It also fails in verify mode (the ones labelled TV). You can check this by using the --verify flag when running the test.

Comment 8

[bootleq]

Thanks, I've reproduced failures and made fixes.
Latest diff can be reviewed at https://phabricator.services.mozilla.com/D215306?vs=911592&id=913684#toc

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5632d6b41ee6
Move findbar sound handling to new JS module, r=NeilDeakin
https://hg.mozilla.org/integration/autoland/rev/93742758f826
Add findbar "not found" sound to PDF view, r=pdfjs-reviewers,robwu
https://hg.mozilla.org/integration/autoland/rev/e495bf9b0749
Add test for findbar sound, r=NeilDeakin,pdfjs-reviewers,robwu

Comment 10

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/5632d6b41ee6
https://hg.mozilla.org/mozilla-central/rev/93742758f826
https://hg.mozilla.org/mozilla-central/rev/e495bf9b0749