FocusTrapZone does not correctly trap focus when last child is FocusZone #4172
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SpencerLynn
changed the title
|
It would be great if someone could review this PR. It looks like several people are also experiencing this issue. Please let me know if I incorrectly filled out the change request files. |
|
Oh this will fix a bug that we encountered a bit ago. Glad you found a solution! |
|
@joschect can you take a quick look. I'm not at all familiar with this code, but I can certainly attest for the bug being a pain in the butt. |
joschect
reviewed
Mar 9, 2018
| @@ -137,8 +137,12 @@ describe('FocusTrapZone', () => { | |||
| } | |||
| }); | |||
|
|
|||
| // Focus the first button. | |||
| ReactTestUtils.Simulate.focus(buttonA); | |||
There was a problem hiding this comment.
Lets still make sure that focusing the first first element first works, keep this in, test it, and then focus last element and make sure it wraps correctly.
There was a problem hiding this comment.
I may split this into it's own test in that case. The reason being that the issue does not reproduce if you shift+tab from the first element to wrap backwards before trying to flow out of the end.
joschect
reviewed
Mar 9, 2018
packages/utilities/src/focus.ts
Outdated
| @@ -100,7 +100,7 @@ export function getPreviousElement( | |||
| tabbable); | |||
|
|
|||
| if (childMatch) { | |||
| if ((tabbable && (isElementTabbable(childMatch, true))) || !tabbable) { | |||
| if ((tabbable && isElementTabbable(childMatch, true)) || !tabbable) { | |||
There was a problem hiding this comment.
Can you add a comment or change the name tabbable. This was named confusingly and if you are up to changing it that would be great. Tabbable makes me think that the element is tabbable, rather than something like elementNeedsToBeTabbable, which is what it really corresponds to. If you'd rather just add a comment to make that more clear that's cool too.
joschect
approved these changes
Mar 9, 2018
…he trap zone when tabbing through. When calculating last tabbable element, consider the tabbable argument when navigating inside focus zones.
SpencerLynn
force-pushed
the
focustrapzone-last-child-focuszone
branch
from
48eebeb to
55b6d62
Compare
|
Thank you! |
joschect
added a commit
to joschect/office-ui-fabric-react
that referenced
this pull request
Mar 15, 2018
…s FocusZone (microsoft#4172)" This reverts commit 699fa69.
2 tasks
dzearing
pushed a commit
that referenced
this pull request
Mar 27, 2018
joschect
added a commit
that referenced
this pull request
Mar 30, 2018
* Revert "FocusTrapZone does not correctly trap focus when last child is FocusZone (#4172)" This reverts commit 699fa69. * fix picker suggestions and add many tests * adding snap file * adding change file * improve test * fix tests and config * revert test changes * last test update * update test name and snapshot * remove tests to expedite checkin
dzearing
pushed a commit
that referenced
this pull request
Mar 30, 2018
* Revert "FocusTrapZone does not correctly trap focus when last child is FocusZone (#4172)" This reverts commit 699fa69. * fix picker suggestions and add many tests * adding snap file * adding change file * improve test * fix tests and config * revert test changes * last test update * update test name and snapshot * undo suggestions change so it will merge properly * revert change to suggestions
christiango
added a commit
to christiango/office-ui-fabric-react
that referenced
this pull request
Mar 30, 2018
* In Panel component, isClickableOutsideFocusTrap should not be overriden back to false if it's already set to true in focusTrapZoneProps. (microsoft#4354) * In Panel, a true value of isClickableOutsideFocusTrap prop in focusTrapZoneProps should not be overriden. * Add change file. * MessageBar: For single line, put action button before dismiss button (microsoft#4365) * Reordered button elements for single line messagebars * New example for single line with dismiss and action buttons * Added change file * [TextField, Panel] Removed componentId internal prop (microsoft#3896) * Remove componentId internal prop * change file * Reinstate prop as deprecated * Fixed tslint issue * Update magellan-componentIdInternal_2018-02-06-19-27.json * Fix errors in Signal styles (microsoft#4367) * Fix: Website: Does not show neutral quaternary colors (microsoft#4188) * Add missing quaternary and alt colors on doc page * + change file * Fix focuszone props (microsoft#4335) * Revert "FocusTrapZone does not correctly trap focus when last child is FocusZone (microsoft#4172)" This reverts commit 699fa69. * Fix focus zone props so that it doesn't have a typescript error * adding change file * Searchbox: deprecate defaultValue (microsoft#4225) * Deprecate SearchBox defaultValue prop. * npm run change output * Do not try to show the suggestions when input is undefined (no more exceptions in IE) (microsoft#4294) * [BasePicker] suggestions should not be shown when input does not exist * added change file * Update master_2018-03-16-09-03.json * Remove a redundant "value" prop in ComboBox multi select example to eliminate confusing behaviors. (microsoft#4366) * Add multiSelect for ComboBox * Unbreak a unit test * rush change * bug fixes * Merge from upstream and resolve local build issues * Fix merge styles * Fix pending background * Unbreak unit test cases * Fix the merge * Add controled multi-select in the example page to make sure onChanged cb works * Remove trailing white spaces * Fix bug: enter key will not trigger onChanged * Dummy change to trigger another build * refactor * Fix some minor bugs * Update some comments for ComboBox * Minor bug fixes to the example page * rush change * Delete the old rush change file * Remove a trailing space * Remove a redundant property from the ComboBox example * rush chagne * Remove old rush change file * Update stanleyy-fillin_2018-03-26-22-09.json * Moving to ts-loader, throwing on console.warn in ssr tests (microsoft#4370) * Break on warnings. * Shrinkwrap update. * Applying package updates. * Missed a reference to awesome-typescript-loader in the webpack-resources file. * Rewrite suggestions for BaseFloatingPicker (experiments) (microsoft#4273) * create new suggestions, suggestionsControl, and suggestionsStore in experiments * add change files * fix picker tests and separate scss file into two files * Update BaseFloatingPicker.tsx * remove autobind reference from bad merge * address PR comments * address PR comments, fix selection for shouldSelectFirstItem * Checkbox: text should be selectable, icon shouldn't. (microsoft#4378) * Updating checkbox select. * addin change. * Fix snapshot. * Update change output (microsoft#4380) * Contextual Menu: Revert Split Button Alignment to Previous Left Aligntment (microsoft#4369) * made spin button and base button menu option un-focusable * added spin button test * added split button contextual menu change * got rid of un-needed code * added change files * added abillitiy to not focus on primary button for split button, but only on the container * enter on focus on split button trigger primary action * added split button style for disabled * prevent opening submenu if item is disabled * tighten restrictions on opening split buttons to open with alt + down * refactored button name and fixed test * updated bundle size * changed on menu item click to bw able to take in a keyboard event * updated package json * fixed weird tabbing * changed way we structured the focus * added stop propagation * changed split button container key down to work with other buttons now * update bundle size * added correct split button container styles * removed unnecessary attribute * fixed focusing on menu button when primary button is disabled * added change to focus * added aria hidden * fixed span location and moved TODO * added support to focus on whole container in split button for contextual menus * added comment * fixed styling issues with buttons * fixed style problems for contextual split button where content is centered * reverted change * added appropriate change file * Update contextualMenuAlignSplitButton_2018-03-27-18-13.json * Integrate markdown-to-jsx in example-app-base for use in documentation (microsoft#4302) * Add markdown to jsx related packages and update shrinkwrap * Copy template components to example app base and correct imports. * Create example md files * Add Markdown typings * Remove commented code * Test using md files in Activity Item Page * Simplify raw-loader md imports * npm run change output * Expand md import to full path * Applying package updates. * Contextual Menu Fix Split Button Submenu Target (microsoft#4368) * Update SplitButton submenu in ContextualMenu to base itself of the splitButtonContainer * rush change * used ref to pass in container instead of parent element for more defenseive code * changed class to use correct refs * added example and refactored focus * Initials for phone numbers (microsoft#4376) * Adding option for calculating initials for phone numbers * Added change description * Name change and cleanup * Update initials-for-phonenumbers_2018-03-27-14-10.json * Update initials-for-phonenumbers_2018-03-27-14-10.json * Applying package updates. * Markdown-to-jsx: fix dependencies (microsoft#4389) * Add markdown to jsx related packages and update shrinkwrap * Copy template components to example app base and correct imports. * Create example md files * Add Markdown typings * Remove commented code * Test using md files in Activity Item Page * Simplify raw-loader md imports * npm run change output * Expand md import to full path * Move markdown and syntax packages to dependencies. * Move md files to docs folder. * Applying package updates. * Remove root-level imports of office-ui-fabric-react from /experiments (microsoft#4392) * Remove root imports of office-ui-fabric-react * Update change output * Fixing tests in experiments to not generate warnings in `npm start`. (microsoft#4391) * Applying package updates. * Variants: fix theming output so original input theme values are respected (microsoft#4393) * oops * changefile * dry * max line length error * Applying package updates. * OverflowSet: allow the OverflowSet to not be contained within a FocusZone (microsoft#4400) * Add a prop so that OverflowSets do not have to contain FocusZones * pull the common aspects into variables so they can be reused * simplify OverflowSet even more (use Tag to remove duplication) * Revert back and remove the key since it's no longer needed * rush change * Make sure the focusZoneProps are still getting passed down * The divProperties only need to be used when doNotContainWithinFocusZone is true * Remove unneeded import * DetailsList: link color invisible for selected row in high-contrast-white (microsoft#4395) * DetailsList: link color too close to selected background in high-contrast-white * add change file * SplitButton: Close menu when primary button is clicked (microsoft#4403) * Updating the split button to close the menu when the primary button is executed. * change * Fix Signal rendering in IE (microsoft#4404) * More Signals fixes * Update change output * Pass all props to Signal Icon elements * Update change output * Pickers: Fix suggestions not having proper value selected (microsoft#4408) * Revert "FocusTrapZone does not correctly trap focus when last child is FocusZone (microsoft#4172)" This reverts commit 699fa69. * fix picker suggestions and add many tests * adding snap file * adding change file * improve test * fix tests and config * revert test changes * last test update * update test name and snapshot * remove tests to expedite checkin * Breadcrumb: File rename to make the 6.0 merge diff more readable. (microsoft#4394) * File rename to make the 6.0 merge diff more readable. * Adding checkbox. * Also moving a few more. * Pickers: Add suggestions tests (microsoft#4409) * Revert "FocusTrapZone does not correctly trap focus when last child is FocusZone (microsoft#4172)" This reverts commit 699fa69. * fix picker suggestions and add many tests * adding snap file * adding change file * improve test * fix tests and config * revert test changes * last test update * update test name and snapshot * undo suggestions change so it will merge properly * revert change to suggestions * Disable tree shaking (temporarily) (microsoft#4410) * Temporarily disable tree shaking. * Adding change files.
dzearing
pushed a commit
that referenced
this pull request
Apr 15, 2018
* rush generate * fix utilities * Upgrade fabric react * Rush change * Merge upstream (#2) * In Panel component, isClickableOutsideFocusTrap should not be overriden back to false if it's already set to true in focusTrapZoneProps. (#4354) * In Panel, a true value of isClickableOutsideFocusTrap prop in focusTrapZoneProps should not be overriden. * Add change file. * MessageBar: For single line, put action button before dismiss button (#4365) * Reordered button elements for single line messagebars * New example for single line with dismiss and action buttons * Added change file * [TextField, Panel] Removed componentId internal prop (#3896) * Remove componentId internal prop * change file * Reinstate prop as deprecated * Fixed tslint issue * Update magellan-componentIdInternal_2018-02-06-19-27.json * Fix errors in Signal styles (#4367) * Fix: Website: Does not show neutral quaternary colors (#4188) * Add missing quaternary and alt colors on doc page * + change file * Fix focuszone props (#4335) * Revert "FocusTrapZone does not correctly trap focus when last child is FocusZone (#4172)" This reverts commit 699fa69. * Fix focus zone props so that it doesn't have a typescript error * adding change file * Searchbox: deprecate defaultValue (#4225) * Deprecate SearchBox defaultValue prop. * npm run change output * Do not try to show the suggestions when input is undefined (no more exceptions in IE) (#4294) * [BasePicker] suggestions should not be shown when input does not exist * added change file * Update master_2018-03-16-09-03.json * Remove a redundant "value" prop in ComboBox multi select example to eliminate confusing behaviors. (#4366) * Add multiSelect for ComboBox * Unbreak a unit test * rush change * bug fixes * Merge from upstream and resolve local build issues * Fix merge styles * Fix pending background * Unbreak unit test cases * Fix the merge * Add controled multi-select in the example page to make sure onChanged cb works * Remove trailing white spaces * Fix bug: enter key will not trigger onChanged * Dummy change to trigger another build * refactor * Fix some minor bugs * Update some comments for ComboBox * Minor bug fixes to the example page * rush change * Delete the old rush change file * Remove a trailing space * Remove a redundant property from the ComboBox example * rush chagne * Remove old rush change file * Update stanleyy-fillin_2018-03-26-22-09.json * Moving to ts-loader, throwing on console.warn in ssr tests (#4370) * Break on warnings. * Shrinkwrap update. * Applying package updates. * Missed a reference to awesome-typescript-loader in the webpack-resources file. * Rewrite suggestions for BaseFloatingPicker (experiments) (#4273) * create new suggestions, suggestionsControl, and suggestionsStore in experiments * add change files * fix picker tests and separate scss file into two files * Update BaseFloatingPicker.tsx * remove autobind reference from bad merge * address PR comments * address PR comments, fix selection for shouldSelectFirstItem * Checkbox: text should be selectable, icon shouldn't. (#4378) * Updating checkbox select. * addin change. * Fix snapshot. * Update change output (#4380) * Contextual Menu: Revert Split Button Alignment to Previous Left Aligntment (#4369) * made spin button and base button menu option un-focusable * added spin button test * added split button contextual menu change * got rid of un-needed code * added change files * added abillitiy to not focus on primary button for split button, but only on the container * enter on focus on split button trigger primary action * added split button style for disabled * prevent opening submenu if item is disabled * tighten restrictions on opening split buttons to open with alt + down * refactored button name and fixed test * updated bundle size * changed on menu item click to bw able to take in a keyboard event * updated package json * fixed weird tabbing * changed way we structured the focus * added stop propagation * changed split button container key down to work with other buttons now * update bundle size * added correct split button container styles * removed unnecessary attribute * fixed focusing on menu button when primary button is disabled * added change to focus * added aria hidden * fixed span location and moved TODO * added support to focus on whole container in split button for contextual menus * added comment * fixed styling issues with buttons * fixed style problems for contextual split button where content is centered * reverted change * added appropriate change file * Update contextualMenuAlignSplitButton_2018-03-27-18-13.json * Integrate markdown-to-jsx in example-app-base for use in documentation (#4302) * Add markdown to jsx related packages and update shrinkwrap * Copy template components to example app base and correct imports. * Create example md files * Add Markdown typings * Remove commented code * Test using md files in Activity Item Page * Simplify raw-loader md imports * npm run change output * Expand md import to full path * Applying package updates. * Contextual Menu Fix Split Button Submenu Target (#4368) * Update SplitButton submenu in ContextualMenu to base itself of the splitButtonContainer * rush change * used ref to pass in container instead of parent element for more defenseive code * changed class to use correct refs * added example and refactored focus * Initials for phone numbers (#4376) * Adding option for calculating initials for phone numbers * Added change description * Name change and cleanup * Update initials-for-phonenumbers_2018-03-27-14-10.json * Update initials-for-phonenumbers_2018-03-27-14-10.json * Applying package updates. * Markdown-to-jsx: fix dependencies (#4389) * Add markdown to jsx related packages and update shrinkwrap * Copy template components to example app base and correct imports. * Create example md files * Add Markdown typings * Remove commented code * Test using md files in Activity Item Page * Simplify raw-loader md imports * npm run change output * Expand md import to full path * Move markdown and syntax packages to dependencies. * Move md files to docs folder. * Applying package updates. * Remove root-level imports of office-ui-fabric-react from /experiments (#4392) * Remove root imports of office-ui-fabric-react * Update change output * Fixing tests in experiments to not generate warnings in `npm start`. (#4391) * Applying package updates. * Variants: fix theming output so original input theme values are respected (#4393) * oops * changefile * dry * max line length error * Applying package updates. * OverflowSet: allow the OverflowSet to not be contained within a FocusZone (#4400) * Add a prop so that OverflowSets do not have to contain FocusZones * pull the common aspects into variables so they can be reused * simplify OverflowSet even more (use Tag to remove duplication) * Revert back and remove the key since it's no longer needed * rush change * Make sure the focusZoneProps are still getting passed down * The divProperties only need to be used when doNotContainWithinFocusZone is true * Remove unneeded import * DetailsList: link color invisible for selected row in high-contrast-white (#4395) * DetailsList: link color too close to selected background in high-contrast-white * add change file * SplitButton: Close menu when primary button is clicked (#4403) * Updating the split button to close the menu when the primary button is executed. * change * Fix Signal rendering in IE (#4404) * More Signals fixes * Update change output * Pass all props to Signal Icon elements * Update change output * Pickers: Fix suggestions not having proper value selected (#4408) * Revert "FocusTrapZone does not correctly trap focus when last child is FocusZone (#4172)" This reverts commit 699fa69. * fix picker suggestions and add many tests * adding snap file * adding change file * improve test * fix tests and config * revert test changes * last test update * update test name and snapshot * remove tests to expedite checkin * Breadcrumb: File rename to make the 6.0 merge diff more readable. (#4394) * File rename to make the 6.0 merge diff more readable. * Adding checkbox. * Also moving a few more. * Pickers: Add suggestions tests (#4409) * Revert "FocusTrapZone does not correctly trap focus when last child is FocusZone (#4172)" This reverts commit 699fa69. * fix picker suggestions and add many tests * adding snap file * adding change file * improve test * fix tests and config * revert test changes * last test update * update test name and snapshot * undo suggestions change so it will merge properly * revert change to suggestions * Disable tree shaking (temporarily) (#4410) * Temporarily disable tree shaking. * Adding change files. * HTMLButton -> HTML Element
chrismohr
pushed a commit
to chrismohr/office-ui-fabric-react
that referenced
this pull request
Apr 17, 2018
…one (microsoft#4172) * When a FocusTrapZone has a FocusZone as a last child, focus escapes the trap zone when tabbing through. When calculating last tabbable element, consider the tabbable argument when navigating inside focus zones. * Adding change files. * Addressing comments.
chrismohr
pushed a commit
to chrismohr/office-ui-fabric-react
that referenced
this pull request
Apr 17, 2018
microsoft#4280) * Revert "FocusTrapZone does not correctly trap focus when last child is FocusZone (microsoft#4172)" This reverts commit 699fa69. * revert focus changes
chrismohr
pushed a commit
to chrismohr/office-ui-fabric-react
that referenced
this pull request
Apr 17, 2018
* Revert "FocusTrapZone does not correctly trap focus when last child is FocusZone (microsoft#4172)" This reverts commit 699fa69. * Fix focus zone props so that it doesn't have a typescript error * adding change file
chrismohr
pushed a commit
to chrismohr/office-ui-fabric-react
that referenced
this pull request
Apr 17, 2018
…4408) * Revert "FocusTrapZone does not correctly trap focus when last child is FocusZone (microsoft#4172)" This reverts commit 699fa69. * fix picker suggestions and add many tests * adding snap file * adding change file * improve test * fix tests and config * revert test changes * last test update * update test name and snapshot * remove tests to expedite checkin
chrismohr
pushed a commit
to chrismohr/office-ui-fabric-react
that referenced
this pull request
Apr 17, 2018
* Revert "FocusTrapZone does not correctly trap focus when last child is FocusZone (microsoft#4172)" This reverts commit 699fa69. * fix picker suggestions and add many tests * adding snap file * adding change file * improve test * fix tests and config * revert test changes * last test update * update test name and snapshot * undo suggestions change so it will merge properly * revert change to suggestions
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull request checklist
$ npm run changeDescription of changes
When a FocusTrapZone has a FocusZone as a last child, focus escapes the trap zone when tabbing through. When calculating last tabbable element, consider the tabbable argument when navigating inside focus zones.
Focus utility getPreviousElement did not correctly consider the tabbable argument when considering the current node. This can affect how FocusZones are processed, since only one element in a zone will have tab index set. This, in turn, affects how FocusTrapZone traps focus, since getPreviousElement is used during trapping focus.