-
Notifications
You must be signed in to change notification settings - Fork 402
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(engine): fix for delegatesFocus with tabindex=0 #812
Merged
davidturissini
merged 8 commits into
master
from
dturissini/delegates-focus-tabindex-zero
Nov 8, 2018
Merged
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
acb6ae0
fix(engine): fix for delegatesFocus with tabindex=0
ca58fe5
fix(engine): handling empty shadow roots
19129ef
fix(engine): separating focus and focusin handlers
c5074a5
fix(engine): lint
d667dc5
fix(engine): lint
4ed3a04
fix(engine): ie11 flappers
cf8e8c4
fix(engine): using focusin because ie11
686704c
fix(engine): ie11
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ import { DOCUMENT_POSITION_CONTAINED_BY, compareDocumentPosition, DOCUMENT_POSIT | |
import { ArraySlice, ArrayIndexOf, isFalse, isNull, toString, ArrayReverse, hasOwnProperty } from '../shared/language'; | ||
import { DocumentPrototypeActiveElement, querySelectorAll as documentQuerySelectorAll } from '../env/document'; | ||
import { eventCurrentTargetGetter, eventTargetGetter, focusEventRelatedTargetGetter } from '../env/dom'; | ||
import { isDelegatingFocus } from "./shadow-root"; | ||
|
||
const TabbableElementsQuery = ` | ||
button:not([tabindex="-1"]):not([disabled]), | ||
|
@@ -111,12 +112,21 @@ function getTabbableSegments(host: HTMLElement): QuerySegments { | |
const all = documentQuerySelectorAll.call(document, TabbableElementsQuery); | ||
const inner = querySelectorAll.call(host, TabbableElementsQuery); | ||
if (process.env.NODE_ENV !== 'production') { | ||
assert.invariant(tabIndexGetter.call(host) === -1, `The focusin event is only relevant when the tabIndex property is -1 on the host.`); | ||
assert.invariant(inner.length > 0 || (tabIndexGetter.call(host) === 0 && isDelegatingFocus(host)), `When focusin event is received, there has to be a focusable target at least.`); | ||
assert.invariant(tabIndexGetter.call(host) === -1 || isDelegatingFocus(host), `The focusin event is only relevant when the tabIndex property is -1 on the host.`); | ||
} | ||
const firstChild = inner[0]; | ||
const lastChild = inner[inner.length - 1]; | ||
const prev = ArraySlice.call(all, 0, ArrayIndexOf.call(all, firstChild)); | ||
const next = ArraySlice.call(all, ArrayIndexOf.call(all, lastChild) + 1); | ||
const hostIndex = ArrayIndexOf.call(all, host); | ||
|
||
// Host element can show up in our "previous" section if its tabindex is 0 | ||
// We want to filter that out here | ||
const firstChildIndex = (hostIndex > -1) ? hostIndex : ArrayIndexOf.call(all, firstChild); | ||
|
||
// Account for an empty inner list | ||
const lastChildIndex = inner.length === 0 ? firstChildIndex + 1 : ArrayIndexOf.call(all, lastChild) + 1; | ||
const prev = ArraySlice.call(all, 0, firstChildIndex); | ||
const next = ArraySlice.call(all, lastChildIndex); | ||
return { | ||
prev, | ||
inner, | ||
|
@@ -193,62 +203,86 @@ function isLastTabbableChild(target: EventTarget, segments: QuerySegments): bool | |
return getLastTabbableMatch(segments.inner) === target; | ||
} | ||
|
||
function focusInEventHandler(event: FocusEvent) { | ||
function keyboardFocusHandler(event: FocusEvent) { | ||
const host: EventTarget = eventCurrentTargetGetter.call(event); | ||
const target: EventTarget = eventTargetGetter.call(event); | ||
|
||
// Ideally, we would be able to use a "focus" event that doesn't bubble | ||
// but, IE11 doesn't support relatedTarget on focus events so we have to use | ||
// focusin instead. The logic below is predicated on non-bubbling events | ||
// So, if currentTarget(host) ir not target, we know that the event is bubbling | ||
// and we escape because focus occured on something below the host. | ||
if (host !== target) { | ||
return; | ||
} | ||
|
||
const relatedTarget: EventTarget = focusEventRelatedTargetGetter.call(event); | ||
|
||
if (isNull(relatedTarget)) { | ||
return; | ||
} | ||
|
||
const segments = getTabbableSegments(host as HTMLElement); | ||
const position = relatedTargetPosition(host as HTMLElement, relatedTarget as HTMLElement); | ||
|
||
if (position === 1) { | ||
// probably tabbing into element | ||
const first = getFirstTabbableMatch(segments.inner); | ||
if (!isNull(first)) { | ||
first.focus(); | ||
} else { | ||
focusOnNextOrBlur(target, segments); | ||
} | ||
return; | ||
} else if (host === target) { // Shift tabbed back to the host | ||
focusOnPrevOrBlur(host, segments); | ||
} | ||
} | ||
|
||
// focusin handler for custom elements | ||
// This handler should only be called when a user | ||
// focuses on either the custom element, or an internal element | ||
// via keyboard navigation (tab or shift+tab) | ||
// Focusing via mouse should be disqualified before this gets called | ||
function keyboardFocusInHandler(event: FocusEvent) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Rename for clarity |
||
const host: EventTarget = eventCurrentTargetGetter.call(event); | ||
const target: EventTarget = eventTargetGetter.call(event); | ||
const relatedTarget: EventTarget = focusEventRelatedTargetGetter.call(event); | ||
const segments = getTabbableSegments(host as HTMLElement); | ||
const isFirstFocusableChildReceivingFocus = isFirstTabbableChild(target, segments); | ||
const isLastFocusableChildReceivingFocus = isLastTabbableChild(target, segments); | ||
if ((isFalse(isFirstFocusableChildReceivingFocus) && isFalse(isLastFocusableChildReceivingFocus)) || isNull(relatedTarget)) { | ||
// the focus is definitely not a result of tab or shift-tab interaction | ||
if ( | ||
|
||
// If we receive a focusin event that is not focusing on the first or last | ||
// element inside of a shadow, we can assume that the user is tabbing between | ||
// elements inside of the custom element shadow, so we do nothing | ||
(isFalse(isFirstFocusableChildReceivingFocus) && isFalse(isLastFocusableChildReceivingFocus)) || | ||
|
||
// If related target is null, user is probably tabbing into the document from the browser chrome (location bar?) | ||
// If relatedTarget is null, we can't do much here because we don't know what direction the user is tabbing | ||
// This is a bit of an edge case, and only comes up if the custom element is the very first or very last | ||
// tabbable element in a document | ||
isNull(relatedTarget)) { | ||
return; | ||
} | ||
// If there is a related target, everything is easier | ||
|
||
// Determine where the focus is coming from (Tab or Shift+Tab) | ||
const post = relatedTargetPosition(host as HTMLElement, relatedTarget as HTMLElement); | ||
switch (post) { | ||
case 1: | ||
// focus is probably coming from above | ||
case 1: // focus is probably coming from above | ||
|
||
if (isFirstFocusableChildReceivingFocus && relatedTarget === getPreviousTabbableElement(segments)) { | ||
// the focus was on the immediate focusable elements from above, | ||
// it is almost certain that the focus is due to tab keypress | ||
focusOnNextOrBlur(target, segments); | ||
} | ||
/** | ||
* note: false positive here is when the user is clicking | ||
* directly on the first focusable element inside the next | ||
* custom element that is wrapping it, and it has | ||
* delegatesFocus and tabindex="-1", this is very very rare, e.g.: | ||
* <body> | ||
* <x-input> | ||
* #shadowRoot(delegatesFocus=true) | ||
* <input /> <--- focus in here | ||
* <x-input tabindex="-1"> | ||
* #shadowRoot(delegatesFocus=true) | ||
* <input /> <--- user clicks here | ||
**/ | ||
break; | ||
case 2: | ||
// focus is probably coming from below | ||
// focus is probably coming from above | ||
case 2: // focus is probably coming from below | ||
if (isLastFocusableChildReceivingFocus && relatedTarget === getNextTabbableElement(segments)) { | ||
// the focus was on the immediate focusable elements from above, | ||
// it is almost certain that the focus is due to tab keypress | ||
focusOnPrevOrBlur(target, segments); | ||
} | ||
/** | ||
* note: false positive here is when the user is clicking | ||
* directly on the last focusable element inside the next | ||
* custom element that is wrapping it, and it has | ||
* delegatesFocus and tabindex="-1", this is very very rare, e.g.: | ||
* <body> | ||
* <x-input tabindex="-1"> | ||
* #shadowRoot(delegatesFocus=true) | ||
* <input /> <--- user clicks here | ||
* <x-input> | ||
* #shadowRoot(delegatesFocus=true) | ||
* <input /> <--- focus in here | ||
**/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. These comments are no longer relevant because we disqualify based on |
||
break; | ||
} | ||
} | ||
|
@@ -262,11 +296,11 @@ function willTriggerFocusInEvent(target: HTMLElement): boolean { | |
|
||
function stopFocusIn(evt) { | ||
const currentTarget = eventCurrentTargetGetter.call(evt); | ||
removeEventListener.call(currentTarget, 'focusin', focusInEventHandler); | ||
removeEventListener.call(currentTarget, 'focusin', keyboardFocusInHandler); | ||
setTimeout(() => { | ||
// only reinstate the focus if the tabindex is still -1 | ||
if (tabIndexGetter.call(currentTarget) === -1) { | ||
addEventListener.call(currentTarget, 'focusin', focusInEventHandler); | ||
addEventListener.call(currentTarget, 'focusin', keyboardFocusInHandler); | ||
} | ||
}, 0); | ||
} | ||
|
@@ -281,10 +315,29 @@ function handleFocusMouseDown(evt) { | |
} | ||
} | ||
|
||
export function handleFocus(elm: HTMLElement) { | ||
if (process.env.NODE_ENV !== 'production') { | ||
assert.invariant(isDelegatingFocus(elm), `Invalid attempt to handle focus event for ${toString(elm)}. ${toString(elm)} should have delegates focus true, but is not delegating focus`); | ||
} | ||
|
||
// Unbind any focusin listeners we may have going on | ||
ignoreFocusIn(elm); | ||
addEventListener.call(elm, 'focusin', keyboardFocusHandler, true); | ||
|
||
} | ||
|
||
export function ignoreFocus(elm: HTMLElement) { | ||
removeEventListener.call(elm, 'focusin', keyboardFocusHandler, true); | ||
} | ||
|
||
export function handleFocusIn(elm: HTMLElement) { | ||
if (process.env.NODE_ENV !== 'production') { | ||
assert.invariant(tabIndexGetter.call(elm) === -1, `Invalid attempt to handle focus in ${toString(elm)}. ${toString(elm)} should have tabIndex -1, but has tabIndex ${tabIndexGetter.call(elm)}`); | ||
} | ||
|
||
// Unbind any focus listeners we may have going on | ||
ignoreFocus(elm); | ||
|
||
// We want to listen for mousedown | ||
// If the user is triggering a mousedown event on an element | ||
// That can trigger a focus event, then we need to opt out | ||
|
@@ -296,13 +349,13 @@ export function handleFocusIn(elm: HTMLElement) { | |
// the keydown event happens on whatever element already has focus (or no element | ||
// at all in the case of the location bar. So, instead we have to assume that focusin | ||
// without a mousedown means keyboard navigation | ||
addEventListener.call(elm, 'focusin', focusInEventHandler); | ||
addEventListener.call(elm, 'focusin', keyboardFocusInHandler); | ||
} | ||
|
||
export function ignoreFocusIn(elm: HTMLElement) { | ||
if (process.env.NODE_ENV !== 'production') { | ||
assert.invariant(tabIndexGetter.call(elm) !== -1, `Invalid attempt to ignore focus in ${toString(elm)}. ${toString(elm)} should not have tabIndex -1`); | ||
} | ||
removeEventListener.call(elm, 'focusin', focusInEventHandler); | ||
removeEventListener.call(elm, 'focusin', keyboardFocusInHandler); | ||
removeEventListener.call(elm, 'mousedown', handleFocusMouseDown, true); | ||
} |
22 changes: 22 additions & 0 deletions
22
...r-elements/delegates-focus-tab-index-zero-no-focusable-elements-no-after-elements.spec.js
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
const assert = require('assert'); | ||
describe('Delegate focus with tabindex 0, no tabbable elements, and no tabbable elements after', () => { | ||
const URL = 'http://localhost:4567/delegates-focus-tab-index-zero-no-focusable-elements-no-after-elements'; | ||
let element; | ||
|
||
before(() => { | ||
browser.url(URL); | ||
}); | ||
|
||
it('should correctly have no activeelement', function () { | ||
browser.keys(['Tab']); | ||
browser.keys(['Tab']); | ||
|
||
|
||
browser.waitUntil(() => { | ||
const active = browser.execute(function () { | ||
return document.activeElement; | ||
}); | ||
return active.getTagName().toLowerCase() === 'body'; | ||
}, 500, 'It should focus the body'); | ||
}); | ||
}); |
3 changes: 3 additions & 0 deletions
3
...focus-tab-index-zero-no-focusable-elements-no-after-elements/integration/child/child.html
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
<template> | ||
No focusable elements | ||
</template> |
5 changes: 5 additions & 0 deletions
5
...s-focus-tab-index-zero-no-focusable-elements-no-after-elements/integration/child/child.js
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import { LightningElement } from 'lwc'; | ||
|
||
export default class Child extends LightningElement { | ||
static delegatesFocus = true; | ||
} |
4 changes: 4 additions & 0 deletions
4
...fter-elements/delegates-focus-tab-index-zero-no-focusable-elements-no-after-elements.html
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
<template> | ||
<button>first button</button> | ||
<integration-child tabindex="0"></integration-child> | ||
</template> |
5 changes: 5 additions & 0 deletions
5
...-after-elements/delegates-focus-tab-index-zero-no-focusable-elements-no-after-elements.js
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import { LightningElement } from 'lwc'; | ||
|
||
export default class Parent extends LightningElement { | ||
|
||
} |
32 changes: 32 additions & 0 deletions
32
...x-zero-no-focusable-elements/delegates-focus-tab-index-zero-no-focusable-elements.spec.js
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,32 @@ | ||
const assert = require('assert'); | ||
describe('Delegate focus with tabindex 0 and no tabbable elements', () => { | ||
const URL = 'http://localhost:4567/delegates-focus-tab-index-zero-no-focusable-elements'; | ||
let element; | ||
|
||
before(() => { | ||
browser.url(URL); | ||
}); | ||
|
||
it('should correctly skip the custom element', function () { | ||
browser.keys(['Tab']); | ||
browser.keys(['Tab']); | ||
|
||
browser.waitUntil(() => { | ||
const active = browser.execute(function () { | ||
return document.activeElement.shadowRoot.activeElement; | ||
}); | ||
|
||
return active.getText() === 'second button'; | ||
}, 500, 'Second button should be focused'); | ||
|
||
browser.keys(['Shift', 'Tab']); | ||
|
||
browser.waitUntil(() => { | ||
const active = browser.execute(function () { | ||
return document.activeElement.shadowRoot.activeElement; | ||
}); | ||
|
||
return active.getText() === 'first button'; | ||
}, 500, 'First button should be focused'); | ||
}); | ||
}); |
3 changes: 3 additions & 0 deletions
3
...ty/test-delegates-focus-tab-index-zero-no-focusable-elements/integration/child/child.html
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
<template> | ||
No focusable elements | ||
</template> |
5 changes: 5 additions & 0 deletions
5
...lity/test-delegates-focus-tab-index-zero-no-focusable-elements/integration/child/child.js
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import { LightningElement } from 'lwc'; | ||
|
||
export default class Child extends LightningElement { | ||
static delegatesFocus = true; | ||
} |
5 changes: 5 additions & 0 deletions
5
...ndex-zero-no-focusable-elements/delegates-focus-tab-index-zero-no-focusable-elements.html
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
<template> | ||
<button>first button</button> | ||
<integration-child tabindex="0"></integration-child> | ||
<button>second button</button> | ||
</template> |
5 changes: 5 additions & 0 deletions
5
...-index-zero-no-focusable-elements/delegates-focus-tab-index-zero-no-focusable-elements.js
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
import { LightningElement } from 'lwc'; | ||
|
||
export default class Parent extends LightningElement { | ||
|
||
} |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When
tabindex
is 0 on the host,host
will actually show up in the previous array. We have to make sure that doesn't happen. I think this is quicker than usingArrayFilter
call but I could be wrong.