Fix front-end action button label updating logic (#4242)

* Close #4239: fix front-end action button label updating logic (follow up to #3996)

* Update news

* Use a separator instead of putting markup in attributes

* `yarn build` (GitHub Actions)

* Address feedback

* Cleanup

* Refactor into a single method to split icon/label

* `yarn build` (GitHub Actions)

* Better naming

* Add some padding to the separator

* Add some unit tests for R logic

* Update NEWS.md

* Update NEWS.md

* Update NEWS.md

* Update NEWS.md

* Increase backcompat (keep same R structure when no icon is provided)

* Refine comment

---------

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,8 +1,17 @@
 # shiny (development version)
 
+## New features
+
+* The `icon` argument of `actionButton()`, `downloadButton()`, etc. now accepts values other than `shiny::icon()` (like `fontawesome::fa()` and `bsicons::bs_icon()`). (#4242)
+
+## Improvements
+
+* Padding is now provided between the `icon` and `label` of an `actionButton()`. (#4242)
 ## Bug fixes
 
-* Fixed a regression in v1.11.0 where `InputBinding`'s that didn't pass a value to their `subscribe` callback where to no longer working. (#4243)
+* Fixed a regression in v1.11.0 where `InputBinding` implementations that don't pass a value to their `subscribe` callback were no longer notifying Shiny of input changes. (#4243)
+
+* `updateActionButton()` and `updateActionLink()` once again handle `label` updates correctly (which can now include HTML). (#4242)
 
 # shiny 1.11.0
 

R/input-action.R

@@ -56,13 +56,14 @@ actionButton <- function(inputId, label, icon = NULL, width = NULL,
 
   value <- restoreInput(id = inputId, default = NULL)
 
-  tags$button(id=inputId,
+  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),
+    get_action_children(label, icon),
     ...
   )
 }
@@ -72,30 +73,52 @@ 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",
+  tags$a(
+    id = inputId,
+    href = "#",
+    class = "action-button",
     `data-val` = value,
-    list(validateIcon(icon), label),
+    get_action_children(label, icon),
     ...
   )
 }
 
+get_action_children <- function(label, icon) {
+  icon <- validateIcon(icon)
 
-# 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)
-validateIcon <- function(icon) {
-  if (is.null(icon) || identical(icon, character(0))) {
-    return(icon)
-  } else if (inherits(icon, "shiny.tag") && icon$name == "i") {
-    return(icon)
+  if (length(icon) > 0) {
+    # The separator elements helps us distinguish between the icon and label
+    # when dynamically updating the button/link. Ideally, we would wrap each
+    # in a container element, but is currently done with a separator to help
+    # minimize the chance of breaking existing code.
+    tagList(
+      icon,
+      tags$span(class = "shiny-icon-separator"),
+      label
+    )
   } else {
-    stop("Invalid icon. Use Shiny's 'icon()' function to generate a valid icon")
+    # Technically, we don't need the `icon` here, but keeping it maintains
+    # backwards compatibility of `btn$children[[1]][[2]]` to get the label.
+    # The shinyGovstyle package is at least one example of this.
+    tagList(icon, label)
   }
 }
+
+# Throw an informative warning if icon isn't html-ish
+validateIcon <- function(icon) {
+  if (length(icon) == 0) {
+    return(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,10 +181,9 @@ 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 = if (!is.null(label)) processDeps(label, session),
-    icon = icon,
+    icon = if (!is.null(icon)) processDeps(validateIcon(icon), session),
     disabled = disabled
   ))
   session$sendInputMessage(inputId, message)

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

@@ -1137,6 +1137,8 @@
 
   // srcts/src/bindings/input/actionbutton.ts
   var import_jquery7 = __toESM(require_jquery());
+  var iconSeparatorClass = "shiny-icon-separator";
+  var iconSeparatorHTML = `<span class='${iconSeparatorClass}'></span>`;
   var ActionButtonInputBinding = class extends InputBinding {
     find(scope) {
       return (0, import_jquery7.default)(scope).find(".action-button");
@@ -1165,24 +1167,23 @@
     getState(el) {
       return { value: this.getValue(el) };
     }
-    receiveMessage(el, data) {
+    async 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");
-          }
-        }
+        let { label, icon } = this._getIconLabel(el);
+        const deps = [];
         if (hasDefinedProperty(data, "label")) {
-          label = data.label;
+          label = data.label.html;
+          deps.push(...data.label.deps);
         }
         if (hasDefinedProperty(data, "icon")) {
-          icon = Array.isArray(data.icon) ? "" : data.icon ?? "";
+          icon = data.icon.html;
+          deps.push(...data.icon.deps);
         }
-        $el.html(icon + " " + label);
+        if (icon.trim()) {
+          icon = icon + iconSeparatorHTML;
+        }
+        await renderContent(el, { html: icon + label, deps });
       }
       if (hasDefinedProperty(data, "disabled")) {
         if (data.disabled) {
@@ -1195,6 +1196,21 @@
     unsubscribe(el) {
       (0, import_jquery7.default)(el).off(".actionButtonInputBinding");
     }
+    _getIconLabel(el) {
+      const nodes = Array.from(el.childNodes);
+      const nodeContents = nodes.map(
+        (node) => node instanceof Element ? node.outerHTML : node.textContent
+      );
+      const separator = el.querySelector(`.${iconSeparatorClass}`);
+      if (!separator) {
+        return { icon: "", label: nodeContents.join("") };
+      }
+      const idx = nodes.indexOf(separator);
+      return {
+        icon: nodeContents.slice(0, idx).join(""),
+        label: nodeContents.slice(idx + 1).join("")
+      };
+    }
   };
   (0, import_jquery7.default)(document).on("click", "a.action-button", function(e4) {
     e4.preventDefault();

inst/www/shared/shiny_scss/shiny.scss

@@ -463,6 +463,10 @@ textarea.textarea-autoresize.form-control {
   }
 }
 
+.shiny-icon-separator {
+  padding-right: 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,13 +1,19 @@
 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;
 };
 
+// Needs to mirror shiny:::icon_separator()'s markup.
+const iconSeparatorClass = "shiny-icon-separator";
+const iconSeparatorHTML = `<span class='${iconSeparatorClass}'></span>`;
+
 class ActionButtonInputBinding extends InputBinding {
   find(scope: HTMLElement): JQuery<HTMLElement> {
     return $(scope).find(".action-button");
@@ -39,38 +45,31 @@ class ActionButtonInputBinding extends InputBinding {
   getState(el: HTMLElement): { value: number } {
     return { value: this.getValue(el) };
   }
-  receiveMessage(el: HTMLElement, data: ActionButtonReceiveMessageData): void {
+  async receiveMessage(
+    el: HTMLElement,
+    data: ActionButtonReceiveMessageData
+  ): Promise<void> {
     const $el = $(el);
 
     if (hasDefinedProperty(data, "label") || hasDefinedProperty(data, "icon")) {
-      // retrieve current label and icon
-      let label: string = $el.text();
-      let icon = "";
+      let { label, icon } = this._getIconLabel(el);
+      const deps: HtmlDep[] = [];
 
-      // 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");
-        }
-      }
-
-      // update the requested properties
       if (hasDefinedProperty(data, "label")) {
-        label = data.label;
-      }
-      if (hasDefinedProperty(data, "icon")) {
-        // `data.icon` can be an [] if user gave `character(0)`.
-        icon = Array.isArray(data.icon) ? "" : data.icon ?? "";
+        label = data.label.html;
+        deps.push(...data.label.deps);
       }
 
-      // produce new html
-      $el.html(icon + " " + label);
+      if (hasDefinedProperty(data, "icon")) {
+        icon = data.icon.html;
+        deps.push(...data.icon.deps);
+      }
+
+      if (icon.trim()) {
+        icon = icon + iconSeparatorHTML;
+      }
+
+      await renderContent(el, { html: icon + label, deps });
     }
 
     if (hasDefinedProperty(data, "disabled")) {
@@ -85,6 +84,29 @@ class ActionButtonInputBinding extends InputBinding {
   unsubscribe(el: HTMLElement): void {
     $(el).off(".actionButtonInputBinding");
   }
+
+  // Split the contents of the element into the icon and label.
+  private _getIconLabel(el: HTMLElement): { icon: string; label: string } {
+    const nodes = Array.from(el.childNodes);
+    const nodeContents = nodes.map((node) =>
+      node instanceof Element ? node.outerHTML : node.textContent
+    );
+
+    // Query the separator element
+    const separator = el.querySelector(`.${iconSeparatorClass}`);
+
+    // No separator found, so the entire contents are the label.
+    if (!separator) {
+      return { icon: "", label: nodeContents.join("") };
+    }
+
+    // Find the index of the separator element in the child nodes.
+    const idx = nodes.indexOf(separator);
+    return {
+      icon: nodeContents.slice(0, idx).join(""),
+      label: nodeContents.slice(idx + 1).join(""),
+    };
+  }
 }
 
 // TODO-barret should this be put in the init methods?

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,8 +20,9 @@ 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;
+    private _getIconLabel;
 }
 export { ActionButtonInputBinding };
 export type { ActionButtonReceiveMessageData };

tests/testthat/_snaps/actionButton.md

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

tests/testthat/test-actionButton.R

@@ -55,3 +55,38 @@ 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>"))
+  )
+  expect_equal(as.character(btn), as.character(btn2))
+  expect_equal(as.character(btn2), as.character(btn3))
+  expect_equal(as.character(btn3), as.character(btn4))
+})