FocusUtil: fix getPreviousElement to include previous sibling elements correctly#3928
Conversation
…ut not tabbable (with tabindex=0), this fixes that
|
|
||
| if (childMatch && ((tabbable && (isElementTabbable(childMatch, true))) || !tabbable)) { | ||
| return childMatch; | ||
| if (childMatch) { |
There was a problem hiding this comment.
Is it possible to write a unit test for this scenario?
Also, is there logic missing on "findNextElement"?
There was a problem hiding this comment.
getNextElement doesn't need to be updated, the logic is slightly different and doesn't check for tabindex when checking if an element is tabbable. There may be some other bug hidden in here from that difference, but it doesn't need this update
There was a problem hiding this comment.
and yes, I'll look at adding a unit test
There was a problem hiding this comment.
cool ping me once you have a test to cover it (so we don't break it later!) and let's get it merged.
There was a problem hiding this comment.
@dzearing done, ut added. I also verified that this breaks without my fix and works with it :)
…react into jspurlin/FocusFixFindPrevElement
…xed by this change
…c-react into jspurlin/FocusFixFindPrevElement
* master: (71 commits) Applying package updates. Delete initials_2018-02-07-13-49.json Delete initials_2018-02-07-13-49.json Delete jolore-addingWorkWeekDateRange_2018-01-24-01-39.json Delete initials_2018-02-07-13-49.json Delete jolore-addingWorkWeekDateRange_2018-01-24-01-39.json Delete initials_2018-02-07-13-49.json Update .npmrc Cleaning Up Console Log in CommandBar Test (microsoft#4011) Convert Overlay to mergeStyles (microsoft#3978) ScrollablePane SCSS to MergeStyles Part 1: File Structure (microsoft#4008) [ContextualMenu] Fixes useTargetWidth property (microsoft#3943) DetailsList: Consider groups when setting aria-rowcount List: Add a _notifyPageChanges function (microsoft#3990) Applying package updates. Migrating Coachmark to main Package, Added a beak component and updated experiment PositioningContainer. (microsoft#3919) Update package.json Added enum for triggering menu with arrow keys and bool to allow it or not (microsoft#3950) BaseExtendedPicker: Hook up onPaste (microsoft#3885) FocusUtil: fix getPreviousElement to include previous sibling elements correctly (microsoft#3928) ...
3 participants
Pull request checklist
$ npm run changeDescription of changes
The getPreviousElement functionality missed elements if a potential childMatch was found, but it was not tabbable (or tabbable is false) and the elemeent that should be found is a previous sibiling of childMatch.
Here;s some example:
This worked:
<Focus starts before the zone, and is being put into the focusZone via the logic in FocusTrapZone>
FocusZone
OverflowSetItem
button (tabindex=0)
OverflowSetItem
button (tabindex=-1)
OverflowSetItem
button (tabindex=-1)
It goes down to the last button. The currentElement is on the last OverflowSetItem and it finds the button when looking for childMatch. From there determines it is not focusable (because of the isElementTabbable(childMatch, true /checkTabIndex/) call) so it would go to currentElement's previousSibling to continue looking (until it gets to the first button and then it finds athat as a match).
This did not work:
<Focus starts before the zone, and is being put into the focusZone via the logic in FocusTrapZone>
FocusZone
OverflowSetItem
button (tabindex=0)
button (tabindex=-1)
button (tabindex=-1)
It goes down to the last button. The currentElement is on the last OverflowSetItem and it finds the button when looking for childMatch. From there determines it is not focusable (because of the isElementTabbable(childMatch, true /checkTabIndex/) call) so it would go to currentElement's previousSibling to continue looking. Note that this skips all the other sibling buttons.
This fix is when we find a potential childMatch which we determine we cannot focus, move to that element's previous child instead of moving on to the next sibling of the parent (currentElement)
Focus areas to test
Verified that focus correctly moves to the previous element in all tested cases (including the two above)