WIP: fix rmarkdown regarding new theme behaviour in pandoc #1489

cderv · 2018-12-04T19:32:14Z

This is a proposition to fix #1471. The issue resumes the problem introduced by recent changes in pandoc 2. The way pandoc > 2 insert css styling for highligting when converting to html conflicts with boostrap and theme.

This proposition tries to adapt rmarkdown default html template to insert css rules that deals with that.

This may be temporary if we try to report this impact to pandoc and find a clever solution that does not break this.

This is work in progress to get feedback and iterate. Also because I think the impact is rather broad among different format that rmarkdown convert to thanks to pandoc.

This change is

cderv · 2018-12-04T20:38:59Z

This simple insertion of to css rules fix the issue with html_document

but now, as pandoc 2 applies the highlighting rule to div, with notebook we have a the background color applied even when code is hidden.

and when opening the code

Not sure we want that... I'll see if I can add something to change that.

yihui · 2018-12-04T21:15:26Z

Perhaps we should check if the parent node is div.sourceCode before deciding where to insert the div containing the Show/Hide button:

rmarkdown/inst/rmd/h/navigation-1.1/codefolding.js

Line 48 in 34d2fd0

div.before(buttonRow);

If we are inside div.sourceCode, we need to insert this div to the parent node of div.sourceCode.

cderv · 2018-12-04T21:19:15Z

Thanks for pointing this out.
It is exactly what I am trying to do ! That confirms the idea - thanks!
it took me some time to discover how rmarkdown inserted the collapsible code ! I do not know the 📦 inside / out ... yet 😉
I am currently modifying this js file. Will update the PR soon.

cderv · 2018-12-04T21:49:18Z

Ok for notebook I guess

Small customisation pandoc css introduce

div.sourceCode {
    margin-top: 1em;
    margin-right: 0px;
    margin-bottom: 1em;
    margin-left: 0px;
}

As we insert <div class='row'> just before, I found the margin a little big. So I modified to reduce, using em also and not px

div.row {
  /* to reduce the space introduce in pandoc div.sourceCode */
  margin-bottom: -0.8em;
}

🤔 not sure of that change...
I think I'll test several other format and option to see if something is wrong or if all is fixed.

yihui · 2018-12-04T22:12:53Z

inst/rmd/h/navigation-1.1/codefolding.js

