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

@@ -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" });
     }
   },

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;

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

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}
 

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
     <div class="flex flex-grow max-h-full"
          id="session"
          phx-hook="Session"
-         data-focused-cell-id="<%= @focused_cell_id %>">
+         data-focused-cell-id="<%= @focused_cell_id %>"
+         data-focused-cell-type="<%= @focused_cell_type %>">
       <div class="w-1/5 bg-gray-100 border-r-2 border-gray-200">
         <h1 id="notebook-name"
             contenteditable
@@ -135,12 +137,34 @@ defmodule LiveBookWeb.SessionLive do
     {:noreply, socket}
   end
 
+  def handle_event("insert_cell_below_focused", %{"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 ->

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