Skip to content

rest2html: treat referenced wrapped images in base document as inlined - #1935

Open
jdknight wants to merge 2 commits into
github:masterfrom
jdknight:strip-suffix-on-referenced-imgs-at-doc-root
Open

rest2html: treat referenced wrapped images in base document as inlined#1935
jdknight wants to merge 2 commits into
github:masterfrom
jdknight:strip-suffix-on-referenced-imgs-at-doc-root

Conversation

@jdknight

Copy link
Copy Markdown

docutils will only add newlines around images it believes are inlined. For images held in references, it checks the parent of the reference if its a TextElement to consider it inlined. Since a document is not a TextElement type, it will wrap an image with newlines. For GitHub output, this is not desired and will result in the extra whitespace being rendered with a reference's decorative line. To avoid this, always strip any appended suffixes for images in this scenario.


There are a various GitHub projects which reveal the issue. For example, Sphinx's README.rst shows this issue:

With the changes made in this merge request, the following shows a rendering of HTML before and after the change:

@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@jdknight

Copy link
Copy Markdown
Author

!unstale

@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@jdknight

Copy link
Copy Markdown
Author

!unstale

@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@jdknight

Copy link
Copy Markdown
Author

!unstale

@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@jdknight

Copy link
Copy Markdown
Author

!unstale

@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@jdknight

Copy link
Copy Markdown
Author

!unstale

CopilotAI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR updates the ReST-to-HTML translation to avoid emitting trailing whitespace after an image when the image is wrapped in a reference that is a direct child of the document, preventing GitHub’s link decoration from rendering across the whitespace after the image.

Changes:

  • Strip the trailing newline emitted for a referenced top-level image so it behaves like an inline image in GitHub HTML output.
Show a summary per file
FileDescription
lib/github/commands/rest2htmlAdjusts depart_image output to remove trailing newline for document-level referenced images to avoid decorative underline/whitespace artifacts.

Copilot's findings

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 1/1 changed files
  • Comments generated: 1

Comment on lines +261 to +266
# treat images held in a reference on the base document as inlined
# images; this is to help avoid rendering a reference's decorative
# line for the spacing after an image
if (isinstance(node.parent, nodes.reference)
and isinstance(node.parent.parent, nodes.document)):
self.body.append(self.body.pop().rstrip('\n'))

CopilotAIApr 22, 2026

Copy link

Choose a reason for hiding this comment

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

This change adjusts HTML whitespace semantics in a subtle way, but the existing markup tests normalize/strip blank text nodes via Nokogiri (noblanks), so they likely won’t fail if the newline/whitespace regresses. Consider adding a targeted regression test that asserts the raw HTML output for a top-level .. image:: with :target: does not contain whitespace between the <img> and the closing </a> (string-level assertion, not DOM-equality).

Copilot uses AI. Check for mistakes.
@zkoppertzkoppert self-assigned this Apr 22, 2026
@zkoppert
zkoppertforce-pushed the strip-suffix-on-referenced-imgs-at-doc-root branch from 600f67a to ee00347CompareApril 22, 2026 05:06
@zkoppert

Copy link
Copy Markdown
Member

Hey @jdknight — thanks for sticking with this PR, and sorry it took so long to get eyes on it.

I rebased your branch onto master to get CI running (it was 46 commits behind), and all checks pass across Ruby 3.2/3.3/3.4. The fix itself looks good — we reviewed it thoroughly and the logic is sound.

One thing we'd like to see before merging: a regression test. The existing test suite uses Nokogiri with noblanks, which normalizes whitespace-only text nodes — so the current tests wouldn't catch it if this behavior regressed. A string-level assertion (not DOM-equality) that verifies there's no trailing whitespace between the <img> and closing </a> for a top-level referenced image would do the trick.

Something like an RST fixture with a .. image:: directive using :target:, and an assertion on the raw HTML output before Nokogiri parsing. Happy to help brainstorm the approach if that would be useful.

@jdknight

Copy link
Copy Markdown
Author

@zkoppert, there appears to be a couple of locations in assert_html_equal/normalize_html that would need to be adjusted to allow assert_html_equal to support strict whitespace checks, let alone finding a graceful way to opt-in to strict checks through the generic markup test discovery logic.

Maybe for this case a simple test can be appended. For example:

 def test_blanks_rst_inlined_reference
expected = '<img src="https://example.com/img.svg"></a>'
actual = GitHub::Markup.render_s(GitHub::Markups::MARKUP_RST, <<~RST
.. image:: https://example.com/img.svg
:target: https://example.com/
RST
)
assert_equal expected, actual.strip.lines.last
end

And if more tests are needed later that require strict whitespace checks, this can be looked at then.

docutils will only add newlines around images it believes are inlined.
For images held in references, it checks the parent of the reference if
its a `TextElement` to consider it inlined. Since a document is not a
`TextElement` type, it will wrap an image with newlines. For GitHub
output, this is not desired and will result in the extra whitespace
being rendered with a reference's decorative line. To avoid this,
always strip any appended suffixes for images in this scenario.
Signed-off-by: James Knight <git@jdknight.me>
@jdknight
jdknightforce-pushed the strip-suffix-on-referenced-imgs-at-doc-root branch from a2d325b to 4610a00CompareJune 13, 2026 14:28
Verifying that inlined references generated by reStructuredText/docutil
do no generate whitespaces between the img tag and closing a tag.
Signed-off-by: James Knight <git@jdknight.me>
@github-actions

Copy link
Copy Markdown

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@jdknight

Copy link
Copy Markdown
Author

!unstale

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@jdknight@zkoppert@ccxcvv@joselopez94275