Patternfly-react: Popover: vertical scrolling on close

[eyevana]

It doesn't happen every time, but if you open a popover and click on the close button, the page seems to jump. Here's a screen share to demonstrate --> https://drive.google.com/file/d/1A4QMnZXaISBLknQtCRU0_7LBtH_Kt0qJ/view?usp=sharing

[jschuler]

It's because we "focus trap" within the popover, and when the popover is closed, focus is returned to the toggling element, which scrolls the element into view if it is not already.
We could add a prop to enable/disable the returnFocus feature, or disable it by default if the popover was opened via mouse click (instead of the enter key). @seanforyou23 WDYT?

[seanforyou23]

Yes I suppose we could add some logic to treat this more smoothly, but I wouldn't think to do it by disabling the behavior of sending focus back to the triggering element, IMO that's an important aspect of the experience we want to try and preserve. It's important for both accessibility as well as usability in general (we need consistency with focus mgmt).

Maybe we could consider attempting to normalize the scroll transition so that it only minimally scrolls as necessary to bring the focus element into view? Keep in mind, we have all the standard supported browser/devices to test this against. My vote would be to pursue a quick POC for that instead of spending time adding ability to do something that in the end goes against what we generally want to happen.

If we're in agreement that the sending of focus back to target element is part of the experience we want people to have, I think we could switch this issue from labeled as bug to feature request. What we're currently getting seems to be the browser default behavior which brings the element into view "best effort". Does it sound ok @jschuler @eyevana ? Lemme know what you think!

[jschuler]

I think if the user used the mouse to open the Popover we should not automatically focus the close button in the popover and also not return focus to the trigger if again the mouse was used to close the popover. I think we should auto-focus only if keyboard was used:

• Enter key used to open popover: auto-focus close button
• Mouse used to open popover: Do not auto-focus close button
• ESC key used to close popover: auto-focus trigger
• Enter key used on close button to close the popover: auto-focus trigger
• Mouse used on close button to close the popover: Do not auto-focus trigger

[seanforyou23]

Yea we've generally treated the mouse click event pattern slightly different from how we treat keyboard interactions so probably best to keep that convention for now. Makes sense to me @jschuler

[jessiehuff]

Sounds good to me! As long as we test the interaction out with a screen reader and make sure that still works, I'm ok with this solution. 🙂

[jschuler]

The behavior described above has been implemented
https://www.patternfly.org/v4/documentation/react/components/popover

[eyevana]

Sweet. Thanks, y'all! 💯