fix(koreader): make the catalog navigable without a touchscreen

Kindle 3 and 4 drive the catalog entirely through KOReader's FocusManager, but none of the
plugin's own widgets answered a Focus event. The cursor moved invisibly, so Press looked
like it activated something at random.

Give every tappable widget a focus box. Draw it as an inner border: these pages lay
themselves out from fixed dimensions, and a cursor that changed a widget's size would
reflow the page under it. Menu marks its own rows with a hairline that upstream declines
to thicken because that shifts the row's text; the same inner border gives those rows a
visible cursor without that cost.

Keep the cursor across the rebuilds these pages do constantly. The dashboard and detail
pages replace their whole widget tree on a page turn, a reroll or a status change, and
Menu's bookkeeping cannot place a cursor on rows of unequal width. Remember the footer by
name rather than by row index: its controls are what cause the rebuild, so it has to
survive even a move to another book. Record a page's cursor on the navigation stack as it
is left, so returning lands where the reader was.

Reach what was unreachable. Menu skips the title bar on Kindle keypads because its own
title bar actions have hardware keys, but the search and refresh icons are this plugin's
and have none. The footer paginator and return arrow join the layout too; on the detail
page those chevrons step through books, which no key covers. Back walks up a level and
closes only at the root, the rule Menu already applies to its own stack. The detail header
walks vertically like the stack it draws, instead of eleven Right presses to reach
Download, and opens on the button the page was opened for.

Two of the fixes here are not specific to the D-Pad. The search and refresh icons were
missing from every section page for all users: TitleBar clears its children and re-inits
whenever a shrink-to-fit title changes, and a run-once guard stopped them being rebuilt.
And a footer chevron hidden while focused kept an inverted frame, painting as a black
block.

Closes #880

koreader-plugin/bookorbit.koplugin/bookorbit_catalog.lua

@@ -49,6 +49,7 @@ local CatalogDownload = require("bookorbit_catalog_download")
 local CatalogBulkDownload = require("bookorbit_catalog_bulk_download")
 local CatalogDashboard = require("bookorbit_catalog_dashboard")
 local CatalogDetail = require("bookorbit_catalog_detail")
+local CatalogFocus = require("bookorbit_catalog_focus")
 local CatalogThumbnails = require("bookorbit_catalog_thumbnails")
 
 local Screen = Device.screen
@@ -139,10 +140,28 @@ local BookOrbitTitleBar = TitleBar:extend{
     refresh_icon_enabled = true,
 }
 
+-- Drops any previously appended buttons from the OverlapGroup, so init can
+-- rebuild them without leaving a stale copy behind.
+function BookOrbitTitleBar:_clearExtraButtons()
+    for index = #self, 1, -1 do
+        if self[index] == self.search_button or self[index] == self.refresh_button then
+            table.remove(self, index)
+        end
+    end
+    self.search_button = nil
+    self.refresh_button = nil
+end
+
 function BookOrbitTitleBar:init()
     TitleBar.init(self)
-    if self._bookorbit_extra_buttons then return end
-    self._bookorbit_extra_buttons = true
+    -- These are appended after TitleBar has built its own children, so they only
+    -- survive as long as that child list does. TitleBar:setTitle re-inits the
+    -- whole bar when the title can shrink to fit - which this catalog asks for -
+    -- and clears the group on the way, taking the appended buttons with it.
+    -- Rebuilding them on every init is what keeps them on screen; skipping the
+    -- rebuild left the *fields* pointing at buttons that were no longer drawn,
+    -- so the D-Pad walked onto controls that were not there.
+    self:_clearExtraButtons()
 
     local icon_size = Screen:scaleBySize(DGENERIC_ICON_SIZE * (self.left_icon_size_ratio or 0.6))
     local button_padding = self.button_padding or 0
@@ -194,21 +213,24 @@ function BookOrbitTitleBar:init()
     OverlapGroup.init(self)
 end
 
+local TITLE_BAR_BUTTONS = { "left_button", "search_button", "refresh_button", "right_button" }
+-- The two this plugin appends itself, and so the two that can go missing.
+local TITLE_BAR_APPENDED = { search_button = true, refresh_button = true }
+
+function BookOrbitTitleBar:_drawnButtons()
+    return CatalogFocus.drawnTitleBarButtons(self, TITLE_BAR_BUTTONS, TITLE_BAR_APPENDED)
+end
+
 function BookOrbitTitleBar:generateHorizontalLayout()
-    local row = {}
-    if self.left_button then table.insert(row, self.left_button) end
-    if self.search_button then table.insert(row, self.search_button) end
-    if self.refresh_button then table.insert(row, self.refresh_button) end
-    if self.right_button then table.insert(row, self.right_button) end
+    local row = self:_drawnButtons()
     return #row > 0 and { row } or {}
 end
 
 function BookOrbitTitleBar:generateVerticalLayout()
     local layout = {}
-    if self.left_button then table.insert(layout, { self.left_button }) end
-    if self.search_button then table.insert(layout, { self.search_button }) end
-    if self.refresh_button then table.insert(layout, { self.refresh_button }) end
-    if self.right_button then table.insert(layout, { self.right_button }) end
+    for _, button in ipairs(self:_drawnButtons()) do
+        table.insert(layout, { button })
+    end
     return layout
 end
 
@@ -222,6 +244,7 @@ CatalogDownload.install(BookOrbitCatalog)
 CatalogBulkDownload.install(BookOrbitCatalog)
 CatalogDashboard.install(BookOrbitCatalog)
 CatalogDetail.install(BookOrbitCatalog)
+CatalogFocus.install(BookOrbitCatalog)
 CatalogThumbnails.install(BookOrbitCatalog)
 
 local Menu_recalculateDimen = Menu._recalculateDimen
@@ -231,6 +254,7 @@ local Menu_onNextPage = Menu.onNextPage
 local Menu_onPrevPage = Menu.onPrevPage
 local Menu_onFirstPage = Menu.onFirstPage
 local Menu_onLastPage = Menu.onLastPage
+local Menu_onClose = Menu.onClose
 
 function BookOrbitCatalog:init()
     self.client = BookOrbitApi.new(self.api)
@@ -739,6 +763,9 @@ function BookOrbitCatalog:switchTo(title, item_table, context, push)
             subtitle = self.current_context.subtitle,
             item_table = self.item_table,
             context = self.current_context,
+            -- Where the cursor was when this page was left, so returning to it
+            -- lands back on the row the reader was working in.
+            focus = self:captureFocus(),
         })
     end
     self:updateReturnPath()
@@ -1957,13 +1984,28 @@ function BookOrbitCatalog:updateItems(select_number, no_recalculate_dimen)
     elseif self:detailMode() then
         return self:updateDetailItems(select_number, no_recalculate_dimen)
     end
-    return Menu_updateItems(self, select_number, no_recalculate_dimen)
+    -- A plain Menu page does not go through prepareCustomUpdate, so drop the
+    -- remembered cursor here: coming back to the dashboard later must not
+    -- restore coordinates that belong to this list.
+    -- Menu builds its own layout and merges the title bar; the footer, and
+    -- putting the cursor back on it, are ours. saveCustomFocus also clears the
+    -- remembered page key, so returning to the dashboard later cannot restore
+    -- coordinates that belong to this list.
+    self:saveCustomFocus()
+    local result = Menu_updateItems(self, select_number, no_recalculate_dimen)
+    self:makeMenuItemsFocusable()
+    local footer_row = self:footerFocusRow()
+    if footer_row then table.insert(self.layout, footer_row) end
+    self:restoreCustomFocus()
+    self:refocusCurrentRow()
+    return result
 end
 
 function BookOrbitCatalog:prepareCustomUpdate(no_recalculate_dimen)
     local old_dimen = self.dimen and self.dimen:copy()
     local context = self.current_context or {}
     self:resetTitleBar(context.title or self.title, context.subtitle or "")
+    self:saveCustomFocus()
     self.layout = {}
     self.item_group:clear()
     self.page_info:resetLayout()
@@ -1975,7 +2017,11 @@ end
 
 function BookOrbitCatalog:finishCustomUpdate(old_dimen, select_number)
     self:updatePageInfo(select_number)
-    Menu.mergeTitleBarIntoLayout(self)
+    self:mergeTitleBarIntoLayout()
+    -- After the title bar, so the footer stays the last row the cursor reaches.
+    local footer_row = self:footerFocusRow()
+    if footer_row then table.insert(self.layout, footer_row) end
+    self:restoreCustomFocus()
     -- A view may narrow the e-ink refresh to the region it actually changed
     -- (the dashboard does for row page turns); everything else refreshes the
     -- whole menu as before.
@@ -2310,11 +2356,21 @@ function BookOrbitCatalog:onMenuHoldSelect(item)
     return self:onMenuSelect(item)
 end
 
+-- Back walks up the browsing stack on a device that cannot reach the footer's
+-- return arrow; see CatalogFocus:shouldReturnOnBack for why.
+function BookOrbitCatalog:onClose()
+    if self:shouldReturnOnBack() then
+        return self:onReturn()
+    end
+    return Menu_onClose(self)
+end
+
 function BookOrbitCatalog:onReturn()
     self:cancelThumbnailJobs()
     local previous = table.remove(self.stack)
     local dirty = false
     if previous then
