Strip clipping optional

[teunbrand]

This PR follows from issue #4118.

I think the changes proposed here are the minimal ones required to go make strip clipping an option from the theme setting.

Briefly, strip.clip becomes a defined, logical theme-element in the element tree. Also it has gained a corresponding argument in theme(), and has defaults in 'root' themes (theme_grey / theme_void / theme_test). In downstream strip rendering, the clipping argument to assemble_strips() is taken from the theme instead of immutably "on".

A thing I was unsure about, was wether to use calc_element() for consistency, or to take the strip clipping setting directly from the theme, as there isn't anything really anything to inherit from as far as I could tell.

Here is a code example and visual comparison:

library(ggplot2)
library(patchwork)

# Default clipping is TRUE
p1 <- ggplot(mpg, aes(displ, hwy)) +
  geom_point() +
  facet_wrap(~ drv) +
  theme(
    strip.background = element_rect(colour = "red", size = 5),
    axis.line.y.left = element_line(colour = "blue", size = 5)
  )

p2 <- p1 + theme(strip.clip = FALSE)
(p1 / p2)

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

Thanks for considering!

[clauswilke]

Not sure whether this should be a theme option or just an argument to facet_grid(). It's an argument to coord_cartesian().

@thomasp85 Any opinion on this?

If we use a theme element, please use calc_element() to calculate it. Also, I think the value of the setting should be taken as a string, "on", "inherit", or "off", as is used throughout grid.

[teunbrand]

Not sure whether this should be a theme option or just an argument to facet_grid(). It's an argument to coord_cartesian().

My thinking on this was that defining strip clipping in the theme would automatically transfer the setting to facet_wrap() as well as other facet configurations in extension packages that use the build_strip() function to render strips. Also it would allow us to define it in a global theme that applies the setting to subsequent plots.

I agree it would be more consistent to follow coord_cartesian() behavior. On the other hand, I feel that clipping the panel is a data-decision that belongs in the coordinate system, whereas strip clipping is more of a 'personal taste'-decision that doesn't directly affect the interpretation of a plot.

Also, I think the value of the setting should be taken as a string, "on", "inherit", or "off", as is used throughout grid.

The "on" or "off" strings make sense to me, I'm slightly less comfortable with "inherit" (the whole gtable system is effectively invisible for many users). If we would take the theme element route, where would be a natural place to check the validity of the character argument?

[clauswilke]

The "on" or "off" strings make sense to me, I'm slightly less comfortable with "inherit" (the whole gtable system is effectively invisible for many users).

With forthcoming improvements in clipping, we should actually not normally use "off" but rather "inherit" as the default for no clipping. "off" turns all clipping off.

If we would take the theme element route, where would be a natural place to check the validity of the character argument?

If you think it needs to be checked, check it right before you hand it off to grid.

[teunbrand]

With forthcoming improvements in clipping

Ah yes, excellent point. I'm excited about the expanded capabilities of grid!
I've changed the type from logical to character, defaulting to "inherit"; it indeed seems more appropriate.
If anyone wants to chime in on where this argument should be specified (theme or facet), let me know.