Fixing Dialog focus modifying the page scroll

[Mil4n0r]

Checklist (Check off all the items before submitting)

• [x] Build process is done without errors. All tests pass in the /lib directory.
• [x] Self-reviewed the code before submitting.
• [x] Meets accessibility standards.
• [x] Added/updated documentation to /website as needed.
• [x] Added/updated tests as needed.

Description The logic that handles focus and keypresses inside the Dialog component has been modified in order to prevent a bug with the page automatically scrolling when we reached a limit object.

Additional context The new implementation for the FocusLock component seems to fix the issue. However I can think of certain edge cases that would need to be reviewed being:

• How to detect which one is the first focusable object in the focusableElements array without actually focusing it. Right now, the first and last are being considered the ones with index 0 and length - 1. (Is being in the focusableElements array a warranty that the item can be focused? If so, is it necessary that we use the attemptFocus function?)
• I have realized that the scroll check inside the testing section also passes with the previous version, which makes me wonder how can we properly check the case documented in the issue #1532

Closes #1532

[GomezIvann]

Hi @Mil4n0r!

I've reviewed your PR and the issue and your proposal is fair. However, the problem could not be related to the behaviour of the Focus Lock, which is working fine. It's a matter of CSS.

As you already know, the Focus Locks wraps its children component between two divs that prevent the focus from moving out of it. The scroll when tabbing is caused by those divs that gain the focus momentarily because they are placed at the very beginning of the page. As we are using a React Portal, they have been placed there, outside of the main flow, but not with the positioning of their children (in this case the Dialog).

In my opinion, the problem should be solved via CSS rather than more logic that can muddy the code. What do you think?

In regards to how to test it, I think it can be done with Storybook easily. Scroll is a visual aspect so it can not be tested with Jest.

[Mil4n0r]

I agree that the problem can be solved with CSS. However, I believe the Focus event momentarily reaching adjacent divs should not be considered an expected or desired behavior, as it triggers an extra action unnecessarily.

In my latest commit, I propose (from my point of view) a more accurate and readable approach that builds on some ideas of the old version, using an iterative way of "attempting to focus" the next component. This time, thanks to the use of the % operator, it removes the assumption of 0 and length - 1 being focusable and handles the whole Tab logic inside a single div that wraps the children component without the possibility of exiting that focus in any moment.

Another good approach would be to consider using an external library like this one: react-focus-lock - npm (npmjs.com) (I Recommend reading this article as well to understand the benefits) It’s a (focus) Trap!. It is a trap! We got your focus and… | by Anton Korzunov | Medium As you said, though, we have to take into account that adding new dependencies is unwanted.

In any case, I think that seeking a third opinion would be appropriate. We can even open a discussion maybe.

[GomezIvann]

Hello again.

As I already told you, the current Focus Lock works fine and hasn't produced any bug since it was created. Using two colliding divs was a decision based on the fact that you couldn't always programmatically keep the focus on one component. With those two divs we make sure that when it goes out, the focus is controlled and doesn't go to any unwanted elements. The extra action in this case and from my point of view is worthy (no performance issues and is transparent to the user). But, again, this is just an opinion. What we cannot afford is to change every existing implementation every time we find a small bug, Enrique. It's not just your time but the time of the person reviewing it.

I've reviewed the current code and the problem is solved by just moving the FocusLock in the Dialog from the outside of the component to the inside, which is trivial since we want to keep the focus locked in between the elements inside the Dialog:

I've tested this and it works fine. If you want we can seek for a third opinion but I think we are going too far for just a bug.

You can check my proposal here: gomezivann/dialog-fix

[Mil4n0r]

Seems like a fair approach for me, it does seem to cover all the cases.

I will close this PR and leave you merge with your proposal (gomezivann/dialog-fix).

In case any problem related to this functionality appears in the future we can always take a look at this branch 😄

Thanks for the feedback!