Code review; fix tooltips inside a card

Author: cpsievert

R/card.R

@@ -274,10 +274,9 @@ full_screen_toggle <- function() {
     tags$span(
       class = "bslib-full-screen-enter",
       class = "badge rounded-pill bg-dark",
-      title = "Expand",
       full_screen_toggle_icon()
     ),
-    placement = "bottom"
+    "Expand"
   )
 }
 

R/tooltip.R

@@ -3,23 +3,18 @@
 #' Tooltips are useful for showing additional information when focusing (or
 #' hovering over) a UI element.
 #'
-#' @param ... Unnamed arguments can be any valid child of an [htmltools
-#'   tag][htmltools::tags]. Named arguments become HTML attributes on returned
-#'   UI element. Attributes starting with `data-bs-*` may be supplied to further
-#'   customize [tooltip
-#'   options](https://getbootstrap.com/docs/5.3/components/tooltips/).
-#' @param body Content to display in the tooltip.
-#' @param placement The placement of the tooltip relative to the target element.
-#' @param html Whether to treat `body` as HTML. WARNING: setting this `TRUE`
-#'   when the `body` is a function of user `inputs` is dangerous and not
-#'   recommended.
-#' @param sanitize Whether to sanitize HTML (only relevant when `html = TRUE`).
-#'   This can be useful if `body` is a function of user `inputs`, but should
-#'   also be treated as HTML.
+#' @param trigger A UI element (i.e., [htmltools tag][htmltools::tags]) to serve as the
+#'   tooltips trigger. It's good practice for this element to be a keyboard-focusable
+#'   and interactive element (e.g., `actionButton()`, `actionLink()`, etc) so that
+#'   the tooltip is accessible to keyboard and assistive technology users.
+#' @param ... UI elements for the tooltip. Character strings are automatically
+#'   [htmlEscape()]d unless marked as [HTML()].
 #' @param id If provided, you can use `input$id` in your server logic to
 #'   determine whether the tooltip is currently shown/hidden.
+#' @param placement The placement of the tooltip relative to its trigger.
+#' @param options A list of additional [Bootstrap options](https://getbootstrap.com/docs/5.3/components/tooltips/#options).
 #'
-#' @details If multiple UI elements are provided to `...`, the last element is
+#' @details If `x` yields multiple HTML elements, the last element is
 #' used as the tooltip's target. If the target should contain multiple elements,
 #' then wrap those elements in a [span()] or [div()].
 #'
@@ -30,38 +25,48 @@
 #'
 #' tooltip(
 #'   shiny::actionButton("btn", "A button"),
-#'   body = "A message"
+#'   "A message"
 #' )
 #'
 #' card(
 #'   card_header(
 #'     tooltip(
 #'       span("Card title ", bsicons::bs_icon("question-circle-fill")),
