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.

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

Redundant removal of attributes on HTML headers #865

Closed
3 tasks done
salim-b opened this issue Mar 9, 2020 · 9 comments
Closed
3 tasks done

Redundant removal of attributes on HTML headers #865

salim-b opened this issue Mar 9, 2020 · 9 comments
Labels
feature a feature request or enhancement next to consider for next release

Comments

@salim-b
Copy link
Contributor

salim-b commented Mar 9, 2020

I just noticed that when using Bookdown/Blogdown in combination with Pandoc 2.9+, the following JavaScript dependency gets inserted in the rendered HTML:

<script src="/rmarkdown-libs/header-attrs/header-attrs.js"></script>

The content of header-attrs.js is found here and was added with rstudio/rmarkdown@7f51e23.

At the same time, the R function bookdown:::clean_header_tags() removes those attributes during rendering (added with a07ae07).

So – if I didn't miss anything – this header attribute removal is done twice: At first during rendering the R Markdown to HTML using R code (fine) and a second time during rendering the HTML by the browser using JS code (redundant and thus obsolete in the case of Bookdown/Blogdown).

Therefore I think it would make sense to avoid adding this JS dependency when bookdown/blogdown renders a file. Or maybe, the R-based header attribute removal could be performed directly by rmarkdown::render() (instead of adding the JS).

(IMHO: The less JS, the better 😬)


By filing an issue to this repo, I promise that

  • I have fully read the issue guide at https://yihui.org/issue/.
  • I have provided the necessary information about my issue.
    • If I'm asking a question, I have already asked it on Stack Overflow or RStudio Community, waited for at least 24 hours, and included a link to my question there.
    • If I'm filing a bug report, I have included a minimal, self-contained, and reproducible example, and have also included xfun::session_info('bookdown'). I have upgraded all my packages to their latest versions (e.g., R, RStudio, and R packages), and also tried the development version: remotes::install_github('rstudio/bookdown').
    • If I have posted the same issue elsewhere, I have also mentioned it in this issue.
  • I have learned the Github Markdown syntax, and formatted my issue correctly.

I understand that my issue may be closed if I don't fulfill my promises.

@yihui
Copy link
Member

yihui commented May 14, 2020

Thanks for the analysis! There is indeed redundancy. For bookdown, we have to keep a07ae07, so the only option is to get rid of header-attrs.js, which is currently not possible. I agree with you that the less JS the better, but this JS script is only 12 lines, and I'm not sure if it is worth the effort to provide an option to exclude it...

@salim-b
Copy link
Contributor Author

salim-b commented May 14, 2020

so the only option is to get rid of header-attrs.js, which is currently not possible.

?

Or maybe, the R-based header attribute removal could be performed directly by rmarkdown::render()?

This isn't an option?

@yihui
Copy link
Member

yihui commented May 15, 2020

the R-based header attribute removal could be performed directly by rmarkdown::render()?

I don't know how to do this...

rmarkdown::html_document() introduced header-attrs.js (as an HTML dependency), and if I understand you correctly, you want to remove it for bookdown output formats, but I don't know how to remove it.

@cderv cderv added the feature a feature request or enhancement label Mar 29, 2021
@cderv
Copy link
Collaborator

cderv commented Mar 29, 2021

Overall, I think we should make JS script in rmarkdown more conditional than they are. So that a user can decide to opt-out a feature brought by a JS script if desired. Doing so in rmarkdown would allow to opt-out in bookdown.

Otherwise, maybe we should start setting a knitr option in each package format so that rmarkdown can sometimes adapt its behavior depending on which the format is ?

Like we do for bookdown label but more generic, something like

knitr:::opts_knit$set(bookdown = TRUE)

This would be set in a bookdown format, so that it can be used by other packages that needs to modify their behavior.

@yihui what do you think ?

@yihui
Copy link
Member

yihui commented Apr 26, 2021

Perhaps adding getOption('rmarkdown.html_dependency.header_attr', TRUE) before pandoc_available('2.9'): https://github.com/rstudio/rmarkdown/blob/6e572ca665d19d05aadb08a4309fc5478b840aee/R/html_dependencies.R#L424

so that we can set this option to FALSE in bookdown.

@cderv
Copy link
Collaborator

cderv commented Apr 26, 2021

Yes I think that would be the better. This would be a way also to opt-out of this if desired. I think this is good practice if we can offer an opt-out flag when we add things like CSS or JS.

Regarding this header-attr.js, it could be interested to revisit it also - is it still the solution with Pandoc 2.11.4 now. But that is another story.

@cderv cderv added the next to consider for next release label Apr 26, 2021
@cderv cderv unassigned yihui Apr 26, 2021
@salim-b
Copy link
Contributor Author

salim-b commented Jul 2, 2021

Another thing I've noticed: The header attribute removal executed by header-attrs.js breaks anchor link highlighting in pkgdown articles (vignettes).

Since rmarkdown:::html_dependency_header_attrs() is added unconditionally in rmarkdown::html_document_base(), the user cannot easily change that...

Why do header attributes need to be removed in the first place again?

@cderv
Copy link
Collaborator

cderv commented Jul 5, 2021

Why do header attributes need to be removed in the first place again?

This is a long old story due to a Pandoc change but it started here: rstudio/rmarkdown#1723

As commented in the file (https://github.com/rstudio/rmarkdown/blob/e44a23f3e508534966717e9c9dce07afd2e37002/R/html_dependencies.R#L440)

Pandoc 2.9 adds attributes on both headers and their parent divs. We remove
the ones on headers since they are unnecessary

At first it was a quick fix for tabsets in rmarkdown following a change in Pandoc 2.8, and then became more generic for other format (like flexdashboard (rstudio/rmarkdown#1732) . At last, it was changed to the current code applied only for Pandoc 2.9 and later (rstudio/rmarkdown@7f51e23) so that it works the same for different pandoc version (except 2.8 which will be buggy anyway).

We still need to provide an opt out way as discussed in #865 (comment)

This will be done for next version of rmarkdown

cderv added a commit that referenced this issue Feb 4, 2022
… formats

in order to deactivate double handling of header attributes

Closes #865. Requires rmarkdown 2.12
@github-actions
Copy link

github-actions bot commented Apr 6, 2022

This old thread has been automatically locked. If you think you have found something related to this, please open a new issue by following the issue guide (https://yihui.org/issue/), and link to this old issue if necessary.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
feature a feature request or enhancement next to consider for next release
Projects
None yet
Development

No branches or pull requests

3 participants