+        self:restoreFocusOnNextUpdate(previous.focus)
         self.current_context = previous.context
         dirty = self.current_context.dirty == true
         self.current_context.dirty = nil

koreader-plugin/bookorbit.koplugin/bookorbit_catalog_dashboard.lua

@@ -468,6 +468,10 @@ function CatalogDashboard:addDashboardHeader(text, controls, focus_controls)
     self:addDashboardInset(CatalogWidgets.buildDashboardSectionHeader(text, self.content_w, right))
     local focus_row = focus_controls or controls
     if focus_row and #focus_row > 0 then
+        -- Paging chevrons and the reroll are chrome for the shelf below them, so
+        -- a D-Pad cursor opening the page skips past to the books themselves
+        -- rather than starting on a chevron that is disabled on page one.
+        focus_row.is_chrome = true
         table.insert(self.layout, focus_row)
     end
 end

koreader-plugin/bookorbit.koplugin/bookorbit_catalog_detail.lua

@@ -134,11 +134,11 @@ function DetailDescriptionWidget:init()
         }
     end
 
-    self[1] = HorizontalGroup:new{
+    self[1] = CatalogWidgets.focusable(HorizontalGroup:new{
         HorizontalSpan:new{ width = DETAIL_INSET },
         box,
         HorizontalSpan:new{ width = DETAIL_INSET },
-    }
+    })
 end
 
 function DetailDescriptionWidget:onTapSelect()
@@ -184,11 +184,11 @@ function DetailOverviewWidget:init()
         }
     end
 
-    self[1] = HorizontalGroup:new{
+    self[1] = CatalogWidgets.focusable(HorizontalGroup:new{
         HorizontalSpan:new{ width = DETAIL_INSET },
         box,
         HorizontalSpan:new{ width = DETAIL_INSET },
-    }
+    })
 end
 
 function DetailOverviewWidget:onTapSelect()
@@ -212,7 +212,7 @@ function DetailPillWidget:init()
     self.ges_events = {
         TapSelect = { GestureRange:new{ ges = "tap", range = dimen } },
     }
-    self[1] = pill
+    self[1] = CatalogWidgets.focusable(pill)
 end
 
 function DetailPillWidget:onTapSelect()
@@ -244,7 +244,7 @@ function DetailTapLineWidget:init()
     self.ges_events = {
         TapSelect = { GestureRange:new{ ges = "tap", range = dimen } },
     }
-    self[1] = box
+    self[1] = CatalogWidgets.focusable(box)
 end
 
 function DetailTapLineWidget:onTapSelect()
@@ -1169,6 +1169,7 @@ function CatalogDetail:buildDetailPillRow(items, width, title, more_button_key)
     local row = HorizontalGroup:new{ align = "center" }
     local used = 0
     local shown = 0
+    local first_pill
     if more_button_key then self[more_button_key] = nil end
 
     for i = 1, #items do
@@ -1199,6 +1200,7 @@ function CatalogDetail:buildDetailPillRow(items, width, title, more_button_key)
             used = used + DETAIL_GAP_S
         end
         table.insert(row, pill)
+        first_pill = first_pill or pill
         used = used + pill_w
         shown = shown + 1
     end
@@ -1218,6 +1220,13 @@ function CatalogDetail:buildDetailPillRow(items, width, title, more_button_key)
             if more_button_key then self[more_button_key] = more end
         end
     end
+    -- Every pill opens the same full list, so only one of them needs to be in the
+    -- focus layout, and the overflow "+N" pill is the natural pick. When nothing
+    -- overflowed there is no "+N", so fall back to the first pill: otherwise the
+    -- list has no reachable trigger at all on a device that cannot tap.
+    if more_button_key and not self[more_button_key] then
+        self[more_button_key] = first_pill
+    end
     return row
 end
 
@@ -1604,6 +1613,37 @@ function CatalogDetail:buildDetailRelatedSection(section, width, height)
     }, focus_rows
 end
 
+-- The header is a stack of lines on screen - series, metadata, rating, genres,
+-- progress, action - so the cursor moves through it a line at a time. Left and
+-- Right are reserved for what genuinely sits side by side, which is only the
+-- rating stars. Packing the lot into one row meant up to eleven Right presses
+-- to reach the download button, along an axis the layout never suggested.
+function CatalogDetail:detailHeaderFocusRows()
+    local rows = {}
+    local function addRow(...)
+        local row = {}
+        for index = 1, select("#", ...) do
+            local widget = select(index, ...)
+            if widget then table.insert(row, widget) end
+        end
+        if #row == 0 then return nil end
+        table.insert(rows, row)
+        return row
+    end
+
+    addRow(self.detail_series_line)
+    addRow(self.detail_meta_line)
+    addRow(unpack(self.detail_rating_stars or {}))
+    addRow(self.detail_more_pills_button)
+    addRow(self.detail_status_button)
+    -- Read and Download are the same slot: a book on the device shows one, a
+    -- book still on the server the other. It is also why the page was opened,
+    -- so the cursor starts here instead of five rows above it.
+    local action_row = addRow(self.detail_read_button, self.detail_download_button)
+    if action_row then action_row.is_primary = true end
+    return rows
+end
+
 function CatalogDetail:updateDetailItems(select_number, no_recalculate_dimen)
     local old_dimen = self:prepareCustomUpdate(no_recalculate_dimen)
     local detail = self.current_context.detail
@@ -1621,19 +1661,7 @@ function CatalogDetail:updateDetailItems(select_number, no_recalculate_dimen)
     table.insert(self.item_group, header)
     local used = DETAIL_GAP_S + header:getSize().h
 
-    local focus_row = {}
-    if self.detail_series_line then table.insert(focus_row, self.detail_series_line) end
-    if self.detail_meta_line then table.insert(focus_row, self.detail_meta_line) end
-    for _, star in ipairs(self.detail_rating_stars or {}) do
-        table.insert(focus_row, star)
-    end
-    if self.detail_more_pills_button then
-        table.insert(focus_row, self.detail_more_pills_button)
-    end
-    if self.detail_status_button then table.insert(focus_row, self.detail_status_button) end
-    if self.detail_read_button then table.insert(focus_row, self.detail_read_button) end
-    if self.detail_download_button then table.insert(focus_row, self.detail_download_button) end
-    self.layout = { focus_row }
+    self.layout = self:detailHeaderFocusRows()
 
     if active_related then
         local desc_max = DETAIL_DESCRIPTION_HEIGHT

koreader-plugin/bookorbit.koplugin/bookorbit_catalog_focus.lua

