Revert popover bug fix #2447
Revert popover bug fix #2447
Conversation
This reverts commit c9b0347.
… need it to do, and would require further adjustments or changes to Perseus widgets. Better to just wait for an updated implementation.
🦋 Changeset detectedLatest commit: 0b526d9 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
GeraldRequired Reviewers
Don't want to be involved in this pull request? Comment |
npm Snapshot: Published🎉 Good news!! We've packaged up the latest commit from this PR (4c5f44b) and published all packages with changesets to npm. You can install the packages in webapp by running: ./services/static/dev/tools/deploy_wonder_blocks.js --tag="PR2447"Packages can also be installed manually by running: yarn add @khanacademy/wonder-blocks-<package-name>@PR2447 |
|
Size Change: -27 B (-0.03%) Total Size: 98.2 kB
ℹ️ View Unchanged
|
A new build was pushed to Chromatic! 🚀https://5e1bf4b385e3fb0020b7073c-prmppfffjy.chromatic.com/ Chromatic results:
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2447 +/- ##
============================
============================
Continue to review full report in Codecov by Sentry.
|
Not only does the fix not meet the intended use case (allowing roving tabindex), it creates additional problems for other consumers with multiple roving tabindexes within their popovers.
Since the method to update the tabindexes doesn't use refs, it has stale tabindex data and will override any dynamically-set tabindex with its original index. In the case of a roving tabindex implementation, that's -1. Therefore, we are trading everything being
tabindex=0fortabindex=-1.That works fine for the doodlepad controls use case since it has one focus trap and the focus manager will programmatically focus the first element when tabbing from the anchor. From there, event listeners handle the focus fine, but the tabindex never changes to 0 so the position isn't retained.
Unfortunately, the Perseus keypad has two areas that want a roving tabindex. Focus first goes to the close button, and there is no way to get into the roving tabindex areas since they all have -1s. Even if we programmatically focus into the first area by listening to
Tabkey presses, we'd then have to programmatically focus into the second area.So there is a heavy burden on Perseus, and even the intended behavior is still partially broken.
The best path forward without any additional adjustments by consumers is to wait for a WB Popover that doesn't hack its childrens' focus.