Skip to content
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

Fixes 11053: focus event causing jumpy scroll effect #11056

Open
wants to merge 1 commit into
base: v5
Choose a base branch
from

Conversation

Andrewgdewar
Copy link

@Andrewgdewar Andrewgdewar commented Sep 24, 2024

Closes #11053

Prevents inconsistent scrolling behaviour on a number of select/menu components.

This also removes the timeout, which appears is no longer needed.
(Slightly snappier without, with/without barely noticeable).

What: Closes 11053

Additional issues:
Associated External issue from Red Hat Insights team

@patternfly-build
Copy link
Contributor

patternfly-build commented Sep 24, 2024

@Andrewgdewar
Copy link
Author

Someone please point me to the documentation to run the linter.
🙄

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Some initial comments below that would most likely apply to all components updated.

For the linter docs, should be the last bullet in the React component requirements of our contributing guide.

packages/react-core/src/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved
@@ -114,20 +114,18 @@ const DropdownBase: React.FunctionComponent<DropdownProps> = ({
) {
if (onOpenChangeKeys.includes(event.key)) {
onOpenChange(false);
toggleRef.current?.focus();
toggleRef.current?.focus({preventScroll:true});
Copy link
Contributor

Choose a reason for hiding this comment

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

What was the reason for preventing scroll on this focus call? This focus call should only be called when the menu toggle is closed via Escape or Tab key (or whatever array of keys is passed to onOpenChangeKeys), in which case I think it makes sense that we scroll the page back to whatever item (the menu toggle) has focus.

Imagining a scenario where the Dropdown/menu has a lot of items on a page, or the menu toggle is just scrolled out of view, and focus is on the last item. Pressing Escape or Tab should close the dropdown and focus the menu toggle, but if the menu toggle isn't in view then it may be confusing where the keyboard focus went.

Copy link
Author

Choose a reason for hiding this comment

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

I was still seeing the described problem on menudropdowns on filter views without the above change.
Why this is being triggered on multi-select components I don't know.
With the other fix in place, and this one not.
The same behaviour below was occurring:

Screen.Recording.2024-09-27.at.5.01.27.PM.mov

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, this toggleRef...focus() line is being called when the menu is opened and is causing the scroll jump effect to occur as shown in the video above? Could you share the code of the Select being used that is showing this behavior to test out? I wouldn't expect this line to cause the scroll jump bug to trigger, but could definitely just be something odd going on unexpectedly.

Copy link
Author

Choose a reason for hiding this comment

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

This is the above component.

This is the wrapper I've built to contain the madness that is the patternfly Select component. 😆

Copy link
Contributor

Choose a reason for hiding this comment

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

So I tried mimicking the setup from both files linked above just within a PatternFly example (wasn't able to get the repo linked in the above files running locally), and I'm not able to reproduce the jump scroll when simply removing the preventScroll option from this toggleRef.focus call of the Select component. May not be an exact 1:1 mimic, and it could also be a difference between the PatternFly workspace and yours, but as far as I can tell reverting this specific line in the changed files shouldn't cause that jump scroll bug when the menu is opened.

Personally I would opt to remove the options being passed to the toggleRef.focus() calls in these files (but keep the option in the other focus calls, i.e. firstElement...focus()), as the expected behavior should be that the page will scroll back to where the MenuToggle receives focus upon the Menu being closed via keyboard. Then if the jump scroll bug is still occurring with the rest of these updates merged in, we could revisit again to investigate further or add the options in as you did originally here.

How did you go about testing the updates in this PR in your product running locally? And you can confirm that reverting the toggleRef.current?.focus({preventScroll:true}); lines in this PR back to just toggleRef.current?.focus(); (but keeping all other changes) causes the bug to still occur when clicking on a MenuToggle to open the select/dropdown/menu?

Copy link
Author

@Andrewgdewar Andrewgdewar Oct 15, 2024

Choose a reason for hiding this comment

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

How did you go about testing the updates

The other component changes were not tested; the copied logic matched and thus I added the changes as I presumed they would be desired.

How did you go about testing the updates in this PR

I simply made the changes to the patternfly components in node_modules > patternfly, and rebuilt.

Personally I would opt to remove the options being passed to the toggleRef.focus()

This is fine, I'll either use another component or find another way to prevent the behaviour.
In my mind, this is a bug needing a fix, but there's no need to force all to subscribe to this.

can confirm that reverting the toggleRef.current?.focus({preventScroll:true})

I'll test again today

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey @Andrewgdewar , just wanted to check in on if you were able to test the above?

Also wanted to clarify in case I might be misinterpreting the issue here. Can you confirm between the foilowing which you originally meant in regards to this toggleRef...focus call needing the preventScroll option?

  1. When the toggleRef focus call does NOT have the preventScroll option passed in, the jump scroll bug still occurs when the menu/select/drodown is opened
  2. Or, were you saying that you believe that we shouldn't scroll the page when the menu/select/dropdown is closed and the MenuToggle (toggleRef in this case) receives focus?

packages/react-core/src/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved
@Andrewgdewar
Copy link
Author

Andrewgdewar commented Sep 27, 2024

Updated:

  • Lint
  • Added paramater preventScrollOnItemFocus (feedback on description copy desired)
  • setTimeout is removed, waiting on explanation of it's requirement, setting preventScrollOnItemFocus to false with setTimeout in place creates inconsistent scroll behaviour.

I suggest either making preventScroll > true and including the timeout without the new paramater, or removing setTimeout and including the preventScrollOnItemFocus paramater (as is currently the case with this PR).

@Andrewgdewar
Copy link
Author

/retest

@tlabaj
Copy link
Contributor

tlabaj commented Oct 8, 2024

@Andrewgdewar try and rebase your PR to fix the test failure. I pushed a change yesterday.

Copy link
Contributor

@tlabaj tlabaj left a comment

Choose a reason for hiding this comment

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

I few nits

packages/react-core/src/components/Select/Select.tsx Outdated Show resolved Hide resolved
@Andrewgdewar
Copy link
Author

I few nits

Thanks for the review @tlabaj!

Copy link
Contributor

@thatblindgeye thatblindgeye left a comment

Choose a reason for hiding this comment

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

Small casing fix noted below (usage of the prop will also need to be updated), and also left additional comments on previous replies above.

@Andrewgdewar
Copy link
Author

Andrewgdewar commented Oct 10, 2024

Small casing fix noted below (usage of the prop will also need to be updated), and also left additional comments on previous replies above.

Thanks! I took the recommended variable too literally (lol.. copied casing).
I also re-added the timeout and associated timeout variable as requested, let me know if copy is okay for that variable name/description.

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.

Bug - Dropdown, Select, and MenuContainer - focus event causing jumpy scroll effect
4 participants