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

v8: Ensure all other elements can't be focused while infinite editing is active (Focus/Tab lock) #4526

Open
wants to merge 50 commits into
base: v8/dev
from

Conversation

@BatJan
Copy link
Contributor

commented Feb 11, 2019

Prerequisites

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

If there's an existing issue for this PR then this fixes: #4525

Description

The intend is to make sure that it's not possible to tab out of the infinite overlay at first - I aim to make some reusable code, which could also be used for normal/ordinary overlays so we keep focused trapped in the overlays until they're removed again. For further details please see the description in the referenced issue.

… way of doing this for easy reuse in ordinary overlays too
@emmaburstow

This comment has been minimized.

Copy link
Contributor

commented Feb 13, 2019

Thanks for this WIP @BatJan

We'll take a look when you're ready for us.

Em

@nul800sebastiaan nul800sebastiaan changed the base branch from temp8 to dev-v8 Feb 19, 2019
@BatJan BatJan changed the title WIP: v8: Ensure all other elements can't be focused while infinite editing is active WIP: v8: Ensure all other elements can't be focused while infinite editing is active (Focus/Tab lock) Mar 22, 2019
@zpqrtbnk zpqrtbnk changed the base branch from dev-v8 to v8/dev Mar 30, 2019
BatJan added 6 commits Mar 31, 2019
…verlay is triggered then we figure out how to "hide" those that should not be tabable and ensure that the current one is
… clear difference between infinite overlays and modal overlays. Also adding the start of method trying to add focus to the "previous" infinte overlay in case we have multiple open and close the active one
…red but it will do for now
@BatJan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 1, 2019

@nielslyngsoe @nul800sebastiaan @emmaburstow

Ok so it's time for at little update on my progress with this one.

Currently I have a working POC that acts and behaves in a better way - However I'm relying heavily on jQuery and I have 2 setTimeout functions in order to make sure I can run some DOM operations when the nodes I need to target have been set/updated - I need to find another way of doing this and I hope to have it refactored during the upcoming sunday - Fingers crossed 🤞

As mentioned I'm using a polyfill for the "inert" attribute, which is in "incubation" state currently. It's my understanding that over time this will mean it will make it into the HTML spec at some point and that browser vendors will start implementing the attribute. It's not a big polyfill so even if the attribute may not make it into the official HTML spec in the future I think it's still a valid approach to use it since it handles setting aria-hidden="true", which will hide the entire section this attribute is setup on for screen readers and furthermore it sets tabindex="-1" on each interactive element child of the parent element where the inert attribute has been placed.

I have added a "focustrap.service.js" file, which contains functions for adding and removing focus traps on both "infinite overlays" but also on "modal" overlays. It turned out to be quite quick to add the method in the overlay.service.js file while I was at it anyway 😃 and locking the "tabable" fields to those in the overlay matters in both scenarios so we might as well fix it all in one go.

So when using the inert attribute the default behavior is that tabbing outside the overlay is allowed in the sense that it's possible to tab to the browsers address bar so they're not completely locked to the overlay when tabbing - It's possible to break out if you want to browse somewhere else. But I'm wondering if perhaps we should add some functionality so one will not be able to tab out into the browser address bar? The current functionality is a lot better than what we currently have but I can't make my mind up about whether or not to lock tabbing to only the open overlay. I think I might like how the menu at https://24ways.org/ better - Try opening the menu by clicking the right side burger icon and then start tabbing - You're trapped in the overlay and not able to get to the browser address bar. What do you think @nielslyngsoe ? Should we aim for this or should we leave the current solution as is? (I still need to do some refactoring though) 😃

It's not possible to see it on the video when I enter the browser bar since it's cut off from the recording area so to get a sense of what it works like it might help to check out the inert attribute demo page tabbing through it? https://wicg.github.io/inert/demo/

Before
In this video I open an infinite overlay for dealing with a media item - Once I have browsed through the overlay I keep tabbing but it's hard to know when watching the video - Around 15-18 second in you can notice that the area where the page name is edited get's a blue highlight - This illustrates that currently tabbing is not locked inside the overlay - https://www.youtube.com/watch?v=es8i3ALmmm0&feature=youtu.be

