Skip to content

fix(many): fix Tooltip focus issues and make Tooltip closeable inside of a Modal#1889

Merged
ToMESSKa merged 1 commit intomasterfrom
INSTUI-4459_tooltip_dismissible_hoverable_persistent
Mar 6, 2025
Merged

fix(many): fix Tooltip focus issues and make Tooltip closeable inside of a Modal#1889
ToMESSKa merged 1 commit intomasterfrom
INSTUI-4459_tooltip_dismissible_hoverable_persistent

Conversation

@ToMESSKa
Copy link
Contributor

@ToMESSKa ToMESSKa commented Mar 5, 2025

INSTUI-4459

TEST PLAN:

test the examples in Tooltip:

  • the Tooltip should remain visible when it is hovered over
  • it should close on ESC, even when it is hovered over
  • the Tooltip examples should work as expected, even with keyboard navigation

test the Tooltip when inside of a modal:

  • open the TEST#1 code in the comments below
  • the Tooltips should close on ESC without closing the modal, even when it is hovered over

test the Tooltip with Tray:

  • open the TEST#2 code in the comments below
  • open the Tray via keyboard
  • Tab to the "Hello Instructure" button
  • observe that the tooltip appears
  • pressing space should close the tooltip and tray at the same time
  • you should now be able to tab away from the tray triggering element
  • you should be able to trigger the tray again and tab to different elements (before,
    after closing the tray you couldn't tab anywhere)

test the Tooltip with Buttons opening modals:

  • open the TEST#3 code in the comments below
  • you should be able to close each Button with ESC and focus should return to the triggering button after closing the modal
  • (before the second button's modal couldn't be closed with ESC)

test the Tooltip with a complex Dialog example:

@ToMESSKa ToMESSKa self-assigned this Mar 5, 2025
@github-actions
Copy link

github-actions bot commented Mar 5, 2025

PR Preview Action v1.6.0
Preview removed because the pull request was closed.
2025-03-06 12:08 UTC

@ToMESSKa ToMESSKa force-pushed the INSTUI-4459_tooltip_dismissible_hoverable_persistent branch from 47231e7 to 65d3089 Compare March 6, 2025 08:11
@ToMESSKa ToMESSKa changed the title fix(many): fix Tooltip focus issues fix(many): fix Tooltip focus issues and make Tooltip closeable inside of a Modal Mar 6, 2025
@ToMESSKa
Copy link
Contributor Author

ToMESSKa commented Mar 6, 2025

TEST#1

const Example = () => {
  const [open, setOpen] = useState(false)

  const handleButtonClick = () => {
    setOpen((state) => !state)
  }

  const renderCloseButton = () => {
    return (
      <CloseButton
        placement="end"
        offset="small"
        onClick={handleButtonClick}
        screenReaderLabel="Close"
      />
    )
  }

  return (
    <div style={{ padding: '0 0 11rem 0', margin: '0 auto' }}>
      <Button onClick={handleButtonClick}>
        {open ? 'Close' : 'Open'} the Modal
      </Button>
      <Modal
        open={open}
        onDismiss={() => {
          setOpen(false)
        }}
        size="auto"
        label="Hello World"
        shouldCloseOnDocumentClick
      >
        <Modal.Header>
          {renderCloseButton()}
          <Heading>Hello World</Heading>
        </Modal.Header>
        <Modal.Body>
          <Tooltip
            renderTip="Hello. I'm a tool tip"
            placement="start"
            on={['click', 'hover', 'focus']}
          >
            <IconInfoLine />
          </Tooltip>

          <Tooltip renderTip="Hello. I'm a tool tip" as={Button}>
            Hover or focus me
          </Tooltip>
        </Modal.Body>
      </Modal>
    </div>
  )
}
render(<Example />)

@ToMESSKa
Copy link
Contributor Author

ToMESSKa commented Mar 6, 2025

TEST#2

const FPO = lorem.paragraph()

class Example extends React.Component {
  constructor(props) {
    super(props)
    this.state = {
      open: false
    }
  }

  hideTray = () => {
    this.setState({
      open: false
    })
  }

  renderCloseButton() {
    return (
      <Flex>
        <Flex.Item shouldGrow shouldShrink>
          <Heading>Hello</Heading>
        </Flex.Item>
        <Flex.Item>
          <CloseButton
            placement="end"
            offset="small"
            screenReaderLabel="Close"
            onClick={this.hideTray}
          />
        </Flex.Item>
      </Flex>
    )
  }

  render() {
    return (
      <div style={{ padding: '0 0 16rem 0', margin: '0 auto' }}>
        <Button
          onClick={() => {
            this.setState({ open: true })
          }}
        >
          Show the Tray
        </Button>
        <Tray
          label="Tray Example"
          open={this.state.open}
          onDismiss={() => {
            this.setState({ open: false })
          }}
          size="small"
          placement="start"
        >
          <View as="div" padding="medium">
            {this.renderCloseButton()}
            <Text as="p" lineHeight="double">
              {FPO}
            </Text>
          </View>
          <Tooltip
            renderTip="Hello. I'm a tool tip"

            // onShowContent={() => console.log('showing')}
            // onHideContent={() => console.log('hidden')}
          >
            <Button
              onClick={() => {
                this.setState({ open: false })
              }}
            >
              Hello Instructure
            </Button>
          </Tooltip>
        </Tray>
      </div>
    )
  }
}
render(<Example />)

@ToMESSKa
Copy link
Contributor Author

ToMESSKa commented Mar 6, 2025

TEST#3

const fpo = lorem.paragraphs(5);

const Example = () => {
  const [open, setOpen] = useState(false);
  const ref = useRef();

  const handleButtonClick = () => {
    setOpen((state) => !state);
  };

  const handleFormSubmit = (e) => {
    e.preventDefault();
    console.log("form submitted");
    setOpen(false);
  };

  const renderCloseButton = () => {
    return (
      <CloseButton
        placement="end"
        offset="small"
        onClick={handleButtonClick}
        screenReaderLabel="Close"
      />
    );
  };

  // useEffect(() => {
  //   if (!open && ref.current) ref.current.focus();
  // }, [open]);

  return (
    <div style={{ padding: "0 0 11rem 0" }}>
      <Button onClick={handleButtonClick}>
        {open ? "Close" : "Open"} the Modal
      </Button>

      <Tooltip renderTip="Hello. I'm a tool tip">
        <Button onClick={handleButtonClick}>
          {open ? "Close" : "Open"} the Modal
        </Button>
      </Tooltip>

      <Tooltip renderTip="Hello. I'm a tool tip">
        <Button ref={(el) => (ref.current = el)}>Other button</Button>
      </Tooltip>

      <Modal
        as="form"
        open={open}
        onDismiss={() => {
          setOpen(false);
        }}
        onSubmit={handleFormSubmit}
        size="auto"
        label="Hello World"
        shouldCloseOnDocumentClick
      >
        <Modal.Header>
          {renderCloseButton()}
          <Heading>Hello World</Heading>
        </Modal.Header>
        <Modal.Body>
          <TextInput
            renderLabel="Example"
            placeholder="if you hit enter here, it should submit the form"
          />
          <Text lineHeight="double">{fpo}</Text>
        </Modal.Body>
        <Modal.Footer>
          <Button onClick={handleButtonClick} margin="0 x-small 0 0">
            Close
          </Button>
          <Button color="primary" type="submit">
            Submit
          </Button>
        </Modal.Footer>
      </Modal>
    </div>
  );
};

render(<Example />);

Comment on lines +126 to +129
//This should prevent a Tooltip from closing when inside of a Modal
if (this._options.isTooltip) {
event.stopPropagation()
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This solution is based on registering the Tooltips ESC event listener first, even before the Modal's, so when pressing ESC, the first registered event listener (the one belonging to the Tooltip) is executed first as well. Then the event propagation is stoped from reaching the Modal.

@ToMESSKa ToMESSKa requested review from balzss and matyasf March 6, 2025 08:54
on={on}
shouldRenderOffscreen
shouldReturnFocus={true}
shouldReturnFocus={false}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this line which caused the whole issue is reverted to false

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.

very nice work! Lets merge this because its urgent, but we should write some unit tests for this in the future

Copy link
Contributor

@balzss balzss left a comment

Choose a reason for hiding this comment

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

left one comment, otherwise it look good, great work!

/**
* Whether or not the element is a Tooltip
*/
isTooltip: PropTypes.bool
Copy link
Contributor

Choose a reason for hiding this comment

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

is this needed for something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is needed because of the FocusRegionOptions is joined with the Dialog's props

@ToMESSKa ToMESSKa requested a review from balzss March 6, 2025 12:04
@ToMESSKa ToMESSKa merged commit 3d05afe into master Mar 6, 2025
11 checks passed
@ToMESSKa ToMESSKa deleted the INSTUI-4459_tooltip_dismissible_hoverable_persistent branch March 6, 2025 12:08
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