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

Fix for #7468 - Cleared all highlights when find bar is closed #7501

Closed
wants to merge 2 commits into from
Closed

Fix for #7468 - Cleared all highlights when find bar is closed #7501

wants to merge 2 commits into from

Conversation

OmarShehata
Copy link

This is a fix for this issue: #7468

In the PDFFindBar_close function, I added a loop to go through and clear all the highlights from all the pages. I wasn't sure whether to do it directly in that function, or create a wipeAll function in pdf_find_controller.js, so I just went with the most straightforward option. Let me know if this needs to be reworked in any way or have any feedback!

I also fixed a small grammatical typo in the comments. "Once the textLayer is built" instead of "Once the textLayer is build"

@Snuffleupagus
Copy link
Collaborator

Thanks for submitting the patch!
However, as is, this is introducing inconsistent behaviour between closing of the PDF.js findbar, respectively closing the Firefox findbar (note that we're using the browser findbar in the Firefox addon/built-in version of PDF.js).
I think that we need to decide if we actually want to fix #7468, and if so the behaviour really needs to be consistent between the two findbar implementations.

@OmarShehata
Copy link
Author

Thanks for the clarification @Snuffleupagus !

I personally think it makes sense to clear the highlights on closing the findbar, but that's probably I got used to Adobe Reader's behavior doing that. I'll this as is, as a reference for anyone until a decision is made.

@qwer1304
Copy link

In latest pdf.js fd242ad this issue is back:
Search for anything
Close the search bar

Searched text is still highlighted

@Snuffleupagus
Copy link
Collaborator

This was superseded by PR #10100; @timvandermeij mind closing this one?

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

Successfully merging this pull request may close these issues.

4 participants