Move the filled star icon for feedback widget from python code to web…

… app (#9097) ## Describe your changes Merge after #9094 We want to move the icon so that the `ButtonGroup.tsx` component does not allow arbitrary HTML strings for security reasons, but only parses the filled star icon as such a string. ## GitHub Issue Link (if applicable) ## Testing Plan - Updates the tests according to the new helper functions --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
streamlit · Jul 19, 2024 · 40303e1 · 40303e1
1 parent 6296baf
commit 40303e1
Show file tree

Hide file tree

Showing 7 changed files with 48 additions and 67 deletions.
diff --git a/frontend/lib/src/components/shared/Icon/DynamicIcon.test.tsx b/frontend/lib/src/components/shared/Icon/DynamicIcon.test.tsx
@@ -19,7 +19,11 @@ import { render } from "@streamlit/lib/src/test_util"
 import { screen } from "@testing-library/react"
 import "@testing-library/jest-dom"
 
-import { DynamicIcon, DynamicIconProps, isMaterialIcon } from "./DynamicIcon"
+import {
+  DynamicIcon,
+  DynamicIconProps,
+  getFilledStarIconSrc,
+} from "./DynamicIcon"
 
 const getProps = (
   props: Partial<DynamicIconProps> = {}
@@ -51,13 +55,13 @@ describe("Dynamic icon", () => {
     expect(testId.textContent).toEqual(icon.textContent)
   })
 
-  it("isMaterialIcon returns correct results", () => {
-    expect(isMaterialIcon(":material/test:")).toBeTruthy()
-    expect(isMaterialIcon(":material/test-hyphen:")).toBeTruthy()
-    expect(isMaterialIcon(":material/test_underscore:")).toBeTruthy()
-    expect(isMaterialIcon(":material/test")).toBeFalsy()
-    expect(isMaterialIcon("material/test:")).toBeFalsy()
-    expect(isMaterialIcon("material/test")).toBeFalsy()
-    expect(isMaterialIcon(":materialtest:")).toBeFalsy()
+  it("renders without crashing Styled image", () => {
+    const props = getProps({ iconValue: ":material/star_filled:" })
+    render(<DynamicIcon {...props} />)
+    const testId = screen.getByTestId("stImageIcon")
+    const srcAttr = testId.getAttribute("src")
+
+    expect(testId).toBeInTheDocument()
+    expect(srcAttr).toEqual(getFilledStarIconSrc())
   })
 })
diff --git a/frontend/lib/src/components/shared/Icon/DynamicIcon.tsx b/frontend/lib/src/components/shared/Icon/DynamicIcon.tsx
@@ -19,7 +19,7 @@ import React, { Suspense } from "react"
 import { IconSize, ThemeColor } from "@streamlit/lib/src/theme"
 import { EmojiIcon } from "./Icon"
 import MaterialFontIcon from "./Material/MaterialFontIcon"
-import { StyledDynamicIcon } from "./styled-components"
+import { StyledDynamicIcon, StyledImageIcon } from "./styled-components"
 
 interface IconPackEntry {
   pack: string
@@ -39,10 +39,12 @@ function parseIconPackEntry(iconName: string): IconPackEntry {
   return { pack: iconPack, icon: iconNameInPack }
 }
 
-export function isMaterialIcon(option: string): boolean {
-  const materialIconRegexp = /^:material\/(.+):$/
-  const materialIconMatch = materialIconRegexp.exec(option)
-  return materialIconMatch !== null
+/**
+ *
+ * @returns returns an img tag with a yellow filled star icon svg as base64 data
+ */
+export function getFilledStarIconSrc(): string {
+  return "data:image/svg+xml;base64,PHN2ZyB3aWR0aD0iMjAiIGhlaWdodD0iMjAiIHZpZXdCb3g9IjAgMCAyMCAyMCIgZmlsbD0ibm9uZSIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIj48ZyBjbGlwLXBhdGg9InVybCgjY2xpcDBfMTg2MF84NDMpIj48cGF0aCBkPSJNOS45OTk5NCAxNC4zOTE2TDEzLjQ1ODMgMTYuNDgzM0MxNC4wOTE2IDE2Ljg2NjYgMTQuODY2NiAxNi4zIDE0LjY5OTkgMTUuNTgzM0wxMy43ODMzIDExLjY1TDE2Ljg0MTYgOC45OTk5N0MxNy4zOTk5IDguNTE2NjMgMTcuMDk5OSA3LjU5OTk3IDE2LjM2NjYgNy41NDE2M0wxMi4zNDE2IDcuMTk5OTdMMTAuNzY2NiAzLjQ4MzNDMTAuNDgzMyAyLjgwODMgOS41MTY2MSAyLjgwODMgOS4yMzMyNyAzLjQ4MzNMNy42NTgyNyA3LjE5MTYzTDMuNjMzMjcgNy41MzMzQzIuODk5OTQgNy41OTE2MyAyLjU5OTk0IDguNTA4MyAzLjE1ODI3IDguOTkxNjNMNi4yMTY2MSAxMS42NDE2TDUuMjk5OTQgMTUuNTc1QzUuMTMzMjcgMTYuMjkxNiA1LjkwODI3IDE2Ljg1ODMgNi41NDE2MSAxNi40NzVMOS45OTk5NCAxNC4zOTE2WiIgZmlsbD0iI0ZBQ0EyQiIvPjwvZz48ZGVmcz48Y2xpcFBhdGggaWQ9ImNsaXAwXzE4NjBfODQzIj48cmVjdCB3aWR0aD0iMjAiIGhlaWdodD0iMjAiIGZpbGw9IndoaXRlIi8+PC9jbGlwUGF0aD48L2RlZnM+PC9zdmc+"
 }
 
 export interface DynamicIconProps {
@@ -61,11 +63,23 @@ const DynamicIconDispatcher = ({
   const { pack, icon } = parseIconPackEntry(iconValue)
   switch (pack) {
     case "material":
-      return (
-        <StyledDynamicIcon {...props}>
-          <MaterialFontIcon pack={pack} iconName={icon} {...props} />
-        </StyledDynamicIcon>
-      )
+      switch (icon) {
+        case "star_filled":
+          return (
+            <StyledDynamicIcon {...props}>
+              <StyledImageIcon
+                src={getFilledStarIconSrc()}
+                data-testid={props.testid || "stImageIcon"}
+              />
+            </StyledDynamicIcon>
+          )
+        default:
+          return (
+            <StyledDynamicIcon {...props}>
+              <MaterialFontIcon pack={pack} iconName={icon} {...props} />
+            </StyledDynamicIcon>
+          )
+      }
     case "emoji":
     default:
       return (

diff --git a/frontend/lib/src/components/shared/Icon/index.tsx b/frontend/lib/src/components/shared/Icon/index.tsx
@@ -15,5 +15,5 @@
  */
 
 export { default, EmojiIcon } from "./Icon"
-export { DynamicIcon, isMaterialIcon } from "./DynamicIcon"
+export { DynamicIcon, getFilledStarIconSrc } from "./DynamicIcon"
 export { StyledIcon, StyledSpinnerIcon } from "./styled-components"
diff --git a/frontend/lib/src/components/shared/Icon/styled-components.ts b/frontend/lib/src/components/shared/Icon/styled-components.ts
@@ -105,6 +105,13 @@ export const StyledDynamicIcon = styled.span<StyledDynamicIconProps>(
   }
 )
 
+export const StyledImageIcon = styled.img(({}) => {
+  return {
+    width: "100%",
+    height: "100%",
+  }
+})
+
 interface StyledEmojiIconProps {
   size: IconSize
   margin: string

diff --git a/frontend/lib/src/components/widgets/ButtonGroup/ButtonGroup.test.tsx b/frontend/lib/src/components/widgets/ButtonGroup/ButtonGroup.test.tsx
@@ -102,27 +102,6 @@ describe("ButtonGroup widget", () => {
     })
   })
 
-  it("option-children with markdown render correctly", () => {
-    const markdownOptions = [
-      ButtonGroupProto.Option.create({ content: "Some text" }),
-      ButtonGroupProto.Option.create({
-        content: "Some text 2",
-      }),
-    ]
-    const props = getProps({ options: markdownOptions })
-    render(<ButtonGroup {...props} />)
-
-    const buttonGroupWidget = screen.getByTestId("stButtonGroup")
-    const buttons = within(buttonGroupWidget).getAllByRole("button")
-    expect(buttons).toHaveLength(2)
-    buttons.forEach(button => {
-      expect(button).toHaveAttribute("kind", "borderlessIcon")
-      within(button).getByTestId("stMarkdownContainer")
-    })
-    expect(buttons[0].textContent).toContain("Some text")
-    expect(buttons[1].textContent).toContain("Some text 2")
-  })
-
   it("sets widget value on mount", () => {
     const props = getProps()
     jest.spyOn(props.widgetMgr, "setIntArrayValue")

diff --git a/frontend/lib/src/components/widgets/ButtonGroup/ButtonGroup.tsx b/frontend/lib/src/components/widgets/ButtonGroup/ButtonGroup.tsx
@@ -32,16 +32,11 @@ import BaseButton, {
   BaseButtonKind,
   BaseButtonSize,
 } from "@streamlit/lib/src/components/shared/BaseButton"
-import {
-  DynamicIcon,
-  isMaterialIcon,
-} from "@streamlit/lib/src/components/shared/Icon"
+import { DynamicIcon } from "@streamlit/lib/src/components/shared/Icon"
 import { EmotionTheme } from "@streamlit/lib/src/theme"
 
 import { ButtonGroup as ButtonGroupProto } from "@streamlit/lib/src/proto"
 import { WidgetStateManager } from "@streamlit/lib/src/WidgetStateManager"
-import StreamlitMarkdown from "@streamlit/lib/src/components/shared/StreamlitMarkdown"
-import { iconSizes } from "@streamlit/lib/src/theme/primitives"
 import { FormClearHelper } from "@streamlit/lib/src/components/widgets/Form/FormClearHelper"
 
 export interface Props {
@@ -90,22 +85,7 @@ function syncWithWidgetManager(
 }
 
 function getContentElement(content: string): ReactElement {
-  const fontSize = "lg"
-  if (isMaterialIcon(content)) {
-    return <DynamicIcon size={fontSize} iconValue={content} />
-  }
-
-  return (
-    <StreamlitMarkdown
-      source={content}
-      allowHTML={true}
-      style={{
-        marginBottom: 0,
-        width: iconSizes[fontSize],
-        display: "inline-flex",
-      }}
-    />
-  )
+  return <DynamicIcon size="lg" iconValue={content} />
 }
 
 /**

diff --git a/lib/streamlit/elements/widgets/button_group.py b/lib/streamlit/elements/widgets/button_group.py
@@ -78,10 +78,7 @@
 _STAR_ICON: Final = ":material/star:"
 # we don't have the filled-material icon library as a dependency. Hence, we have it here
 # in base64 format and send it over the wire as an image.
-_SELECTED_STAR_ICON: Final = (
-    "<img src='data:image/svg+xml;base64,"
-    "PHN2ZyB3aWR0aD0iMjAiIGhlaWdodD0iMjAiIHZpZXdCb3g9IjAgMCAyMCAyMCIgZmlsbD0ibm9uZSIgeG1sbnM9Imh0dHA6Ly93d3cudzMub3JnLzIwMDAvc3ZnIj48ZyBjbGlwLXBhdGg9InVybCgjY2xpcDBfMTg2MF84NDMpIj48cGF0aCBkPSJNOS45OTk5NCAxNC4zOTE2TDEzLjQ1ODMgMTYuNDgzM0MxNC4wOTE2IDE2Ljg2NjYgMTQuODY2NiAxNi4zIDE0LjY5OTkgMTUuNTgzM0wxMy43ODMzIDExLjY1TDE2Ljg0MTYgOC45OTk5N0MxNy4zOTk5IDguNTE2NjMgMTcuMDk5OSA3LjU5OTk3IDE2LjM2NjYgNy41NDE2M0wxMi4zNDE2IDcuMTk5OTdMMTAuNzY2NiAzLjQ4MzNDMTAuNDgzMyAyLjgwODMgOS41MTY2MSAyLjgwODMgOS4yMzMyNyAzLjQ4MzNMNy42NTgyNyA3LjE5MTYzTDMuNjMzMjcgNy41MzMzQzIuODk5OTQgNy41OTE2MyAyLjU5OTk0IDguNTA4MyAzLjE1ODI3IDguOTkxNjNMNi4yMTY2MSAxMS42NDE2TDUuMjk5OTQgMTUuNTc1QzUuMTMzMjcgMTYuMjkxNiA1LjkwODI3IDE2Ljg1ODMgNi41NDE2MSAxNi40NzVMOS45OTk5NCAxNC4zOTE2WiIgZmlsbD0iI0ZBQ0EyQiIvPjwvZz48ZGVmcz48Y2xpcFBhdGggaWQ9ImNsaXAwXzE4NjBfODQzIj48cmVjdCB3aWR0aD0iMjAiIGhlaWdodD0iMjAiIGZpbGw9IndoaXRlIi8+PC9jbGlwUGF0aD48L2RlZnM+PC9zdmc+'/>"
-)
+_SELECTED_STAR_ICON: Final = ":material/star_filled:"
 
 _FeedbackOptions: TypeAlias = Literal["thumbs", "faces", "stars"]