-#'       body = "Additional info",
+#'       "Additional info",
 #'       placement = "right"
 #'     )
 #'   ),
 #'   "Card body content..."
 #' )
 tooltip <- function(
-  ...,
-  body = "Tooltip",
-  placement = c("auto", "top", "right", "bottom", "left"),
-  html = FALSE,
-  sanitize = FALSE,
-  id = NULL
-) {
+    trigger, ..., id = NULL,
+    placement = c("auto", "top", "right", "bottom", "left"),
+    options = list()
+  ) {
+
+  args <- rlang::list2(...)
+  argnames <- rlang::names2(args)
+
+  children <- args[!nzchar(argnames)]
+  attribs <- args[nzchar(argnames)]
+
+  #if (length(attribs) > 0) {
+  #  abort(c(
+  #    paste0("Unknown named argument: '", names(attribs)[1], "'."),
+  #    "i" = "Did you intend to pass it to `options`?"
+  #  ))
+  #}
 
   res <- web_component(
     "bslib-tooltip",
-    placement = rlang::arg_match(placement),
-    html = if (html) NA,
-    sanitize = if (sanitize) NA,
     id = id,
+    placement = rlang::arg_match(placement),
+    options = jsonlite::toJSON(options),
+    !!!attribs,
     # Use display:none instead of <template> since shiny.js
     # doesn't bind to the contents of the latter
-    div(body, style = "display:none;"),
-    ...
+    div(!!!children, style = "display:none;"),
+    trigger
   )
 
   res <- tag_require(res, version = 5, caller = "tooltip()")
@@ -89,13 +94,15 @@ tooltip_toggle <- function(id, show = NULL, session = get_current_session()) {
 }
 
 
-#' @describeIn tooltip Update the `body` of a tooltip.
+#' @describeIn tooltip Update the contents of a tooltip.
 #' @export
-tooltip_update <- function(id, body = NULL, session = get_current_session()) {
+tooltip_update <- function(id, ..., session = get_current_session()) {
+
+  title <- tagList(...)
 
   msg <- dropNulls(list(
     method = "update",
-    title = if (!is.null(body)) processDeps(body, session)
+    title = if (length(title) > 0) processDeps(title, session)
   ))
 
   force(id)
@@ -105,6 +112,10 @@ tooltip_update <- function(id, body = NULL, session = get_current_session()) {
   session$onFlush(callback, once = TRUE)
 }
 
+# TODO: worth it?
+# tooltip_disable <- function(id) {}
+# tooltip_enable <- function(id) {}
+
 
 normalize_show_value <- function(show) {
   if (is.null(show)) return("toggle")

srcts/src/components/card.ts

@@ -157,7 +157,7 @@ class Card {
    */
   private _addEventListeners(): void {
     const btnFullScreen = this.card.querySelector(
-      `:scope > .${Card.attr.CLASS_FULL_SCREEN_ENTER}`
+      `:scope > * > .${Card.attr.CLASS_FULL_SCREEN_ENTER}`
     );
     if (!btnFullScreen) return;
     btnFullScreen.addEventListener("click", (ev) => this.enterFullScreen(ev));

srcts/src/components/tooltip.ts

@@ -34,27 +34,18 @@ export class BslibTooltip extends LightElement {
   _tooltip!: TooltipType;
 
   @property({ type: String }) placement: TooltipOptions["placement"] = "auto";
-  @property({ type: Boolean }) html = false;
-  @property({ type: Boolean }) sanitize = false;
+  @property({ type: String }) options = "{}";
 
-  get options(): TooltipOptions {
-    // Offical 'public' API
-    const opts: TooltipOptions = {
+  get allOptions(): TooltipOptions {
+    const opts = JSON.parse(this.options);
+    return {
       title: this.title,
       placement: this.placement,
-      html: this.html,
-      sanitize: this.sanitize,
+      // Bootstrap defaults to false, but we have our own HTML escaping
+      html: true,
+      sanitize: true,
+      ...opts,
     };
-
-    // Support 'unofficial' `data-bs-*` attributes
-    for (const attr of this.attributes) {
-      if (attr.name.startsWith("data-bs-")) {
-        const key = attr.name.replace("data-bs-", "");
-        (opts as any)[key] = attr.value;
-      }
-    }
-
-    return opts;
   }
 
   get title(): string {
@@ -71,7 +62,7 @@ export class BslibTooltip extends LightElement {
   connectedCallback(): void {
     super.connectedCallback();
     this.reference.setAttribute("data-bs-toggle", "tooltip");
-    this._tooltip = new Tooltip(this.reference, this.options);
+    this._tooltip = new Tooltip(this.reference, this.allOptions);
 
     this.reference.addEventListener("shown.bs.tooltip", this._onShown);
     this.reference.addEventListener("hidden.bs.tooltip", this._onHidden);
@@ -98,12 +89,15 @@ export class BslibTooltip extends LightElement {
     // Note: a child template will always be present as the first child,
     // so ignore the 1st child
     if (this.children.length > 1) {
-      return this.children[this.children.length - 1];
+      const ref = this.children[this.children.length - 1];
+      ref.setAttribute("tabindex", "0");
+      return ref;
     }
     // If there are childNodes (i.e., a text node), then wrap the last one in a
     // span and use that as the reference
     if (this.childNodes.length > 1) {
       const ref = document.createElement("span");
+      ref.setAttribute("tabindex", "0");
       ref.append(this.childNodes[this.childNodes.length - 1]);
       this.appendChild(ref);
       return ref;
@@ -119,7 +113,6 @@ export class BslibTooltip extends LightElement {
 
   private _onShown(): void {
     this.visible = true;
-    // TODO: do we need to trigger a shown/hidden for Shiny?
     this.onChangeCallback(true);
   }
 
@@ -143,6 +136,7 @@ export class BslibTooltip extends LightElement {
       this._tooltip[data.value]();
     } else if (method === "update") {
       if (data.title) {
+        Shiny.renderDependencies(data.title.deps);
         // eslint-disable-next-line @typescript-eslint/naming-convention
         this._tooltip.setContent({ ".tooltip-inner": data.title.html });
       }