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

Set timeout to focus input after DOM rendering #2163

Merged
merged 2 commits into from Feb 8, 2019

Conversation

Projects
None yet
3 participants
@vanesa
Copy link
Member

vanesa commented Feb 5, 2019

This fixes #631 for Safari and Firefox, where opening the command pallet scrolled the page to the bottom. This was caused by setting the autofocus attribute on the input element. The input was not yet rendered into the DOM because of it being inside a popover, which lead to the unintended scroll. Chrome accepts passing in the parameter preventScroll: true (introduced in this PR: #1767). However, this is not compatible with Safari or Firefox: https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/focus#Browser_compatibility.

Solution

Manually focus the rendered input element after the popover has opened by setting a timeout to wait for the input to be rendered in the DOM. This is a workaround solution as the library we use Popper.js doesn't support callbacks to get notified when the popover has finished rendering. There was an issue about it on GitHub (FezVrasta/popper.js#412) but they didn't implement it.

@vanesa vanesa requested review from chrismwendt and lguychard as code owners Feb 5, 2019

@vanesa vanesa force-pushed the vo/fix-command-palette-fx-saf branch from 2ce336f to 9bc1835 Feb 8, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Feb 8, 2019

Codecov Report

Merging #2163 into master will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted Files Coverage Δ
shared/src/commandPalette/CommandList.tsx 31.66% <0%> (-0.54%) ⬇️
@@ -82,6 +84,11 @@ export class CommandList extends React.PureComponent<Props, State> {
.getContributions()
.subscribe(contributions => this.setState({ contributions }))
)

// Only focus input after it has been rendered in the DOM
setTimeout(() => {

This comment has been minimized.

@lguychard

lguychard Feb 8, 2019

Member

Can you add a comment mentioning that this is needed for FF/Safari? Otherwise the next person reading this code might think that this is redundant with preventScroll.

This comment has been minimized.

@vanesa

vanesa Feb 8, 2019

Author Member

Good call! Will do.

@vanesa vanesa merged commit 89a594f into master Feb 8, 2019

1 check passed

buildkite/sourcegraph Build #27584 passed (5 minutes, 6 seconds)
Details

@vanesa vanesa deleted the vo/fix-command-palette-fx-saf branch Feb 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment