Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Determine our policy for find bar changes #12149

Closed
timvandermeij opened this issue Jul 31, 2020 · 4 comments
Closed

Determine our policy for find bar changes #12149

timvandermeij opened this issue Jul 31, 2020 · 4 comments
Labels

Comments

@timvandermeij
Copy link
Contributor

timvandermeij commented Jul 31, 2020

The viewer contains a custom find bar, but we also support the integrated find bar in Firefox. While this might provide a better integration, it does lead to maintenance issues since the integrated find bar in Firefox is practically uncustomizable. This has led to users requesting valid features that we cannot fulfill without introducing a difference in behavior between the custom and the integrated find bar.

The most recent example is #12141, but issues #3565, #8185 and #9831 are also examples of this.

I personally think that supporting the integrated find bar was beneficial and easier before, but is nowadays holding us back from making further changes that users request and that make sense. I therefore open this issue for discussion on this topic.

I see the following options:

  1. Keep a strict policy that differences between the custom/integrated find bar are not accepted. This is what we do now, but as said I feel like it's holding back development.
  2. Accept differences between the implementations. This seems fair to me since a user never uses both find bars at the same time and it would allow us to fulfill fair user requests again.
  3. Drop the integrated find bar support altogether. Aside from simplifying the code and preventing the problems described here, I also don't really see a clear advantage nowadays to use the integrated find bar over the custom one since both do the same and the custom one is even styled nicely according to the PDF.js style, and thus "blends in" more.

I personally favor option 3, but if that is not preferred option 2 seems best to me. I feel like option 1 is no longer viable.

What do you think @brendandahl @Snuffleupagus and others?

(If we decide to go for option 2 or 3, we should introduce #12141 again since it's reverted pending the outcome of this discussion.)

@Snuffleupagus
Copy link
Collaborator

Snuffleupagus commented Aug 1, 2020

Keep a strict policy that differences between the custom/integrated find bar are not accepted. This is what we do now, but as said I feel like it's holding back development.

Honestly, I do think that this is a reasonable policy and it doesn't really bother me all that much given that the features in question are (somewhat) minor. Also, keeping in mind that the Firefox PDF viewer is the main reason for this library existing in the first place.

Accept differences between the implementations. This seems fair to me since a user never uses both find bars at the same time and it would allow us to fulfill fair user requests again.

Assuming that the differences are indeed very small, I suppose that this might be OK on a case-by-case basis.

Drop the integrated find bar support altogether. Aside from simplifying the code and preventing the problems described here, I also don't really see a clear advantage nowadays to use the integrated find bar over the custom one since both do the same and the custom one is even styled nicely according to the PDF.js style, and thus "blends in" more.

Using the Firefox browser findbar, together with the Firefox PDF viewer, has the distinct advantage of making the PDF viewer feel "part of the browser" itself (rather than being a completely separate entity). Hence I completely understand why you want to use the browser findbar, and would really advice against taking this route.

Edit: Also, simply using the browser findbar, besides the improved UI/UX-consistency around search in Firefox, means that whatever way the findbar may be triggered (keyboard shortcut, menuitem, ...) it will always be handled by the PDF.js find controller.
Trying to forcibly disable the browser findbar when the PDF viewer is open would probably be difficult, and cannot be made to work well in general as far as I can see. For example: What about the case where a HTML page is open in Firefox, and the browser findbar is also open, and the user now navigates to a PDF file (e.g. by a clicking a link or typing in the address bar). Should we now forcibly close the browser findbar, if that's no longer used with the PDF viewer. That really doesn't seem like great UX, and this hopefully illustrates why I believe that this point is not a good idea.


Generally speaking: I do believe that the best course of action here would be for someone to actually spend the necessary time looking into good/safe ways of solving issues such as #3565 and #8185 for the Firefox browser findbar integration.
Writing the code would be (almost) trivial, it's determining what to do about things like #3565 (comment) that's the difficult part.

@brendandahl
Copy link
Contributor

An integrated findbar was a pretty early requirement in Firefox and I don't think that has changed. I think the main thing keeping us back from #2 is testing for the integrated version and web version. If they're both thoroughly tested I'd be fine with them diverging more.

@Snuffleupagus
Copy link
Collaborator

Keeping the PDF.js findbar consistent with the native Firefox findbar has, over the years with changes/improvements to the PDFFindController, significantly (as far as I'm concerned) simplified maintenance of the relevant code.

Hence I'd suggest, as before, that we settle on option 1 from #12149 (comment) since that's the simplest solution and carries the least risk of introducing bugs (or confusion inconsistencies) in the find-related code.

@timvandermeij
Copy link
Contributor Author

timvandermeij commented Sep 10, 2022

Let's go for option 1 then since if the native findbar is still useful it makes things easier if they are in sync indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants