Fixes 11053: focus event causing jumpy scroll effect #11056

Andrewgdewar · 2024-09-24T20:53:16Z

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 · 2024-09-24T20:57:48Z

Preview: https://patternfly-react-pr-11056.surge.sh

A11y report: https://patternfly-react-pr-11056-a11y.surge.sh

Andrewgdewar · 2024-09-26T19:43:50Z

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

thatblindgeye

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

thatblindgeye · 2024-09-25T12:08:26Z

packages/react-core/src/components/Dropdown/Dropdown.tsx

@@ -114,20 +114,18 @@ const DropdownBase: React.FunctionComponent<DropdownProps> = ({
      ) {
        if (onOpenChangeKeys.includes(event.key)) {
          onOpenChange(false);
-          toggleRef.current?.focus();
+          toggleRef.current?.focus({preventScroll:true});


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.

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

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.

This is the above component.

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

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?

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

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?

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

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?

When the toggleRef focus call does NOT have the preventScroll option passed in, the jump scroll bug still occurs when the menu/select/dropdown is opened

This will likely make the above described behaviour persist.
revert prevent scroll on toggle

We can try this release and see though, perhaps I was getting mixed results.

packages/react-core/src/components/Dropdown/Dropdown.tsx

Andrewgdewar · 2024-09-27T16:45:09Z

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 · 2024-10-08T16:21:11Z

/retest

tlabaj · 2024-10-08T17:03:18Z

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

packages/react-core/src/components/Dropdown/Dropdown.tsx

packages/react-core/src/components/Menu/MenuContainer.tsx

packages/react-core/src/components/Pagination/PaginationOptionsMenu.tsx

packages/react-core/src/components/Tabs/OverflowTab.tsx

tlabaj

I few nits

packages/react-core/src/components/Select/Select.tsx

Andrewgdewar · 2024-10-09T16:25:27Z

I few nits

Thanks for the review @tlabaj!

thatblindgeye

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

packages/react-core/src/components/Select/Select.tsx

packages/react-core/src/components/Tabs/OverflowTab.tsx

packages/react-core/src/components/Menu/MenuContainer.tsx

packages/react-core/src/components/Pagination/PaginationOptionsMenu.tsx

packages/react-core/src/components/Dropdown/Dropdown.tsx

Andrewgdewar · 2024-10-10T17:01:33Z

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.

patternfly-build · 2024-10-17T19:22:38Z

Your changes have been released in:

@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
@patternfly/[email protected]
[email protected]
@patternfly/[email protected]
@patternfly/[email protected]

Thanks for your contribution! 🎉

thatblindgeye requested review from thatblindgeye and tlabaj September 25, 2024 12:04

tlabaj requested a review from kmcfaul September 25, 2024 14:05

kmcfaul linked an issue Sep 26, 2024 that may be closed by this pull request

Bug - Dropdown, Select, and MenuContainer - focus event causing jumpy scroll effect #11053

Closed

thatblindgeye requested changes Sep 27, 2024

View reviewed changes

Andrewgdewar force-pushed the 11053 branch from f065503 to 27b8aeb Compare September 27, 2024 16:42

Andrewgdewar requested a review from thatblindgeye September 27, 2024 16:47

tlabaj reviewed Oct 8, 2024

View reviewed changes

packages/react-core/src/components/Dropdown/Dropdown.tsx Outdated Show resolved Hide resolved

tlabaj reviewed Oct 8, 2024

View reviewed changes

packages/react-core/src/components/Menu/MenuContainer.tsx Outdated Show resolved Hide resolved

tlabaj reviewed Oct 8, 2024

View reviewed changes

packages/react-core/src/components/Pagination/PaginationOptionsMenu.tsx Outdated Show resolved Hide resolved

tlabaj reviewed Oct 8, 2024

View reviewed changes

packages/react-core/src/components/Tabs/OverflowTab.tsx Outdated Show resolved Hide resolved

tlabaj requested changes Oct 8, 2024

View reviewed changes

packages/react-core/src/components/Select/Select.tsx Outdated Show resolved Hide resolved

Andrewgdewar force-pushed the 11053 branch from 27b8aeb to 49129ed Compare October 9, 2024 16:24

Andrewgdewar requested a review from tlabaj October 9, 2024 16:25

thatblindgeye requested changes Oct 10, 2024

View reviewed changes

Andrewgdewar force-pushed the 11053 branch from 49129ed to 1b3ffd0 Compare October 10, 2024 14:20

Fixes 11053: focus event causing jumpy scroll effect

7289988

Andrewgdewar force-pushed the 11053 branch from 1b3ffd0 to 7289988 Compare October 10, 2024 17:00

Andrewgdewar requested a review from thatblindgeye October 10, 2024 17:01

tlabaj approved these changes Oct 16, 2024

View reviewed changes

dlabaj approved these changes Oct 17, 2024

View reviewed changes

revert prevent scroll on toggle

6c5abac

thatblindgeye approved these changes Oct 17, 2024

View reviewed changes

thatblindgeye mentioned this pull request Oct 17, 2024

fix(Menus): added new fix for scroll jump bug #11119

Merged

Merge branch 'v5' into 11053

38c3fbe

thatblindgeye merged commit 01b3094 into patternfly:v5 Oct 17, 2024
13 checks passed

Andrewgdewar mentioned this pull request Oct 22, 2024

Fixes 4746: Jumpiness when clicking menus/dropdowns content-services/content-sources-frontend#364

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes 11053: focus event causing jumpy scroll effect #11056

Fixes 11053: focus event causing jumpy scroll effect #11056

Andrewgdewar commented Sep 24, 2024 •

edited by thatblindgeye

Loading

patternfly-build commented Sep 24, 2024 •

edited

Loading

Andrewgdewar commented Sep 26, 2024

thatblindgeye left a comment

thatblindgeye Sep 25, 2024

Andrewgdewar Sep 27, 2024

thatblindgeye Oct 10, 2024

Andrewgdewar Oct 10, 2024

thatblindgeye Oct 11, 2024

Andrewgdewar Oct 15, 2024 •

edited

Loading

thatblindgeye Oct 16, 2024

Andrewgdewar Oct 17, 2024 •

edited

Loading

Andrewgdewar commented Sep 27, 2024 •

edited

Loading

Andrewgdewar commented Oct 8, 2024

tlabaj commented Oct 8, 2024

tlabaj left a comment

Andrewgdewar commented Oct 9, 2024

thatblindgeye left a comment

Andrewgdewar commented Oct 10, 2024 •

edited

Loading

patternfly-build commented Oct 17, 2024

Fixes 11053: focus event causing jumpy scroll effect #11056

Fixes 11053: focus event causing jumpy scroll effect #11056

Conversation

Andrewgdewar commented Sep 24, 2024 • edited by thatblindgeye Loading

patternfly-build commented Sep 24, 2024 • edited Loading

Andrewgdewar commented Sep 26, 2024

thatblindgeye left a comment

Choose a reason for hiding this comment

thatblindgeye Sep 25, 2024

Choose a reason for hiding this comment

Andrewgdewar Sep 27, 2024

Choose a reason for hiding this comment

thatblindgeye Oct 10, 2024

Choose a reason for hiding this comment

Andrewgdewar Oct 10, 2024

Choose a reason for hiding this comment

thatblindgeye Oct 11, 2024

Choose a reason for hiding this comment

Andrewgdewar Oct 15, 2024 • edited Loading

Choose a reason for hiding this comment

thatblindgeye Oct 16, 2024

Choose a reason for hiding this comment

Andrewgdewar Oct 17, 2024 • edited Loading

Choose a reason for hiding this comment

Andrewgdewar commented Sep 27, 2024 • edited Loading

Andrewgdewar commented Oct 8, 2024

tlabaj commented Oct 8, 2024

tlabaj left a comment

Choose a reason for hiding this comment

Andrewgdewar commented Oct 9, 2024

thatblindgeye left a comment

Choose a reason for hiding this comment

Andrewgdewar commented Oct 10, 2024 • edited Loading

patternfly-build commented Oct 17, 2024

Andrewgdewar commented Sep 24, 2024 •

edited by thatblindgeye

Loading

patternfly-build commented Sep 24, 2024 •

edited

Loading

Andrewgdewar Oct 15, 2024 •

edited

Loading

Andrewgdewar Oct 17, 2024 •

edited

Loading

Andrewgdewar commented Sep 27, 2024 •

edited

Loading

Andrewgdewar commented Oct 10, 2024 •

edited

Loading