Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 13 additions & 3 deletions packages/ui-a11y-utils/src/FocusRegion.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,8 @@ class FocusRegion {
constructor(element: Element | Node | null, options: FocusRegionOptions) {
this._options = options || {
shouldCloseOnDocumentClick: true,
shouldCloseOnEscape: true
shouldCloseOnEscape: true,
isTooltip: false
}
this._contextElement = element
this._screenReaderFocusRegion = new ScreenReaderFocusRegion(
Expand Down Expand Up @@ -122,6 +123,10 @@ class FocusRegion {
if (fileInputFocused) {
;(<HTMLInputElement>activeElement).blur()
} else {
//This should prevent a Tooltip from closing when inside of a Modal
if (this._options.isTooltip) {
event.stopPropagation()
}
Comment on lines +126 to +129
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.

this.handleDismiss(event)
}
}
Expand Down Expand Up @@ -169,9 +174,14 @@ class FocusRegion {
}
})
}

//This will ensure that the Tooltip's Escape event listener is executed first
//because listeners in the capturing phase are called before other event listeners (like that of the Modal's Escape listener)
//so a Modal with a Tooltip will not get closed when closing the Tooltip with Escape
const useCapture = this._options.isTooltip
if (this._options.shouldCloseOnEscape) {
this._listeners.push(addEventListener(doc, 'keyup', this.handleKeyUp))
this._listeners.push(
addEventListener(doc, 'keyup', this.handleKeyUp, useCapture)
)
}

this._active = true
Expand Down
4 changes: 4 additions & 0 deletions packages/ui-a11y-utils/src/FocusRegionOptions.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,8 @@ export type FocusRegionOptions = {
* provides a reference to the underlying html root element
*/
elementRef?: (element: Element | null) => void
/**
* Whether or not the element is a Tooltip
*/
isTooltip?: boolean
}
6 changes: 5 additions & 1 deletion packages/ui-dialog/src/Dialog/props.ts
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,11 @@ const propTypes: PropValidators<PropKeys> = {
/**
* provides a reference to the underlying html root element
*/
elementRef: PropTypes.func
elementRef: PropTypes.func,
/**
* 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

}

const allowedProps: AllowedPropKeys = [
Expand Down
3 changes: 2 additions & 1 deletion packages/ui-popover/src/Popover/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,8 @@ class Popover extends Component<PopoverProps, PopoverState> {
this._focusRegion = new FocusRegion(this._contentElement, {
shouldCloseOnEscape: this.props.shouldCloseOnEscape,
shouldCloseOnDocumentClick: false,
onDismiss: this.hide
onDismiss: this.hide,
isTooltip: true
})

if (this.shown) {
Expand Down
2 changes: 1 addition & 1 deletion packages/ui-tooltip/src/Tooltip/index.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,7 @@ class Tooltip extends Component<TooltipProps, TooltipState> {
defaultIsShowingContent={defaultIsShowingContent}
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

placement={placement}
color={color === 'primary' ? 'primary-inverse' : 'primary'}
mountNode={mountNode}
Expand Down
Loading