paulmataruso/zulip
1
0
Fork 0
mirror of https://github.com/zulip/zulip.git synced 2025-11-02 04:53:36 +00:00
Code Issues Packages Projects Releases Wiki Activity

left_sidebar: Skip invisible rows in keyboard navigation.

Browse Source 
Collapsed sections and inactive channel lists are not visible and so
should be ignored in keyboard naviation.
This commit is contained in:
Evy Kassirer
2025-07-18 11:58:24 -07:00
committed by Tim Abbott
parent f984b44ab9
commit 18463b45a2
6 changed files with 184 additions and 98 deletions
Download Patch File Download Diff File Expand all files Collapse all files

26
web/e2e-tests/message-basics.test.ts
View File

@@ -304,7 +304,7 @@ async function test_search_venice(page: Page): Promise<void> {
await common.clear_and_type(page, ".stream-list-filter", "vEnI"); // Must be case insensitive.
await page.waitForSelector(await get_stream_li(page, "Denmark"), {hidden: true});
await page.waitForSelector(await get_stream_li(page, "Verona"), {hidden: true});
await page.waitForSelector((await get_stream_li(page, "Venice")) + ".highlighted_stream", {
await page.waitForSelector((await get_stream_li(page, "Venice")) + ".highlighted_row", {
visible: true,
});
@@ -328,25 +328,25 @@ async function test_stream_search_filters_stream_list(page: Page): Promise<void>
// Enter the search box and test highlighted suggestion
await page.click(".stream-list-filter");
await page.waitForSelector("#stream_filters .highlighted_stream", {visible: true});
await page.waitForSelector("#stream_filters .highlighted_row", {visible: true});
// First stream in list gets highlighted on clicking search.
await page.waitForSelector((await get_stream_li(page, "core team")) + ".highlighted_stream", {
await page.waitForSelector((await get_stream_li(page, "core team")) + ".highlighted_row", {
visible: true,
});
await page.waitForSelector((await get_stream_li(page, "Denmark")) + ".highlighted_stream", {
await page.waitForSelector((await get_stream_li(page, "Denmark")) + ".highlighted_row", {
hidden: true,
});
await page.waitForSelector((await get_stream_li(page, "sandbox")) + ".highlighted_stream", {
await page.waitForSelector((await get_stream_li(page, "sandbox")) + ".highlighted_row", {
hidden: true,
});
await page.waitForSelector((await get_stream_li(page, "Venice")) + ".highlighted_stream", {
await page.waitForSelector((await get_stream_li(page, "Venice")) + ".highlighted_row", {
hidden: true,
});
await page.waitForSelector((await get_stream_li(page, "Verona")) + ".highlighted_stream", {
await page.waitForSelector((await get_stream_li(page, "Verona")) + ".highlighted_row", {
hidden: true,
});
await page.waitForSelector((await get_stream_li(page, "Zulip")) + ".highlighted_stream", {
await page.waitForSelector((await get_stream_li(page, "Zulip")) + ".highlighted_row", {
hidden: true,
});
@@ -361,20 +361,20 @@ async function test_stream_search_filters_stream_list(page: Page): Promise<void>
await arrow(page, "Down"); // sandbox-> Venice
await arrow(page, "Down"); // Venice -> Verona
await page.waitForSelector((await get_stream_li(page, "Verona")) + ".highlighted_stream", {
await page.waitForSelector((await get_stream_li(page, "Verona")) + ".highlighted_row", {
visible: true,
});
await page.waitForSelector((await get_stream_li(page, "core team")) + ".highlighted_stream", {
await page.waitForSelector((await get_stream_li(page, "core team")) + ".highlighted_row", {
hidden: true,
});
await page.waitForSelector((await get_stream_li(page, "Denmark")) + ".highlighted_stream", {
await page.waitForSelector((await get_stream_li(page, "Denmark")) + ".highlighted_row", {
hidden: true,
});
await page.waitForSelector((await get_stream_li(page, "Venice")) + ".highlighted_stream", {
await page.waitForSelector((await get_stream_li(page, "Venice")) + ".highlighted_row", {
hidden: true,
});
await page.waitForSelector((await get_stream_li(page, "Zulip")) + ".highlighted_stream", {
await page.waitForSelector((await get_stream_li(page, "Zulip")) + ".highlighted_row", {
hidden: true,
});
await test_search_venice(page);

4
web/src/list_cursor.ts
View File

@@ -82,8 +82,12 @@ export class ListCursor<Key> {
const row = this.get_row(this.curr_key);
if (row === undefined) {
/* TODO/channel-folders: Remove when tests are restored */
/* istanbul ignore next */
return;
}
/* TODO/channel-folders: Remove when tests are restored */
/* istanbul ignore next */
row.highlight();
}

53
web/src/stream_list.ts
View File

@@ -30,7 +30,7 @@ import * as settings_data from "./settings_data.ts";
import * as sidebar_ui from "./sidebar_ui.ts";
import * as stream_data from "./stream_data.ts";
import * as stream_list_sort from "./stream_list_sort.ts";
import type {StreamListSection} from "./stream_list_sort.ts";
import type {StreamListRow, StreamListSection} from "./stream_list_sort.ts";
import * as stream_topic_history from "./stream_topic_history.ts";
import * as stream_topic_history_util from "./stream_topic_history_util.ts";
import * as sub_store from "./sub_store.ts";
@@ -52,7 +52,7 @@ export function set_update_inbox_channel_view_callback(value: (channel_id: numbe
update_inbox_channel_view_callback = value;
}
export let stream_cursor: ListCursor<number>;
export let stream_cursor: ListCursor<StreamListRow>;
export function rewire_stream_cursor(value: typeof stream_cursor): void {
stream_cursor = value;
@@ -863,19 +863,35 @@ const update_streams_for_search = _.throttle(actually_update_streams_for_search,
// Exported for tests only.
export function initialize_stream_cursor(): void {
stream_cursor = new ListCursor({
stream_cursor = new ListCursor<StreamListRow>({
list: {
scroll_container_selector: "#left_sidebar_scroll_container",
find_li(opts) {
const stream_id = opts.key;
const $li = get_stream_li(stream_id);
if (opts.key.type === "stream") {
const $li = get_stream_li(opts.key.stream_id);
return $li;
}
return $(
`#stream-list-${opts.key.section_id}-container .stream-list-toggle-inactive-channels`,
);
},
first_key: stream_list_sort.first_stream_id,
prev_key: stream_list_sort.prev_stream_id,
next_key: stream_list_sort.next_stream_id,
first_key: stream_list_sort.first_row,
prev_key: (row) =>
stream_list_sort.prev_row(
row,
sections_showing_inactive,
collapsed_sections,
topic_list.active_stream_id(),
),
next_key: (row) =>
stream_list_sort.next_row(
row,
sections_showing_inactive,
collapsed_sections,
topic_list.active_stream_id(),
),
},
highlight_class: "highlighted_stream",
highlight_class: "highlighted_row",
});
}
@@ -1116,14 +1132,18 @@ export function set_event_handlers({
const $search_input = $(".stream-list-filter").expectOne();
function keydown_enter_key(): void {
const stream_id = stream_cursor.get_key();
const row = stream_cursor.get_key();
if (stream_id === undefined) {
if (row === undefined) {
// This can happen for empty searches, no need to warn.
return;
}
on_sidebar_channel_click(stream_id, null, show_channel_feed);
if (row.type === "stream") {
on_sidebar_channel_click(row.stream_id, null, show_channel_feed);
} else {
toggle_inactive_channels($(`#stream-list-${row.section_id}-container`));
}
}
keydown_util.handle({
@@ -1176,7 +1196,12 @@ export function set_event_handlers({
".stream-list-toggle-inactive-channels",
function (this: HTMLElement, e: JQuery.ClickEvent) {
e.stopPropagation();
const $section_container = $(this).closest(".stream-list-section-container");
toggle_inactive_channels($(this).closest(".stream-list-section-container"));
},
);
}
function toggle_inactive_channels($section_container: JQuery): void {
$section_container.toggleClass("showing-inactive");
const showing_inactive = $section_container.hasClass("showing-inactive");
const section_id = $section_container.attr("data-section-id")!;
@@ -1185,8 +1210,6 @@ export function set_event_handlers({
} else {
sections_showing_inactive.delete(section_id);
}
},
);
}
export function searching(): boolean {

134
web/src/stream_list_sort.ts
View File

@@ -11,7 +11,18 @@ import * as util from "./util.ts";
let first_render_completed = false;
let current_sections: StreamListSection[] = [];
let all_streams: number[] = [];
export type StreamListRow =
| {
type: "stream";
stream_id: number;
inactive: boolean;
}
| {
type: "inactive_toggle";
section_id: string;
};
let all_rows: StreamListRow[] = [];
// Because we need to check whether we are filtering inactive streams
// in a loop over all streams to render the left sidebar, and the
@@ -21,7 +32,7 @@ let all_streams: number[] = [];
let filter_out_inactives = false;
export function get_stream_ids(): number[] {
return [...all_streams];
return all_rows.flatMap((row) => (row.type === "stream" ? row.stream_id : []));
}
function current_section_ids_for_streams(): Map<number, StreamListSection> {
@@ -213,11 +224,29 @@ export function sort_groups(stream_ids: number[], search_term: string): StreamLi
if (!same_as_before) {
first_render_completed = true;
current_sections = new_sections;
all_streams = current_sections.flatMap((section) => [
...section.streams,
...section.muted_streams,
...section.inactive_streams,
]);
all_rows = [];
for (const section of current_sections) {
for (const stream_id of [...section.streams, ...section.muted_streams]) {
all_rows.push({
type: "stream",
stream_id,
inactive: false,
});
}
for (const stream_id of section.inactive_streams) {
all_rows.push({
type: "stream",
stream_id,
inactive: true,
});
}
if (section.inactive_streams.length > 0) {
all_rows.push({
type: "inactive_toggle",
section_id: section.id,
});
}
}
}
return {
@@ -226,36 +255,83 @@ export function sort_groups(stream_ids: number[], search_term: string): StreamLi
};
}
function maybe_get_stream_id(i: number): number | undefined {
if (i < 0 || i >= all_streams.length) {
return undefined;
}
return all_streams[i];
export function first_row(): StreamListRow | undefined {
return all_rows.at(0);
}
export function first_stream_id(): number | undefined {
return maybe_get_stream_id(0);
function is_visible_row(
row: StreamListRow,
section_id_map: Map<number, StreamListSection>,
sections_showing_inactive: Set<string>,
collapsed_sections: Set<string>,
active_stream_id: number | undefined,
): boolean {
if (row.type === "stream") {
const stream_id = row.stream_id;
assert(stream_id !== undefined);
const section = section_id_map.get(stream_id)!;
if (collapsed_sections.has(section.id) && active_stream_id !== stream_id) {
return false;
}
if (!sections_showing_inactive.has(section.id) && row.inactive) {
return false;
}
} else if (row.type === "inactive_toggle" && collapsed_sections.has(row.section_id)) {
return false;
}
return true;
}
export function prev_stream_id(stream_id: number): number | undefined {
const i = all_streams.indexOf(stream_id);
if (i === -1) {
return undefined;
export function prev_row(
row: StreamListRow,
sections_showing_inactive: Set<string>,
collapsed_sections: Set<string>,
active_stream_id: number | undefined,
): StreamListRow | undefined {
let i = all_rows.indexOf(row);
const section_id_map = current_section_ids_for_streams();
while (i > 0) {
i -= 1;
const prev_row = all_rows[i]!;
if (
is_visible_row(
prev_row,
section_id_map,
sections_showing_inactive,
collapsed_sections,
active_stream_id,
)
) {
return prev_row;
}
return maybe_get_stream_id(i - 1);
}
return undefined;
}
export function next_stream_id(stream_id: number): number | undefined {
const i = all_streams.indexOf(stream_id);
if (i === -1) {
return undefined;
export function next_row(
row: StreamListRow,
sections_showing_inactive: Set<string>,
collapsed_sections: Set<string>,
active_stream_id: number | undefined,
): StreamListRow | undefined {
let i = all_rows.indexOf(row);
const section_id_map = current_section_ids_for_streams();
while (i + 1 < all_rows.length) {
i += 1;
const next_row = all_rows[i]!;
if (
is_visible_row(
next_row,
section_id_map,
sections_showing_inactive,
collapsed_sections,
active_stream_id,
)
) {
return next_row;
}
return maybe_get_stream_id(i + 1);
}
return undefined;
}
export function initialize(): void {

8
web/styles/left_sidebar.css
View File

@@ -635,8 +635,14 @@
}
}
.stream-list-toggle-inactive-channels.highlighted_row {
outline: 2px solid var(--color-outline-focus);
outline-offset: -2px;
background-color: var(--color-background-sidebar-action-heading-hover);
}
#stream_filters .narrow-filter:has(a.subscription_block:focus-visible),
#stream_filters .narrow-filter.highlighted_stream {
#stream_filters .narrow-filter.highlighted_row {
&.active-filter > .bottom_left_row {
background-color: var(--color-background-hover-narrow-filter);
}

41
web/tests/stream_list_sort.test.cjs
View File

@@ -118,7 +118,7 @@ test("no_subscribed_streams", () => {
],
same_as_before: sorted.same_as_before,
});
assert.equal(stream_list_sort.first_stream_id(), undefined);
assert.equal(stream_list_sort.first_row(), undefined);
});
test("basics", () => {
@@ -147,27 +147,14 @@ test("basics", () => {
]);
assert.deepEqual(normal.muted_streams, [muted_active.stream_id]);
// Test cursor helpers.
assert.equal(stream_list_sort.first_stream_id(), scalene.stream_id);
assert.equal(stream_list_sort.prev_stream_id(scalene.stream_id), undefined);
assert.equal(stream_list_sort.prev_stream_id(muted_pinned.stream_id), scalene.stream_id);
assert.equal(stream_list_sort.prev_stream_id(clarinet.stream_id), muted_pinned.stream_id);
assert.equal(
stream_list_sort.next_stream_id(fast_tortoise.stream_id),
stream_hyphen_underscore_slash_colon.stream_id,
);
assert.equal(
stream_list_sort.next_stream_id(stream_hyphen_underscore_slash_colon.stream_id),
muted_active.stream_id,
);
assert.equal(
stream_list_sort.next_stream_id(fast_tortoise.stream_id),
stream_hyphen_underscore_slash_colon.stream_id,
);
assert.equal(stream_list_sort.next_stream_id(muted_active.stream_id), pneumonia.stream_id);
assert.equal(stream_list_sort.next_stream_id(pneumonia.stream_id), undefined);
// Test keyboard UI / cursor code (currently mostly deleted).
// TODO/channel-folders: Re-add keyboard navigation tests,
// including some with filtering. This mainly requires either
// exporting some parts of the stream_list module, or refactoring
// to move some of the stream_list data objects to another module.
const row = stream_list_sort.first_row();
assert.equal(row.type, "stream");
assert.equal(row.stream_id, scalene.stream_id);
// Test filtering
sorted_sections = sort_groups("s").sections;
@@ -179,10 +166,6 @@ test("basics", () => {
assert.deepEqual(sorted_sections[1].inactive_streams, []);
assert.deepEqual(sorted_sections[1].streams, [stream_hyphen_underscore_slash_colon.stream_id]);
assert.equal(stream_list_sort.prev_stream_id(clarinet.stream_id), undefined);
assert.equal(stream_list_sort.next_stream_id(clarinet.stream_id), undefined);
// Test searching entire word, case-insensitive
sorted_sections = sort_groups("PnEuMoNiA").sections;
assert.deepEqual(sorted_sections.length, 2);
@@ -193,13 +176,7 @@ test("basics", () => {
// Test searching part of word
sorted_sections = sort_groups("tortoise").sections;
<<<<<<< HEAD
assert.deepEqual(sorted_sections.length, 3);
||||||| parent of e3f8b5fa9f (left_sidebar: Remove inactive section, put inactive channels in regular section.)
assert.deepEqual(sorted_sections.length, 3);
=======
assert.deepEqual(sorted_sections.length, 2);
>>>>>>> e3f8b5fa9f (left_sidebar: Remove inactive section, put inactive channels in regular section.)
assert.deepEqual(sorted_sections[0].streams, []);
assert.deepEqual(sorted_sections[0].inactive_streams, []);
assert.deepEqual(sorted_sections[1].id, "normal-streams");