Correctly set focus in FocusTrapZone even if focus event does not bubble #15804
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 3c74328:
|
Perf AnalysisNo significant results to display. All results
Perf Analysis (Fluent)Perf comparison
Perf tests with no regressions
|
Asset size changes
Baseline commit: d820d6ee1aafcd0a1669ba5bc139db8ab0663a92 (build) |
|
/azp run |
|
Azure Pipelines successfully started running 3 pipeline(s). |
|
@taurheim it seems that there failing unit tests due this change. Can you please check? |
|
Hi @layershifter, one of the errors in the build is "ERROR: there is an invalid change type detected @fluentui-react-internal-2020-11-02-08-54-48-taurheim-focustrap-onfocus.json: "patch" is not a valid change type" Based on the documentation here, patch should be a valid change type, right? Can you help me understand what I need to do to run a passing build? Edit: I've fixed the UT that was breaking. I had to learn a bit about how JS handles event delegation - onFocusCapture gets called before onFocus which was causing issues in onBumperFocus since it expects to be the called before onFocus. Instead I've just made onBumperFocus set internal "focused" state to true, since that should happen due to bubbling anyway. This change should have no effect on existing functionality, but there may still be an issue in preact since internalState.hasFocus never gets set to false without bubbling. |
taurheim
changed the title
|
@taurheim I think that now it's |
taurheim
requested review from
Jahnp,
joschect,
micahgodbolt,
Vitalius1 and
xugao
as code owners
|
If someone more familiar with FocusTrapZone could take another look at this change that would be great -- to make sure I've covered everything I had to make another change, moving the code that was previously run onBlur to run onBlurCapture. This means that it will be run before blur is called on the target element. Similarly to the other change (setting internal state on bumper focus), it is necessary for cases where onBlur and onFocus do not bubble (see #15530 for more explanation). I don't see any issues with this change but there may be repercussions I haven't considered and want to make sure I'm not breaking a key scenario here :) |
|
@dzearing can you please check this one? |
packages/react-internal/src/components/FocusTrapZone/FocusTrapZone.tsx
Outdated
Show resolved
Hide resolved
…ugh onFocusCapture
packages/react-internal/src/components/FocusTrapZone/FocusTrapZone.tsx
Outdated
Show resolved
Hide resolved
|
I've fixed the tests - apologies for all the commits, I was having trouble getting a full test run to finish on my local machine :( |
|
@taurheim - I will go ahead help merge this PR. for the change to be published for v7, could you also make the same fix in |
|
🎉 Handy links: |
Pull request checklist
$ yarn changeDescription of changes
Currently FocusTrapZone only sets its internal focus state onFocus. In some cases if events do not bubble correctly, only onFocusCapture will be called, causing the FocusTrapZone to get "stuck" at the end and not loop back to the front. See more information in issue #15530
Focus areas to test
FocusTrapZone, Modal dialog usage of FocusTrapZone