Skip to content

fix: protect page count alias from UBA reordering (issue #1925) - #1929

Merged
andersonhc merged 2 commits into
py-pdf:masterfrom
prateek-dagar:fix-issue-1925
Sep 10, 2026
Merged

andersonhc merged 2 commits into
py-pdf:masterfrom
prateek-dagar:fix-issue-1925

Conversation

@prateek-dagar

Copy link
Copy Markdown

Changes
Replace the alias with a single placeholder PUA character (\uE000 with collision-safe scanning) before UBA bidi processing.
Because a single character is atomic, it survives UBA reordering intact at its correct semantic location.
Reconstruct the bidi fragments by expanding the PUA placeholder back into a TotalPagesSubstitutionFragment.

Fixes #1925

  • A unit test is covering the code added / modified by this PR

  • In case of a new feature, docstrings have been added, with also some documentation in the docs/ folder

  • A mention of the change is present in CHANGELOG.md

  • This PR is ready to be merged

By submitting this pull request, I confirm that my contribution is made under the terms of the GNU LGPL 3.0 license.

Comment thread test/test_alias.py
@prateek-dagar
prateek-dagar force-pushed the fix-issue-1925 branch 2 times, most recently from b3a99d5 to 919c52e Compare August 28, 2026 15:10
@prateek-dagar

Copy link
Copy Markdown
Author

Hi @andersonhc,

I updated the branch to fix the markdown formatting issue:

  • Changed placeholder to \u001f (neutral character) to prevent UBA from splitting RTL runs and breaking markdown tags.
  • Overrode page count fragment direction to LTR to prevent numbers from being reversed to "21" in RTL text.
  • Added test_alias_in_rtl_text_rtl_alias to verify custom RTL script aliases shape correctly.

All tests are now passing!

@andersonhc andersonhc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I wonder if we could encapsulate the placeholder handling inside BidiParagraph.

Perhaps BidiParagraph could accept an optional alias or protected_token parameter.

When present, it would:

  • Select an unusual sentinel character that does not occur in the input.
  • Replace every occurrence of the alias with that sentinel.
  • Run the bidi algorithm.
  • Replace the sentinel with the original alias in each string returned by get_bidi_fragments().

This would keep the sentinel internal to the bidi implementation. The caller would receive {nb} again, allowing the existing _preload_font_styles() logic to create TotalPagesSubstitutionFragment normally. It would also prevent the sentinel from reaching font lookup and fallback handling.

The sentinel should specifically have bidi class EN (“European Number”), rather than S or BN. The eventual page count consists of ASCII digits, which are also EN. Matching that class is important because the Unicode bidi algorithm resolves adjacent punctuation using the number’s bidi class. For example, 10/{nb}, {nb}-10, and {nb}% should be resolved as numeric LTR runs inside an RTL paragraph. A non-numeric sentinel can cause /, -, or % to be assigned to a different directional run.

Could we create a small list of unusual EN characters, selecting the first one absent from the text?
The available characters can be reviewed here: https://www.compart.com/en/unicode/bidiclass/EN.

U+1D7FF is good candidate for main sentinel in my opinion, and we could add a list of fallback ones in BidiParagraph

It would be good to add test cases involving punctuation, for example:

"אבג 10/{nb} דהו"
"אבג {nb}-10 דהו"
"אבג ({nb}%) דהו"

On the tests make 2 calls to cell(), one with the alias and one with numeric values, and both must resolve identically on the output pdf.

@prateek-dagar

Copy link
Copy Markdown
Author

Thanks @andersonhc!
I'll encapsulate the alias substitution inside BidiParagraph using U+1D7FF (and fallback EN characters) so that the sentinel stays completely internal to bidi processing and resolves punctuation rules correctly. I'll also add the comparison tests for 10/{nb}, {nb}-10, and ({nb}%)

@prateek-dagar

Copy link
Copy Markdown
Author

Hi @andersonhc ,
I have incorporated these changes, please have a look at it.

  1. Sentinel Placeholder in BidiParagraph:

    • BidiParagraph now substitutes the alias with an EN (European Number) sentinel digit before executing the Unicode Bidirectional Algorithm (UBA), ensuring page placeholders surrounded by RTL text, punctuation (slashes, hyphens, parentheses), and percentage signs are treated with numeric directionality.
  2. Markdown Styling across Directional Runs:

    • Updated _preload_bidirectional_text() to track and propagate markdown emphasis states (bold, italics, strikethrough, underline) across directional segments, allowing continuous styling (e.g. --אבג {nb} דהו--) across RTL/LTR transitions.
  3. Test Suite & Side-by-Side Verification:

    • Added test cases in test/test_alias.py covering standard aliases, custom aliases, RTL script aliases, adjacent punctuation, and markdown formatting.
    • Added parameterized side-by-side comparison tests comparing the alias output directly against literal number renderings in the same document.

@andersonhc andersonhc left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

See comment.

Also, can you add a CHANGELOG entry?

Comment thread fpdf/fpdf.py Outdated
bidi_text, markdown, _initial_emphasis=emphasis
)
if styled_frags:
last = styled_frags[-1]

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you double-check the assumption that the Markdown state can be copied from the last emitted fragment?

I tested this with:

--אבג {nb} דהו--ABC

and ABC is incorrectly underlined. Adding a space between the closing marker and A makes it render correctly:

--אבג {nb} דהו-- ABC

It looks like the closing -- updates the parser’s internal state but does not emit a new fragment. Therefore, the last fragment remains underlined even though the actual terminal Markdown state is no longer underlined. Copying the style from styled_frags[-1] consequently carries the stale underline state into the next bidi segment.

Maybe _parse_chars() could expose its final emphasis state directly instead?

@prateek-dagar
prateek-dagar force-pushed the fix-issue-1925 branch 2 times, most recently from 86db5b8 to 266a2be Compare September 5, 2026 10:01
@prateek-dagar

Copy link
Copy Markdown
Author

See comment.

Also, can you add a CHANGELOG entry?

Thanks for the review!

  • Updated _parse_chars() to record the terminal markdown emphasis state directly instead of reading stale styles from styled_frags[-1].
  • Added test coverage in test_alias_in_rtl_text_markdown for trailing LTR (ABC), trailing RTL, and mixed bidi segments.

@andersonhc
andersonhc merged commit db990aa into py-pdf:master Sep 10, 2026
23 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.

Support page number alias substitution with bidi / RTL text shaping

2 participants