Fix false enable of highlights when opening the editor menu#21535
Fix false enable of highlights when opening the editor menu#21535LoukasPap wants to merge 1 commit into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #21535 +/- ##
==========================================
+ Coverage 89.71% 89.73% +0.01%
==========================================
Files 262 262
Lines 66709 66713 +4
==========================================
+ Hits 59847 59863 +16
+ Misses 6862 6850 -12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
timvandermeij
left a comment
timvandermeij
left a comment
There was a problem hiding this comment.
Code-wise this patch LGTM with the comments addressed, but I'd prefer @calixteman also review this to given his familiarity with this particular code.
After addressing the review comments, please keep the commits squashed into one; see https://github.com/mozilla/pdf.js/wiki/Squashing-Commits if you're not familiar with how to do that.
Thank you for your contribution!
|
I'll wait for @calixteman to see the PR too and address all issues together (and then squash). Thank you for reviewing! |
|
|
||
| await page.waitForSelector(".editToolbar"); | ||
| const highlightFromToolbar = await page.$( | ||
| ".floatingToolbar button[data-action='highlight']" |
There was a problem hiding this comment.
Is it an AI hallucination ?
.floatingToolbar exists only in the geckoview and is unrelated to highlighting.
There was a problem hiding this comment.
I sometimes called it floatingToolbar having seen it in editor/toolbar.js:
pdf.js/src/display/editor/toolbar.js
Line 265 in 8bc2fe6
Changed it to .editToolbar
| const highlightFromToolbar = await page.$( | ||
| ".floatingToolbar button[data-action='highlight']" | ||
| ); | ||
| if (highlightFromToolbar) { |
There was a problem hiding this comment.
I don't understand this check by itself... except that it's fixing the floatingToolbar mentioned issue.
So this code is just dead code.
There was a problem hiding this comment.
The code locates the highlight button from the editToolbar (or floatingToolbar) and verifies it exists. If for example after selecting a text, the editToolbar hadn't show up for a reason, then the highlightFromToolbar button would not exist.
We could rename it to editToolbarHighlightBtn to be more clear.
There was a problem hiding this comment.
It must show up, if it didn't then something is wrong in your test.
You can wait for having it visible and then get it.
b441120 to
a7645cc
Compare
|
@calixteman I applied the changes discussed |
| layer.updateMode(mode); | ||
| } | ||
|
|
||
| const highlightShowAllState = this.#showAllStates?.get( |
There was a problem hiding this comment.
I don't think it's enough in general.
With tracemonkey.pdf, you can
- highlight on the 1st page
- scroll and highlight on the last one
- scroll back to the first
- hide the highlights
- turn off the highlight mode
- turn it on
- scroll to the last page.
4 participants
Fixes #21498
The fix ensures that the "Show all" toggle button remains consistent across updates in editor mode and it reflects the correct highlight state (even if we highlight from floating toolbar).
The two integration tests cover the following cases:
Test 1
Test 2