Fix no blur called on activeelement removed from dom #3393
Conversation
| } | ||
| }); | ||
| }, [scopeRef, focusedNode]); | ||
| let onSyntheticFocus = useSyntheticBlurEvent(onBlur); |
There was a problem hiding this comment.
need two of these or target ends up becoming stale
…com:adobe/react-spectrum into fix-no-blur-for-activeelement-being-removed
|
Build successful! 🎉 |
todo: turn into single mutation observer
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| <h1 id={`heading-${index}`}>Dialog {index + 1}</h1> | ||
| {index === 2 ? | ||
| {index > 2 ? |
There was a problem hiding this comment.
This case is not showing in storybook example. There are 3 instances of Dialog content, so the "end of the road" statement should come when index === 2.
| {index > 2 ? | |
| {index === 2 ? |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Build successful! 🎉 |
| } | ||
| } | ||
| } | ||
| let domNodeRemovedObserver = new DOMNodeRemovedObserver(); |
There was a problem hiding this comment.
Making this a singleton means it can never be tree shaken, even if useSyntheticBlurEvent is unused. I'd initialize this on first use if we can.
There was a problem hiding this comment.
I also wonder how much making this a singleton actually helps? I assume it was done for perf? But we still create a mutation observer every time for the disabled input/button case?
There was a problem hiding this comment.
correct, this one is a document level mutation observer, so we only need one of them. The ones created for disabled inputs/buttons is specifically targeted at those individual elements and don't watch childLists or subTrees, so it's not too bad creating a bunch of them
interesting about the singleton not being tree shaken, good news is we only start observing on first use, so even though the mutation observer object is created here, it won't do anything and won't affect anything.
There was a problem hiding this comment.
yeah but if we move the new DOMNodeRemovedObserver to first use as well, then it can be tree-shaken.
| } | ||
| if (target) { | ||
| target.removeEventListener('focusout', this.onBlurHandler); | ||
| } |
There was a problem hiding this comment.
do we need to reset this.target to null here? I think if we don't line 184 won't do anything when a new target is set.
| @@ -215,9 +215,10 @@ describe('DatePicker', function () { | |||
| expect(onChange).toHaveBeenCalledTimes(1); | |||
| expect(onChange).toHaveBeenCalledWith(new CalendarDate(2019, 2, 4)); | |||
| expect(getTextValue(combobox)).toBe('2/3/2019'); // controlled | |||
| await act(async () => Promise.resolve()); | |||
There was a problem hiding this comment.
All these extra promises in the tests worry me... why are they needed?
There was a problem hiding this comment.
these are cases where focus used to be lost, now it's being handled
the promise is to get the mutation observer to fire
| } | ||
| setTarget(target: HTMLElement) { | ||
| if (!this.target && target && this.observer) { | ||
| this.observer.observe(document, {childList: true, subtree: true, attributes: false, characterData: false}); |
There was a problem hiding this comment.
Still slightly concerned about the impacts of having a document level mutation observer pretty much at all times (while anything is focused, which is basically always?). 🤔
|
Also can you make sure all the browser bugs you linked to in the code get added to the wiki page? |
| if (this.target) { | ||
| for (const mutation of mutationList) { | ||
| for (const node of mutation.removedNodes) { | ||
| if (node?.contains?.(this.target)) { |
There was a problem hiding this comment.
Small perf optimization could be to do if (!document.body.contains(this.target) rather than looping through all the mutations. There could potentially be a lot of mutations at once.
|
Two other perf ideas:
but still might be a lot of mutations to process in cases like Virtualizer scrolling, where we’re adding and removing elements all the time. |
# Conflicts: # packages/@react-aria/focus/src/FocusScope.tsx
|
Build successful! 🎉 |
|
Build successful! 🎉 |
|
Too scary right now, closing until we decide we want to tackle this again |
Closes
// Some browsers do not fire onBlur when the document.activeElement is removed from the dom.
// Firefox has had a bug open about it for 13 years https://bugzilla.mozilla.org/show_bug.cgi?id=559561
// A Safari bug has been logged for it as well https://bugs.webkit.org/show_bug.cgi?id=243749
This results in FocusScope losing contain sometimes.
✅ Pull Request Checklist:
📝 Test Instructions:
Go to FocusableFirstInScope story in FocusScope stories
Tab to the button, hit enter
Repeat until just a blank area is left with a focus outline
This should work across Chrome/FF/Safari
🧢 Your Project: