Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Retain forward slashes in HTML img src paths #2811

Merged
merged 4 commits into from
Nov 8, 2024
Merged

Retain forward slashes in HTML img src paths #2811

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
04a03da
Fixes issue where forward slashes are encoded in HTML img src paths
Andrew0Hill Nov 7, 2024
c019e26
Test that slashes in image src attributes are handled correctly
Andrew0Hill Nov 8, 2024
8d2b45d
Added call to path_real to ensure input image path is not relative.
Andrew0Hill Nov 8, 2024
20b22b6
Added a comment with summary of temporary pkgdown directory discussion.
Andrew0Hill Nov 8, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion R/tweak-page.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ tweak_rmarkdown_html <- function(html, input_path, pkg = list(bs_version = 3)) {
img_src_real <- path_real(xml2::url_unescape(src[abs_src]))
input_path_real <- path_real(xml2::url_unescape(input_path))
img_rel_paths <- path_rel(path = img_src_real, start = input_path_real)
img_rel_paths <- xml2::url_escape(img_rel_paths)
img_rel_paths <- xml2::url_escape(img_rel_paths, reserved="/")

purrr::walk2(
.x = img_target_nodes,
Expand Down
27 changes: 27 additions & 0 deletions tests/testthat/test-tweak-page.R
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,3 +135,30 @@ test_that("h1 section headings adjusted to h2 (and so on)", {
c("page-header", "section level2", "section level3", "section level2")
)
})

test_that("slashes not URL encoded during relative path conversion", {
# Create a site
Copy link
Member

Choose a reason for hiding this comment

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

Since you've changed only tweak_rmarkdown_html(), it would be better if the test could more explicitly check just that function — i.e. you don't need to set up an entire placeholder site, you can just pass html to tweak_rmarkdown_html().

Copy link
Contributor Author

@Andrew0Hill Andrew0Hill Nov 8, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for reviewing.

I originally tried something similar to what you suggest, but I found that the path conversion portion of tweak_rmarkdown_html() makes two calls to fs::path_real() (lines 51 and 52), which will fail if the path referenced does not exist on the filesystem.

This means that whatever path is provided in the test HTML also needs to physically exist on the filesystem for the path conversion portion of the call to tweak_rmarkdown_html() to work properly. For this reason, I added the placeholder site with image, so that the paths referenced in the test HTML will exist, and the calls to fs::path_real() will succeed.

In the other existing test for tweak_rmarkdown_html() (line 112 in test_tweak_page.R) the test HTML doesn't contain any <img/> tags, so the path resolution code never runs (and therefore that test does not run into the same issue with the calls to fs::path_real()).

The only other way I could think of avoiding the placeholder package would be to set the img src in the test HTML to an image path that we know exists in the repository (like test_path("assets/kitten.jpg"), and then having it resolve the path relative to test_path() (untested idea below):

# Get the full path to the image.
img_path <- path_real(test_path("assets/kitten.jpg"))

# Get the path we want to resolve the relative path with respect to
par_img_path <- path_real(test_path())

# Simulate an output HTML file referencing the absolute path.
html <- xml2::read_html(
  sprintf('
  <body>
    <img src="%s" />
  </body>
  ', sim_path)
)

# Update absolute path to a relative path
tweak_rmarkdown_html(html, par_img_path)

# Output path should be a relative path with respect to `test_path()`
expect_equal(xpath_attr(html, ".//img", "src"), "assets/kitten.jpg")

I went with the method in my current commit though since the above version would fail if the assets/kitten.jpg file ever moved. (The version with the placeholder package still refers to this resource, but indirectly through pkg_add_kitten())

I'm happy to take another shot at this if you have any other suggestions?

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the detailed explanation! In that case it sounds like you have written the minimal test, and I'd just suggest briefly summarising this discussion as a comment.

pkg <- local_pkgdown_site()

# Add the referenced image in a subdirectory of vignettes.
pkg <- pkg_add_kitten(pkg, "vignettes/img/")

# Get the full path to the image.
sim_path <- path(pkg$src_path, "vignettes/img/kitten.jpg")

# Simulate an output HTML file referencing the absolute path.
html <- xml2::read_html(
sprintf('
<body>
<img src="%s" />
</body>
', sim_path)
)

# Function should update the absolute path to a relative path.
tweak_rmarkdown_html(html, path(pkg$src_path, "vignettes"))

# Check that the relative path has a non-encoded slash.
expect_equal(xpath_attr(html, ".//img", "src"), "img/kitten.jpg")

})
Loading