@@ -0,0 +1,303 @@
+--[[--
+D-Pad focus bookkeeping for the catalog's custom pages.
+
+A device with a D-Pad but no touchscreen (Kindle 3/4 and friends) drives the
+whole catalog through KOReader's FocusManager, which needs two things this
+plugin's pages cannot get from Menu alone: a cursor that survives a rebuild,
+and a reachable title bar. Both are installed onto the catalog widget by
+`install`, alongside the other catalog mixins.
+]]
+
+local Device = require("device")
+local Event = require("ui/event")
+local FocusManager = require("ui/widget/focusmanager")
+
+local CatalogWidgets = require("bookorbit_catalog_widgets")
+
+local CatalogFocus = {}
+
+local FOOTER_ROW_ID = "footer"
+
+--- Filters a title bar's buttons down to the ones it actually draws.
+---
+--- A focus layout may only name widgets that are on screen: a button reachable
+--- by the cursor but absent from the paint is worse than one that cannot be
+--- reached at all. TitleBar clears its own children and re-inits whenever a
+--- shrink-to-fit title changes, which drops any button appended afterwards while
+--- leaving the field pointing at it, so membership of the group is the only
+--- trustworthy test for those.
+---
+--- `order` lists the button fields left to right. `appended` names the ones added
+--- as direct children; TitleBar nests its own inside containers, so those are
+--- taken on trust.
+function CatalogFocus.drawnTitleBarButtons(bar, order, appended)
+    local drawn = {}
+    for _, child in ipairs(bar) do
+        drawn[child] = true
+    end
+    local buttons = {}
+    for _, field in ipairs(order) do
+        local button = bar[field]
+        if button and (not appended[field] or drawn[button]) then
+            table.insert(buttons, button)
+        end
+    end
+    return buttons
+end
+
+-- Identifies the page the cursor is currently on, so a rebuild can tell "the
+-- same page refreshed" from "a different page opened".
+function CatalogFocus:customFocusKey()
+    local context = self.current_context or {}
+    if context.kind == "dashboard" then
+        return "dashboard"
+    elseif context.kind == "detail" then
+        return "detail:" .. tostring(context.detail and context.detail.id)
+    end
+    return nil
+end
+
+-- The custom pages rebuild their whole widget tree on every refresh - a shelf
+-- page turn, a Discover reroll, a status change - which throws away the widget
+-- the cursor was sitting on. Menu's own bookkeeping cannot put it back: it
+-- derives a position from the selected item number over a uniform grid, and
+-- these pages are rows of unequal width whose first row is a cluster of header
+-- controls. Remember the coordinates before the layout is discarded.
+function CatalogFocus:saveCustomFocus()
+    self.custom_focus = nil
+    if not CatalogWidgets.focusNavigation() then return end
+    local key = self:customFocusKey()
+    -- Returning to a page hands back the cursor it was left at, whatever page
+    -- the reader wandered off to in between.
+    if self.pending_focus then
+        self.custom_focus = self.pending_focus
+        self.pending_focus = nil
+        self.custom_focus_key = key
+        return
+    end
+    local row_id = self:focusRowId()
+    -- A named row is remembered whatever page comes next. The footer is the same
+    -- strip of controls on every page, and its controls are what *cause* the
+    -- rebuild: pressing "next page" or "next book" from there has to leave the
+    -- cursor on the button just pressed, or the reader cannot press it twice.
+    if row_id or (key and key == self.custom_focus_key) then
+        self.custom_focus = { x = self.selected.x, y = self.selected.y, row_id = row_id }
+    end
+    self.custom_focus_key = key
+end
+
+-- The id of the row the cursor is in, when that row has one.
+function CatalogFocus:focusRowId()
+    local row = (self.layout or {})[self.selected.y]
+    return row and row.id
+end
+
+-- The cursor a page is being left at, stored on the navigation stack entry so
+-- returning to the page puts it back. Pages are pushed and popped together with
+-- their contexts, so this scopes and expires with them.
+function CatalogFocus:captureFocus()
+    if not CatalogWidgets.focusNavigation() then return nil end
+    return { x = self.selected.x, y = self.selected.y, row_id = self:focusRowId() }
+end
+
+-- Hands a remembered cursor to the next rebuild, used when popping the stack.
+function CatalogFocus:restoreFocusOnNextUpdate(focus)
+    if focus and CatalogWidgets.focusNavigation() then
+        self.pending_focus = focus
+    end
+end
+
+-- The row a cursor should land on when the page is opened rather than rebuilt.
+--
+-- A page may nominate the row it exists for - the detail page marks its Read or
+-- Download button - and the cursor opens there. Otherwise it takes the first row
+-- that is neither a merged title bar row nor a section header's controls, and
+-- falls back to the first row when a page is nothing but chrome.
+function CatalogFocus:firstContentFocusRow()
+    local layout = self.layout or {}
+    local start = (self.title_bar_layout_rows or 0) + 1
+    for index = start, #layout do
+        if layout[index].is_primary then return index end
+    end
+    local y = start
+    while layout[y] and layout[y].is_chrome do
+        y = y + 1
+    end
+    if not layout[y] then
+        return math.min((self.title_bar_layout_rows or 0) + 1, math.max(1, #layout))
+    end
+    return y
+end
+
+-- Puts the cursor back, clamped into the layout that just replaced the old
+-- one: a shelf that lost a row, or a header that lost its paging chevrons,
+-- leaves the remembered coordinates pointing past the end.
+function CatalogFocus:restoreCustomFocus()
+    if not CatalogWidgets.focusNavigation() then return end
+    local layout = self.layout
+    if not layout or #layout == 0 then return end
+    local target = self.custom_focus
+    self.custom_focus = nil
+
+    local function focusAt(x, y)
+        y = math.max(1, math.min(y, #layout))
+        while y > 1 and #layout[y] == 0 do
+            y = y - 1
+        end
+        if #layout[y] == 0 then return end
+        self:moveFocusTo(math.max(1, math.min(x, #layout[y])), y,
+            FocusManager.FOCUS_ONLY_ON_NT)
+    end
+
+    -- A cursor we remembered is honoured on every page, including the mosaic and
+    -- list grids we otherwise leave to Menu: a named row is found again wherever
+    -- it landed this time, and anything else falls back to its coordinates.
+    if target then
+        if target.row_id then
+            for index = 1, #layout do
+                if layout[index].id == target.row_id and #layout[index] > 0 then
+                    return focusAt(target.x, index)
+                end
+            end
+        end
+        return focusAt(target.x, target.y)
+    end
+
+    -- Nothing remembered, so this page is being opened rather than rebuilt. Only
+    -- the pages we own pick their own opening row; the uniform grids are the
+    -- ones Menu's own bookkeeping was written for.
+    if not self.custom_focus_key then return end
+    focusAt(1, self:firstContentFocusRow())
+end
+
+-- Menu skips the title bar on Kindle-style keypads because its own title bar
+-- actions all have hardware keys. Ours do not: the search and refresh icons are
+-- this plugin's, with no key mapping behind them, so skipping would strand them
+-- on exactly the devices that cannot tap. Kindle 3/4 have a full 5-way pad, so
+-- the buttons go in as one row and cost a single Down press to leave.
+--
+-- The D-Pad check is ours too: a focus layout is only ever walked by a D-Pad
+-- (FocusManager binds no keys without one), so on a device that has none these
+-- rows would be dead entries that only shift the cursor's row index.
+function CatalogFocus:mergeTitleBarIntoLayout()
+    self.title_bar_layout_rows = 0
+    if not self.title_bar or not Device:hasDPad() then return end
+    -- A few-keys device spends "Right" on the context menu, so its title bar
+    -- has to stack vertically to stay reachable.
+    local title_bar_layout = Device:hasFewKeys()
+        and self.title_bar:generateVerticalLayout()
+        or self.title_bar:generateHorizontalLayout()
+    for index, row in ipairs(title_bar_layout) do
+        table.insert(self.layout, index, row)
+    end
+    self.title_bar_layout_rows = #title_bar_layout
+    self.selected.y = self.selected.y + self.title_bar_layout_rows
+end
+
+-- The footer's paginator and return arrow are Buttons that Menu never puts in
+-- the focus layout, on the assumption that the hardware page-turn and Back keys
+-- cover them. They mostly do, but those keys are easy to miss, and on the detail
+-- page the same chevrons step through *books* rather than pages, which no key
+-- covers at all.
+--
+-- So append whatever the footer is currently showing as the last row. It is
+-- marked chrome, so a page opened fresh still starts on its content, and only
+-- visible controls go in: the plugin hides the chevrons outright on pages that
+-- do not page.
+function CatalogFocus:footerFocusRow()
+    if not CatalogWidgets.focusNavigation() then return nil end
+    -- Screen order: the return arrow sits bottom-left, the paginator centred.
+    local candidates = {
+        self.page_return_arrow,
+        self.page_info_first_chev,
+        self.page_info_left_chev,
+        self.page_info_text,
+        self.page_info_right_chev,
+        self.page_info_last_chev,
+    }
+    local row, included = {}, {}
+    for _, widget in ipairs(candidates) do
+        local shown = not widget.hidden
+        -- The page indicator is a button in its own right - it opens "go to
+        -- page" - but it is emptied rather than hidden when there is nothing to
+        -- page through.
+        if widget == self.page_info_text then
+            shown = (widget.text or "") ~= ""
+        end
+        if shown then
+            table.insert(row, widget)
+            included[widget] = true
+        end
+    end
+
+    -- A control that has just been hidden has to be told it is no longer
+    -- focused. It is not in the layout any more, so no Unfocus would reach it,
+    -- and a hidden Button still paints its frame: left inverted, it shows up as
+    -- a black block where the chevron used to be.
+    for _, widget in ipairs(candidates) do
+        if not included[widget] then
+            widget:handleEvent(Event:new("Unfocus"))
+        end
+    end
+
+    if #row == 0 then return nil end
+    row.is_chrome = true
+    -- Named, so the cursor can be put back on it after a rebuild without relying
+    -- on a row index that shifts with however much content the page has.
+    row.id = FOOTER_ROW_ID
+    return row
+end
+
+-- The Series, Authors, Libraries and Collections pages are drawn by plain Menu,
+-- whose rows mark focus with a hairline that is invisible in a list of rows that
+-- already have separators. Give them the catalog's own focus box.
+--
+-- Menu has already placed the cursor by the time this runs, so the box has to be
+-- re-asserted afterwards or the row the page opens on stays unmarked until the
+-- reader moves.
+function CatalogFocus:makeMenuItemsFocusable()
+    if not CatalogWidgets.focusNavigation() then return end
+    for _, row in ipairs(self.layout or {}) do
+        for _, widget in ipairs(row) do
+            -- The underline container is what identifies a Menu row; the title
+            -- bar's buttons and our own widgets bring their own focus styling.
+            if type(widget) == "table" and widget._underline_container then
+                CatalogWidgets.focusableMenuItem(widget)
+            end
+        end
+    end
+end
+
+-- Redraws the focus box on whatever is currently selected.
+function CatalogFocus:refocusCurrentRow()
+    if not CatalogWidgets.focusNavigation() then return end
+    local layout = self.layout
+    if not layout or not layout[self.selected.y] then return end
+    if not layout[self.selected.y][self.selected.x] then return end
+    self:moveFocusTo(self.selected.x, self.selected.y, FocusManager.FOCUS_ONLY_ON_NT)
+end
+
+-- Whether the Back key should walk up a level rather than close the catalog.
+--
+-- Menu closes on Back because its own back affordance is the footer's return
+-- arrow, and that arrow is a Button it never puts in the focus layout: a device
+-- with no touchscreen cannot press it, leaving Back as the only key here and
+-- closing the whole catalog from however deep the reader had browsed. Menu
+-- already has the right rule for its own stack - go up a level, close only at
+-- the root - so we apply it to ours.
+--
+-- Touch devices keep closing on Back: they can still tap the arrow, and a
+-- multiswipe to close should not turn into one swipe per level.
+function CatalogFocus:shouldReturnOnBack()
+    return CatalogWidgets.focusNavigation() and #(self.stack or {}) > 0
+end
+
+function CatalogFocus.install(Catalog)
+    for name, fn in pairs(CatalogFocus) do
+        if name ~= "install" then
+            Catalog[name] = fn
+        end
+    end
+end
+
+return CatalogFocus

koreader-plugin/bookorbit.koplugin/bookorbit_catalog_widgets.lua

@@ -60,6 +60,55 @@ local function lineHeight(face)
     return math.floor(1.3 * face.size + 0.5)
 end
 
+-- A device with a D-Pad but no touchscreen (Kindle 3/4 and friends) drives the
+-- catalog entirely through FocusManager, so every tappable widget has to be
+-- able to draw the cursor that says where the next Press will land. Touch
+-- devices never show it, so they never pay for the extra frame.
+local FOCUS_NAV = Device:hasDPad() and not Device:isTouchDevice()
+local FOCUS_BORDER = Screen:scaleBySize(3)
+
+--- Wraps a widget's root container in a frame that paints a focus box while the
+--- FocusManager has it selected. The box is an *inner* border, drawn within the
+--- frame's own bounds, so the widget measures the same focused or not: the
+--- dashboard and detail pages lay themselves out from fixed dimensions, and a
+--- cursor that changed a widget's size would reflow the page under it.
+function CatalogWidgets.focusable(child)
+    if not FOCUS_NAV then return child end
+    return FrameContainer:new{
+        margin = 0,
+        padding = 0,
+        bordersize = 0,
+        focusable = true,
+        focus_inner_border = true,
+        focus_border_size = FOCUS_BORDER,
+        child,
+    }
+end
+
+--- Whether the running device navigates by focus rather than by touch. Callers
+--- use it to skip focus bookkeeping that would be dead weight on a touchscreen.
+function CatalogWidgets.focusNavigation()
+    return FOCUS_NAV
+end
+
+--- Gives a stock KOReader menu row the same focus box as the widgets above.
+---
+--- Menu marks its focused row with a hairline underneath it. Upstream calls that
+--- line "really, really, really thin" and declines to thicken it because doing
+--- so shifts the row's text; on a list of rows that already have separators it
+--- is not a cursor a reader can find. An inner border is as visible as the boxes
+--- everywhere else in the catalog and changes no geometry, so it does not carry
+--- that cost. Applied to the item's own frame, which answers the Focus event
+--- before the row does and so replaces the underline rather than adding to it.
+function CatalogWidgets.focusableMenuItem(item)
+    if not FOCUS_NAV then return end
+    local frame = item and item[1]
+    if type(frame) ~= "table" or frame.focusable then return end
+    frame.focusable = true
+    frame.focus_inner_border = true
+    frame.focus_border_size = FOCUS_BORDER
+end
+
 -- Absolute paths to the plugin's own SVG icons, resolved once by the catalog
 -- (widgets cannot know the plugin directory). A missing entry falls back to
 -- the stock KOReader glyph named alongside it.
@@ -427,7 +476,7 @@ function MosaicItem:init()
         dimen = Geom:new{ w = self.dimen.w, h = self.dimen.h },
         content,
     }
-    self[1] = FrameContainer:new{
+    self[1] = CatalogWidgets.focusable(FrameContainer:new{
         width = self.dimen.w,
         height = self.dimen.h,
         margin = 0,
@@ -435,7 +484,7 @@ function MosaicItem:init()
         bordersize = 0,
         background = selectedBackground(selected),
         body,
-    }
+    })
 end
 
 function MosaicItem:onTapSelect()
@@ -587,7 +636,7 @@ function ListItem:init()
         dimen = Geom:new{ w = self.dimen.w, h = separator_h },
     })
 
-    self[1] = FrameContainer:new{
+    self[1] = CatalogWidgets.focusable(FrameContainer:new{
         width = self.dimen.w,
         height = self.dimen.h,
         margin = 0,
@@ -595,7 +644,7 @@ function ListItem:init()
         bordersize = 0,
         background = selectedBackground(selected),
         content,
-    }
+    })
 end
 
 function ListItem:onTapSelect()
@@ -1024,10 +1073,10 @@ function DashboardCoverCard:init()
         })
     end
 
-    self[1] = cardFrame(self.dimen.w, self.dimen.h, pad, CenterContainer:new{
+    self[1] = CatalogWidgets.focusable(cardFrame(self.dimen.w, self.dimen.h, pad, CenterContainer:new{
         dimen = Geom:new{ w = inner_w, h = inner_h },
         col,
-    })
+    }))
 end
 
 function DashboardCoverCard:onTapSelect()
@@ -1119,12 +1168,12 @@ function DashboardHeroCard:init()
     table.insert(right, VerticalSpan:new{ width = flex })
     table.insert(right, bottom)
 
-    self[1] = cardFrame(self.dimen.w, self.dimen.h, pad, HorizontalGroup:new{
+    self[1] = CatalogWidgets.focusable(cardFrame(self.dimen.w, self.dimen.h, pad, HorizontalGroup:new{
         align = "top",
         CatalogWidgets.buildDashboardCoverWidget(book, cover_w, cover_h, path, state, downloaded),
         HorizontalSpan:new{ width = gap },
         right,
-    })
+    }))
 end
 
 function DashboardHeroCard:onTapSelect()
@@ -1204,10 +1253,10 @@ function DashboardHighlightCard:init()
     local pad = Size.padding.default
     local inner_w = math.max(1, self.dimen.w - 2 * pad - 2 * CARD_BORDER)
     local inner_h = math.max(1, self.dimen.h - 2 * pad - 2 * CARD_BORDER)
-    self[1] = cardFrame(self.dimen.w, self.dimen.h, pad, LeftContainer:new{
+    self[1] = CatalogWidgets.focusable(cardFrame(self.dimen.w, self.dimen.h, pad, LeftContainer:new{
         dimen = Geom:new{ w = inner_w, h = inner_h },
         CatalogWidgets.buildDashboardHighlightContent(self.entry.highlight, inner_w),
-    })
+    }))
 end
 
 function DashboardHighlightCard:onTapSelect()
@@ -1245,11 +1294,11 @@ function DetailRelatedCard:init()
     local path, state = self.menu:thumbnailDisplay(book)
     local downloaded = self.menu:isOnDevice(book)
 
-    self[1] = cardFrame(self.dimen.w, self.dimen.h, pad, CenterContainer:new{
+    self[1] = CatalogWidgets.focusable(cardFrame(self.dimen.w, self.dimen.h, pad, CenterContainer:new{
         dimen = Geom:new{ w = inner_w, h = inner_h },
         CatalogWidgets.buildDashboardCoverWidget(
             book, inner_w, inner_h, path, state, downloaded, { no_border = true }),
-    })
+    }))
 end
 
 function DetailRelatedCard:onTapSelect()
@@ -1308,7 +1357,7 @@ function DetailTabButton:init()
         table.insert(col, VerticalSpan:new{ width = underline_h })
     end
 
-    self[1] = FrameContainer:new{
+    self[1] = CatalogWidgets.focusable(FrameContainer:new{
         width = w,
         height = h,
         margin = 0,
@@ -1316,7 +1365,7 @@ function DetailTabButton:init()
         bordersize = 0,
         background = Blitbuffer.COLOR_WHITE,
         col,
-    }
+    })
 end
 
 function DetailTabButton:onTapSelect()
@@ -1357,7 +1406,7 @@ function DetailRatingStar:init()
     }
 
     local text = self.entry.filled and "★" or "☆"
