Skip to content

Ensure TextLayer hiddenCanvasElement is layout-neutral by default#21218

Merged
calixteman merged 1 commit into
mozilla:masterfrom
RolandWArnold:fix/text-layer-hidden-canvas-layout-neutral
May 14, 2026
Merged

Ensure TextLayer hiddenCanvasElement is layout-neutral by default#21218
calixteman merged 1 commit into
mozilla:masterfrom
RolandWArnold:fix/text-layer-hidden-canvas-layout-neutral

Conversation

@RolandWArnold
Copy link
Copy Markdown
Contributor

TextLayer creates a helper canvas with class hiddenCanvasElement and appends it directly to document.body, which can adversely affect the host-page layout/scrolling behavior.

The stock viewer stylesheet web/pdf_viewer.css already makes this element layout-neutral via .hiddenCanvasElement { ... display: none; }, but direct pdfjs-dist / TextLayer consumers are not required to load web/pdf_viewer.css. Consumers that scope the viewer stylesheet to avoid global CSS leakage also lose this protection, since the helper canvas is appended outside the scoped viewer subtree.

This change applies the essential layout-neutralizing rule at the creation site, so the display-layer helper does not depend on viewer CSS to avoid participating in host-page layout.

Related context:

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented May 3, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.50%. Comparing base (7ade637) to head (16d82e0).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master   #21218      +/-   ##
==========================================
+ Coverage   79.48%   79.50%   +0.01%     
==========================================
  Files         254      254              
  Lines       64859    64860       +1     
==========================================
+ Hits        51553    51565      +12     
+ Misses      13306    13295      -11     
Flag Coverage Δ
fonttest 8.64% <ø> (ø)
integrationtest 66.18% <100.00%> (+<0.01%) ⬆️
unittest 55.47% <50.00%> (+0.01%) ⬆️
unittestcli 54.72% <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 Sentry.
📢 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.

Copy link
Copy Markdown
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Given that using the textLayer requires also using the pdf_viewer.css file, since otherwise things are simply not guaranteed to work, it's not clear (at least to me) why this effectively duplicated code should be added here!?

@RolandWArnold
Copy link
Copy Markdown
Contributor Author

Thanks for the quick review.

I understand that TextLayer is expected to be used with web/pdf_viewer.css. The case I was trying to cover here is that this particular rule is unusual because the element it targets is not inside the viewer subtree: TextLayer creates the canvas and appends it directly to document.body.

Consumers building custom viewer chrome may include the viewer CSS but scope/rewrite its selectors to avoid leaking PDF.js viewer styles into the surrounding application. In that setup, the rest of the text-layer CSS is present, but .hiddenCanvasElement can be scoped along with everything else and no longer matches the body-appended helper canvas.

That's why I thought applying the essential layout-neutralizing property at the creation site might be appropriate: the body-appended helper would not depend on a viewer-subtree stylesheet rule to avoid participating in host-page layout. react-pdf also had to add the .hiddenCanvasElement rule downstream in wojtekmaj/react-pdf#1815.

If the project’s position is that TextLayer consumers must load web/pdf_viewer.css without selector scoping, and scoped or rewritten viewer CSS is unsupported, I’m happy to close this.

Comment thread src/display/text_layer.js
@RolandWArnold RolandWArnold force-pushed the fix/text-layer-hidden-canvas-layout-neutral branch 3 times, most recently from fcfd0f3 to 3b05d2e Compare May 5, 2026 14:22
Copy link
Copy Markdown
Collaborator

@Snuffleupagus Snuffleupagus left a comment

Choose a reason for hiding this comment

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

Please also remember to write a good commit message, since a single line is rarely sufficient.

Comment thread src/display/text_layer.js Outdated
Comment thread web/pdf_viewer.js
@RolandWArnold RolandWArnold force-pushed the fix/text-layer-hidden-canvas-layout-neutral branch from 3b05d2e to ceb7875 Compare May 5, 2026 18:34
@RolandWArnold
Copy link
Copy Markdown
Contributor Author

Updated again:

  • removed the now-unused hiddenCanvasElement class
  • kept hiddenCopyElement’s id because test/integration/copy_paste_spec.mjs uses #hiddenCopyElement to wait for/query the copy helper element
  • expanded the commit message

@Snuffleupagus
Copy link
Copy Markdown
Collaborator

/botio browsertest

@moz-tools-bot
Copy link
Copy Markdown
Collaborator

From: Bot.io (Windows)


Received

Command cmd_browsertest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/5780beff9529d55/output.txt

@moz-tools-bot
Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_browsertest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/05f99bdd87ab526/output.txt

@moz-tools-bot
Copy link
Copy Markdown
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/05f99bdd87ab526/output.txt

Total script time: 18.64 mins

  • Regression tests: FAILED
  different ref/snapshot: 1

Image differences available at: http://54.241.84.105:8877/05f99bdd87ab526/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Copy Markdown
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/5780beff9529d55/output.txt

Total script time: 24.59 mins

  • Regression tests: FAILED
  different ref/snapshot: 1

Image differences available at: http://54.193.163.58:8877/5780beff9529d55/reftest-analyzer.html#web=eq.log

Set layout-neutral styles at the creation sites for the hidden TextLayer
canvas and PDFViewer copy element rather than relying on the shared
web/pdf_viewer.css rule.

This keeps the helper elements invisible and out of layout when viewer CSS
selectors are scoped or omitted, and removes the obsolete hiddenCanvasElement
class and shared CSS rule.
@RolandWArnold RolandWArnold force-pushed the fix/text-layer-hidden-canvas-layout-neutral branch from ceb7875 to 16d82e0 Compare May 14, 2026 13:31
@Snuffleupagus Snuffleupagus requested a review from calixteman May 14, 2026 14:45
@RolandWArnold
Copy link
Copy Markdown
Contributor Author

Rebased on current master and force-pushed.

I also rechecked the helper-element changes and don't see anything that would obviously explain the issue17779-partial diff. Could you rerun botio when convenient? If it still reproduces on this rebased branch, I'll investigate further.

@timvandermeij
Copy link
Copy Markdown
Contributor

The firefox-issue17779-partial-page1 failure is not related to this patch, and instead a pre-existing failure that we need to fix. I have created issue #21276 to track that, but for the scope of this PR you can ignore it.

@RolandWArnold
Copy link
Copy Markdown
Contributor Author

Thanks for letting me know.

Copy link
Copy Markdown
Contributor

@calixteman calixteman left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you.

@calixteman calixteman merged commit cd4fd75 into mozilla:master May 14, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants