Skip to content

fix(ui-a11y-utils): prevent clicking on a Tooltip from closing the pa… #1905

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

ToMESSKa
Copy link
Contributor

@ToMESSKa ToMESSKa commented Mar 14, 2025

…rent dialog

INSTUI-4475

ISSUE:
-when user clicks on a Tooltip inside of a dialog with shouldCloseOnDocumentClick, the dialog closes

TEST PLAN:

  • copy test case below in the documentation
  • open the modal
  • hover over the text or button and click on the appearing tooltips
  • the modal should remain open
  • try Tooltip's renderTip prop with HTML element <div>HTML content</div> or nested HTML element like <div><div>HTML content</div></div> , the modal should remain open
  • try clicking on the Button and while the Button is focused, click on the Tooltip, the modal should remain open
  • the same example should result in closing the modal in the current version
  • Tooltip should have the same behavior (should close on Esc in a modal etc.)
  • the example should work with a Tray too (just replace the Modal tags with Tray)
 const TestModal = () => {
      const [open, setOpen] = useState(false)

      return (
         <div style={{ padding: "0 0 16rem 0" }}>
        <Button
          onClick={() => {setOpen(true)}}
        >
          Show the Modal
        </Button>

        <Modal
          label="modal"
          open={open}
          onDismiss={() => {setOpen(false)}}
          shouldCloseOnDocumentClick
        >
          <Modal.Header>
            <CloseButton
            placement="end"
            offset="small"
            screenReaderLabel="Close"
            onClick={() => {setOpen(false)}}
          />
            <Heading>Hello World</Heading>
          </Modal.Header>
          <View as="div" padding="medium" id="tray-content">
            <Tooltip
              renderTip="This is the tooltip text"
              mountNode={document.getElementById("tray-content")}
            >
              <Text>This text has a tooltip</Text>
            </Tooltip>

            <Tooltip
              renderTip="This is the tooltip text"
              mountNode={document.getElementById("tray-content")}
            >
              <Button>This button has a tooltip</Button>
            </Tooltip>
          </View>
        </Modal>
      </div>
      )
    }
    render(<TestModal />)

Copy link

github-actions bot commented Mar 14, 2025

PR Preview Action v1.6.0
Preview removed because the pull request was closed.
2025-04-07 09:40 UTC

@ToMESSKa ToMESSKa self-assigned this Mar 14, 2025
@ToMESSKa ToMESSKa requested review from matyasf and balzss March 14, 2025 13:51
Copy link
Collaborator

@matyasf matyasf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This wont work if I put HTML content into the tooltip, e.g.

 const TestModal = () => {
      const [open, setOpen] = useState(false)
      return (
         <div style={{ padding: "0 0 16rem 0" }}>
        <Button onClick={() => {setOpen(true)}} >Show the Modal</Button>
        <Modal
          label="modal"
          open={open}
          onDismiss={() => {setOpen(false)}}
          shouldCloseOnDocumentClick
        >
          <Modal.Header>
            <CloseButton placement="end" offset="small"
            screenReaderLabel="Close"
            onClick={() => {setOpen(false)}}
          />
            <Heading>Hello World</Heading>
          </Modal.Header>
          <View as="div" padding="medium" id="tray-content">
            <Tooltip
              renderTip={<div>This wont work :(</div>}
              mountNode={document.getElementById("tray-content")}
            >
              <Text>This text has a tooltip</Text>
            </Tooltip>

            <Tooltip
              renderTip={<div>This wont work :(</div>}
              mountNode={document.getElementById("tray-content")}
            >
              <Button>This button has a tooltip</Button>
            </Tooltip>
          </View>
        </Modal>
      </div>
      )
    }
    render(<TestModal />)

In FocusRegion why are you using ARIA tooltip role instead isTooltip prop of FocusRegion to determine whether its a tooltip? Now we have 2 methods of determining whether something is a tooltip (the other one is in Popover), this feels off. If it must be determined this way you could walk up in the DOM to check for this role but this would be ugly...

@ToMESSKa ToMESSKa force-pushed the INSTUI-4475_tray_with_should_close_on_document_click_closes_if_a_tooltip_is_clicked_inside_the_tray branch from c18cbe8 to 4eb6c7b Compare April 3, 2025 09:32
!(
canUseDOM &&
this._documentClickTarget instanceof HTMLElement &&
this._documentClickTarget.closest('[role="tooltip"]')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

closest returns the first (starting at element) inclusive ancestor that matches selectors, and null otherwise.

@ToMESSKa
Copy link
Contributor Author

ToMESSKa commented Apr 3, 2025

This wont work if I put HTML content into the tooltip, e.g.

 const TestModal = () => {
      const [open, setOpen] = useState(false)
      return (
         <div style={{ padding: "0 0 16rem 0" }}>
        <Button onClick={() => {setOpen(true)}} >Show the Modal</Button>
        <Modal
          label="modal"
          open={open}
          onDismiss={() => {setOpen(false)}}
          shouldCloseOnDocumentClick
        >
          <Modal.Header>
            <CloseButton placement="end" offset="small"
            screenReaderLabel="Close"
            onClick={() => {setOpen(false)}}
          />
            <Heading>Hello World</Heading>
          </Modal.Header>
          <View as="div" padding="medium" id="tray-content">
            <Tooltip
              renderTip={<div>This wont work :(</div>}
              mountNode={document.getElementById("tray-content")}
            >
              <Text>This text has a tooltip</Text>
            </Tooltip>

            <Tooltip
              renderTip={<div>This wont work :(</div>}
              mountNode={document.getElementById("tray-content")}
            >
              <Button>This button has a tooltip</Button>
            </Tooltip>
          </View>
        </Modal>
      </div>
      )
    }
    render(<TestModal />)

In FocusRegion why are you using ARIA tooltip role instead isTooltip prop of FocusRegion to determine whether its a tooltip? Now we have 2 methods of determining whether something is a tooltip (the other one is in Popover), this feels off. If it must be determined this way you could walk up in the DOM to check for this role but this would be ugly...

@matyasf This event listener is attached to the containing dialog's FocusRegion, not the Tooltip's (since the Tooltip has a separate FocusRegion). The isTooltip value is not available here because of that.

I updated the code so that it now works with an HTML element as renderTip as well. It also correctly handles cases where the button is focused when the Tooltip is clicked.

I’ve updated the test plan accordingly.

@balzss and I tried different approaches, but this current solution using contains seemed the best (even though it's still not the prettiest).

@ToMESSKa ToMESSKa requested a review from matyasf April 3, 2025 10:04
Copy link
Collaborator

@matyasf matyasf left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, I like this solution :)

@matyasf matyasf merged commit 89910fa into master Apr 7, 2025
10 of 11 checks passed
@matyasf matyasf deleted the INSTUI-4475_tray_with_should_close_on_document_click_closes_if_a_tooltip_is_clicked_inside_the_tray branch April 7, 2025 09:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants