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

manual autofocus of command pallet input to prevent scroll to bottom of page, fixes #631 #1767

Open
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@vanesa
Copy link
Member

vanesa commented Jan 10, 2019

This fixes #631, 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 being inside a popover, which lead to the unintended scroll.

Solution

Manually focus the rendered input element after the popover has opened.

@felixfbecker

This comment has been minimized.

Copy link
Member

felixfbecker commented Jan 10, 2019

How could styling affect this at all? Intuitively I would guess this is caused by using an <a> with href="#" and no onClick that prevents default

@vanesa

This comment has been minimized.

Copy link
Member

vanesa commented Jan 10, 2019

@felixfbecker I tried preventing default but it didn't work. I can't explain it either, but this solves the issue.

@felixfbecker

This comment has been minimized.

Copy link
Member

felixfbecker commented Jan 10, 2019

main-page is a class that gets added to body. Changing display: block could cause problems that are not immediately visible, and I don't have confidence that it properly fixes the issue reliably in all cases

@nicksnyder

This comment has been minimized.

Copy link
Member

nicksnyder commented Jan 11, 2019

I tried preventing default

Which file/code did you try? I agree with Felix that this seems like the wrong fix

@vanesa vanesa force-pushed the vo/fix-command-palette branch from 5c62c13 to 5a58602 Jan 12, 2019

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Jan 12, 2019

Codecov Report

Merging #1767 into master will not change coverage.
The diff coverage is 0%.

Impacted Files Coverage Δ
shared/src/commandPalette/CommandList.tsx 31.66% <0%> (ø) ⬆️

@vanesa vanesa requested review from chrismwendt and lguychard as code owners Jan 12, 2019

@vanesa vanesa changed the title delete display:block causing scroll issue, fix #631 manual autofocus of command pallet input to prevent scroll to bottom of page, fixes #631 Jan 12, 2019

})
// Only focus input after it has been rendered in the DOM
if (this.props.autoFocus) {
setTimeout(() => {

This comment has been minimized.

@felixfbecker

felixfbecker Jan 14, 2019

Member

Why the setTimeout?

This comment has been minimized.

@vanesa

vanesa Jan 14, 2019

Member

To make sure the input is already rendered in the DOM.

This comment has been minimized.

@felixfbecker

felixfbecker Jan 14, 2019

Member

A setTimeout doesn't guarantee that. The ref callback does

This comment has been minimized.

@vanesa

vanesa Jan 14, 2019

Member

Without the setTimeout it still scrolls to the bottom. I assume the popper library is rendering the popover content at the bottom of the page first and the setTimeout allows the component to focus the input once its rendered at the top.

This comment has been minimized.

@felixfbecker

felixfbecker Jan 14, 2019

Member

Then this solution is very brittle. Did you confirm your assumption, e.g. by setting a breakpoint and checking the element in the DOM?

This comment has been minimized.

@vanesa

vanesa Jan 14, 2019

Member

The poppver is located all the way to the bottom of DOM. Adding a breakpoint at the timeout shows that the element has not yet been positioned top: 0px.

Breakpoint at timeout:
image

After the timeout is when you can see it fully rendered:
image

This comment has been minimized.

@felixfbecker

felixfbecker Jan 14, 2019

Member

How are we calling the Bootstrap popover code? Through a React wrapper or through the jQuery plugin? Could we listen to an event for when the positioning is done?

this.setState({ autoFocus: true })
})
// Only focus input after it has been rendered in the DOM
if (this.props.autoFocus) {

This comment has been minimized.

@lguychard

lguychard Jan 14, 2019

Member

It looks like this was here as a workaround to eventually focus the field even if props.autoFocus was false, essentially overriding the props.

This changes that behaviour by always trusting the props. Did you check the experience pre/post fix in the browser extension?

This comment has been minimized.

@vanesa

vanesa Jan 14, 2019

Member

I considered not respecting props.autoFocus as a bug but it was workaround that we no longer need, so I can just remove that prop.

This comment has been minimized.

@vanesa

vanesa Jan 14, 2019

Member

RE behavior of the browser extension: I tested it and it didn't scroll. However, Quinn's comment says "sometimes", so I probably have to test it more.

@@ -134,14 +130,14 @@ export class CommandList extends React.PureComponent<Props, State> {
</label>
<input
id="command-list__input"
ref={input => input && this.state.autoFocus && input.focus()}

This comment has been minimized.

@lguychard

lguychard Jan 14, 2019

Member

this.state.autoFocus is set async above, so I'd check this.props here to avoid race conditions.

This comment has been minimized.

@vanesa

vanesa Jan 14, 2019

Member

We're no longer going to be using props.autoFocus.

@vanesa vanesa requested a review from ijsnow as a code owner Jan 14, 2019

@vanesa vanesa force-pushed the vo/fix-command-palette branch 2 times, most recently from d6ce85b to 93eada0 Jan 14, 2019

@vanesa vanesa removed the request for review from ijsnow Jan 15, 2019

@vanesa vanesa force-pushed the vo/fix-command-palette branch from 93eada0 to 21ab014 Jan 16, 2019

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