Skip to content

Fix impersistent highlight visibility - #21535

Open
LoukasPap wants to merge 2 commits into
mozilla:masterfrom
LoukasPap:fix/false-enable-of-highlights
Open

Fix impersistent highlight visibility#21535
LoukasPap wants to merge 2 commits into
mozilla:masterfrom
LoukasPap:fix/false-enable-of-highlights

Conversation

@LoukasPap

@LoukasPap LoukasPap commented Jul 3, 2026

Copy link
Copy Markdown

Fixes #21498

The fix ensures that the highlight visibility is persistent across all pages regardless updates in editor mode or scrolling while the "Show all" toggle button always reflects the correct state.

The three integration tests cover the following cases:

Test 1

  1. Open highlight editor
  2. Highlight text ("Abstract")
  3. Click "Show all" toggle to disable (hide highlights)
  4. Close highlight editor
  5. Open ink editor
  6. Switch to highlight editor
  7. Assert: Show all button is still disabled (aria-pressed="false")
  8. Assert: Highlight remains hidden

Test 2

  1. Open highlight editor
  2. Highlight text ("Abstract")
  3. Click "Show all" toggle to disable (hide highlights)
  4. Close highlight editor
  5. Double-click to select text
  6. Highlight it from floating toolbar
  7. Close highlight editor
  8. Re-open highlight editor
  9. Assert: Show All is still disabled (aria-pressed="false")
  10. Assert: Highlight is still hidden

Test 3

  1. Open highlight editor
  2. Highlight text (“Abstract”)
  3. Scroll to page 14
  4. Highlight text (“Reference”)
  5. Close highlight editor
  6. Re-open highlight editor
  7. Scroll back to page 1
  8. Assert: Highlights in page 1 are still hidden

Comment thread test/integration/highlight_editor_spec.mjs Outdated
@codecov-commenter

codecov-commenter commented Jul 4, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 80.00000% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.85%. Comparing base (a97d0aa) to head (4d5fca8).
⚠️ Report is 49 commits behind head on master.

Files with missing lines Patch % Lines
src/display/editor/tools.js 75.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #21535      +/-   ##
==========================================
+ Coverage   89.83%   89.85%   +0.01%     
==========================================
  Files         263      263              
  Lines       66802    66806       +4     
==========================================
+ Hits        60014    60027      +13     
+ Misses       6788     6779       -9     
Flag Coverage Δ
browsertest 66.51% <0.00%> (+0.02%) ⬆️
fonttest 9.05% <ø> (ø)
integrationtest 69.25% <80.00%> (+<0.01%) ⬆️
unittest 57.52% <0.00%> (+0.01%) ⬆️
unittestcli 56.48% <0.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@LoukasPap

Copy link
Copy Markdown
Author

@calixteman I applied the changes discussed

layer.updateMode(mode);
}

const highlightShowAllState = this.#showAllStates?.get(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for drawing my attention to these cases! I'll check them out.

@LoukasPap
LoukasPap marked this pull request as draft July 22, 2026 13:48
@LoukasPap
LoukasPap force-pushed the fix/false-enable-of-highlights branch from a7645cc to 9ab860e Compare July 25, 2026 22:00
@LoukasPap
LoukasPap force-pushed the fix/false-enable-of-highlights branch from 9ab860e to 4d5fca8 Compare July 25, 2026 22:01
@LoukasPap
LoukasPap marked this pull request as ready for review July 25, 2026 22:02
for (const editor of this.#allEditorsIterator) {
editor.enableEditing();
editor.show(true);
editor.show();

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calixteman When a page layer was turned back on, every editor was shown again and so did the highlights.

@calixteman

Copy link
Copy Markdown
Contributor

It doesn't work unfortunately, here's a STR:

  • open freetexts.pdf;
  • switch to freetext mode;
  • modify an existing one;
  • switch to reading mode;
  • switch to freetext mode.

The modified freetext is no more visible.

An other problem, here's a STR:

  • open comments.pdf at page 1;
  • switch to highlight mode and hide all highlights;
  • scroll to page 6.

A highlight is visible.

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.

[Bug]: Opening editor toolbar falsely enables hidden highlights

4 participants