[patch] INTENG-22824: fix keyboard access to journeys and remove tabindex=-1 #1122
Conversation
|
Pull Request TemplateDescriptionSummary By MatterAI
🔄 What ChangedThis pull request primarily focuses on enhancing the accessibility of embedded iframes and updating the library version. Key changes include:
🔍 Impact of the Change
📁 Total Files Changed3 files were changed in this pull request. 🧪 Test AddedNo specific tests were explicitly added or mentioned in the provided patch or PR description for the new keyboard navigation logic. The PR does not include new test files or modifications to existing test files to cover the accessibility enhancements. 🔒Security VulnerabilitiesNo new security vulnerabilities were detected in the provided code changes. The accessibility improvements are a positive change and do not introduce any known security risks. Type of change
How Has This Been Tested?Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
N/A - No specific tests or testing instructions were provided in the pull request description. JS Budget CheckPlease mention the size in kb before abd after this PR
Checklist:
Mentions:List the person or team responsible for reviewing proposed changes. cc @BranchMetrics/saas-sdk-devs for visibility. Tip Quality Recommendations
Sequence DiagramsequenceDiagram
participant ClientApp
participant journeys_utils
participant Iframe
participant Document
participant User
ClientApp->>journeys_utils: Call createIframe()
journeys_utils-->>Iframe: Create iframe element (id: 'branch-banner-iframe', title: 'Branch Banner Frame', aria-label: 'Branch Banner Frame')
journeys_utils-->>ClientApp: Return iframe element
ClientApp->>journeys_utils: Call addHtmlToIframe(iframe, html, userAgent)
journeys_utils->>Iframe: Inject HTML content
journeys_utils->>Document: Add keydown event listener (handleKeyboardNavigation)
journeys_utils->>Iframe: autoFocus(100) on initial focusable element
User->>Document: Press keyboard key (e.g., Tab, Enter)
Document->>journeys_utils: Trigger handleKeyboardNavigation(event)
alt If Tab key is pressed (event.key === 'Tab' || event.keyCode === 9)
journeys_utils->>journeys_utils: Determine next/previous focusable element index (focusElementIdx) based on shiftKey
journeys_utils->>Iframe: focusableContent[focusElementIdx].focus()
journeys_utils->>Document: event.preventDefault()
else If Enter key is pressed (event.key === 'Enter' || event.keyCode === 13)
journeys_utils->>Document: Get activeElement (focusedElement)
alt If focusedElement is a Button, A, or has role='button'
journeys_utils->>focusedElement: Simulate click()
journeys_utils->>Document: event.preventDefault()
end
end
|
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
|
/matter review |
![matter-code-review[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
| if (isTabPressed) { | ||
| console.log("tab pressed"); | ||
| if (e.shiftKey) { | ||
| if (focusElementIdx <= 0) { | ||
| focusElementIdx = focusableContent.length - 1; | ||
| } else { | ||
| focusElementIdx = focusElementIdx - 1; | ||
| } |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Console.log statements should not be included in production code as they can expose information and impact performance.
Fix: Remove the console.log debug statements.
Impact: Improves code cleanliness and prevents potential information leakage in production.
| if (isTabPressed) { | |
| console.log("tab pressed"); | |
| if (e.shiftKey) { | |
| if (focusElementIdx <= 0) { | |
| focusElementIdx = focusableContent.length - 1; | |
| } else { | |
| focusElementIdx = focusElementIdx - 1; | |
| } | |
| if (isTabPressed) { | |
| if (e.shiftKey) { | |
| if (focusElementIdx <= 0) { | |
| focusElementIdx = focusableContent.length - 1; | |
| } else { | |
| focusElementIdx = focusElementIdx - 1; | |
| } |
| if (isEnterPressed) { | ||
| console.log("enter pressed"); | ||
| // Get the currently focused element | ||
| var focusedElement = document.activeElement; |
There was a problem hiding this comment.
🛠️ Code Refactor
Issue: Console.log statements should not be included in production code as they can expose information and impact performance.
Fix: Remove the console.log debug statement.
Impact: Improves code cleanliness and prevents potential information leakage in production.
| if (isEnterPressed) { | |
| console.log("enter pressed"); | |
| // Get the currently focused element | |
| var focusedElement = document.activeElement; | |
| if (isEnterPressed) { | |
| // Get the currently focused element | |
| var focusedElement = document.activeElement; |
src/branch_view.js
Outdated
| obj = utils.addPropertyIfNotNull(obj, 'no_journeys', options['no_journeys']); | ||
| obj = utils.addPropertyIfNotNull(obj, 'is_iframe', utils.isIframe()); | ||
| obj = utils.addPropertyIfNotNull(obj, 'journey_dismissals', journeyDismissals); | ||
| // obj = utils.addPropertyIfNotNull(obj, 'journey_dismissals', journeyDismissals); |
There was a problem hiding this comment.
Echoing suggestion to remove commented code.
There was a problem hiding this comment.
ope yeah, sorry this wasnt fully ready for review
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
bredmond5
changed the title
2 participants
Pull Request Template
Description
A customer's automation has found our journeys to not be WCAG compliant because our iframe that we load the journey into has tabindex = -1.
What they think this is doing is making it so that the user can not tab into the journey using the keyboard. The real issues are as follows:
Issue 1: CSS in the journey template
at the bottom of the template (this being the template in the dashboard that the customer has the ability to edit)
what this is doing is making it so that when the journey is tabbed onto, you dont actually see what is being highlighted. In my own template, I updated it to
and now i can see that when tabbing i am actually highlighting onto various parts of the journey.
Issue 2: our handling of keyboard input on the journey
We created special logic to handle tabs in the journey. This function though is handling all keyboard input. So, since it is handling all keyboard input, including enter, but without any specific way to handle enter, then pressing the enter key will not do anything
So, i fix the issue of keyboard input in this PR. I also removed the tabindex and added some other WCAG tags because they dont change anything, but will hopefully make the customer happy.
Fixes # (issue)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also list any relevant details for your test configuration
JS Budget Check
Please mention the size in kb before abd after this PR
Checklist:
Mentions:
List the person or team responsible for reviewing proposed changes.
cc @BranchMetrics/saas-sdk-devs for visibility.