@@ -45,7 +45,9 @@ window.initializeCodeFolding = function(show) {
    buttonCol.append(showCodeButton);
    buttonRow.append(buttonCol);

-    div.before(buttonRow);
+    var sourceCodeBlock = $('div.sourceCode');


Will this also work with Pandoc 1.9.x? i.e. Does Pandoc 1.9 also generate div.sourceCode?

This is part of the test I need to make. Yes pandoc 1.9.x have a div.sourceCode but the css applied to those part have changed.

So yes this line work and the button is added above without issue, but the margin trick does not work. Will change that.

Also the main fix

pre.sourceCode { color: inherit; background-color: transparent; }

does not work with pandoc 1.9 because in this pandoc version the background is applied to pre.sourceCode so it erase it... -
if we want something that work with old and new, I need to find another one... 😕

ok - there is not div.sourceCode inserted if we use highlight = 'default', whatever the version of pandoc. This is because highlight default and textmate does not use pandoc but uses highlightjs and this does not insert the div. I did not know about that and I need to deal with that... More complicated that I thought 🤔

cderv · 2018-12-04T23:23:22Z

@yihui here is a proposition to make the fix conditional to pandoc > 2. As the issue is due to change from pandoc 2, we can activate a variable for pandoc template based on the version. This way the changes are only applied if used with pandoc 2.
This also could be useful to add new features - at some point two template could exist.

This was for me the more obvious fix right now, rather than going with css mastery. (I did not find one that could work both with pandoc < 2 and pandoc > 2.

What do you think ?

cderv · 2018-12-05T00:24:40Z

I just found that some highlight parameter for yaml header in Rmardown does not use pandoc. So the proposition does not deal with those, I need to continue working on a solution.

cderv · 2018-12-05T01:20:50Z

this should now work. I will try to test thoroughly. You can give it a try also.

default and textmate (not pandoc theme) are working.
1.9 and > 2 works also in both case.
new code is activated only when pandoc theme are added and when pandoc 2 is used

yihui

Sounds like there will be more work to do... :(

inst/rmd/h/default.html

yihui · 2018-12-05T05:32:13Z

inst/rmd/h/default.html

+$if(ispandoc2)$
+pre.sourceCode  {
+  color: inherit;
+  background-color: transparent;


Actually this will break themes that don't have a background color, such as pygments #654 (comment) Such themes should inherit the background color from Bootstrap's pre.

My head is starting to hurt...

Yes it was not clear to me and I wanted to test it further. Thanks for the confirmation. This blog post shows the result of pandoc styling, and indeed pygment as a white background, but with bootstrap it is not the case. As we need to

override the pre style of bootstrap because pandoc apply the background color and color to the parent

inherits for the pre style of bootstrap to keep the pre css rule from bootstrap for theme that don't have background and color
I think we should also do something conditional.

Of pandoc highlighting style, we have frome the src code

pygments with nothing ➡️ should inherit boostrap color and background-color

tango with no default color ➡️ should inherit boostrap color only

haddock is like pygments

monochrome is like pygments

I'll come up with something... 🤔 and yes 🤯

I made a proposition by manually setting which style has background and color or not. Then with two more template variable, the correct rule is activated.

In the same idea, with pandoc2 we can do something more dynamic. I hesitate to introduce this in rmarkdown but it could be interesting. We can get highlight information with

if (pandoc2.0()) { arg <- c("--print-highlight-style", pandoc_highlight) json_res <- system2(pandoc(), arg, stdout = TRUE) properties <- jsonlite::fromJSON(paste0(json_res, collapse = "\n")) }

we then get for pygments

> properties$`background-color` NULL > properties$`text-color` NULL

and for tango

> properties$`text-color` NULL > properties$`background-color` [1] "#f8f8f8"

We could introduce a function get_higligthing_styles in rmarkdown (that we could export) and use this here to test if is.null or not. This could even mean that we could fill in the value in
pre.sourceCode css rule, instead of inherits or transparent

I run this by you before pushing anything. I have a working code locally though - I can share then revert.

in fact you can see it in another branch

R/html_document.R

yihui

I thought for another hour on this, and I tend to use JavaScript to replace div.sourceCode{...} with pre.sourceCode{...} in CSS if div.sourceCode contains rules for color or background-color: https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet

e.g. change

div.sourceCode
  { color: #cccccc; background-color: #303030; }

to

pre.sourceCode
  { color: #cccccc; background-color: #303030; }

cderv · 2018-12-05T16:35:16Z

Initially, I did not want to touch what pandoc is inserting.
I'll give it a go then.

Rather than modified, could'nt we add pre.sourceCode to the rule if existing with background or color ?

div.sourceCode, pre.sourceCode
  { color: #cccccc; background-color: #303030; }

kind of the same but maybe more compatible with pandoc ?

yihui · 2018-12-05T16:44:47Z

Yes you certainly can. I guess the risk is minimal if we only modify this single rule from div.sourceCode to pre.sourceCode, though. If we define rules for both div and pre, there is a subtle disadvantage:

pre.sourceCode only

both div.sourceCode and pre.sourceCode

Did you notice the differences in the four corners? That's because Bootstrap defined rounded corners for pre. If the div has a background color, the corners will be filled.

Implementation-wise, you probably have to loop through document.styleSheets to find this particular stylesheet.

cderv · 2018-12-05T17:37:18Z

oh ! I did not notice before reading what the difference was. I read the doc you linked to and some others, I agree that this require iteration. I'll work on that ! thanks for pointing the direction.

cderv · 2018-12-06T00:49:54Z

I reverted back everything and deal with this using javascript code now. So just one file changed 😄

Do you find this better and easier to maintain ?

I put the script at the end of the template but I wonder if it should be elsewhere... 🤔 Normally, if no pandoc highlighting is chosen, no need to insert this script. I hesitated to put it in this part, that i believe is activated only by using pandoc highlighting
https://github.com/rstudio/rmarkdown/blob/e91e4c27bb223b6b0380a185c6f2fe5fb2ee2404/inst/rmd/h/default.html#L50-65

Also, I think there is another small change. New pandoc 2.0 added the div.sourceCode part and applied also a css rule for margin (see in skylighting 25 and 26 that modified the style applied by pandoc)

div.sourceCode { margin: 1em 0; }

this create a difference in rendered rmarkdown. I think it is not really impacting but I let you decide if it should be dealt with.

yihui

Many thanks! I'll take it from here. I don't care about the margin issue.

yihui · 2018-12-06T04:37:18Z

inst/rmd/h/default.html

+  (function() {
+    var stylesheets = document.styleSheets;
+    for(var i=0; i<stylesheets.length; i++){
+      var rules = stylesheets[i].cssRules;


Need try/catch here since the cssRules property is not necessarily accessible: https://developer.mozilla.org/en-US/docs/Web/API/CSSStyleSheet#Notes

- This allows to deal with the issue in html_notebook format. - A css rule is added for style to reduce the space introduce in pandoc css for div.sourceCode

for easier maintenance as it is related

it was introduced in rstudio#1399 initially

if exists, change div.sourceCode css rule about background-color and color by applying to pre.sourceCode

also add an identifier on Pandoc's CSS to save time on looking for the right <style> sheet to modify

cderv · 2018-12-06T06:57:33Z

Perfect ! So, my intuition was correct to move the hack. For rest, I am always learning - nice JS improvement.

Thanks @yihui for your review!

yihui · 2018-12-06T15:45:31Z

You did 99% of the work, and I only did the rest 1% :)

…eCode (rstudio#1489)

yihui reviewed Dec 4, 2018

View reviewed changes

yihui reviewed Dec 5, 2018

View reviewed changes

yihui added this to the v1.11 milestone Dec 5, 2018

yihui reviewed Dec 5, 2018

View reviewed changes

yihui approved these changes Dec 6, 2018

View reviewed changes

cderv added 13 commits December 5, 2018 22:40

add css rule to adapt to new pandoc 2 theme behaviour.

6c22d71

hide/show button is now inserted above the div.sourceCode from pandoc

54bb625

- This allows to deal with the issue in html_notebook format. - A css rule is added for style to reduce the space introduce in pandoc css for div.sourceCode

make the fix conditional to pandoc version > 2 or not

ec80473

move pandoc2 conditional arg in template section

7b27605

for easier maintenance as it is related

insert before div.sourceCode if exists

cd31e85

css rule about pre.sourceCode in correct place

17a782e

do not use isTRUE

228ca14

remove css rule not needed anymore

71cd5d4

it was introduced in rstudio#1399 initially

activate css rule based on highlight selected

ad4f5df

typo

968274a

mixing up function name

52e61a0

revert back to master to apply another fix

b7d0b8a

modify css stylesheet dynamically with javascript

a4a144a

if exists, change div.sourceCode css rule about background-color and color by applying to pre.sourceCode

yihui force-pushed the fix-1471-pandoc-css branch from 3f2399d to a4a144a Compare December 6, 2018 04:41

yihui added 2 commits December 5, 2018 23:26

apply the JS hack only if Pandoc's syntax highlighting is turned on

9f38e7b

also add an identifier on Pandoc's CSS to save time on looking for the right <style> sheet to modify

turn the HTML comment to JS comment

bb9f672

yihui approved these changes Dec 6, 2018

View reviewed changes

yihui added 2 commits December 6, 2018 00:09

add a news item

0a5170a

add @cderv to ctb

21c766d

yihui merged commit a0c0c68 into rstudio:master Dec 6, 2018

cderv deleted the fix-1471-pandoc-css branch December 6, 2018 06:57

yihui pushed a commit to RLesur/rmarkdown that referenced this pull request Apr 1, 2019

Fix rstudio#1471: move Pandoc's div.sourceCode CSS rules to pre.sourc…

4a5ff9a

…eCode (rstudio#1489)

github-actions bot locked as resolved and limited conversation to collaborators Nov 3, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: fix rmarkdown regarding new theme behaviour in pandoc #1489

WIP: fix rmarkdown regarding new theme behaviour in pandoc #1489

cderv commented Dec 4, 2018 •

edited by yihui

cderv commented Dec 4, 2018

yihui commented Dec 4, 2018

cderv commented Dec 4, 2018 •

edited

cderv commented Dec 4, 2018

yihui Dec 4, 2018

cderv Dec 4, 2018

cderv Dec 5, 2018 •

edited

cderv commented Dec 4, 2018

cderv commented Dec 5, 2018

cderv commented Dec 5, 2018

yihui left a comment

yihui Dec 5, 2018

cderv Dec 5, 2018

cderv Dec 5, 2018

cderv Dec 5, 2018

yihui left a comment

cderv commented Dec 5, 2018

yihui commented Dec 5, 2018

cderv commented Dec 5, 2018

cderv commented Dec 6, 2018

yihui left a comment

yihui Dec 6, 2018

cderv commented Dec 6, 2018

yihui commented Dec 6, 2018

WIP: fix rmarkdown regarding new theme behaviour in pandoc #1489

WIP: fix rmarkdown regarding new theme behaviour in pandoc #1489

Conversation

cderv commented Dec 4, 2018 • edited by yihui

cderv commented Dec 4, 2018

yihui commented Dec 4, 2018

cderv commented Dec 4, 2018 • edited

cderv commented Dec 4, 2018

yihui Dec 4, 2018

Choose a reason for hiding this comment

cderv Dec 4, 2018

Choose a reason for hiding this comment

cderv Dec 5, 2018 • edited

Choose a reason for hiding this comment

cderv commented Dec 4, 2018

cderv commented Dec 5, 2018

cderv commented Dec 5, 2018

yihui left a comment

Choose a reason for hiding this comment

yihui Dec 5, 2018

Choose a reason for hiding this comment

cderv Dec 5, 2018

Choose a reason for hiding this comment

cderv Dec 5, 2018

Choose a reason for hiding this comment

cderv Dec 5, 2018

Choose a reason for hiding this comment

yihui left a comment

Choose a reason for hiding this comment

cderv commented Dec 5, 2018

yihui commented Dec 5, 2018

pre.sourceCode only

both div.sourceCode and pre.sourceCode

cderv commented Dec 5, 2018

cderv commented Dec 6, 2018

yihui left a comment

Choose a reason for hiding this comment

yihui Dec 6, 2018

Choose a reason for hiding this comment

cderv commented Dec 6, 2018

yihui commented Dec 6, 2018

cderv commented Dec 4, 2018 •

edited by yihui

cderv commented Dec 4, 2018 •

edited

cderv Dec 5, 2018 •

edited