-    self[1] = CenterContainer:new{
+    self[1] = CatalogWidgets.focusable(CenterContainer:new{
         dimen = Geom:new{ w = self.dimen.w, h = self.dimen.h },
         TextBoxWidget:new{
             text = text,
@@ -1368,7 +1417,7 @@ function DetailRatingStar:init()
             face = Font:getFace("cfont", DETAIL_STAR_FONT_SIZE),
             height_overflow_show_ellipsis = true,
         },
-    }
+    })
 end
 
 function DetailRatingStar:onTapSelect()
@@ -1455,14 +1504,14 @@ function DashboardBrowseRow:init()
         })
     end
 
-    self[1] = VerticalGroup:new{
+    self[1] = CatalogWidgets.focusable(VerticalGroup:new{
         align = "left",
         row,
         LineWidget:new{
             background = Blitbuffer.COLOR_LIGHT_GRAY,
             dimen = Geom:new{ w = self.dimen.w, h = separator_h },
         },
-    }
+    })
 end
 
 function DashboardBrowseRow:onTapSelect()
@@ -1515,10 +1564,10 @@ function DashboardIconButton:init()
     icon_opts.width = icon_size
     icon_opts.height = icon_size
     icon_opts.dim = not self.enabled
-    self[1] = CenterContainer:new{
+    self[1] = CatalogWidgets.focusable(CenterContainer:new{
         dimen = Geom:new{ w = self.dimen.w, h = self.dimen.h },
         IconWidget:new(icon_opts),
-    }
+    })
 end
 
 function DashboardIconButton:onTapSelect()

