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

Add focus-lock directive #8141

Merged
merged 26 commits into from
Jul 23, 2020
Merged

Add focus-lock directive #8141

merged 26 commits into from
Jul 23, 2020

Conversation

BatJan
Copy link
Contributor

@BatJan BatJan commented May 19, 2020

Prerequisites

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

Description

This PR is still WIP but will be the successor of #4526 - I plan to initially make the directive work on overlays injected by the overlay service and once this works I will move onto fixing it in the infinite editor as originally planned - I should have a much better idea of achieving the desired goal in the best manner now. The old PR got a bit messy 😅 Stay tuned!

What has been done?

I have installed the wicg-inert polyfill since while the inert attribute is supported by a variety of browser it does not do much on it's own. The idea is that when using the attribute the entire section where it's added will be inert meaning you can't interact with any <button>, <a>, <input> etc. withing that section and that content will be ignored by screen readers as well. Therfore the polyfill is currently needed until browsers start doing the heavy lifting for us.

The polyfill will add aria-hidden="true" and tabindex="-1" so the elements can't be interacted with.

When an overlay is activated the #mainwrapper will be inerted meaning only the interactive elements within the overlay will be possible to interact with. Therefore overlays must be placed outside this container in order to be focusable, which is also the case when using the overlay service or infinite ediotrs 😃

Currently a "focus lock service" has been added to add and remove the attribute, whenever an overlay is activated using the "overlay service". Ideally the attribute could just have been added/removed within the "backdrop service" but since we need to ensure that 3rd party packages still might be using the umb-overlay directive to open overlays we need to ensure it's still backward compatible. Backwards compatibility is the main reason why this extra service is needed to set and remove the attribute.

I have also added a "focus lock directive", which has been placed on the umb-overlay view so we can capture all the focusable elements and ensure that our tabbing does not escape into the browser address bar, which would be the default behavior if we only set the inert attribute. The directive also sets the focusable element with respect to the umb-auto-focus directive. If this directive is used to specifically set focus on an element this will be focused when the overlay opens up and otherwise the first focusable element will be set.

Gotcha's

I noticed that some interactive elements within an overlay was hidden from view using either ng-show og ng-hide meaning the interactive element would still be captured by our focus lock and break it in a way that tabbing ends up being locked to the last interactive element in the overlay when you have tabbed through all elements once. I think it would be too heavy to check if the elements parent elements are hidden or not so I converted these expressions to use ng-if instead ensuring the elements are not available in the DOM, which fixed the tabbing issues.

I have ironed out the issues I could find but it will require some thorough testing though. I will keep testing myself as well since there are many corners to checkout in our beloved CMS ❤️

What is the scope of the test?¨

The focus lock itself will work on overlays triggered by the "overlay service" as well as the "overlay directive". The difference is that the inert attribute will only be setup when the "overlay service" is used. However it appears that there are some PR's converting the old way to using the overlay service - So at some point all internal overlays, will set the inert attribute as well. And old overlays will still get a nice UX improvement.

Currently the "search" overlay is done by itself so neither the inert attribute nor the focus lock will be initiated by this currently - But I think there is a PR in place for making this be triggered by the overlay service as well if I'm not mistaken.

IMPORTANT! Make sure to run npm install before trying to test this, since I added the wicg-inert polyfill to actually inert the interactive elements like, <button>, <input> etc. so screen readers won't "see" them.

I noticed a few minor issues that I will address in seperate PR's in a short while, which I will address as well. One of them being the focus styling on <a> missing on the user overlay as seen in this photo below.

focus-bug

Below you can see the before and after experience 🤩

Before
Currently focus is not locked inside the open overlay, which is a bad user experience as well as it's bad for accessibility - Notice how I actually tab quite a lot in this gif but the only things highlighted are all the stuff behind the overlay.

umb-overlay-without-focus-lock

After
With the focus service and the focus lock in place the focus is locked/trapped inside the open overlay. I tab backwards and forward in this GIF.

umb-overlay-with-focus-lock

@nielslyngsoe @nul800sebastiaan

@BatJan
Copy link
Contributor Author

BatJan commented May 22, 2020

Oh, it appears that the gulp task is failing... Is anyone from HQ or the pull request team able to give me some further details about what is wrong so I can fix please? 😃

@BatJan
Copy link
Contributor Author

BatJan commented May 27, 2020

Just for the record I have tested this PR with a 3rd party package that makes use of the umb-overlay directive and it appears to work just dine - However without the inert attribute set as I mentioned in the long explanation above 😅 The used package was https://our.umbraco.com/packages/backoffice-extensions/bg-icon-picker-v8/

@poornimanayar
Copy link
Contributor

poornimanayar commented Jun 26, 2020

Hi @BatJan ,

I have tested your work on the following

  1. adding content to grid - works
  2. nested content overlay - works
  3. adding element type to datatype in datatypes tree - works
  4. issue mentioned in Using the tab key doesn't work anymore when selecting datatype #8310 - this PR fixes it
  5. creating grid configuration in datatype - :( does not work. I am not sure whether I am supposed to be testing here and its one of a gotcha situation
  6. Selecting a media item in content - does not work, the issue mentioned in Using the tab key doesn't work anymore when selecting datatype #8310 happens
  7. schedule publishing overlay - generally inaccessible, no caused by this PR :)

Is there anything else to be tested here?

Poornima

@BatJan
Copy link
Contributor Author

BatJan commented Jun 26, 2020

Hi @poornimanayar thank you for testing 🙌 - The scenarios you mention where it does not work can you record some gif's maybe please? 😃 Just to I get the context right and remember that this will only work with overlays triggered by the overlay service or using the old overlay directive (except for the inert attribute not being set in this scenario for backwards compatibility) - If any of the mentioned scenarios trigger an infinite overlay then this PR does not cover them - It will be addressed once this PR is good to go, since it's a different and more complicated beast - So I'm trying to keep the PR's as simple as possible to hopefully make it easier on everyone to test and review as well 👼

@poornimanayar
Copy link
Contributor

Grid
grid

Media
media

I think this is quite a huge PR so some more testing will be required and there could be areas where we miss out too .Let me try and see whether I can see anything from your old PR.

@BatJan
Copy link
Contributor Author

BatJan commented Jun 26, 2020

@poornimanayar The two GIF's are illustrating overlay's that are based on the "infinite editor" - The focus lock is not implemented for those yet. This is on purpose since the old PR assumed to many things so for this initial PR I'm keeping it simpler this time ensuring it only works with "normal" overlay initially. Once this PR is merged the intention is to make it work with the "infinite editor" too 😃

# Conflicts:
#	src/Umbraco.Web.UI.Client/src/views/common/overlays/itempicker/itempicker.html
#	src/Umbraco.Web.UI.Client/src/views/components/overlays/umb-overlay.html
@nul800sebastiaan nul800sebastiaan merged commit c95a7f2 into umbraco:v8/contrib Jul 23, 2020
@nul800sebastiaan
Copy link
Member

Mr @BatJan ! 🎉 ⭐

A year and a half in the making, what a ride! I've just ran through this and am so very happy with what I see. Not everything is covered yet, but the things that are covered work so very well. Really, really awesome work! This is going to make keyboard navigation so much better as well. I LOVE it! 😍

I've merged this now and very much look forward to the updates to the infinite editors. This is going to be SO good!

#h5yr 🍻 🍻

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.

5 participants