Put actionButton()s icon and label into containers (#4249)

* Put action icon and label into containers

* Update snaps

* More robust test

* Don't include container if icon/label isn't specified

* `yarn build` (GitHub Actions)

* Send HTML string/deps on update; update news

---------

Co-authored-by: cpsievert <cpsievert@users.noreply.github.com>

DESCRIPTION

@@ -202,6 +202,7 @@ Collate:
     'test.R'
     'update-input.R'
     'utils-lang.R'
+    'utils-tags.R'
     'version_bs_date_picker.R'
     'version_ion_range_slider.R'
     'version_jquery.R'

NEWS.md

@@ -1,5 +1,17 @@
 # shiny (development version)
 
+## New features
+
+* The `icon` argument of `updateActionButton()`/`updateActionLink()` nows allows values other than `shiny::icon()` (e.g., `fontawesome::fa()`, `bsicons::bs_icon()`, etc). (#4249)
+
+## Bug fixes
+
+* `updateActionButton()`/`updateActionLink()` now correctly renders HTML content passed to the `label` argument. (#4249)
+
+## Changes
+
+* The return value of `actionButton()`/`actionLink()` changed slightly: `label` and `icon` are wrapped in an additional HTML container element. This allows for: 1. `updateActionButton()`/`updateActionLink()` to distinguish between the `label` and `icon` when making updates and 2. spacing between `label` and `icon` to be more easily customized via CSS. 
+
 # shiny 1.11.1
 
 This is a patch release primarily for addressing the bugs introduced in v1.11.0.

R/input-action.R

@@ -56,13 +56,24 @@ actionButton <- function(inputId, label, icon = NULL, width = NULL,
 
   value <- restoreInput(id = inputId, default = NULL)
 
-  tags$button(id=inputId,
+  icon <- validateIcon(icon)
+
+  if (!is.null(icon)) {
+    icon <- span(icon, class = "action-icon")
+  }
+
+  if (!is.null(label)) {
+    label <- span(label, class = "action-label")
+  }
+
+  tags$button(
+    id = inputId,
     style = css(width = validateCssUnit(width)),
-    type="button",
-    class="btn btn-default action-button",
+    type = "button",
+    class = "btn btn-default action-button",
     `data-val` = value,
     disabled = if (isTRUE(disabled)) NA else NULL,
-    list(validateIcon(icon), label),
+    icon, label,
     ...
   )
 }
@@ -72,30 +83,40 @@ actionButton <- function(inputId, label, icon = NULL, width = NULL,
 actionLink <- function(inputId, label, icon = NULL, ...) {
   value <- restoreInput(id = inputId, default = NULL)
 
-  tags$a(id=inputId,
-    href="#",
-    class="action-button",
+  icon <- validateIcon(icon)
+
+  if (!is.null(icon)) {
+    icon <- span(icon, class = "action-icon")
+  }
+
+  if (!is.null(label)) {
+    label <- span(label, class = "action-label")
+  }
+
+  tags$a(
+    id = inputId,
+    href = "#",
+    class = "action-button action-link",
     `data-val` = value,
-    list(validateIcon(icon), label),
+    icon, label,
     ...
   )
 }
 
 
-# Check that the icon parameter is valid:
-# 1) Check  if the user wants to actually add an icon:
-#    -- if icon=NULL, it means leave the icon unchanged
-#    -- if icon=character(0), it means don't add an icon or, more usefully,
-#       remove the previous icon
-# 2) If so, check that the icon has the right format (this does not check whether
-# it is a *real* icon - currently that would require a massive cross reference
-# with the "font-awesome" and the "glyphicon" libraries)
+# Throw an informative warning if icon isn't html-ish
 validateIcon <- function(icon) {
-  if (is.null(icon) || identical(icon, character(0))) {
+  if (length(icon) == 0) {
     return(icon)
-  } else if (inherits(icon, "shiny.tag") && icon$name == "i") {
-    return(icon)
-  } else {
-    stop("Invalid icon. Use Shiny's 'icon()' function to generate a valid icon")
   }
+  if (!isTagLike(icon)) {
+    rlang::warn(
+      c(
+        "It appears that a non-HTML value was provided to `icon`.",
+        i = "Try using a `shiny::icon()` (or an equivalent) to get an icon."
+      ),
+      class = "shiny-validate-icon"
+    )
+  }
+  icon
 }

R/update-input.R

@@ -181,8 +181,11 @@ updateCheckboxInput <- function(session = getDefaultReactiveDomain(), inputId, l
 updateActionButton <- function(session = getDefaultReactiveDomain(), inputId, label = NULL, icon = NULL, disabled = NULL) {
   validate_session_object(session)
 
-  if (!is.null(icon)) icon <- as.character(validateIcon(icon))
-  message <- dropNulls(list(label=label, icon=icon, disabled=disabled))
+  message <- dropNulls(list(
+    label = if (!is.null(label)) processDeps(label, session),
+    icon = if (!is.null(icon)) processDeps(validateIcon(icon), session),
+    disabled = disabled
+  ))
   session$sendInputMessage(inputId, message)
 }
 #' @rdname updateActionButton

R/utils-tags.R

@@ -0,0 +1,21 @@
+# Check if `x` is a tag(), tagList(), or HTML()
+# @param strict If `FALSE`, also consider a normal list() of 'tags' to be a tag list.
+isTagLike <- function(x, strict = FALSE) {
+  isTag(x) || isTagList(x, strict = strict) || isTRUE(attr(x, "html"))
+}
+
+isTag <- function(x) {
+  inherits(x, "shiny.tag")
+}
+
+isTagList <- function(x, strict = TRUE) {
+  if (strict) {
+    return(inherits(x, "shiny.tag.list"))
+  }
+
+  if (!is.list(x)) {
+    return(FALSE)
+  }
+
+  all(vapply(x, isTagLike, logical(1)))
+}

inst/www/shared/shiny.js

@@ -1165,30 +1165,34 @@
     getState(el) {
       return { value: this.getValue(el) };
     }
-    receiveMessage(el, data) {
-      const $el = (0, import_jquery7.default)(el);
-      if (hasDefinedProperty(data, "label") || hasDefinedProperty(data, "icon")) {
-        let label = $el.text();
-        let icon = "";
-        if ($el.find("i[class]").length > 0) {
-          const iconHtml = $el.find("i[class]")[0];
-          if (iconHtml === $el.children()[0]) {
-            icon = (0, import_jquery7.default)(iconHtml).prop("outerHTML");
-          }
+    async receiveMessage(el, data) {
+      if (hasDefinedProperty(data, "icon")) {
+        let iconContainer = el.querySelector(
+          ":scope > .action-icon"
+        );
+        if (!iconContainer) {
+          iconContainer = document.createElement("span");
+          iconContainer.className = "action-icon";
+          el.prepend(iconContainer);
         }
-        if (hasDefinedProperty(data, "label")) {
-          label = data.label;
+        await renderContent(iconContainer, data.icon);
+      }
+      if (hasDefinedProperty(data, "label")) {
+        let labelContainer = el.querySelector(
+          ":scope > .action-label"
+        );
+        if (!labelContainer) {
+          labelContainer = document.createElement("span");
+          labelContainer.className = "action-label";
+          el.appendChild(labelContainer);
         }
-        if (hasDefinedProperty(data, "icon")) {
-          icon = Array.isArray(data.icon) ? "" : data.icon ?? "";
-        }
-        $el.html(icon + " " + label);
+        await renderContent(labelContainer, data.label);
       }
       if (hasDefinedProperty(data, "disabled")) {
         if (data.disabled) {
-          $el.attr("disabled", "");
+          el.setAttribute("disabled", "");
         } else {
-          $el.attr("disabled", null);
+          el.removeAttribute("disabled");
         }
       }
     }

inst/www/shared/shiny_scss/shiny.scss

@@ -463,6 +463,13 @@ textarea.textarea-autoresize.form-control {
   }
 }
 
+// Add spacing between icon and label for actionButton()
+.action-button:not(.action-link) {
+  .action-icon + .action-label {
+    padding-left: 0.5ch;
+  }
+}
+
 /* Overrides bootstrap-datepicker3.css styling for invalid date ranges.
    See https://github.com/rstudio/shiny/issues/2042 for details. */
 .datepicker table tbody tr td.disabled,

srcts/src/bindings/input/actionbutton.ts

@@ -1,10 +1,12 @@
 import $ from "jquery";
+import type { HtmlDep } from "../../shiny/render";
+import { renderContent } from "../../shiny/render";
 import { hasDefinedProperty } from "../../utils";
 import { InputBinding } from "./inputBinding";
 
 type ActionButtonReceiveMessageData = {
-  label?: string;
-  icon?: string | [];
+  label?: { html: string; deps: HtmlDep[] };
+  icon?: { html: string; deps: HtmlDep[] };
   disabled?: boolean;
 };
 
@@ -39,45 +41,40 @@ class ActionButtonInputBinding extends InputBinding {
   getState(el: HTMLElement): { value: number } {
     return { value: this.getValue(el) };
   }
-  receiveMessage(el: HTMLElement, data: ActionButtonReceiveMessageData): void {
-    const $el = $(el);
-
-    if (hasDefinedProperty(data, "label") || hasDefinedProperty(data, "icon")) {
-      // retrieve current label and icon
-      let label: string = $el.text();
-      let icon = "";
-
-      // to check (and store) the previous icon, we look for a $el child
-      // object that has an i tag, and some (any) class (this prevents
-      // italicized text - which has an i tag but, usually, no class -
-      // from being mistakenly selected)
-      if ($el.find("i[class]").length > 0) {
-        const iconHtml = $el.find("i[class]")[0];
-
-        if (iconHtml === $el.children()[0]) {
-          // another check for robustness
-          icon = $(iconHtml).prop("outerHTML");
-        }
+  async receiveMessage(
+    el: HTMLElement,
+    data: ActionButtonReceiveMessageData
+  ): Promise<void> {
+    if (hasDefinedProperty(data, "icon")) {
+      let iconContainer = el.querySelector<HTMLElement>(
+        ":scope > .action-icon"
+      );
+      // If no container exists yet, create one
+      if (!iconContainer) {
+        iconContainer = document.createElement("span");
+        iconContainer.className = "action-icon";
+        el.prepend(iconContainer);
       }
+      await renderContent(iconContainer, data.icon);
+    }
 
-      // update the requested properties
-      if (hasDefinedProperty(data, "label")) {
-        label = data.label;
+    if (hasDefinedProperty(data, "label")) {
+      let labelContainer = el.querySelector<HTMLElement>(
+        ":scope > .action-label"
+      );
+      if (!labelContainer) {
+        labelContainer = document.createElement("span");
+        labelContainer.className = "action-label";
+        el.appendChild(labelContainer);
       }
-      if (hasDefinedProperty(data, "icon")) {
-        // `data.icon` can be an [] if user gave `character(0)`.
-        icon = Array.isArray(data.icon) ? "" : data.icon ?? "";
-      }
-
-      // produce new html
-      $el.html(icon + " " + label);
+      await renderContent(labelContainer, data.label);
     }
 
     if (hasDefinedProperty(data, "disabled")) {
       if (data.disabled) {
-        $el.attr("disabled", "");
+        el.setAttribute("disabled", "");
       } else {
-        $el.attr("disabled", null);
+        el.removeAttribute("disabled");
       }
     }
   }

srcts/types/src/bindings/input/actionbutton.d.ts

@@ -1,7 +1,14 @@
+import type { HtmlDep } from "../../shiny/render";
 import { InputBinding } from "./inputBinding";
 type ActionButtonReceiveMessageData = {
-    label?: string;
-    icon?: string | [];
+    label?: {
+        html: string;
+        deps: HtmlDep[];
+    };
+    icon?: {
+        html: string;
+        deps: HtmlDep[];
+    };
     disabled?: boolean;
 };
 declare class ActionButtonInputBinding extends InputBinding {
@@ -13,7 +20,7 @@ declare class ActionButtonInputBinding extends InputBinding {
     getState(el: HTMLElement): {
         value: number;
     };
-    receiveMessage(el: HTMLElement, data: ActionButtonReceiveMessageData): void;
+    receiveMessage(el: HTMLElement, data: ActionButtonReceiveMessageData): Promise<void>;
     unsubscribe(el: HTMLElement): void;
 }
 export { ActionButtonInputBinding };

tests/testthat/_snaps/actionButton.md

@@ -0,0 +1,21 @@
+# Action button allows icon customization
+
+    Code
+      actionButton("foo", "Click me")
+    Output
+      <button id="foo" type="button" class="btn btn-default action-button">
+        <span class="action-label">Click me</span>
+      </button>
+
+---
+
+    Code
+      actionButton("foo", "Click me", icon = icon("star"))
+    Output
+      <button id="foo" type="button" class="btn btn-default action-button">
+        <span class="action-icon">
+          <i class="far fa-star" role="presentation" aria-label="star icon"></i>
+        </span>
+        <span class="action-label">Click me</span>
+      </button>
+

tests/testthat/test-actionButton.R

@@ -55,3 +55,42 @@ test_that("Action link accepts class arguments", {
     get_class(make_link("extra extra2")), sub("\"$", " extra extra2\"", act_class)
   )
 })
+
+test_that("Action button allows icon customization", {
+  # No separator between icon and label
+  expect_snapshot(actionButton("foo", "Click me"))
+
+  # Should include separator between icon and label
+  expect_snapshot(
+    actionButton("foo", "Click me", icon = icon("star"))
+  )
+
+  # Warn on a non-HTML icon
+  expect_warning(
+    actionButton("foo", "Click me", icon = "not an icon"),
+    "non-HTML value was provided"
+  )
+
+  # Allows for arbitrary HTML as icon
+  btn <- expect_no_warning(
+    actionButton("foo", "Click me", icon = tags$svg())
+  )
+  btn2 <- expect_no_warning(
+    actionButton("foo", "Click me", icon = tagList(tags$svg()))
+  )
+  btn3 <- expect_no_warning(
+    actionButton("foo", "Click me", icon = list(tags$svg()))
+  )
+  btn4 <- expect_no_warning(
+    actionButton("foo", "Click me", icon = HTML("<svg></svg>"))
+  )
+
+  # Ignore newlines+indentation for comparison
+  as_character <- function(x) {
+    gsub("\\n\\s*", "", as.character(x))
+  }
+
+  expect_equal(as_character(btn), as_character(btn2))
+  expect_equal(as_character(btn2), as_character(btn3))
+  expect_equal(as_character(btn3), as_character(btn4))
+})