Skip to content
This repository has been archived by the owner on Mar 4, 2020. It is now read-only.

fix(FocusZone|FocusTrapZone): correctly define current document object of the components #1820

Merged
merged 5 commits into from
Aug 19, 2019

Conversation

sophieH29
Copy link
Contributor

Fixes #1817

By replacing document to getDocument from focusUtilities we're making sure that the right document object will be used for correct FocusZone and FocusTrapZone function.

getDocument is called with node elements this._root._current and implementation looks like this:

export function getDocument(rootElement?: Element | null): Document | undefined {
  return (rootElement && rootElement.ownerDocument) || document
}

@sophieH29 sophieH29 added 🚀 ready for review 🧰 fix Introduces fix for broken behavior. accessibility All the Accessibility tasks and bugs should be tagged with this. labels Aug 19, 2019
@sophieH29 sophieH29 self-assigned this Aug 19, 2019
@codecov
Copy link

codecov bot commented Aug 19, 2019

Codecov Report

❗ No coverage uploaded for pull request base (master@3795470). Click here to learn what that means.
The diff coverage is 70.83%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1820   +/-   ##
=========================================
  Coverage          ?   69.56%           
=========================================
  Files             ?      875           
  Lines             ?     7596           
  Branches          ?     2219           
=========================================
  Hits              ?     5284           
  Misses            ?     2304           
  Partials          ?        8
Impacted Files Coverage Δ
...eact/src/lib/accessibility/FocusZone/FocusZone.tsx 89.08% <100%> (ø)
.../src/lib/accessibility/FocusZone/FocusTrapZone.tsx 71.71% <61.11%> (ø)

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 3795470...3703eec. Read the comment docs.

@lucivpav
Copy link
Contributor

Looks good to me

Copy link
Member

@dzearing dzearing left a comment

Choose a reason for hiding this comment

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

Yep change makes sense. If this is a bug in the other copy, we need to merge the two asap to stop the branching 😀 or fix the bug in both places.

@sophieH29
Copy link
Contributor Author

sophieH29 commented Aug 19, 2019

Yep change makes sense. If this is a bug in the other copy, we need to merge the two asap to stop the branching 😀 or fix the bug in both places.

@davezuko totally agree on keeping consistency between copies. We encountered the bug when had a case of opening another window with components through window.open, so when document.active was used, it would use the wrong (initial) document object from the parent window. I'll create a PR on Fabric side to replicate the change.

Update: created a PR microsoft/fluentui#10187

@sophieH29 sophieH29 merged commit 881d670 into master Aug 19, 2019
@delete-merged-branch delete-merged-branch bot deleted the fix/focus-zone-respect-document-from-context branch August 19, 2019 14:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
accessibility All the Accessibility tasks and bugs should be tagged with this. 🧰 fix Introduces fix for broken behavior. 🚀 ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FocusZone and FocusTrapZone do not respect target in Provider context
4 participants