Trap lightbox, and fix and bug in umbfocuslock found under test by rammi987 · Pull Request #12787 · umbraco/Umbraco-CMS

rammi987 · 2022-08-05T05:56:25Z

Prerequisites

umbraco/Umbraco-CMS.Accessibility.Issues#61

Description

Steps to replicate:

Go To packages tab
Open a package (e.g. Umbraco forms)
Scroll to bottom of the page and open the image modal

At the package details, can you at the bottom view some images, where if you tabbed around you ended outside of the ligthbox.
I have added the umb-focus-lock to the ligthbox, and now focus is trapped.

Doing my test I saw if you opened up the lightbox, go next on the images until the last (so the next arrow disappere) and tab to move focus, the focus esacpe the trap. So I extended the directive. So whenever the DOM changes inside the trap, we are getting the focusableelement, but also see if current focus still is on a trapped focusabled element, if not we set the focus at new.

github-actions · 2022-08-05T05:56:38Z

Hi there @rammi987, thank you for this contribution! 👍

While we wait for one of the Core Collaborators team to have a look at your work, we wanted to let you know about that we have a checklist for some of the things we will consider during review:

It's clear what problem this is solving, there's a connected issue or a description of what the changes do and how to test them
The automated tests all pass (see "Checks" tab on this PR)
The level of security for this contribution is the same or improved
The level of performance for this contribution is the same or improved
Avoids creating breaking changes; note that behavioral changes might also be perceived as breaking
If this is a new feature, Umbraco HQ provided guidance on the implementation beforehand
💡 The contribution looks original and the contributor is presumably allowed to share it

Don't worry if you got something wrong. We like to think of a pull request as the start of a conversation, we're happy to provide guidance on improving your contribution.

If you realize that you might want to make some changes then you can do that by adding new commits to the branch you created for this work and pushing new commits. They should then automatically show up as updates to this pull request.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

bjarnef · 2022-08-17T06:37:27Z

src/Umbraco.Web.UI.Client/src/views/packages/views/repo.html

                        ng-if="vm.lightbox.show"
                        items="vm.lightbox.items"
                        active-item-index="vm.lightbox.activeIndex"
+                        ng-attr-umb-focus-lock="true"


Does it need the ng-attr- part here?

@bjarnef I acctuly don´t know. I put more weight on what there has done in the past. See https://github.com/umbraco/Umbraco-CMS/blob/v10/contrib/src/Umbraco.Web.UI.Client/src/views/components/editor/umb-editors.html line 5

First I did this PR based on above files:
https://github.com/umbraco/Umbraco-CMS/blob/v10/contrib/src/Umbraco.Cms.StaticAssets/umbraco/UmbracoBackOffice/Default.cshtml line 91

And now this PR, so it was more an alignment, maybe is something to do white the ng-attr tell to run expresstion and just umb-focus-lock would bee a binding to the Angular directive scope

Okay, I only think it makes a difference to ensure to binding is ready, e.g. style vs ng-style. I recall originalen IE had issues when using expression directly in style attribute.

But this should work as well: https://github.com/umbraco/Umbraco-CMS/blob/v10/contrib/src/Umbraco.Web.UI.Client/src/views/components/overlays/umb-overlay.html#L5 (default is true).

I did a small test. It dons´t matter if you type in ng-attr-umb-focus-trap or just umb-focus-trap. Both work and fix the modal trap issue. So it is more what will be more likeable to look at?

I guess you mean umb-focus-lock 😉

yea - need a Redbull

bjarnef · 2022-08-17T06:39:35Z

src/Umbraco.Web.UI.Client/src/common/directives/components/forms/umbfocuslock.directive.js

+
+                    // When an element is remove or become not focusable, but focus is on it,
+                    // we need to ensure a now focus inside the trap, else the focus can escape the trap.
+                    if (focusableElements.filter(elm => elm == document.activeElement).length === 0) {


Could it use the following instead?

if(focusableElements.some(elm => elm == document.activeElement)) {

@bjarnef I have made a slighty change to the line, instead of filter or some. I used the IndexOf()
IndexOf is the most readable line and fasted execution time, https://www.measurethat.net/Benchmarks/Show/20458/0/some-vs-filter-vs-indexof-vs-includes1123123123

Okay, I ran the test and it says includes are the fastest 😄

Maybe it is different result from at mobile device 😄

bjarnef · 2022-08-17T11:31:59Z

src/Umbraco.Web.UI.Client/src/common/directives/components/forms/umbfocuslock.directive.js

                    // When an element is remove or become not focusable, but focus is on it,
                    // we need to ensure a now focus inside the trap, else the focus can escape the trap.
-                    if (focusableElements.filter(elm => elm == document.activeElement).length === 0) {
+                    if (focusableElements.indexOf(document.activeElement) === -1) {


I think if (focusableElements.includes(document.activeElement) === false would work as well?

rammi987

@bjarnef I removed ng-attr-* it was unnecceary.

I did a bounche of test on IndexOf vs Includes. Well sometimes is IndexOf but most of the time and on diffenct device includes is the fastes. So to maintian good speed i´ll go with includes as my finally commit :)

bjarnef · 2022-08-17T12:06:24Z

@rammi987 if you are looking further into focus lock, I have noticed issues with it in block list configuration: #12851 (see the "after" video which open the second overlays - after close the label get focus)

From a block configuration after opening e.g. settings element type or thumbnail, it seems the label property at top steal the focus and thus scroll to top of the overlay, which is a bit annoying 🙈

emmagarland · 2023-05-23T19:49:40Z

Hi @rammi987!

Just going through some of the longer running PRs, and noticed this one (it's been a while, so there are now some conflicts sorry!)

I know it was a while ago but did you need anything else for this one, or do you need any help to progress? Or will it be ready for review? Let me know if you need any help to get this one rebooted.

Thanks,

Emma 🙂

rammi987 · 2023-05-24T09:35:32Z

Hello @emmagarland

unfortunately i think the best idea is to reject this and start over. I have seen the bug fix for the directive has been accepted by an other PR from the community.

So I think is a better idea to see what is wrong with the light box now and fix it if there is accessibility errors still, in version 12

emmagarland · 2023-06-07T16:31:17Z

Sorry, missed this comment @rammi987 but thanks for the update to let me know 🙂

Much appreciate your efforts to progress it, though.

Thanks,
Emma

Trap lightbox, and fix and bug in umbfocuslock found under test

8dab0d1

bjarnef reviewed Aug 17, 2022

View reviewed changes

Remove unreadable line and clean it and top performed it

bf3f913

bjarnef reviewed Aug 17, 2022

View reviewed changes

Remove uneccery ng-attr and change indexOf to includes

6809bf8

rammi987 commented Aug 17, 2022

View reviewed changes

nul800sebastiaan changed the base branch from v10/contrib to v11/contrib October 25, 2022 13:06

nul800sebastiaan changed the base branch from v11/contrib to contrib January 26, 2023 14:52

emmagarland self-assigned this May 23, 2023

rammi987 closed this May 24, 2023

Conversation

rammi987 commented Aug 5, 2022

Prerequisites

Description

Uh oh!

github-actions bot commented Aug 5, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjarnef Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rammi987 left a comment

Choose a reason for hiding this comment

Uh oh!

bjarnef commented Aug 17, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emmagarland commented May 23, 2023

Uh oh!

rammi987 commented May 24, 2023

Uh oh!

emmagarland commented Jun 7, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bjarnef Aug 17, 2022 •

edited

Loading

bjarnef commented Aug 17, 2022 •

edited

Loading