From 8acb483bcdcb60ca112f2dc6194328a2f5c675fc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonatan=20K=C5=82osko?= Date: Thu, 4 Feb 2021 16:01:59 +0100 Subject: [PATCH] Improve keyboard navigation and focus (#19) * Adjust result length * Add more keyboard navigation actions * Improve inserted/deleted cells focus and add some tests * Some refactoring * Run formatter --- assets/js/cell/index.js | 18 ++- assets/js/session/index.js | 78 ++++++++-- lib/live_book/notebook.ex | 6 +- lib/live_book_web/live/cell.ex | 2 +- lib/live_book_web/live/session_live.ex | 146 +++++++++++++++++- test/live_book_web/live/session_live_test.exs | 75 +++++++-- 6 files changed, 285 insertions(+), 40 deletions(-) diff --git a/assets/js/cell/index.js b/assets/js/cell/index.js index f96fe3aeb..2e8acf4eb 100644 --- a/assets/js/cell/index.js +++ b/assets/js/cell/index.js @@ -53,6 +53,16 @@ const Cell = { markdown.setContent(newSource); }); } + + // New cells are initially focused, so check for such case. + + if (isActive(this.props)) { + this.liveEditor.focus(); + } + + if (this.props.isFocused) { + this.el.scrollIntoView({ behavior: "smooth", block: "center" }); + } }); }, @@ -60,15 +70,19 @@ const Cell = { const prevProps = this.props; this.props = getProps(this); + // Note: this.liveEditor is crated once we receive initial data + // so here we have to make sure it's defined. + if (!isActive(prevProps) && isActive(this.props)) { - this.liveEditor.focus(); + this.liveEditor && this.liveEditor.focus(); } if (isActive(prevProps) && !isActive(this.props)) { - this.liveEditor.blur(); + this.liveEditor && this.liveEditor.blur(); } if (!prevProps.isFocused && this.props.isFocused) { + // Note: it's important to trigger scrolling after focus, so it doesn't get interrupted. this.el.scrollIntoView({ behavior: "smooth", block: "center" }); } }, diff --git a/assets/js/session/index.js b/assets/js/session/index.js index 5c279675b..011878256 100644 --- a/assets/js/session/index.js +++ b/assets/js/session/index.js @@ -15,21 +15,65 @@ const Session = { // Keybindings this.handleDocumentKeydown = (event) => { - if (event.shiftKey && event.key === "Enter" && !event.repeat) { - if (this.props.focusedCellId !== null) { - // If the editor is focused we don't want it to receive the input - event.preventDefault(); - this.pushEvent("toggle_cell_expanded", {}); + const key = event.key.toLowerCase(); + const shift = event.shiftKey; + const alt = event.altKey; + const ctrl = event.ctrlKey; + + if (event.repeat) { + return; + } + + if (shift && key === "enter") { + cancelEvent(event); + + if (this.props.focusedCellType === "elixir") { + this.pushEvent("queue_focused_cell_evaluation"); } - } else if (event.altKey && event.key === "j") { - event.preventDefault(); + this.pushEvent("move_cell_focus", { offset: 1 }); - } else if (event.altKey && event.key === "k") { - event.preventDefault(); + } else if (alt && ctrl && key === "enter") { + cancelEvent(event); + + this.pushEvent("queue_child_cells_evaluation", {}); + } else if (ctrl && key === "enter") { + cancelEvent(event); + + if (this.props.focusedCellType === "elixir") { + this.pushEvent("queue_focused_cell_evaluation"); + } + + if (this.props.focusedCellType === "markdown") { + this.pushEvent("toggle_cell_expanded"); + } + } else if (alt && key === "j") { + cancelEvent(event); + + this.pushEvent("move_cell_focus", { offset: 1 }); + } else if (alt && key === "k") { + cancelEvent(event); + this.pushEvent("move_cell_focus", { offset: -1 }); - } else if (event.ctrlKey && event.key === "Enter") { - event.stopPropagation(); - this.pushEvent("queue_cell_evaluation", {}); + } else if (alt && key === "n") { + cancelEvent(event); + + if (shift) { + this.pushEvent("insert_cell_above_focused", { type: "elixir" }); + } else { + this.pushEvent("insert_cell_below_focused", { type: "elixir" }); + } + } else if (alt && key === "m") { + cancelEvent(event); + + if (shift) { + this.pushEvent("insert_cell_above_focused", { type: "markdown" }); + } else { + this.pushEvent("insert_cell_below_focused", { type: "markdown" }); + } + } else if (alt && key === "w") { + cancelEvent(event); + + this.pushEvent("delete_focused_cell", {}); // TODO: focused:delete_cell ? } }; @@ -55,7 +99,7 @@ const Session = { destroyed() { document.removeEventListener("keydown", this.handleDocumentKeydown); document.removeEventListener("click", this.handleDocumentClick); - } + }, }; function getProps(hook) { @@ -65,7 +109,15 @@ function getProps(hook) { "data-focused-cell-id", (value) => value || null ), + focusedCellType: getAttributeOrThrow(hook.el, "data-focused-cell-type"), }; } +function cancelEvent(event) { + // Cancel any default browser behavior. + event.preventDefault(); + // Stop event propagation (e.g. so it doesn't reach the editor). + event.stopPropagation(); +} + export default Session; diff --git a/lib/live_book/notebook.ex b/lib/live_book/notebook.ex index 2b4d7341b..4655f1907 100644 --- a/lib/live_book/notebook.ex +++ b/lib/live_book/notebook.ex @@ -172,9 +172,9 @@ defmodule LiveBook.Notebook do with {:ok, _, section} <- LiveBook.Notebook.fetch_cell_and_section(notebook, cell_id) do # A cell depends on all previous cells within the same section. section.cells - |> Enum.filter(&(&1.type == :elixir)) |> Enum.take_while(&(&1.id != cell_id)) |> Enum.reverse() + |> Enum.filter(&(&1.type == :elixir)) else _ -> [] end @@ -190,9 +190,9 @@ defmodule LiveBook.Notebook do with {:ok, _, section} <- LiveBook.Notebook.fetch_cell_and_section(notebook, cell_id) do # A cell affects all the cells below it within the same section. section.cells + |> Enum.drop_while(&(&1.id != cell_id)) + |> Enum.drop(1) |> Enum.filter(&(&1.type == :elixir)) - |> Enum.reverse() - |> Enum.take_while(&(&1.id != cell_id)) else _ -> [] end diff --git a/lib/live_book_web/live/cell.ex b/lib/live_book_web/live/cell.ex index 54072f8fb..9706c3144 100644 --- a/lib/live_book_web/live/cell.ex +++ b/lib/live_book_web/live/cell.ex @@ -154,7 +154,7 @@ defmodule LiveBookWeb.Cell do end defp render_output({:ok, value}) do - inspected = Utils.inspect_as_html(value, pretty: true, width: 140) + inspected = Utils.inspect_as_html(value, pretty: true, width: 100) assigns = %{inspected: inspected} diff --git a/lib/live_book_web/live/session_live.ex b/lib/live_book_web/live/session_live.ex index 854ac823d..c4f2b84b6 100644 --- a/lib/live_book_web/live/session_live.ex +++ b/lib/live_book_web/live/session_live.ex @@ -34,6 +34,7 @@ defmodule LiveBookWeb.SessionLive do data: data, selected_section_id: first_section_id, focused_cell_id: nil, + focused_cell_type: nil, focused_cell_expanded: false } end @@ -44,7 +45,8 @@ defmodule LiveBookWeb.SessionLive do
+ data-focused-cell-id="<%= @focused_cell_id %>" + data-focused-cell-type="<%= @focused_cell_type %>">

type}, socket) do + type = String.to_atom(type) + insert_cell_next_to_focused(socket.assigns, type, idx_offset: 1) + + {:noreply, socket} + end + + def handle_event("insert_cell_above_focused", %{"type" => type}, socket) do + type = String.to_atom(type) + insert_cell_next_to_focused(socket.assigns, type, idx_offset: 0) + + {:noreply, socket} + end + def handle_event("delete_cell", %{"cell_id" => cell_id}, socket) do Session.delete_cell(socket.assigns.session_id, cell_id) {:noreply, socket} end + def handle_event("delete_focused_cell", %{}, socket) do + if socket.assigns.focused_cell_id do + Session.delete_cell(socket.assigns.session_id, socket.assigns.focused_cell_id) + end + + {:noreply, socket} + end + def handle_event("set_notebook_name", %{"name" => name}, socket) do name = normalize_name(name) Session.set_notebook_name(socket.assigns.session_id, name) @@ -166,14 +190,24 @@ defmodule LiveBookWeb.SessionLive do {:noreply, socket} end + def handle_event("focus_cell", %{"cell_id" => nil}, socket) do + {:noreply, focus_cell(socket, nil)} + end + def handle_event("focus_cell", %{"cell_id" => cell_id}, socket) do - {:noreply, assign(socket, focused_cell_id: cell_id, focused_cell_expanded: false)} + case Notebook.fetch_cell_and_section(socket.assigns.data.notebook, cell_id) do + {:ok, cell, _section} -> + {:noreply, focus_cell(socket, cell)} + + :error -> + {:noreply, socket} + end end def handle_event("move_cell_focus", %{"offset" => offset}, socket) do case new_focused_cell_from_offset(socket.assigns, offset) do {:ok, cell} -> - {:noreply, assign(socket, focused_cell_id: cell.id, focused_cell_expanded: false)} + {:noreply, focus_cell(socket, cell)} :error -> {:noreply, socket} @@ -194,7 +228,7 @@ defmodule LiveBookWeb.SessionLive do {:noreply, socket} end - def handle_event("queue_cell_evaluation", %{}, socket) do + def handle_event("queue_focused_cell_evaluation", %{}, socket) do if socket.assigns.focused_cell_id do Session.queue_cell_evaluation(socket.assigns.session_id, socket.assigns.focused_cell_id) end @@ -202,20 +236,78 @@ defmodule LiveBookWeb.SessionLive do {:noreply, socket} end + def handle_event("queue_child_cells_evaluation", %{}, socket) do + if socket.assigns.focused_cell_id do + {:ok, cell, _section} = + Notebook.fetch_cell_and_section( + socket.assigns.data.notebook, + socket.assigns.focused_cell_id + ) + + cells = Notebook.child_cells(socket.assigns.data.notebook, cell.id) + + for cell <- [cell | cells], cell.type == :elixir do + Session.queue_cell_evaluation(socket.assigns.session_id, cell.id) + end + end + + {:noreply, socket} + end + @impl true def handle_info({:operation, operation}, socket) do case Session.Data.apply_operation(socket.assigns.data, operation) do {:ok, data, actions} -> - new_socket = assign(socket, data: data) - {:noreply, handle_actions(new_socket, actions)} + new_socket = + socket + |> assign(data: data) + |> after_operation(socket, operation) + |> handle_actions(actions) + + {:noreply, new_socket} :error -> {:noreply, socket} end end - defp handle_actions(state, actions) do - Enum.reduce(actions, state, &handle_action(&2, &1)) + defp after_operation(socket, _prev_socket, {:insert_section, _index, section_id}) do + assign(socket, selected_section_id: section_id) + end + + defp after_operation(socket, _prev_socket, {:delete_section, _section_id}) do + assign(socket, selected_section_id: nil) + end + + defp after_operation(socket, _prev_socket, {:insert_cell, _, _, _, cell_id}) do + {:ok, cell, _section} = Notebook.fetch_cell_and_section(socket.assigns.data.notebook, cell_id) + focus_cell(socket, cell, expanded: true) + end + + defp after_operation(socket, prev_socket, {:delete_cell, cell_id}) do + if cell_id == socket.assigns.focused_cell_id do + case Notebook.fetch_cell_sibling(prev_socket.assigns.data.notebook, cell_id, 1) do + {:ok, next_cell} -> + focus_cell(socket, next_cell) + + :error -> + case Notebook.fetch_cell_sibling(prev_socket.assigns.data.notebook, cell_id, -1) do + {:ok, previous_cell} -> + focus_cell(socket, previous_cell) + + :error -> + focus_cell(socket, nil) + end + end + else + socket + end + end + + defp after_operation(socket, _prev_socket, _operation), do: socket + + defp handle_actions(socket, actions) do + Enum.reduce(actions, socket, &handle_action(&2, &1)) end defp handle_action(socket, {:broadcast_delta, from, cell, delta}) do @@ -238,6 +330,44 @@ defmodule LiveBookWeb.SessionLive do end end + defp focus_cell(socket, cell, opts \\ []) + + defp focus_cell(socket, nil = _cell, _opts) do + assign(socket, focused_cell_id: nil, focused_cell_type: nil, focused_cell_expanded: false) + end + + defp focus_cell(socket, cell, opts) do + expanded? = Keyword.get(opts, :expanded, false) + + assign(socket, + focused_cell_id: cell.id, + focused_cell_type: cell.type, + focused_cell_expanded: expanded? + ) + end + + defp insert_cell_next_to_focused(assigns, type, idx_offset: idx_offset) do + if assigns.focused_cell_id do + {:ok, cell, section} = + Notebook.fetch_cell_and_section(assigns.data.notebook, assigns.focused_cell_id) + + index = Enum.find_index(section.cells, &(&1 == cell)) + Session.insert_cell(assigns.session_id, section.id, index + idx_offset, type) + else + append_cell_to_section(assigns, type) + end + end + + defp append_cell_to_section(assigns, type) do + if assigns.selected_section_id do + {:ok, section} = Notebook.fetch_section(assigns.data.notebook, assigns.selected_section_id) + + end_index = length(section.cells) + + Session.insert_cell(assigns.session_id, section.id, end_index, type) + end + end + defp new_focused_cell_from_offset(assigns, offset) do cond do assigns.focused_cell_id -> diff --git a/test/live_book_web/live/session_live_test.exs b/test/live_book_web/live/session_live_test.exs index e25289c35..90f5e7956 100644 --- a/test/live_book_web/live/session_live_test.exs +++ b/test/live_book_web/live/session_live_test.exs @@ -68,8 +68,8 @@ defmodule LiveBookWeb.SessionLiveTest do end end - describe "UI-triggered updates" do - test "adding a new session updates the shared state", %{conn: conn, session_id: session_id} do + describe "UI events update the shared state" do + test "adding a new section", %{conn: conn, session_id: session_id} do {:ok, view, _} = live(conn, "/sessions/#{session_id}") view @@ -79,7 +79,7 @@ defmodule LiveBookWeb.SessionLiveTest do assert %{notebook: %{sections: [_section]}} = Session.get_data(session_id) end - test "adding a new cell updates the shared state", %{conn: conn, session_id: session_id} do + test "adding a new cell", %{conn: conn, session_id: session_id} do Session.insert_section(session_id, 0) {:ok, view, _} = live(conn, "/sessions/#{session_id}") @@ -92,10 +92,9 @@ defmodule LiveBookWeb.SessionLiveTest do Session.get_data(session_id) end - test "queueing cell evaluation updates the shared state", - %{conn: conn, session_id: session_id} do + test "queueing cell evaluation", %{conn: conn, session_id: session_id} do section_id = insert_section(session_id) - cell_id = insert_cell(session_id, section_id, :elixir, "Process.sleep(5)") + cell_id = insert_cell(session_id, section_id, :elixir, "Process.sleep(10)") {:ok, view, _} = live(conn, "/sessions/#{session_id}") @@ -107,24 +106,68 @@ defmodule LiveBookWeb.SessionLiveTest do Session.get_data(session_id) end - test "queueing cell evaluation defaults to the focused cell if no cell id is given", - %{conn: conn, session_id: session_id} do + test "queueing focused cell evaluation", %{conn: conn, session_id: session_id} do section_id = insert_section(session_id) - cell_id = insert_cell(session_id, section_id, :elixir, "Process.sleep(5)") + cell_id = insert_cell(session_id, section_id, :elixir, "Process.sleep(10)") {:ok, view, _} = live(conn, "/sessions/#{session_id}") - view - |> element("#session") - |> render_hook("focus_cell", %{"cell_id" => cell_id}) + focus_cell(view, cell_id) view |> element("#session") - |> render_hook("queue_cell_evaluation", %{}) + |> render_hook("queue_focused_cell_evaluation", %{}) assert %{cell_infos: %{^cell_id => %{evaluation_status: :evaluating}}} = Session.get_data(session_id) end + + test "inserting a cell below the focused cell", %{conn: conn, session_id: session_id} do + section_id = insert_section(session_id) + cell_id = insert_cell(session_id, section_id, :elixir) + + {:ok, view, _} = live(conn, "/sessions/#{session_id}") + + focus_cell(view, cell_id) + + view + |> element("#session") + |> render_hook("insert_cell_below_focused", %{"type" => "markdown"}) + + assert %{notebook: %{sections: [%{cells: [_first_cell, %{type: :markdown}]}]}} = + Session.get_data(session_id) + end + + test "inserting a cell above the focused cell", %{conn: conn, session_id: session_id} do + section_id = insert_section(session_id) + cell_id = insert_cell(session_id, section_id, :elixir) + + {:ok, view, _} = live(conn, "/sessions/#{session_id}") + + focus_cell(view, cell_id) + + view + |> element("#session") + |> render_hook("insert_cell_above_focused", %{"type" => "markdown"}) + + assert %{notebook: %{sections: [%{cells: [%{type: :markdown}, _first_cell]}]}} = + Session.get_data(session_id) + end + + test "deleting the focused cell", %{conn: conn, session_id: session_id} do + section_id = insert_section(session_id) + cell_id = insert_cell(session_id, section_id, :elixir) + + {:ok, view, _} = live(conn, "/sessions/#{session_id}") + + focus_cell(view, cell_id) + + view + |> element("#session") + |> render_hook("delete_focused_cell", %{}) + + assert %{notebook: %{sections: [%{cells: []}]}} = Session.get_data(session_id) + end end defp wait_for_session_update(session_id) do @@ -152,4 +195,10 @@ defmodule LiveBookWeb.SessionLiveTest do cell.id end + + defp focus_cell(view, cell_id) do + view + |> element("#session") + |> render_hook("focus_cell", %{"cell_id" => cell_id}) + end end