Clean up strip rendering code

[thomasp85]

Fix #3557

This PR gets strip rendering in line with the rest of the theming by correctly using element_render() to render strip text instead of the current hack. There is still some special considerations to be done, as strips are sized to the largest strip for alignment and this has to be done after the creation of the titleGrob. I don't think there is any way around the current approach of setting width/height directly in the viewport.

The PR also adds explicit theme elements for top, bottom, left, and right strips, which inherits from strip.text.x and strip.text.y. This is necessary to control the angle of vertical strips and is in line with how it is handled for axis titles. We should watch out for any visual breakage downstream that this might affect (I predict none or very few).

[clauswilke]

That was fast! And seems to have improved the readability of the code a lot too. I'll likely need a few days to review this. I also want to check out the branch and make sure it works with ggtext.

[thomasp85]

No problem. And yes the old code was quite polluted

[thomasp85]

This is running into problems on 3.2 because grid was a mess back then (unit subset-assignment was completely borked). @hadley can we drop 3.2 support a bit earlier than anticipated?

[hadley]

That would basically imply dropping 3.2 support globally which I'm loathe to do without a really good reason (and I don't think cleaning up strip rendering is that reason)

[thomasp85]

That is fair... I can try to get around it and then we can clean up my clean up after R4.0

[clauswilke]

If I remember correctly, we ran into a similar issue when implementing the plot.tag theme element and the solution proposed here may be the simplest way forward:
#2405 (comment)

[thomasp85]

Yeah... that, or just reassemble the units. I'd rather not make a conditional on a version that will fall out of support in a few months

[clauswilke]

I've checked out the code. As currently written, it doesn't work with ggtext, because the function assemble_strips() makes assumptions about the internal structure of the grobs it receives as argument. It would be better if it could be rewritten so it can handle arbitrary grobs that properly report their heights and widths.

library(tidyverse)
library(ggtext)
library(glue)

count(mtcars, cyl, carb) %>% 
  mutate(cyl_lab = as.character(
    glue("Cylinders {cyl} <span style='color:red'>(n={n})</span>")
  )) %>% 
  ggplot(aes(carb, n)) +
  geom_col() +
  facet_wrap(~cyl_lab) +
  theme(
    strip.text = element_markdown(),
    strip.text.x = element_markdown(),
    strip.text.y = element_markdown()
  )
#> Error: Unknown input

^{Created on 2019-12-21 by the reprex package (v0.3.0)}

I can work around this issue by recreating the same data structure that ggplot2 is using, and in fact I do so already for rotated text, for a variety of reasons (see example below). However, I wouldn't want to have to wrap every text element into this extended data structure just so strip labels work appropriately. If assemble_strips() can't be easily made to handle arbitrary input grobs, then maybe it would be better to add this additional wrapping in the strip rendering code, maybe conditionally in cases where the input grobs are not titleGrobs.

library(tidyverse)
library(ggtext)
library(glue)

count(mtcars, cyl, carb) %>% 
  mutate(cyl_lab = as.character(
    glue("Cylinders {cyl} <span style='color:red'>(n={n})</span>")
  )) %>% 
  ggplot(aes(carb, n)) +
  geom_col() +
  facet_wrap(~cyl_lab) +
  theme(
    strip.text = element_markdown(angle = 12),
    strip.text.x = element_markdown(angle = 12),
    strip.text.y = element_markdown(angle = 12)
  )

^{Created on 2019-12-21 by the reprex package (v0.3.0)}

[thomasp85]

The problem is not so much reporting the sizes, as setting them. we have to resize the strips to the width or height of the largest strip after they have been created.

[clauswilke]

Sure. So how about adding one step where you iterate over all grobs, check if they are titleGrobs, and if not call add_margins() on them with zero margins?

If you look at the following code in element_markdown(), when we're in the if branch your current code fails, but when we're in the else branch your current code works. The only difference is one add_margins() call.
https://github.com/clauswilke/ggtext/blob/cc8ea0c94fdeb9562268fdbf9be1121172492b3f/R/element-markdown.R#L73-L84

[thomasp85]

I'll have a look at that tomorrow - I'm not entirely sure I understand what is going on inside element_markdown() but it is probably about time I figure that out :-)

[clauswilke]

Demonstration that this should work (since it works fine with my code).

library(tidyverse)
library(ggtext)
library(glue)

df <- count(mtcars, cyl, carb) %>% 
  mutate(cyl_lab = as.character(
    glue("Cylinders {cyl} <span style='color:red'>(n={n})</span>")
  ))

ggplot(df, aes(carb, n)) +
  geom_col() +
  facet_wrap(~cyl_lab) +
  theme(
    strip.text = element_markdown(angle = 1),
    strip.text.x = element_markdown(angle = 1),
    strip.text.y = element_markdown(angle = 1)
  )

df$cyl_lab[1] <- paste0(df$cyl_lab[1], "<br>and a second line of label")
ggplot(df, aes(carb, n)) +
  geom_col() +
  facet_wrap(~cyl_lab) +
  theme(
    strip.text = element_markdown(angle = 1),
    strip.text.x = element_markdown(angle = 1),
    strip.text.y = element_markdown(angle = 1)
  )

^{Created on 2019-12-21 by the reprex package (v0.3.0)}

[thomasp85]

unrelated, but it appears as if the coloured text are written with a shifted baseline. Is that intended?

[clauswilke]

I think it looks like that because of the 1 degree of rotation that has been applied. Without rotation it looks correct to me.

library(tidyverse)
library(ggtext)

df <- count(mtcars, cyl, carb)

ggplot(df, aes(carb, n)) +
  geom_col() +
  ggtitle("Cylinders 4 <span style='color:red'>(n=5)</span>") +
  theme(plot.title = element_markdown())

^{Created on 2019-12-21 by the reprex package (v0.3.0)}

[thomasp85]

Ah - yeah. rendering of rotated text with Cairo is just a sad affair

[clauswilke]

I haven't tested this, because I don't want to jump through the hoops of checking out your branch right now, but adding something like this to the beginning of assemble_strips() should be sufficient:

grobs <- lapply(grobs, function(g) {
  if (inherits(g, "titleGrob")) return(g)
  
  add_margins(g, grobHeight(g), grobWidth(g), margin_x = TRUE, margin_y = TRUE)
})

[thomasp85]

@clauswilke I have fixed the unit operations so the change works on 3.2, as well as added your requested code block. Can you check that it now works with ggtext and review it if it does

[clauswilke]

spelling: "equivalently"

[clauswilke]

Perfect!

library(tidyverse)
library(glue)
#> 
#> Attaching package: 'glue'
#> The following object is masked from 'package:dplyr':
#> 
#>     collapse
library(ggtext)

count(mtcars, cyl, carb) %>% 
  mutate(cyl_lab = as.character(
    glue("Cylinders {cyl} <span style='color:red'>(n={n})</span>")
  )) %>% 
  ggplot(aes(carb, n)) +
  geom_col() +
  facet_wrap(~cyl_lab) +
  theme(
    strip.text = element_markdown(),
    strip.text.x = element_markdown(),
    strip.text.y = element_markdown()
  )

^{Created on 2020-01-06 by the reprex package (v0.3.0)}

[clauswilke]

Maybe just write "with a grob", or "containing a grob, such as a text grob".