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

Accessibility: Add focus lock for the infinite editor #8522

Merged
merged 40 commits into from
May 4, 2021
Merged

Accessibility: Add focus lock for the infinite editor #8522

merged 40 commits into from
May 4, 2021

Conversation

BatJan
Copy link
Contributor

@BatJan BatJan commented Jul 30, 2020

Prerequisites

  • I have added steps to test this contribution in the description below

Description

OK! So I think this PR is in a state where I need HQ feedback and suggestions. I demoed how it currently works for @nul800sebastiaan during the CanCon Umbracathon event. In short it extends the existing focus lock so it also works with infinite editor overlays meaning it works for overlays triggerd by the overlay service and the infinite editor 👍

However there is 1 issues that sometimes the focus start on the "close" button, as showcased below but focus is still locked inside the infinite overlay. The reason why it starts at the "close" button is because once lock initializes when the $includedContentLoaded event occurs it only knows about the "Close" and "Submit" button - The reason why it's still possible to tab into the list is because we make use of a DOM mutation observer, which updates the "focusableElements" variable whenever a change to attributes, childlist or a subtree of a childlist has changes - This is done to also ensure that if something starts out being hidden will be focusable once it's visible. If the view had a "search" field at the beginning it probably would not be an issue 😂 But it would not make sense to add in this context

The issue that's being rambled about / described above
inifnite-overlay-focus-lock-bug

I would have loved to create more stunning GIF's, but my virtual machine has horrible performance so I'm afraid I need to skip this 😢 You can have a little taste of it in the attached GIF below but it does not serve this PR justice. The entire experience setting up a property on a doctype has been improved heavily - Not only is the focus locked but the last focused element is also remembered when closing layers - Give it a spin, it's hard to describe 😂 (But it's good!)

I hope @nielslyngsoe can have some time to look at the code and maybe have an idea to solve the remaining issue so it all just starts at the first focusable element always 🤞 I'm have not doubt that the code can be improved at lot - But I''ve had enough starring at it for now (Made the first stab at this PR around February/March 2019 a few weeks before v8.0.0 was relased 😅 It's been quite a journey!).

I've tested various scenarios and I think I've been around every corner of the backoffice and I have not been able to spot other issues than the one described, which still works better than what we currently have overall 😃 There is another issue not related to the focus lock though - It's due to some paragraphs containing sr-only text, which have a tabindex="0" set, which is something to investigate in another PR and it's not related to this PR.

Please let me know of any changes or ideas you might have 👍

Little demo of what's to be expected
inifnite-overlay-focus-lock-single

Please note! The following list of PR's should ideally be reviewed and merged before proceeding with reviewing and merging this one. They have all derived from issues discovered during the work of making the "infinite editor" tab friendly using a focus lock - But in an attempt to keep this PR as small and focused as possible the follwing PR's have derived from the test work done in this PR 👍

BatJan added 24 commits July 30, 2020 14:34
…st editor has not been closed when the lock is used in infinite mode
… correct target and re-initialising the directive to activate the focus lock
@BatJan BatJan marked this pull request as ready for review October 17, 2020 18:51
@glombek glombek self-assigned this Jan 25, 2021
@glombek
Copy link
Contributor

glombek commented Apr 9, 2021

Hi @BatJan,

Thank you - this is looking great.

In fact, when I merge this with the latest contrib branch, I'm not able to replicate the issue you had with the close button any more.

If you're happy with the merge conflict resolution (see #10074 ) we can look at getting this merged.

@BatJan
Copy link
Contributor Author

BatJan commented May 4, 2021

@glombek Thank you for taking the time to do a thorough review. Sorry for my late feedback - I've had a look at the fix you linked to and it looks good to me 👍🏻 - Do you need me to do anything else before this one can be merged? 😃

disconnectObserver = true;

// Pass the correct editor in order to find the focusable elements
var newTarget = infiniteEditors[infiniteEditors.length - 2];
Copy link
Member

Choose a reason for hiding this comment

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

@nul800sebastiaan I've just experienced a problem with this line.
From reading this line there is an obvious mistake as we easily could have just one or zero infinite editors.

Lets me know if you need anything from me, but I think this will have to be dealt with before making the RC. :-)

@nielslyngsoe
Copy link
Member

nielslyngsoe commented May 18, 2021

@BatJan did you know that you can move focus to the browser when dealing with dialogs. but when in Infinite editors you stay on the layer. Both scenarios works fine in terms of not providing focus to the things in the back.

@BatJan
Copy link
Contributor Author

BatJan commented May 18, 2021

@nielslyngsoe I'm not sure what you mean? 😅 Do you refer to the element? If so as far as I know there are accessibility issues with this element unfortunately https://www.scottohara.me/blog/2019/03/05/open-dialog.html hence why we're rolling our own stuff. If I misunderstand please tell me more 😃

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

Successfully merging this pull request may close these issues.

3 participants