Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Draft: Add jump to date functionality to date headers in timeline #7317

Closed
wants to merge 4 commits into from
Closed

Draft: Add jump to date functionality to date headers in timeline #7317

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
594c7c7
Add jump to date functionality to date headers in timeline
MadLittleMods Dec 9, 2021
23da673
Navigate after date picker selection (native JS behavior)
MadLittleMods Dec 10, 2021
9fa980c
WIP: Keyboard navigate datepicker in ContextMenu
MadLittleMods Dec 10, 2021
e2d9751
Close the ContextMenu when tabbing away from it
MadLittleMods Dec 10, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 6 additions & 6 deletions res/css/views/context_menus/_IconizedContextMenu.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,21 +50,21 @@ limitations under the License.
}
Copy link
Contributor Author

@MadLittleMods MadLittleMods Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

You should probably also add this menu in the room info panel or similar, since unlike in your screenshot the closest date header might be pages and pages back if the room is somewhat active.

#7317 (comment)

@HarHarLinks As separate iterations, the plan is to make the date headers sticky so one is always visible.

And add a slash command /jumptodate 2021-12-10


// round the top corners of the top button for the hover effect to be bounded
&:first-child .mx_AccessibleButton:first-child {
&:first-child .mx_AccessibleButton:not(.mx_AccessibleButton_hasKind):first-child {
Copy link
Contributor Author

@MadLittleMods MadLittleMods Dec 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Avoid style clashes when trying to put a primary accessible button inside of a ContextMenu

Copy link
Member

Choose a reason for hiding this comment

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

There is a reason not to do that, it won't be accessible for keyboard-only users. Context Menus have strict focus management requirements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(new better flow suggestions welcome) 🙂

Popping off to a modal feels a little heavy.

Tabbing to a date picker in the context menu seems in the realm of sane to handle for the keyboard. But the HTML5 date picker here isn't giving the best UX because ideally we could remove the "Go" button and "Go" automatically when a date is picked. This doesn't seem feasible though because there no way to distinguish a change from navigating the months of the picker vs a final selection.

Copy link
Member

Choose a reason for hiding this comment

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

Tab in a context menu closes the context menu though as it is focus-locked

Copy link
Contributor Author

@MadLittleMods MadLittleMods Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think we can make this work. And seems acceptable under ARIA guidelines:

  • Tab: Moves focus to the next element in the tab sequence, and if the item that had focus is not in a menubar, closes its menu and all open parent menu containers.

  • Shift + Tab: Moves focus to the previous element in the tab sequence, and if the item that had focus is not in a menubar, closes its menu and all open parent menu containers.

-- Menu or Menu bar -> Keyboard Interaction, https://www.w3.org/TR/wai-aria-practices/#keyboard-interaction-12

I had this mostly working in 9fa980c except for closing the menu, but Tab was navigating the items as expected.

I think I will try to adapt this to use the RovingTabIndex like the TODO comment in ContextMenu mentions so I can register the items explicitly for navigation and be able to tell when we tab out the end or the start to close the menu.

Copy link
Contributor Author

@MadLittleMods MadLittleMods Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Is there a RovingTabIndex example that is known working and well implemented that I can base this off of? The spaces panel and room list seems like a pretty similar thing with the tree view being navigable with arrow keys and can Tab to the space options of each as you go.

The MessageActionBar which uses Toolbar and RovingTabIndex seems broken as I can't figure out how to navigate by Tab or Arrow keys in it.

It also seems counter-intuitive to me that I'm unable to navigate menus by Tab and have to use Arrow keys but it also seems inconsistent 🤔. Both seem like they should work.

What's the goal of setting tabindex="-1" on elements which are focusable? Maybe to get around focus holes in the page with lots of elements? It doesn't seem as applicable in a ContextMenu which has to be explicitly opened and will close when tabbed out of though.

Copy link
Member

@t3chguy t3chguy Dec 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It also seems counter-intuitive to me that I'm unable to navigate menus by Tab and have to use Arrow keys but it also seems inconsistent 🤔. Both seem like they should work.

What's the goal of setting tabindex="-1" on elements which are focusable?

See https://developer.mozilla.org/en-US/docs/Web/Accessibility/Keyboard-navigable_JavaScript_widgets Technique 1

It is so the app doesn't have a million tabstops, and instead each widget is a tabstop with sane and expected keyboard & focus management, e.g arrows. The roving & treeview stuff was written to match the wai-aria practices

Switching to roving was asked for by a lot of our a11y community, who have only the keyboard to use the app with and if they have to press tab ten times to get through every menu that just makes any action they want to take that much slower.

Copy link
Member

Choose a reason for hiding this comment

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

MessageActionBar does seem to have regressed.
The space panel, room list, space home are all roving focus managed

Copy link
Member

Choose a reason for hiding this comment

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

#7336 fixes MAB, during a refactoring something got missed I think

Copy link
Contributor Author

@MadLittleMods MadLittleMods Dec 11, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for looking into the fixes for the MessageActionBar! 🌟 I've created a v2 PR, #7339, based off of those fixes and wanting to refactor ContextMenu to use RovingTabIndex so that I can leave this PR untouched in its somewhat working state.

border-radius: 8px 8px 0 0; // radius matches .mx_ContextualMenu
}

// round the bottom corners of the bottom button for the hover effect to be bounded
&:last-child .mx_AccessibleButton:last-child {
&:last-child .mx_AccessibleButton:not(.mx_AccessibleButton_hasKind):last-child {
border-radius: 0 0 8px 8px; // radius matches .mx_ContextualMenu
}

// round all corners of the only button for the hover effect to be bounded
&:first-child:last-child .mx_AccessibleButton:first-child:last-child {
&:first-child:last-child .mx_AccessibleButton:not(.mx_AccessibleButton_hasKind):first-child:last-child {
border-radius: 8px; // radius matches .mx_ContextualMenu
}

.mx_AccessibleButton {
.mx_AccessibleButton:not(.mx_AccessibleButton_hasKind) {
// pad the inside of the button so that the hover background is padded too
padding-top: 12px;
padding-bottom: 12px;
Expand Down Expand Up @@ -130,7 +130,7 @@ limitations under the License.
}

.mx_IconizedContextMenu_optionList_red {
.mx_AccessibleButton {
.mx_AccessibleButton:not(.mx_AccessibleButton_hasKind) {
color: $alert !important;
}

Expand All @@ -148,7 +148,7 @@ limitations under the License.
}

.mx_IconizedContextMenu_active {
&.mx_AccessibleButton, .mx_AccessibleButton {
&.mx_AccessibleButton:not(.mx_AccessibleButton_hasKind), .mx_AccessibleButton:not(.mx_AccessibleButton_hasKind) {
color: $accent !important;
}

Expand Down
1 change: 1 addition & 0 deletions res/css/views/elements/_Field.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ limitations under the License.
transform 0.25s ease-out 0s,
background-color 0.25s ease-out 0s;
font-size: $font-10px;
line-height: normal;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reset line-height so it acts normal regardless of environment (ContextMenu has a very large line-height)

transform: translateY(-13px);
padding: 0 2px;
background-color: $background;
Expand Down
34 changes: 34 additions & 0 deletions res/css/views/messages/_DateSeparator.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,3 +33,37 @@ limitations under the License.
margin: 0 25px;
flex: 0 0 auto;
}

.mx_DateSeparator_jumpToDateMenu {
display: flex;
}

.mx_DateSeparator_chevron {
align-self: center;
width: 16px;
height: 16px;
mask-position: center;
mask-size: contain;
mask-repeat: no-repeat;
mask-image: url('$(res)/img/feather-customised/chevron-down.svg');
background-color: $tertiary-content;
}

.mx_DateSeparator_jumpToDateMenuOption > .mx_IconizedContextMenu_label {
flex: initial;
width: auto;
}

.mx_DateSeparator_datePickerForm {
display: flex;
}

.mx_DateSeparator_datePicker {
flex: initial;
margin: 0;
margin-left: 8px;
}

.mx_DateSeparator_datePickerSubmitButton {
margin-left: 8px;
}
4 changes: 2 additions & 2 deletions src/accessibility/context_menu/MenuItem.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,13 +31,13 @@ export const MenuItem: React.FC<IProps> = ({ children, label, tooltip, ...props
const ariaLabel = props["aria-label"] || label;

if (tooltip) {
return <AccessibleTooltipButton {...props} role="menuitem" tabIndex={-1} aria-label={ariaLabel} title={tooltip}>
return <AccessibleTooltipButton {...props} role="menuitem" aria-label={ariaLabel} title={tooltip}>
{ children }
</AccessibleTooltipButton>;
}

return (
<AccessibleButton {...props} role="menuitem" tabIndex={-1} aria-label={ariaLabel}>
<AccessibleButton {...props} role="menuitem" aria-label={ariaLabel}>
{ children }
</AccessibleButton>
);
Expand Down
23 changes: 19 additions & 4 deletions src/components/structures/ContextMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,9 @@ export interface IProps extends IPosition {
// it will be mounted to a container at the root of the DOM.
mountAsChild?: boolean;

// If specified, contents will be wrapped in a FocusLock, this is only needed if the context menu is being rendered
// within an existing FocusLock e.g inside a modal.
// If specified, contents will be wrapped in a FocusLock, this is only
// needed if the context menu is being rendered within an existing FocusLock
// e.g inside a modal.
focusLock?: boolean;

// Function to be called on menu close
Expand Down Expand Up @@ -180,12 +181,14 @@ export default class ContextMenu extends React.PureComponent<IProps, IState> {
if (this.props.onFinished) this.props.onFinished();
};

private onMoveFocus = (element: Element, up: boolean) => {
private onMoveFocus = (element: Element, up: boolean, closeIfOutOfBounds: boolean = false) => {
console.log('onMoveFocus =================', element)
let descending = false; // are we currently descending or ascending through the DOM tree?

do {
const child = up ? element.lastElementChild : element.firstElementChild;
const sibling = up ? element.previousElementSibling : element.nextElementSibling;
console.log('onMoveFocus', child, sibling)

if (descending) {
if (child) {
Expand Down Expand Up @@ -257,7 +260,6 @@ export default class ContextMenu extends React.PureComponent<IProps, IState> {
switch (ev.key) {
// XXX: this is imitating roving behaviour, it should really use the RovingTabIndex utils
// to inherit proper handling of unmount edge cases
case Key.TAB:
case Key.ESCAPE:
case Key.ARROW_LEFT: // close on left and right arrows too for when it is a context menu on a <Toolbar />
case Key.ARROW_RIGHT:
Expand All @@ -269,6 +271,19 @@ export default class ContextMenu extends React.PureComponent<IProps, IState> {
case Key.ARROW_DOWN:
this.onMoveFocus(ev.target as Element, false);
break;
case Key.TAB:
// Pass through listener
handled = false;

// TODO: This is more on PoC level and not very clean.
setTimeout(() => {
console.log('tab', this.state.contextMenuElem, document.activeElement)
// If we've tabbed outside of the menu, then close the menu
if(!this.state.contextMenuElem.contains(document.activeElement)) {
this.props.onFinished();
}
}, 0);
break;
case Key.HOME:
this.onMoveFocusHomeEnd(this.state.contextMenuElem, true);
break;
Expand Down
10 changes: 5 additions & 5 deletions src/components/structures/MessagePanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,8 +715,8 @@ export default class MessagePanel extends React.Component<IProps, IState> {

// do we need a date separator since the last event?
const wantsDateSeparator = this.wantsDateSeparator(prevEvent, eventDate);
if (wantsDateSeparator && !isGrouped) {
const dateSeparator = <li key={ts1}><DateSeparator key={ts1} ts={ts1} /></li>;
if (wantsDateSeparator && !isGrouped && this.props.room) {
const dateSeparator = <li key={ts1}><DateSeparator key={ts1} roomId={this.props.room.roomId} ts={ts1} /></li>;
ret.push(dateSeparator);
}

Expand Down Expand Up @@ -1109,7 +1109,7 @@ class CreationGrouper extends BaseGrouper {
if (panel.wantsDateSeparator(this.prevEvent, createEvent.getDate())) {
const ts = createEvent.getTs();
ret.push(
<li key={ts+'~'}><DateSeparator key={ts+'~'} ts={ts} /></li>,
<li key={ts+'~'}><DateSeparator key={ts+'~'} roomId={createEvent.getRoomId()} ts={ts} /></li>,
);
}

Expand Down Expand Up @@ -1222,7 +1222,7 @@ class RedactionGrouper extends BaseGrouper {
if (panel.wantsDateSeparator(this.prevEvent, this.events[0].getDate())) {
const ts = this.events[0].getTs();
ret.push(
<li key={ts+'~'}><DateSeparator key={ts+'~'} ts={ts} /></li>,
<li key={ts+'~'}><DateSeparator key={ts+'~'} roomId={this.events[0].getRoomId()} ts={ts} /></li>,
);
}

Expand Down Expand Up @@ -1318,7 +1318,7 @@ class MemberGrouper extends BaseGrouper {
if (panel.wantsDateSeparator(this.prevEvent, this.events[0].getDate())) {
const ts = this.events[0].getTs();
ret.push(
<li key={ts+'~'}><DateSeparator key={ts+'~'} ts={ts} /></li>,
<li key={ts+'~'}><DateSeparator key={ts+'~'} roomId={this.events[0].getRoomId()} ts={ts} /></li>,
);
}

Expand Down
2 changes: 1 addition & 1 deletion src/components/views/dialogs/MessageEditHistoryDialog.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ export default class MessageEditHistoryDialog extends React.PureComponent<IProps
const baseEventId = this.props.mxEvent.getId();
allEvents.forEach((e, i) => {
if (!lastEvent || wantsDateSeparator(lastEvent.getDate(), e.getDate())) {
nodes.push(<li key={e.getTs() + "~"}><DateSeparator ts={e.getTs()} /></li>);
nodes.push(<li key={e.getTs() + "~"}><DateSeparator roomId={e.getRoomId()} ts={e.getTs()} /></li>);
}
const isBaseEvent = e.getId() === baseEventId;
nodes.push((
Expand Down
14 changes: 12 additions & 2 deletions src/components/views/elements/Field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import classNames from 'classnames';
import * as sdk from '../../../index';
import { debounce } from "lodash";
import { IFieldState, IValidationResult } from "./Validation";
import { ComponentClass } from "../../../@types/common";

// Invoke validation from user input (when typing, etc.) at most once every N ms.
const VALIDATION_THROTTLE_MS = 200;
Expand Down Expand Up @@ -96,7 +97,16 @@ interface ITextareaProps extends IProps, TextareaHTMLAttributes<HTMLTextAreaElem
value: string;
}

type PropShapes = IInputProps | ISelectProps | ITextareaProps;
export interface ICustomInputProps extends IProps, InputHTMLAttributes<HTMLInputElement> {
// The element to create.
element: ComponentClass;
// The input's value. This is a controlled component, so the value is required.
value: string;
// Optionally can be used for the CustomInput
onInput?: React.ChangeEventHandler<HTMLInputElement>;
}

type PropShapes = IInputProps | ISelectProps | ITextareaProps | ICustomInputProps;

interface IState {
valid: boolean;
Expand Down Expand Up @@ -256,7 +266,7 @@ export default class Field extends React.PureComponent<PropShapes, IState> {
}

const hasValidationFlag = forceValidity !== null && forceValidity !== undefined;
const fieldClasses = classNames("mx_Field", `mx_Field_${this.props.element}`, className, {
const fieldClasses = classNames("mx_Field", `mx_Field_${typeof this.props.element === "string" ? this.props.element : "input"}`, className, {
// If we have a prefix element, leave the label always at the top left and
// don't animate it, as it looks a bit clunky and would add complexity to do
// properly.
Expand Down
Loading