Theme-related bug in release candidate

[clauswilke]

I just found an annoying theme-related bug in the release candidate. Not sure if there's still time to fix it. I don't want to risk messing anything else up.

The issue is: When rendering axis labels, any element properties that are not available in element_text() will get stripped out in this code block:

ggplot2/R/guides-axis.r

Lines 195 to 199 in 94b5a16

	label_overrides <- axis_label_element_overrides(axis_position, angle)
	# label_overrides is always an element_text(), but in order for the merge to
	# keep the new class, the override must also have the new class
	class(label_overrides) <- class(label_element)
	label_element <- merge_element(label_overrides, label_element)

The intent is clearly to work with elements other than element_text() (even the comments state this), but in practice it doesn't work.

Reprex:

library(ggplot2)
library(ggtext)

ggplot(mpg, aes(class)) + 
  geom_bar() +
  scale_x_discrete(
    name = "vehicle *class*",
    labels = c("2seater", "*compact*", "midsize", "**minivan**", "pickup", "<sub>sub</sub>compact", "suv")
  ) +
  theme(
    axis.title.x = element_markdown(
      linetype = 1, fill = "cornsilk", r = unit(5, "pt"), padding = margin(4, 4, 4, 4)
    ),
    axis.text.x = element_markdown(
      # these properties get stripped
      linetype = 1, fill = "cornsilk", r = unit(5, "pt"), padding = margin(4, 4, 4, 4)
    )
  )

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

(The axis tick labels should be enclosed in boxes, just like the axis title. This doesn't happen, because linetype, fill, etc. get stripped. We can see, though, that the tick labels are rendered as markdown, as requested.)

The problem is that merge_element() doesn't keep any properties of the old element that aren't defined in the new element:

ggplot2/R/theme.r

Lines 633 to 640 in 47a0618

	# Override NULL properties of new with the values in old
	# Get logical vector of NULL properties in new
	idx <- vapply(new, is.null, logical(1))
	# Get the names of TRUE items
	idx <- names(idx[idx])
	# Update non-NULL items
	new[idx] <- old[idx]

A fix is theoretically simple, I just don't know immediately off the top of my head whether merge_element() is behaving incorrectly or whether the code that overrides axis text properties should not use merge_element() to combine the elements.

@paleolimbot @thomasp85 Thoughts? Can this be fixed before the release? I'll let it sit for a few hours and then see if I can prepare a PR.

[clauswilke]

Reprex for the element merging code.

library(ggplot2)
library(ggtext)

new <- element_text(angle = 45)
old <- element_markdown(linetype = 1, fill = "cornsilk", r = unit(5, "pt"), padding = margin(4, 4, 4, 4))
class(new) <- class(old)

# properties of `old`
old
#> List of 20
#>  $ family        : NULL
#>  $ face          : NULL
#>  $ size          : NULL
#>  $ colour        : NULL
#>  $ fill          : chr "cornsilk"
#>  $ box.colour    : NULL
#>  $ linetype      : num 1
#>  $ linewidth     : NULL
#>  $ hjust         : NULL
#>  $ vjust         : NULL
#>  $ box.hjust     : NULL
#>  $ box.vjust     : NULL
#>  $ angle         : NULL
#>  $ lineheight    : NULL
#>  $ margin        : NULL
#>  $ padding       : 'margin' num [1:4] 4pt 4pt 4pt 4pt
#>   ..- attr(*, "valid.unit")= int 8
#>   ..- attr(*, "unit")= chr "pt"
#>  $ r             : 'unit' num 5pt
#>   ..- attr(*, "valid.unit")= int 8
#>   ..- attr(*, "unit")= chr "pt"
#>  $ rotate_margins: logi FALSE
#>  $ debug         : logi FALSE
#>  $ inherit.blank : logi FALSE
#>  - attr(*, "class")= chr [1:3] "element_markdown" "element_text" "element"

# properties of merged element. `element_markdown`-specific properties get stripped
merge_element(new, old)
#> List of 11
#>  $ family       : NULL
#>  $ face         : NULL
#>  $ colour       : NULL
#>  $ size         : NULL
#>  $ hjust        : NULL
#>  $ vjust        : NULL
#>  $ angle        : num 45
#>  $ lineheight   : NULL
#>  $ margin       : NULL
#>  $ debug        : logi FALSE
#>  $ inherit.blank: logi FALSE
#>  - attr(*, "class")= chr [1:3] "element_markdown" "element_text" "element"

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

[clauswilke]

Fixed in RC branch. Will make its way to master in due course.