Skip to content
This repository has been archived by the owner on Dec 8, 2022. It is now read-only.

Fix wait not blocking focus nav #4

Merged
merged 29 commits into from
Dec 13, 2018

Conversation

blackbaud-conorwright
Copy link
Contributor

@blackbaud-conorwright blackbaud-conorwright commented Sep 25, 2018

Alright, I have a solution for the navigability for non-full-page waits as I mentioned in second standup. And It really needs some review or alternate opinions. Most of the work is in the wait-adapter-service file.

So the issue is that you can tab navigate into the tab navigable elements inside a waited element. There isn't a good straightforward way of preventing tab navigation within an element that I have come accross looking around. So I have 2 solution approaches for this.

  1. Loop through all children, grandchildren, etc that are tab-navigable within an element and set their tabIndex to -1. Issues with this are:
- I would also have to log the previous tabIndex of each element so I could revert them completely when the wait is over.
- This won't account for elements that are added after the wait begins unless I have it run off mutation events as well.
  1. Event responses. (what I currently am doing)
    I currently see 3 ways to go about this:
- I can put keydown events on the navigable elements before and after the host element to skip the host when tab navigation is attempted on either.
- I can put a listener on the window for keyup events to check if focus has shifted to an element within the host and if it was caused by tab navigation. Then shift the focus to the next legitimate target.

The first two for this would also be susceptible to any mutations list new navigable elements being added before or after the host; or if the elements I listened to events on are removed.
So right now I have the window listener solution implemented.

If anyone has an idea I haven't considered yet or improvement ideas, I would love to hear them cause I still really don't like this implementation. In particular, I'm not fully confident in my selector for all tab navigable elements which seems to be required for all these solutions.
Really still kinda hoping that there is some magical attribute that blocks tab navigation on an element and it's descendants, but I haven't found one.

Resolves: #1580

@blackbaud-conorwright blackbaud-conorwright changed the title moved over changes from skyux2 PR Fix wait not blocking focus nav Sep 25, 2018
@codecov-io
Copy link

codecov-io commented Oct 15, 2018

Codecov Report

Merging #4 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master     #4   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          28     28           
  Lines         493    584   +91     
  Branches       77    103   +26     
=====================================
+ Hits          493    584   +91
Impacted Files Coverage Δ
...rc/app/public/modules/wait/wait-adapter.service.ts 100% <100%> (ø) ⬆️
src/app/public/modules/wait/wait.component.ts 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3bec3e1...6cd6698. Read the comment docs.

@Blackbaud-AlexKingman
Copy link
Contributor

@blackbaud-conorwright this is a tricky one for sure. What would you think about also adding some aria tags to the background content to tell screen readers the content is loading? Check this out:
https://developer.paciellogroup.com/blog/2018/05/short-note-on-being-busy/

Copy link
Contributor

@Blackbaud-AlexKingman Blackbaud-AlexKingman left a comment

Choose a reason for hiding this comment

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

Just a few comments. Also one question: If a user is already focused on an element when the wait triggers, they could still hit enter or spacebar to activate that element. When the wait is triggered, should we re-assign focus to the wait parent element?

Copy link
Contributor

@Blackbaud-AlexKingman Blackbaud-AlexKingman left a comment

Choose a reason for hiding this comment

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

Couple more comments.

src/app/public/modules/wait/wait-adapter.service.ts Outdated Show resolved Hide resolved
src/app/public/modules/wait/wait-adapter.service.ts Outdated Show resolved Hide resolved
src/app/public/modules/wait/wait-adapter.service.ts Outdated Show resolved Hide resolved
@blackbaud-conorwright
Copy link
Contributor Author

Alright, it took quite a bit longer than I expected ;p
But I think I have a nice implementation now that should cover the issues we've seen and ought to be a lighter load now that it will only ever maintain one listener.

Have another look when you get the chance @Blackbaud-AlexKingman

@Blackbaud-SteveBrush Blackbaud-SteveBrush self-assigned this Nov 1, 2018
Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

@blackbaud-conorwright I launched the wait demo, deactivated the "blocking" feature (by clicking on the "Toggle blocking" button) and was able to click the link, but was unable to tab to it. Is that expected? I'm using Chrome.

* implemented focusin strategy

* finished implementing the focusin strategy
Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

Just a few comments for now. When I was testing it out, I toggled the isNonBlocking and isWaiting buttons a few times and in some cases, my tab key got "trapped" on the isNonBlocking button and couldn't move any further. In another case, some combination of clicking those buttons made it so I could still tab through a waited, blocked element. I'm trying to figure out which pattern causes the problem, but I'm guessing it has something to do with isNonBlocking being a basic Input, and doesn't affect the listeners when it is toggled on the component. Should isNonBlocking be a setter, and affect the state after it changes?

src/app/public/modules/wait/wait-adapter.service.ts Outdated Show resolved Hide resolved
src/app/public/modules/wait/wait.component.ts Outdated Show resolved Hide resolved
Copy link
Member

@Blackbaud-SteveBrush Blackbaud-SteveBrush left a comment

Choose a reason for hiding this comment

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

One final thing.

src/app/public/modules/wait/wait-adapter.service.ts Outdated Show resolved Hide resolved
@blackbaud-conorwright blackbaud-conorwright merged commit 488d437 into master Dec 13, 2018
@blackbaud-conorwright blackbaud-conorwright deleted the fix-wait-not-blocking-focus-nav branch December 13, 2018 18:26
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants