leave search terms highlighted if user scrolls#13442
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) ✅ license/snyk check is complete. No issues have been found. (View Details) |
Just to be clear, this behavior is something we could test with playwright. |
|
Well, then I’ll have to add that to the list of things I don’t know how to do. Sorry. |
|
@vezwork Since you've recently looked into quarto-search, can you spot check this PR? I'm generally supportive of it but want to make sure we're not missing anything obvious. |
|
I didn't know we had our own implementation of search highlighting (I have been investigating browser built-in highlighting like text fragment urls). For my own work, I'll have to look into this. In regards to the code change in this PR: yea this looks good. I don't see why we would want to clear the search highlighting when the user scrolls away from it, and it is causing this search highlighting not to work at all. Neither the comments nor the blame on the original code explain a design reason why. This change may not be perfect, but I think it'll be better than how it was! |
|
Thanks for the PR btw @jtbayly ! Much appreciated |
I am seeing now that #14049 is fixing the highlight, but without removing this I don't know also why this was done, I only found 2d3225d at the time, and assume it was on purpose for nice UI, to not keep some highlighted However, I am thinking that removing this scroll detection would be better experience, as a user could still reload the page to remove all marks if needed. (or we could add a modal button floating around to "clear search highlights" 🤷♂️ ) Anyhow, if we merge this I need to modify #14049 then and rebase #14053 |
cderv
left a comment
cderv
left a comment
There was a problem hiding this comment.
Thanks @jtbayly — this is the right fix. Removing the scroll-based clearing entirely is simpler and more robust than trying to work around the timing issues.
This also addresses the root cause of #14047 (search highlights cleared by layout events before users can see them). We'll add Playwright test coverage for this in a follow-up PR (#14049).
PR #13442 removed scroll-based highlight clearing. This commit adds Playwright tests verifying: scroll events never clear marks, query-change clearing still works, and no marks appear without ?q= parameter. Restores test fixture files dropped during rebase (source files were in the skipped JS fix commit). Updates changelog to credit @jtbayly and reference both #9802 and #14047. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The previous test dispatched quarto-hrChanged/quarto-sectionChanged custom events, which were leftovers from when search code listened to those events. Since #13442 removed those listeners entirely, dispatching those events tested nothing. Replace with actual scroll behavior which is the real user scenario from #14047. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…14049) * Add .gitignore for search-highlight test fixture * Add test verifying search highlight listeners register after delay Complements the persistence test by confirming that quarto-hrChanged does clear marks after the 1000ms registration delay elapses. * Add changelog entry for search highlight fix * Add test coverage for search highlight persistence (#14047, #9802) PR #13442 removed scroll-based highlight clearing. This commit adds Playwright tests verifying: scroll events never clear marks, query-change clearing still works, and no marks appear without ?q= parameter. Restores test fixture files dropped during rebase (source files were in the skipped JS fix commit). Updates changelog to credit @jtbayly and reference both #9802 and #14047. * Simplify scroll persistence test to use actual scrolling The previous test dispatched quarto-hrChanged/quarto-sectionChanged custom events, which were leftovers from when search code listened to those events. Since #13442 removed those listeners entirely, dispatching those events tested nothing. Replace with actual scroll behavior which is the real user scenario from #14047. * Use role-based locators instead of Algolia CSS classes Replace .aa-DetachedSearchButton and .aa-Input with ARIA role locators (getByRole('button'), getByRole('searchbox')). These are resilient to Algolia autocomplete class name changes and follow Playwright best practices used elsewhere in the test suite. --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
5 participants
Welcome to the quarto GitHub repo!
We are always happy to hear feedback from our users.
To file a pull request, please follow these instructions carefully: https://yihui.org/issue/#bug-reports
If you're a collaborator from outside
quarto-devmaking changes larger than a typo, please make sure you have filed an individual or corporate contributor agreement. You can send the signed copy to jj@rstudio.com.Also, please complete and keep the checklist below.
Description
Currently search term highlighting is completely broken and has been for quite some time:
#9802
This pull request fixes the problem by not bothering to clear the highlights just because the user scrolled the page. There might be a number of places on the screen or page that contain the search term, and they should all remain highlighted as the user scrolls. And if anybody disagrees, I would only say that it is better for them to remain highlighted than to never be highlighted at all. :)
Checklist
I have (if applicable):