koreader-plugin/bookorbit.koplugin/main.lua

@@ -48,7 +48,7 @@ local BookOrbitProgressSync = require("bookorbit_progress_sync")
 local BookOrbitSweep = require("bookorbit_sweep")
 local BookOrbitUpdater = require("bookorbit_updater")
 
-local PLUGIN_VERSION = "1.4.0"
+local PLUGIN_VERSION = "1.5.0"
 
 local SYNC_STRATEGY = {
     PROMPT = 1,

koreader-plugin/spec/bookorbit_detail_focus_test.lua

@@ -0,0 +1,162 @@
+-- D-Pad reachability on the book detail page: that its header walks vertically
+-- like the stack it draws, and that every pill row keeps one reachable trigger.
+--
+-- Every pill in a detail row opens the same full list, so exactly one of them is
+-- registered for D-Pad focus. The overflow "+N" pill is the natural pick, but a
+-- row short enough to fit has no "+N" - and without a fallback the list becomes
+-- unreachable on a device that cannot tap a pill.
+
+package.path = "koreader-plugin/bookorbit.koplugin/?.lua;koreader-plugin/spec/?.lua;" .. package.path
+
+local function identity(value) return value end
+
+local function stubWidget(width)
+    local widget = {}
+    widget.__index = widget
+    -- Mirrors KOReader's own Widget:new, which runs init(): a pill derives its
+    -- size there, and without it every pill measures zero and nothing overflows.
+    function widget:new(opts)
+        local instance = setmetatable(opts or {}, self)
+        if instance.init then instance:init() end
+        return instance
+    end
+    function widget:extend(opts)
+        local child = setmetatable(opts or {}, { __index = self })
+        child.__index = child
+        child.new = widget.new
+        child.extend = widget.extend
+        return child
+    end
+    -- A pill widget sizes itself from the dimen it derived in init, so honour
+    -- that first; otherwise fall back to the stub's own fixed width.
+    function widget:getSize()
+        if self.dimen then return self.dimen end
+        return { w = self.width or width or 0, h = 0 }
+    end
+    return widget
+end
+
+package.loaded["ui/bidi"] = { auto = identity, dirpath = identity, filepath = identity }
+package.loaded["ffi/blitbuffer"] = { COLOR_GRAY = 0 }
+for _, name in ipairs({
+    "ui/widget/button", "ui/widget/buttondialog", "ui/widget/container/centercontainer",
+    "ui/widget/container/leftcontainer", "ui/widget/container/inputcontainer",
+    "ui/widget/horizontalgroup", "ui/widget/horizontalspan", "ui/widget/verticalgroup",
+    "ui/widget/verticalspan", "ui/widget/linewidget", "ui/widget/textboxwidget",
+    "ui/widget/textviewer", "ui/widget/infomessage", "ui/widget/keyvaluepage",
+    "ui/geometry", "ui/gesturerange",
+}) do
+    package.loaded[name] = stubWidget()
+end
+package.loaded["ui/font"] = { getFace = function() return { size = 12 } end }
+package.loaded["ui/size"] = { line = { medium = 1 } }
+package.loaded["ui/network/manager"] = { isConnected = function() return true end }
+package.loaded["device"] = { screen = { scaleBySize = function(_, value) return value end } }
+package.loaded["ui/uimanager"] = { show = function() end, close = function() end }
+package.loaded["document/documentregistry"] = { hasProvider = function() return true end }
+package.loaded["util"] = {
+    trim = function(value) return tostring(value or ""):match("^%s*(.-)%s*$") end,
+    fixUtf8 = identity,
+    htmlToPlainTextIfHtml = identity,
+}
+package.loaded["ffi/util"] = {
+    template = function(pattern, a) return (pattern:gsub("%%1", tostring(a))) end,
+}
+package.loaded["gettext"] = setmetatable({}, { __call = function(_, value) return value end })
+
+-- Each pill reports a fixed width so overflow can be forced by narrowing the row.
+local PILL_W = 100
+package.loaded["bookorbit_catalog_widgets"] = {
+    buildCoverWidget = function() return stubWidget(PILL_W):new() end,
+    buildDetailPill = function() return stubWidget(PILL_W):new{ width = PILL_W } end,
+    buildDetailProgressBar = function() return stubWidget():new() end,
+    detailRatingStarWidth = function() return 20 end,
+    DetailRelatedCard = stubWidget(),
+    DetailTabButton = stubWidget(),
+    DetailRatingStar = stubWidget(),
+    focusable = identity,
+    focusNavigation = function() return true end,
+}
+
+local CatalogDetail = require("bookorbit_catalog_detail")
+
+local function assertEqual(actual, expected, label)
+    if actual ~= expected then
+        error(string.format("%s: expected %s, got %s", label, tostring(expected), tostring(actual)), 2)
+    end
+end
+
+local function buildRow(items, width)
+    local page = {}
+    page.showFullList = function() end
+    page.buildDetailPillRow = CatalogDetail.buildDetailPillRow
+    page:buildDetailPillRow(items, width, "Genres", "focus_pill")
+    return page
+end
+
+-- A row wide enough for every pill has no overflow button, so the first pill
+-- carries the focus instead of nothing at all.
+local page = buildRow({ "NOVELS" }, 1000)
+assertEqual(page.focus_pill ~= nil, true, "a single pill is reachable by focus")
+assertEqual(page.focus_pill.text, "NOVELS", "and it is the pill that was drawn")
+
+page = buildRow({ "NOVELS", "FICTION", "CLASSICS" }, 1000)
+assertEqual(page.focus_pill ~= nil, true, "a row that fits entirely is still reachable")
+assertEqual(page.focus_pill.text, "NOVELS", "via its first pill")
+
+-- A row too narrow overflows, and the "+N" pill keeps the focus: it is the one
+-- that says how much is hidden.
+page = buildRow({ "NOVELS", "FICTION", "CLASSICS", "ESSAYS" }, 250)
+assertEqual(page.focus_pill ~= nil, true, "an overflowing row is reachable")
+assertEqual(page.focus_pill.text:match("^%+") ~= nil, true,
+    "via its overflow pill, not the first one")
+
+-- No pills at all means nothing to reach, and no stale button left behind.
+page = {}
+page.showFullList = function() end
+page.focus_pill = "stale"
+page.buildDetailPillRow = CatalogDetail.buildDetailPillRow
+local row = page:buildDetailPillRow({}, 1000, "Genres", "focus_pill")
+assertEqual(row, nil, "an empty list builds no row")
+
+
+-- The header is a vertical stack on screen, so the cursor walks it with Up and
+-- Down. Only the rating stars share a row, because only they sit side by side.
+local function rows(page)
+    page.detailHeaderFocusRows = CatalogDetail.detailHeaderFocusRows
+    return page:detailHeaderFocusRows()
+end
+
+local full = rows{
+    detail_series_line = "series",
+    detail_meta_line = "meta",
+    detail_rating_stars = { "s1", "s2", "s3", "s4", "s5" },
+    detail_more_pills_button = "pill",
+    detail_status_button = "status",
+    detail_download_button = "download",
+}
+assertEqual(#full, 6, "each header line gets its own focus row")
+assertEqual(full[1][1], "series", "series line first")
+assertEqual(full[2][1], "meta", "then the metadata line")
+assertEqual(#full[3], 5, "the rating stars share one row")
+assertEqual(full[4][1], "pill", "then the genre pill")
+assertEqual(full[5][1], "status", "then the status button")
+assertEqual(full[6][1], "download", "and the action button last")
+for _, row in ipairs(full) do
+    if row ~= full[3] then
+        assertEqual(#row, 1, "every row but the stars holds a single control")
+    end
+end
+
+-- Read and Download are the same slot, never both.
+local on_device = rows{ detail_meta_line = "meta", detail_read_button = "read" }
+assertEqual(#on_device, 2, "a book on the device drops the rows it has no controls for")
+assertEqual(on_device[2][1], "read", "and offers Read in the action slot")
+
+-- A sparse page must not leave empty rows for the cursor to fall into.
+local sparse = rows{ detail_download_button = "download" }
+assertEqual(#sparse, 1, "a header with one control builds one row")
+assertEqual(sparse[1][1], "download", "holding that control")
+assertEqual(#rows{}, 0, "a header with no controls builds no rows")
+
+print("bookorbit_detail_focus_test.lua: ok")

koreader-plugin/spec/bookorbit_focus_highlight_test.lua

@@ -0,0 +1,125 @@
+-- Proves the focus chrome the catalog's tappable widgets draw for a D-Pad
+-- cursor: that it exists at all on a keyboard-only device, that it is an inner
+-- border so adding it cannot resize a widget and reflow the page around it,
+-- that a touchscreen pays nothing for it, and that no tappable widget is left
+-- without it.
+
+local function stubWidget()
+    local class = {}
+    class.new = function(_, opts) return opts or {} end
+    class.extend = function(_, spec)
+        spec = spec or {}
+        spec.new = class.new
+        spec.extend = class.extend
+        return spec
+    end
+    return class
+end
+
+local function anyNumber()
+    return setmetatable({}, { __index = function() return 1 end })
+end
+
+package.loaded["ui/bidi"] = { auto = function(text) return text end }
+package.loaded["ffi/blitbuffer"] = setmetatable({}, { __index = function() return 0 end })
+package.loaded["ui/font"] = { getFace = function() return { size = 10 } end }
+package.loaded["ui/geometry"] = stubWidget()
+package.loaded["ui/gesturerange"] = stubWidget()
+package.loaded["ui/size"] = setmetatable({}, { __index = function() return anyNumber() end })
+package.loaded["gettext"] = function(text) return text end
+package.loaded["util"] = { fixUtf8 = function(value) return value end }
+package.loaded["ffi/util"] = { template = function(value) return value end }
+package.loaded["document/documentregistry"] = { hasProvider = function() return true end }
+
+for _, name in ipairs({
+    "ui/widget/container/centercontainer",
+    "ui/widget/container/framecontainer",
+    "ui/widget/container/inputcontainer",
+    "ui/widget/container/leftcontainer",
+    "ui/widget/container/rightcontainer",
+    "ui/widget/horizontalgroup",
+    "ui/widget/horizontalspan",
+    "ui/widget/iconwidget",
+    "ui/widget/imagewidget",
+    "ui/widget/linewidget",
+    "ui/widget/overlapgroup",
+    "ui/widget/progresswidget",
+    "ui/widget/textboxwidget",
+    "ui/widget/textwidget",
+    "ui/widget/verticalgroup",
+    "ui/widget/verticalspan",
+}) do
+    package.loaded[name] = stubWidget()
+end
+
+local touch = false
+package.loaded["device"] = {
+    screen = { scaleBySize = function(_, value) return value end },
+    hasDPad = function() return true end,
+    isTouchDevice = function() return touch end,
+}
+
+package.path = "koreader-plugin/bookorbit.koplugin/?.lua;" .. package.path
+
+local function loadWidgets()
+    package.loaded["bookorbit_catalog_widgets"] = nil
+    package.loaded["bookorbit_catalog_util"] = nil
+    return require("bookorbit_catalog_widgets")
+end
+
+local function assertEqual(actual, expected, label)
+    if actual ~= expected then
+        error(string.format("%s: expected %s, got %s", label, tostring(expected), tostring(actual)))
+    end
+end
+
+-- A D-Pad with no touchscreen is the case that has to draw a cursor.
+local CatalogWidgets = loadWidgets()
+assertEqual(CatalogWidgets.focusNavigation(), true, "a keyboard-only device navigates by focus")
+
+local child = { "content" }
+local frame = CatalogWidgets.focusable(child)
+assertEqual(frame ~= child, true, "the widget's root is wrapped in a focus frame")
+assertEqual(frame[1], child, "the frame carries the original root untouched")
+assertEqual(frame.focusable, true, "the frame answers Focus events")
+assertEqual(frame.focus_inner_border, true, "the cursor is drawn inside the frame's own bounds")
+assertEqual(frame.focus_border_size > 0, true, "with a visible border width")
+-- The inner border is the whole point: the dashboard and detail pages size
+-- themselves from fixed dimensions, so a cursor that grew a widget would push
+-- the rest of the page around every time it moved.
+assertEqual(frame.bordersize, 0, "the resting frame adds no border of its own")
+assertEqual(frame.padding, 0, "nor any padding")
+assertEqual(frame.margin, 0, "nor any margin")
+
+-- A touchscreen never shows the cursor, so it should not pay for the frame.
+touch = true
+CatalogWidgets = loadWidgets()
+assertEqual(CatalogWidgets.focusNavigation(), false, "a touch device does not navigate by focus")
+assertEqual(CatalogWidgets.focusable(child), child, "and gets its root back unwrapped")
+touch = false
+
+-- Every tappable widget in these two files sets its root as `self[1]`, and each
+-- one has to route through the helper: a new widget added without it would be
+-- invisible to a D-Pad user, which is exactly the bug this fixes.
+local function read(path)
+    local file = assert(io.open(path, "rb"))
+    local content = file:read("*a")
+    file:close()
+    return content
+end
+
+local plugin_dir = "koreader-plugin/bookorbit.koplugin/"
+for _, name in ipairs({ "bookorbit_catalog_widgets.lua", "bookorbit_catalog_detail.lua" }) do
+    local source = read(plugin_dir .. name)
+    local roots, wrapped = 0, 0
+    for assignment in source:gmatch("self%[1%] = ([%w_.]+)") do
+        roots = roots + 1
+        if assignment == "CatalogWidgets.focusable" then
+            wrapped = wrapped + 1
+        end
+    end
+    assertEqual(roots > 0, true, name .. " defines widget roots")
+    assertEqual(wrapped, roots, "every widget root in " .. name .. " draws a focus cursor")
+end
+
+print("bookorbit_focus_highlight_test.lua: ok")

koreader-plugin/spec/bookorbit_focus_navigation_test.lua

@@ -0,0 +1,516 @@
+-- Proves the D-Pad bookkeeping the custom catalog pages need on a device with
+-- no touchscreen: that the title bar joins the focus layout on the Kindle
+-- keypads Menu skips, and that the cursor survives a rebuild instead of being
+-- teleported by Menu's uniform-grid focus math.
+
+local device = {
+    hasDPad = function() return true end,
+    hasFewKeys = function() return false end,
+}
+package.loaded["device"] = device
+package.loaded["ui/widget/focusmanager"] = { FOCUS_ONLY_ON_NT = 0 }
+package.loaded["ui/event"] = {
+    new = function(_, name, ...) return { handler = "on" .. name, name, ... } end,
+}
+
+local focus_navigation = true
+package.loaded["bookorbit_catalog_widgets"] = {
+    focusNavigation = function() return focus_navigation end,
+}
+
+package.path = "koreader-plugin/bookorbit.koplugin/?.lua;" .. package.path
+
+local CatalogFocus = require("bookorbit_catalog_focus")
+
+local function assertEqual(actual, expected, label)
+    if actual ~= expected then
+        error(string.format("%s: expected %s, got %s", label, tostring(expected), tostring(actual)))
+    end
+end
+
+local function titleBar(buttons)
+    return {
+        generateHorizontalLayout = function()
+            return #buttons > 0 and { buttons } or {}
+        end,
+        generateVerticalLayout = function()
+            local rows = {}
+            for _, button in ipairs(buttons) do
+                table.insert(rows, { button })
+            end
+            return rows
+        end,
+    }
+end
+
+local function newCatalog(opts)
+    opts = opts or {}
+    local catalog = {
+        layout = opts.layout or {},
+        selected = { x = opts.x or 1, y = opts.y or 1 },
+        current_context = opts.context,
+        title_bar = opts.title_bar,
+        title_bar_layout_rows = opts.title_bar_layout_rows,
+        custom_focus_key = opts.custom_focus_key,
+        custom_focus = opts.custom_focus,
+    }
+    function catalog:moveFocusTo(x, y, flags)
+        self.moved = { x = x, y = y, flags = flags }
+        self.selected.x, self.selected.y = x, y
+    end
+    CatalogFocus.install(catalog)
+    return catalog
+end
+
+-- The title bar goes in as one row on a full keypad, and the cursor's row
+-- index shifts to stay on the widget it was already on.
+local catalog = newCatalog{
+    layout = { { "hero" }, { "a", "b" } },
+    title_bar = titleBar{ "back", "search", "refresh", "menu" },
+    y = 1,
+}
+catalog:mergeTitleBarIntoLayout()
+assertEqual(#catalog.layout, 3, "the title bar adds a single row on a full keypad")
+assertEqual(#catalog.layout[1], 4, "every title bar button shares that row")
+assertEqual(catalog.layout[1][2], "search", "the plugin's own search icon is reachable")
+assertEqual(catalog.title_bar_layout_rows, 1, "the merged row count is recorded")
+assertEqual(catalog.selected.y, 2, "the cursor follows its widget down the layout")
+
+-- A few-keys device spends "Right" on the context menu, so its title bar has to
+-- stack instead.
+device.hasFewKeys = function() return true end
+catalog = newCatalog{
+    layout = { { "hero" } },
+    title_bar = titleBar{ "back", "search" },
+    y = 1,
+}
+catalog:mergeTitleBarIntoLayout()
+assertEqual(#catalog.layout, 3, "a few-keys title bar stacks one button per row")
+assertEqual(catalog.title_bar_layout_rows, 2, "both stacked rows are recorded")
+assertEqual(catalog.selected.y, 3, "the cursor shifts by both rows")
+device.hasFewKeys = function() return false end
+
+-- Without a D-Pad there is no cursor to place, and a page with no title bar
+-- must not claim rows it did not add.
+catalog = newCatalog{ layout = { { "a" } }, title_bar = titleBar{ "back" } }
+device.hasDPad = function() return false end
+catalog:mergeTitleBarIntoLayout()
+assertEqual(#catalog.layout, 1, "a device with no D-Pad skips the merge")
+assertEqual(catalog.title_bar_layout_rows, 0, "and records no merged rows")
+device.hasDPad = function() return true end
+
+catalog = newCatalog{ layout = { { "a" } } }
+catalog:mergeTitleBarIntoLayout()
+assertEqual(catalog.title_bar_layout_rows, 0, "a page with no title bar merges nothing")
+
+-- The two devices this exists for. Kindle 3 and Kindle 4 are the models the bug
+-- was reported on, and they are exactly the ones upstream Menu refuses to merge
+-- a title bar for (K3 has a Sym key, K4 an on-screen keyboard). Neither sets
+-- hasFewKeys, so both take the single-row title bar.
+for _, profile in ipairs({
+    { model = "Kindle3", has_sym_key = true, has_screen_kb = false },
+    { model = "Kindle4", has_sym_key = false, has_screen_kb = true },
+}) do
+    device.hasSymKey = function() return profile.has_sym_key end
+    device.hasScreenKB = function() return profile.has_screen_kb end
+    catalog = newCatalog{
+        layout = { { "hero" } },
+        title_bar = titleBar{ "back", "search", "refresh" },
+        y = 1,
+    }
+    catalog:mergeTitleBarIntoLayout()
+    assertEqual(catalog.title_bar_layout_rows, 1,
+        profile.model .. " gets its title bar as one row")
+    assertEqual(#catalog.layout[1], 3,
+        profile.model .. " can reach every title bar button")
+    assertEqual(catalog.selected.y, 2,
+        profile.model .. " shifts the cursor past the merged row")
+end
+device.hasSymKey = nil
+device.hasScreenKB = nil
+
+-- Opening a page for the first time records its key but has nothing to restore.
+catalog = newCatalog{ context = { kind = "dashboard" }, x = 3, y = 4 }
+catalog:saveCustomFocus()
+assertEqual(catalog.custom_focus, nil, "a page opened fresh has no remembered cursor")
+assertEqual(catalog.custom_focus_key, "dashboard", "but its key is recorded for the next rebuild")
+
+-- Refreshing the same page keeps the cursor.
+catalog:saveCustomFocus()
+assertEqual(catalog.custom_focus.x, 3, "a rebuild of the same page remembers the column")
+assertEqual(catalog.custom_focus.y, 4, "and the row")
+
+-- Opening a different page does not.
+catalog.current_context = { kind = "detail", detail = { id = 7 } }
+catalog:saveCustomFocus()
+assertEqual(catalog.custom_focus, nil, "opening another page starts its cursor from scratch")
+assertEqual(catalog.custom_focus_key, "detail:7", "and the detail key names the book")
+
+catalog.current_context = { kind = "detail", detail = { id = 8 } }
+catalog:saveCustomFocus()
+assertEqual(catalog.custom_focus, nil, "another book is another page")
+
+-- A rebuilt layout can be shorter than the one the cursor was remembered
+-- against: a shelf that lost a row, or a header that lost its paging chevrons.
+catalog = newCatalog{
+    layout = { { "a", "b" }, { "c" }, { "d", "e", "f", "g" } },
+    custom_focus_key = "dashboard",
+    custom_focus = { x = 5, y = 9 },
+}
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved.y, 3, "a row past the end clamps to the last row")
+assertEqual(catalog.moved.x, 4, "and a column past the end clamps to that row's width")
+assertEqual(catalog.custom_focus, nil, "the remembered cursor is consumed by the restore")
+
+catalog = newCatalog{
+    layout = { { "a", "b", "c" }, { "d" } },
+    custom_focus_key = "dashboard",
+    custom_focus = { x = 2, y = 1 },
+}
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved.x, 2, "a cursor still inside the layout is left alone")
+assertEqual(catalog.moved.y, 1, "on both axes")
+
+-- An empty row is skipped rather than focused.
+catalog = newCatalog{
+    layout = { { "a" }, { "b" }, {} },
+    custom_focus_key = "dashboard",
+    custom_focus = { x = 1, y = 3 },
+}
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved.y, 2, "an empty row hands the cursor to the one above it")
+
+-- A page opened fresh starts below the title bar, on content.
+catalog = newCatalog{
+    layout = { { "back", "search" }, { "hero" }, { "a", "b" } },
+    custom_focus_key = "dashboard",
+    title_bar_layout_rows = 1,
+}
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved.y, 2, "a fresh page opens on its first content row")
+assertEqual(catalog.moved.x, 1, "at the start of that row")
+
+-- Section headers carry paging chevrons that are disabled on page one, so a
+-- fresh cursor skips past them to the books rather than opening on a dead
+-- control.
+local chrome = { "prev", "next" }
+chrome.is_chrome = true
+catalog = newCatalog{
+    layout = { { "back" }, chrome, { "book" } },
+    custom_focus_key = "dashboard",
+    title_bar_layout_rows = 1,
+}
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved.y, 3, "a fresh page skips a section header's controls")
+
+-- Chrome is only skipped when opening; a remembered cursor may well be sitting
+-- on a chevron the reader was paging with.
+catalog = newCatalog{
+    layout = { { "back" }, chrome, { "book" } },
+    custom_focus_key = "dashboard",
+    custom_focus = { x = 2, y = 2 },
+    title_bar_layout_rows = 1,
+}
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved.y, 2, "a remembered cursor stays on its chevron")
+assertEqual(catalog.moved.x, 2, "in the column it was paging from")
+
+-- A page that is nothing but chrome still has to put the cursor somewhere.
+local only_chrome = { "prev", "next" }
+only_chrome.is_chrome = true
+catalog = newCatalog{
+    layout = { { "back" }, only_chrome },
+    custom_focus_key = "dashboard",
+    title_bar_layout_rows = 1,
+}
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved.y, 2, "a page with no content row falls back to its first row")
+
+-- The mosaic and list views are the uniform grids Menu already handles.
+catalog = newCatalog{ layout = { { "a" } } }
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved, nil, "a plain book grid is left to Menu's own bookkeeping")
+
+-- A touchscreen has no cursor to keep.
+focus_navigation = false
+catalog = newCatalog{ context = { kind = "dashboard" }, layout = { { "a" } }, x = 2, y = 2 }
+catalog:saveCustomFocus()
+assertEqual(catalog.custom_focus_key, nil, "a touch device tracks no page key")
+assertEqual(catalog.custom_focus, nil, "and remembers no cursor")
+catalog.custom_focus_key = "dashboard"
+catalog.custom_focus = { x = 2, y = 2 }
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved, nil, "and never moves the cursor")
+focus_navigation = true
+
+-- The footer's return arrow is a Button Menu never puts in the focus layout, so
+-- Back is the only way up a level on a device that cannot tap it.
+catalog = newCatalog{}
+catalog.stack = { "dashboard", "books" }
+assertEqual(catalog:shouldReturnOnBack(), true, "Back walks up while there is a level to return to")
+catalog.stack = {}
+assertEqual(catalog:shouldReturnOnBack(), false, "Back closes the catalog at the root")
+catalog.stack = nil
+assertEqual(catalog:shouldReturnOnBack(), false, "a catalog with no stack yet closes on Back")
+
+-- A touch device can tap the arrow, so Back keeps closing outright: a multiswipe
+-- to leave should not turn into one swipe per level browsed.
+focus_navigation = false
+catalog = newCatalog{}
+catalog.stack = { "dashboard", "books" }
+assertEqual(catalog:shouldReturnOnBack(), false, "a touch device still closes on Back")
+focus_navigation = true
+
+
+-- A page may nominate the row it exists for. The detail page marks its Read or
+-- Download button, so the cursor opens on the thing the reader came to press
+-- rather than five rows above it.
+local primary = { "download" }
+primary.is_primary = true
+local header_chrome = { "prev", "next" }
+header_chrome.is_chrome = true
+catalog = newCatalog{
+    layout = { { "back" }, header_chrome, { "meta" }, primary },
+    custom_focus_key = "detail:7",
+    title_bar_layout_rows = 1,
+}
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved.y, 4, "a fresh page opens on the row it nominated")
+
+-- A remembered cursor still wins: the reader was somewhere for a reason.
+catalog = newCatalog{
+    layout = { { "back" }, { "meta" }, primary },
+    custom_focus_key = "detail:7",
+    custom_focus = { x = 1, y = 2 },
+    title_bar_layout_rows = 1,
+}
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved.y, 2, "a rebuild keeps the cursor where it was")
+
+-- The footer's paginator and return arrow join the layout as a last row, so a
+-- reader who never learns the page-turn keys can still reach them. Only what is
+-- actually on screen goes in: the plugin hides the chevrons on pages that do
+-- not page.
+local function footerCatalog(opts)
+    local c = newCatalog{}
+    for k, v in pairs(opts) do
+        -- Model a real widget: the footer tells hidden controls they lost focus.
+        if type(v) == "table" and not v.handleEvent then
+            v.handleEvent = function(self, event)
+                if event.handler == "onUnfocus" then self.focused = false end
+                return true
+            end
+        end
+        c[k] = v
+    end
+    return c
+end
+
+local chevrons = footerCatalog{
+    page_return_arrow = { name = "return" },
+    page_info_first_chev = { name = "first" },
+    page_info_left_chev = { name = "prev" },
+    page_info_text = { name = "text", text = "Page 1 of 15" },
+    page_info_right_chev = { name = "next" },
+    page_info_last_chev = { name = "last" },
+}
+local footer = chevrons:footerFocusRow()
+assertEqual(#footer, 6, "every visible footer control is reachable")
+assertEqual(footer[1].name, "return", "the return arrow comes first, as drawn")
+assertEqual(footer[4].name, "text", "the page indicator sits between the chevrons")
+assertEqual(footer.is_chrome, true, "the footer is chrome, so a fresh page skips it")
+
+-- A page that does not page hides its chevrons and empties the indicator.
+local dashboard_footer = footerCatalog{
+    page_return_arrow = { name = "return", hidden = true },
+    page_info_first_chev = { name = "first", hidden = true },
+    page_info_left_chev = { name = "prev", hidden = true },
+    page_info_text = { name = "text", text = "" },
+    page_info_right_chev = { name = "next", hidden = true },
+    page_info_last_chev = { name = "last", hidden = true },
+}
+assertEqual(dashboard_footer:footerFocusRow(), nil, "a page with no footer controls adds no row")
+
+-- A return arrow with nothing beside it is still worth reaching.
+local only_return = footerCatalog{
+    page_return_arrow = { name = "return" },
+    page_info_text = { name = "text", text = "" },
+}
+local lone = only_return:footerFocusRow()
+assertEqual(#lone, 1, "a lone return arrow still gets a row")
+assertEqual(lone[1].name, "return", "holding the arrow")
+
+-- A touch device taps the footer directly.
+focus_navigation = false
+assertEqual(chevrons:footerFocusRow(), nil, "a touch device adds no footer row")
+focus_navigation = true
+
+-- The footer is the one row that has to survive a change of page: its controls
+-- are what cause the rebuild, so pressing "next page" or "next book" from there
+-- must leave the cursor on the button just pressed.
+local function footerRow(...)
+    local row = { ... }
+    row.id = "footer"
+    row.is_chrome = true
+    return row
+end
+
+-- The row index shifts with however much content the page has; the id does not.
+catalog = newCatalog{
+    layout = { { "a" }, { "b" }, { "c" }, footerRow("return", "prev", "next") },
+    custom_focus_key = "books",
+    custom_focus = { x = 3, y = 4, row_id = "footer" },
+}
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved.y, 4, "the footer is found at its new row index")
+assertEqual(catalog.moved.x, 3, "on the same control")
+
+catalog = newCatalog{
+    layout = { { "a" }, footerRow("return", "prev", "next") },
+    custom_focus_key = "books",
+    custom_focus = { x = 3, y = 9, row_id = "footer" },
+}
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved.y, 2, "a shorter page still finds the footer")
+assertEqual(catalog.moved.x, 3, "keeping the control that caused the rebuild")
+
+-- Even where the page has no key of its own: the book grid is Menu's to manage,
+-- but its footer is ours.
+catalog = newCatalog{
+    layout = { { "a" }, footerRow("return", "next") },
+    custom_focus = { x = 2, y = 2, row_id = "footer" },
+}
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved ~= nil, true, "a grid with no page key still restores its footer")
+assertEqual(catalog.moved.x, 2, "on the pressed control")
+
+-- Resting on the footer is remembered whatever page comes next.
+catalog = newCatalog{
+    layout = { { "a" }, footerRow("return", "next") },
+    context = { kind = "detail", detail = { id = 1 } },
+    custom_focus_key = "detail:99",
+    x = 2, y = 2,
+}
+catalog:saveCustomFocus()
+assertEqual(catalog.custom_focus ~= nil, true, "the footer cursor survives moving to another book")
+assertEqual(catalog.custom_focus.row_id, "footer", "and remembers it was the footer")
+
+-- Off the footer, a change of page still starts the new one fresh.
+catalog = newCatalog{
+    layout = { { "a" }, { "b" } },
+    context = { kind = "detail", detail = { id = 1 } },
+    custom_focus_key = "detail:99",
+    x = 1, y = 2,
+}
+catalog:saveCustomFocus()
+assertEqual(catalog.custom_focus, nil, "an ordinary row does not follow the reader to another page")
+
+-- Returning to a page restores the cursor it was left at, across any number of
+-- pages visited in between and whether or not the page keys its own focus.
+catalog = newCatalog{ layout = { { "a", "b" }, { "c" } }, x = 2, y = 1 }
+local left_at = catalog:captureFocus()
+assertEqual(left_at.x, 2, "leaving a page captures the column")
+assertEqual(left_at.y, 1, "and the row")
+
+catalog = newCatalog{ layout = { { "a", "b" }, { "c" } } }
+catalog:restoreFocusOnNextUpdate(left_at)
+catalog:saveCustomFocus()
+catalog:restoreCustomFocus()
+assertEqual(catalog.moved.x, 2, "returning puts the cursor back in its column")
+assertEqual(catalog.moved.y, 1, "and its row")
+
+-- A touch device captures nothing to restore.
+focus_navigation = false
+catalog = newCatalog{ layout = { { "a" } }, x = 1, y = 1 }
+assertEqual(catalog:captureFocus(), nil, "a touch device captures no cursor")
+catalog:restoreFocusOnNextUpdate({ x = 1, y = 1 })
+assertEqual(catalog.pending_focus, nil, "and accepts none")
+focus_navigation = true
+
+-- A focus layout may only name buttons the title bar actually draws. TitleBar
+-- clears its children and re-inits whenever a shrink-to-fit title changes, which
+-- drops any button appended afterwards while leaving the field pointing at it.
+-- Trusting the field sent the cursor to controls that were not on screen - and
+-- the same drop hid those buttons from touch users too.
+local ORDER = { "left_button", "search_button", "refresh_button", "right_button" }
+local APPENDED = { search_button = true, refresh_button = true }
+local function titleBar(fields, drawn)
+    local b = {}
+    for k, v in pairs(fields) do b[k] = v end
+    for _, child in ipairs(drawn or {}) do table.insert(b, child) end
+    return b
+end
+
+local left, search, refresh = { "left" }, { "search" }, { "refresh" }
+
+-- All three drawn: all three reachable.
+local drawnButtons = CatalogFocus.drawnTitleBarButtons(
+    titleBar({ left_button = left, search_button = search, refresh_button = refresh },
+             { search, refresh }), ORDER, APPENDED)
+assertEqual(#drawnButtons, 3, "every drawn button is reachable")
+assertEqual(drawnButtons[2], search, "in left-to-right order")
+
+-- Re-init dropped them from the group but left the fields set.
+drawnButtons = CatalogFocus.drawnTitleBarButtons(
+    titleBar({ left_button = left, search_button = search, refresh_button = refresh }, {}),
+    ORDER, APPENDED)
+assertEqual(#drawnButtons, 1, "buttons dropped from the group are not reachable")
+assertEqual(drawnButtons[1], left, "leaving only the bar's own button")
+
+-- One of the two survived.
+drawnButtons = CatalogFocus.drawnTitleBarButtons(
+    titleBar({ left_button = left, search_button = search, refresh_button = refresh },
+             { refresh }), ORDER, APPENDED)
+assertEqual(#drawnButtons, 2, "a partially rebuilt bar reports only what it drew")
+assertEqual(drawnButtons[2], refresh, "and keeps their order")
+
+-- TitleBar's own buttons are nested rather than direct children, so they are
+-- taken on trust and must not be filtered out.
+drawnButtons = CatalogFocus.drawnTitleBarButtons(
+    titleBar({ left_button = left, right_button = { "right" } }, {}), ORDER, APPENDED)
+assertEqual(#drawnButtons, 2, "the bar's own buttons are trusted without membership")
+
+assertEqual(#CatalogFocus.drawnTitleBarButtons(titleBar({}, {}), ORDER, APPENDED), 0,
+    "a bar with no buttons reports none")
+
+-- A footer control that has just been hidden is no longer in the layout, so no
+-- Unfocus would ever reach it - and a hidden Button still paints its frame, so a
+-- stale inverted one shows up as a black block where the chevron used to be.
+local function footerWidget(name, hidden)
+    return {
+        name = name, hidden = hidden, focused = not not hidden,
+        handleEvent = function(self, event)
+            if event.handler == "onUnfocus" or event[1] == "Unfocus" then
+                self.focused = false
+            end
+            return true
+        end,
+    }
+end
+
+local shown_chev = footerWidget("next", false)
+local hidden_chev = footerWidget("last", true)
+local page_text = footerWidget("text", false)
+page_text.text = "Page 1 of 3"
+catalog = newCatalog{}
+catalog.page_return_arrow = footerWidget("return", false)
+catalog.page_info_first_chev = footerWidget("first", true)
+catalog.page_info_left_chev = footerWidget("prev", true)
+catalog.page_info_text = page_text
+catalog.page_info_right_chev = shown_chev
+catalog.page_info_last_chev = hidden_chev
+
+local built = catalog:footerFocusRow()
+assertEqual(#built, 3, "only the controls actually on screen are reachable")
+assertEqual(hidden_chev.focused, false, "a hidden chevron is told it lost focus")
+assertEqual(catalog.page_info_first_chev.focused, false, "every hidden control is")
+assertEqual(shown_chev.focused, false, "a shown one is left alone to be focused normally")
+
+-- An emptied page indicator is excluded too: it is blanked rather than hidden.
+page_text.text = ""
+built = catalog:footerFocusRow()
+for _, w in ipairs(built) do
+    assertEqual(w ~= page_text, true, "an empty page indicator is not reachable")
+end
+
+print("bookorbit_focus_navigation_test.lua: ok")