Now
In this video I open multiple infinite overlays when dealing with a document type - It should be fairly clear that I can only tab through the opened tab - However it's not clear that after tabbing past the green "submit" button the next target is the browser address bar but tabbing is locked 💯 As mentioned above we can choose not allow people to tab out into the browser address bar but have them go straight to the first interactive field in the overlay - We just need to agree 😉
https://www.youtube.com/watch?v=xrXatqMS4-M&feature=youtu.be

I hope all of the above makes sense - Looking forward to any comments you may have 😃

@BatJan

This comment has been minimized.

Copy link
Contributor Author

commented Apr 2, 2019

Good evening!

Just a little update - I decided to refactor the code into a directive instead of a service since that felt more appropriate and more reusable than having a specific service that could not really be reused.

But functionality wise nothing has changed in the way this affects the accessibility of infinite editors and regular overlays 😃

@BatJan BatJan changed the title WIP: v8: Ensure all other elements can't be focused while infinite editing is active (Focus/Tab lock) v8: Ensure all other elements can't be focused while infinite editing is active (Focus/Tab lock) Apr 2, 2019
@Shazwazza

This comment has been minimized.

Copy link
Member

commented Apr 2, 2019

@nielslyngsoe If you have some time, might be good to provide any feedback you have on this one

@BatJan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 12, 2019

Hello @nul800sebastiaan and @nielslyngsoe

I have refactored the functionality into a "focusLockService" instead of a directive. So the following has happened

  • Removal of the directive I had previous made and any references used to it
  • Creation of a focusLock.service.js with method for adding and removing focus locks for infinite overlays and normal overlays
  • Got rid of the jQuery previously being used
  • Reduced the amount of setTimeout() function to only 1 for resetting focus when infinite overlays are closed
  • Injected the service into umbEditors.directive.js and overlay.service.js enabling focus locking many places in the backoffice 😃

The final thing I need to look into is how to avoid reaching the address bar - I think that the 24ways.org site I referenced previously is also using the same polyfill but on their site when the menu is open focus never reaches the browser address bar.

Apart from that I'd say this PR is finally ready! 🍾

Overlay focus lock - After
overlay-after

Infinite overlay focus lock - After
infinite-overlay-after

@BatJan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2019

As I wrote the above comment I just realized some things I need to fix - So back to the drawing board. At least I see things clearly now! 😅 - I will keep you posted!

@BatJan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 13, 2019

@nul800sebastiaan @nielslyngsoe OK, skip all the previous stuff I have written and just read this comment 😃

I have refactored the original POC from a directive into a focusLock service. The service is injected into the overlay.service and the umbeditors.directive where the necessary methods are being called from.

In the refactor I'm not using jQuery and the timeouts have been eliminated to only one, which is needed in order to ensure that DOM is ready and translucsions have happened.

I have checked with W3C recommendations and it is recommended to lock the focus within the open overlay. For some reason I had got it into my head that the polyfill addressed that but it does the heavy lifting of inerting/disabling background elements - Therefore a function to capture the first and last focusable element has been added.

Infinite overlays
This GIF is a bit long since I demonstrate how it's possible to navigate back and forth using TAB or TAB+SHIFT keys opening and closing layers.

infinite-overlay-lock-after

Regular overlays
overlay-lock-after

I'm eagerly looking forward to receiving your feedback on this 👍

@BatJan BatJan referenced this pull request Oct 15, 2019
1 of 1 task complete
@BatJan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2019

@bjarnef Ah yes! Got it now - Thanks!

@nul800sebastiaan - I'm going to do a little more work. I'll let you know once it's ready 😅

@BatJan

This comment has been minimized.

Copy link
Contributor Author

commented Oct 16, 2019

@nul800sebastiaan Ok, NOW this is ready for being merged. I refactored the directive so the lookup for the labelKey happens in the directive making the view cleaner as @bjarnef suggested (I know that a few moments ago I was reluctant to do it - But I changed my mind 😁 )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.