Populate the find field with the search query when URL has #search hash

[phillipj]

These changes improves the existing search functionality triggered when the URL contains a #search hash.

In addition to performing the actual search immediately like before, this will ensure the search text field inside the find bar gets populated with the text currently being searched for.

Picture says more than a thousand words; the goal is for tesla being populated in the search field below because pdf.js was loaded with #search=tesla in the URL:

Does this look like a viable approach to you?

Kudos to @marill for finding this workaround.

[timvandermeij]

/botio-linux preview

[pdfjsbot]

From: Bot.io (Linux m4)

---

Received

Command cmd_preview from @timvandermeij received. Current queue size: 0

Live output at: http://54.67.70.0:8877/400325baad11d3d/output.txt

[pdfjsbot]

From: Bot.io (Linux m4)

---

Success

Full output at http://54.67.70.0:8877/400325baad11d3d/output.txt

Total script time: 3.36 mins

Published

• Viewer: http://54.67.70.0:8877/400325baad11d3d/web/viewer.html
• Viewer (ES5): http://54.67.70.0:8877/400325baad11d3d/es5/web/viewer.html

[timvandermeij]

Looks good to me. Thank you!

[phillipj]

Appreciate the quick response!

Just noticed what gives an impression findBar.findField could be null at times...

pdf.js/web/pdf_find_bar.js

Line 33 in c5663f2

this.findField = options.findField || null;

Would that ever happen in practise when app.js is executed? If so, I'll happily open a follow-up PR that ensures these changes does not cause TypeError: Cannot set property 'value' of null going forward.

[Snuffleupagus]

Sorry, but why was this patch accepted when it introduces inconsistent behaviour depending on supportsIntegratedFind and thus indirectly the build target (e.g. GENERIC vs. MOZCENTRAL)?

Previously we've rejected these kind of changes, because of the inconsistency they would bring, especially considering that this (for security reason) cannot be easily implemented in the Firefox PDF viewer; please note prior discussion in e.g. issues #3565, #8185, and #9831.

[phillipj]

@Snuffleupagus I wasn't aware of those concerns, thanks for bringing that up.

Sounds like these changes as is should be reverted from master.

Do you have any advice for me to work on improvements that addresses these inconsistencies? Alternatively your recommended approach for doing what was intended with these changes?

[Snuffleupagus]

Sounds like these changes as is should be reverted from master.

This obviously isn't a huge and/or breaking change, so I don't know what the consensus will be regarding a back-out.
(It might simply have been good to have the discussion beforehand, but mistakes can happen since we're all human :-)

Hence I suppose that this could stay, as far I'm concerned, if and only if:

• It's agreed upon that this small behaviour inconsistency is OK, in this case.
• We don't treat this as a precedent for introducing additional/larger inconsistencies, for different build targets, for the find functionality.

Do you have any advice for me to work on improvements that addresses these inconsistencies?

I don't think there's a good/secure way of handling this generally, in the Firefox browser findbar as well, since #3565 (comment) ought to apply here as well (having a web-page control the contents of the browser findbar seems wrong).

[phillipj]

As I obviously can't comment on if's your raising, I can at least share the use case we've got in the project I'm working on -- consider or ignore as you please.

Our end-users finds PDF documents either by navigating to them or as part of the product's search results.

The off the shelf Viewer is embedded via an <iframe> and we set #search= accordingly when we want PDF.js to go into "search mode" immediately and focus the first occurrence.

Without the changes in this PR, the user experience is not totally intuitive though. End-users obviously appreciate the convenience of text matches being highlighted right away. But it's confusing when they open the find bar and the text field is empty -- although it is has obviously searched for something seeing text is highlighted (like tesla in the PR description example).

---

Inspired by the Third party viewer usage wiki I've found another workaround which isn't too bad. Though from a PDF.js rookie and using-project's perspective, it is still awkward that code needs to be written fill in the find text field 🤷‍♂️

document.addEventListener("webviewerloaded", function(event) {
  const PDFViewerApplication = event.detail.source.PDFViewerApplication;

  PDFViewerApplication.initializedPromise.then(function() {
    PDFViewerApplication.findBar.findField.value = "tesla"; // same value as provided in URL's #search hash
  });
});

Would you be more positive to these changes if they were located in ./web/viewer.js instead of ./web/app.js? Or would that still affect the Firefox browser as you mentioned?

[Snuffleupagus]

Would you be more positive to these changes if they were located in ./web/viewer.js instead of ./web/app.js?

That would be a lot worse, in every way, given that web/viewer.js is used only for configuration/loading purposes :-)

There's only one decision to be made here: Either we let these changes stay as-is (since it's a small patch), or we revert them wholesale (and reject any future attempt at changing this).

Or would that still affect the Firefox browser as you mentioned?

Sorry, I really don't understand this question.
The point here is merely that the changes made in this PR unfortunately cannot be applied to the Firefox PDF viewer (which is the reason for this library existing in the first place), since it uses the native browser findbar, for the reasons mention at the bottom of #12141 (comment).

[timvandermeij]

I forgot about the integrated findbar here; sorry for that. I do think that the change makes sense because having a search query enabled yet not seeing it in the findbar is indeed inconsistent.

However you have a valid point about us rejecting such changes before, so making an exception in this case would also be inconsistent.

I'll make a PR to revert this change for now and open an issue where we can discuss how to move forward with the findbar since I do feel that the lack of customizability of the integrated one is holding us back sometimes as evidenced by the various PRs that were made before. I personally don't mind them being a bit inconsistent because they are never used together, but I think this is a discussion for outside of this PR.

[Snuffleupagus]

I do think that the change makes sense because having a search query enabled yet not seeing it in the findbar is indeed inconsistent.

Completely agreed; I've never questioned the utility of the feature itself, just how it's implemented (or not) across various build targets.

I personally don't mind them being a bit inconsistent because they are never used together,

Agreed, and in this case the difference is small enough to probably not matter much...

---

It should be possible, without too much trouble, to support pre-populating the Firefox findbar only when #search=term is used on initial document loading. However, that'd likely be even worse than the current situation of never updating the Firefox findbar at all.

[timvandermeij]

I have reverted the change in #12148 pending the discussion in #12149. Once a decision is made there, this can either simply be merged again or it won't be introduced anymore.

[phillipj]

Sounds good to have a dedicated discussion settling the overall policy for find bar changes and behaviour going forward 👍

I won't be participating in the discussion as my PDF.js incompetence is obvious. Feel free to ping me if you want input from an using project' point of view.

[burotica49]

It is possible to do it this way

<script>
    document.addEventListener("webviewerloaded", function(event) {
      const queryString = window.location.hash;
      const search = queryString.replace('#search=', '');
      const PDFViewerApplication = event.detail.source.PDFViewerApplication;

      PDFViewerApplication.initializedPromise.then(function() {
        PDFViewerApplication.findBar.findField.value = search; // same value as provided in URL's #search hash
      });
    });
  </script>