-
Notifications
You must be signed in to change notification settings - Fork 3.4k
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
Added Optional Roving Tabindex to Toolbar #4557
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -27,6 +27,8 @@ class Toolbar extends Module<ToolbarProps> { | |
controls: [string, HTMLElement][]; | ||
handlers: Record<string, Handler>; | ||
|
||
hasRovingTabindex: boolean = false; | ||
|
||
constructor(quill: Quill, options: Partial<ToolbarProps>) { | ||
super(quill, options); | ||
if (Array.isArray(this.options.container)) { | ||
|
@@ -45,6 +47,10 @@ class Toolbar extends Module<ToolbarProps> { | |
return; | ||
} | ||
this.container.classList.add('ql-toolbar'); | ||
|
||
// Check if the parent element has the custom "roving-tabindex" class in order to enable or disable roving tabindex | ||
this.hasRovingTabindex = this.container.closest('.roving-tabindex') !== null; | ||
|
||
this.controls = []; | ||
this.handlers = {}; | ||
if (this.options.handlers) { | ||
|
@@ -133,9 +139,54 @@ class Toolbar extends Module<ToolbarProps> { | |
} | ||
this.update(range); | ||
}); | ||
|
||
if (this.hasRovingTabindex && input.tagName === 'BUTTON') { | ||
input.addEventListener('keydown', (e) => { | ||
this.handleKeyboardEvent(e); | ||
}); | ||
} | ||
|
||
this.controls.push([format, input]); | ||
} | ||
|
||
handleKeyboardEvent(e: KeyboardEvent) { | ||
var target = e.currentTarget; | ||
if (!target) return; | ||
|
||
switch (e.key) { | ||
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. a switch with only one option is a bit strange, no? |
||
case 'ArrowLeft': | ||
case 'ArrowRight': | ||
this.updateTabIndexes(target, e.key); | ||
break; | ||
} | ||
} | ||
|
||
updateTabIndexes(target: EventTarget, key: string) { | ||
const currentIndex = this.controls.findIndex(control => control[1] === target); | ||
const currentItem = this.controls[currentIndex][1]; | ||
currentItem.tabIndex = -1; | ||
|
||
let nextIndex; | ||
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. add type and initial value |
||
if (key === 'ArrowLeft') { | ||
nextIndex = currentIndex === 0 ? this.controls.length - 1 : currentIndex - 1; | ||
} else if (key === 'ArrowRight') { | ||
nextIndex = currentIndex === this.controls.length - 1 ? 0 : currentIndex + 1; | ||
} | ||
|
||
if (nextIndex === undefined) return; | ||
const nextItem = this.controls[nextIndex][1]; | ||
if (nextItem.tagName === 'SELECT') { | ||
const qlPickerLabel = nextItem.previousElementSibling?.querySelectorAll('.ql-picker-label')[0]; | ||
if (qlPickerLabel && qlPickerLabel.tagName === 'SPAN') { | ||
(qlPickerLabel as HTMLElement).tabIndex = 0; | ||
(qlPickerLabel as HTMLElement).focus(); | ||
} | ||
} else { | ||
nextItem.tabIndex = 0; | ||
nextItem.focus(); | ||
} | ||
} | ||
|
||
update(range: Range | null) { | ||
const formats = range == null ? {} : this.quill.getFormat(range); | ||
this.controls.forEach((pair) => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,6 +14,8 @@ class Picker { | |
container: HTMLElement; | ||
label: HTMLElement; | ||
|
||
hasRovingTabindex: boolean = false; | ||
|
||
constructor(select: HTMLSelectElement) { | ||
this.select = select; | ||
this.container = document.createElement('span'); | ||
|
@@ -22,6 +24,11 @@ class Picker { | |
// @ts-expect-error Fix me later | ||
this.select.parentNode.insertBefore(this.container, this.select); | ||
|
||
// Set tabIndex for the first item in the toolbar | ||
this.hasRovingTabindex = this.container.closest('.roving-tabindex') !== null; | ||
this.setTabIndexes(); | ||
|
||
|
||
this.label.addEventListener('mousedown', () => { | ||
this.togglePicker(); | ||
}); | ||
|
@@ -85,14 +92,72 @@ class Picker { | |
const label = document.createElement('span'); | ||
label.classList.add('ql-picker-label'); | ||
label.innerHTML = DropdownIcon; | ||
|
||
// Set tabIndex to -1 by default to prevent focus | ||
// @ts-expect-error | ||
label.tabIndex = '0'; | ||
label.tabIndex = '-1'; | ||
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. should this only happen for non roving or always? 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. This is now always set to -1 initially because I handle this in the setTabIndexes() method, which is called later once everything is initialized. |
||
label.addEventListener('keydown', (event) => { | ||
this.handleKeyboardEvent(event); | ||
}); | ||
|
||
|
||
label.setAttribute('role', 'button'); | ||
label.setAttribute('aria-expanded', 'false'); | ||
this.container.appendChild(label); | ||
return label; | ||
} | ||
|
||
setTabIndexes() { | ||
const toolbar = this.select.closest('.ql-toolbar'); | ||
if (!toolbar) return; | ||
const items = Array.from(toolbar.querySelectorAll('.ql-picker .ql-picker-label, .ql-toolbar button')); | ||
|
||
if (this.hasRovingTabindex) { | ||
if (items[0] === this.label) { | ||
items[0].setAttribute('tabindex', '0') | ||
} | ||
|
||
} else { | ||
items.forEach((item) => { | ||
item.setAttribute('tabindex', '0'); | ||
}); | ||
} | ||
} | ||
|
||
handleKeyboardEvent(e: KeyboardEvent) { | ||
if (!this.hasRovingTabindex) return; | ||
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. kind of duplicated. Can't you put the key handler on the toolbar or another parent of both buttons and picker? |
||
var target = e.currentTarget; | ||
if (!target) return; | ||
|
||
switch (e.key) { | ||
case 'ArrowLeft': | ||
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. same, strange switch case |
||
case 'ArrowRight': | ||
this.updateTabIndexes(target, e.key); | ||
break; | ||
} | ||
} | ||
|
||
updateTabIndexes(target: EventTarget, key: string) { | ||
this.label.setAttribute('tabindex', '-1'); | ||
|
||
const toolbar = this.container.closest('.ql-toolbar'); | ||
if (!toolbar) return; | ||
const items = Array.from(toolbar.querySelectorAll('.ql-picker .ql-picker-label, .ql-toolbar button')); | ||
const currentIndex = items.indexOf(target as HTMLElement); | ||
let newIndex; | ||
|
||
if (key === 'ArrowLeft') { | ||
newIndex = (currentIndex - 1 + items.length) % items.length; | ||
} else if (key === 'ArrowRight') { | ||
newIndex = (currentIndex + 1) % items.length; | ||
} | ||
|
||
if (!newIndex) return; | ||
|
||
items[newIndex].setAttribute('tabindex', '0'); | ||
(items[newIndex] as HTMLElement).focus(); | ||
} | ||
|
||
buildOptions() { | ||
const options = document.createElement('span'); | ||
options.classList.add('ql-picker-options'); | ||
|
@@ -180,7 +245,7 @@ class Picker { | |
const item = | ||
// @ts-expect-error Fix me later | ||
this.container.querySelector('.ql-picker-options').children[ | ||
this.select.selectedIndex | ||
this.select.selectedIndex | ||
]; | ||
option = this.select.options[this.select.selectedIndex]; | ||
// @ts-expect-error | ||
|
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.
cleanup required for eventListener?