BookMap & PageBrowser: now usable on Non-Touch devices (#12579)

Have them both usable on non-touch devices.
Also:
FrameContainer: fix focus border handling, and draw inner border after the
content (to prevent it from being overridden by the content bgcolor).

frontend/apps/reader/modules/readerhandmade.lua

@@ -121,8 +121,10 @@ function ReaderHandMade:onToggleHandmadeFlows()
 end
 
 function ReaderHandMade:addToMainMenu(menu_items)
-    -- As it's currently impossible to create custom hidden flows on non-touch, and really impractical to create a custom toc, it's better hide these features completely for now.
-    if not Device:isTouchDevice() then
+    if not Device:isTouchDevice() and not Device:useDPadAsActionKeys() then
+        -- As it's currently impossible to create custom hidden flows on non-touch devices without useDPadAsActionKeys,
+        -- (technically speaking, without a 'hold' or 'long-press' event) and really impractical to create a custom toc,
+        -- it's better hide these features completely for now.
         return
     end
     menu_items.handmade_toc = {

frontend/apps/reader/modules/readerthumbnail.lua

@@ -26,7 +26,7 @@ function ReaderThumbnail:init()
         -- The BookMap and PageBrowser widgets depend too much on gestures,
         -- making them work with not enough keys on Non-Touch would be hard and very limited, so
         -- just don't make them available.
-        -- We will only let BookMap run on useDPadAsActionKeys devices.
+        -- We will only let them run on useDPadAsActionKeys devices.
         return
     end
 
@@ -87,8 +87,6 @@ function ReaderThumbnail:addToMainMenu(menu_items)
             self:onShowBookMap(true)
         end,
     }
-    -- PageBrowser still needs some work before we can let it run on non-touch devices with useDPadAsActionKeys
-    if Device:hasDPad() and Device:useDPadAsActionKeys() then return end
     menu_items.page_browser = {
         text = _("Page browser"),
         callback = function()

frontend/dispatcher.lua

@@ -181,7 +181,7 @@ local settingsList = {
     toc = {category="none", event="ShowToc", title=_("Table of contents"), reader=true},
     book_map = {category="none", event="ShowBookMap", title=_("Book map"), reader=true, condition=Device:isTouchDevice() or (Device:hasDPad() and Device:useDPadAsActionKeys())},
     book_map_overview = {category="none", event="ShowBookMap", arg=true, title=_("Book map (overview)"), reader=true, condition=Device:isTouchDevice() or (Device:hasDPad() and Device:useDPadAsActionKeys())},
-    page_browser = {category="none", event="ShowPageBrowser", title=_("Page browser"), reader=true, condition=Device:isTouchDevice()},
+    page_browser = {category="none", event="ShowPageBrowser", title=_("Page browser"), reader=true, condition=Device:isTouchDevice() or (Device:hasDPad() and Device:useDPadAsActionKeys())},
     bookmarks = {category="none", event="ShowBookmark", title=_("Bookmarks"), reader=true},
     bookmark_search = {category="none", event="SearchBookmark", title=_("Bookmark search"), reader=true},
     toggle_bookmark = {category="none", event="ToggleBookmark", title=_("Toggle bookmark"), reader=true, separator=true},

frontend/ui/elements/reader_menu_order.lua

@@ -13,15 +13,15 @@ local order = {
     navi = {
         "table_of_contents",
         "bookmarks",
-        "toggle_bookmark", -- if not Device:isTouchDevice()
+        "toggle_bookmark", -- if not Device:isTouchDevice() and not useDPadAsActionKeys()
         "bookmark_browsing_mode",
         "navi_settings",
         "----------------------------",
         "page_map",
         "hide_nonlinear_flows",
         "----------------------------",
-        "book_map", -- if Device:isTouchDevice()
-        "page_browser", -- if Device:isTouchDevice()
+        "book_map", -- if Device:isTouchDevice() or useDPadAsActionKeys()
+        "page_browser", -- if Device:isTouchDevice() or useDPadAsActionKeys()
         "----------------------------",
         "go_to",
         "skim_to",

frontend/ui/widget/bookmapwidget.lua

@@ -4,6 +4,7 @@ local ButtonDialog = require("ui/widget/buttondialog")
 local CenterContainer = require("ui/widget/container/centercontainer")
 local Device = require("device")
 local Event = require("ui/event")
+local FocusManager = require("ui/widget/focusmanager")
 local Font = require("ui/font")
 local FrameContainer = require("ui/widget/container/framecontainer")
 local Geom = require("ui/geometry")
@@ -111,6 +112,7 @@ function BookMapRow:getLeftSpacingForNumberOfPageSlots(nb_pages, pages_per_row,
 end
 
 function BookMapRow:init()
+    self.focus_layout = {}
     local _mirroredUI = BD.mirroredUILayout()
     self.dimen = Geom:new{ x = 0, y = 0, w = self.width, h = self.height }
 
@@ -167,6 +169,8 @@ function BookMapRow:init()
     local tspan_margin = Size.margin.tiny
     local tspan_padding_h = Size.padding.tiny
     local tspan_height = self.span_height - 2 * (tspan_margin + self.toc_span_border)
+    local focus_row = nil
+    local focus_row_offset_y = 0
     if self.toc_items then
         for lvl, items in pairs(self.toc_items) do
             local offset_y = self.pages_frame_border + self.span_height * (lvl - 1) + tspan_margin
@@ -247,6 +251,9 @@ function BookMapRow:init()
                     padding = 0,
                     bordersize = self.toc_span_border,
                     background = bgcolor,
+                    focusable = self.enable_focus_navigation,
+                    focus_border_size = self.focus_nav_border,
+                    focus_inner_border = true,
                     CenterContainer:new{
                         dimen = Geom:new{
                             w = width - 2 * self.toc_span_border,
@@ -256,6 +263,14 @@ function BookMapRow:init()
                     }
                 }
                 table.insert(self.pages_frame, span_w)
+                if self.enable_focus_navigation then
+                    if not focus_row or focus_row_offset_y ~= offset_y then
+                        focus_row = {}
+                        focus_row_offset_y = offset_y
+                        table.insert(self.focus_layout, focus_row)
+                    end
+                    table.insert(focus_row, span_w)
+                end
             end
         end
     end
@@ -341,6 +356,14 @@ function BookMapRow:init()
     self.pages_markers = {}
     self.indicators = {}
     self.bottom_texts = {}
+
+    -- For focus navigation (with keys), we need empty widgets over page slots,
+    -- that will only get a border when focused and lose it when unfocused.
+    local invisible_focusable_page_slots = {}
+    if self.enable_focus_navigation then
+        table.insert(self.focus_layout, invisible_focusable_page_slots)
+    end
+
     local prev_page_was_read = true -- avoid one at start of row
     local extended_marker_h = { -- maps to extended_marker.SMALL/MEDIUM/LARGE
         math.ceil(self.span_height * 0.12),
@@ -402,6 +425,44 @@ function BookMapRow:init()
             end
             prev_page_was_read = false
         end
+        if self.enable_focus_navigation then
+            local x
+            if _mirroredUI then
+                x = self:getPageX(page, true)
+            else
+                x = self:getPageX(page)
+            end
+            local w = self:getPageX(page, true) - x
+            -- This + 1 and the one below for overlap_offset seem to give the right
+            -- appearance (but I can't really logically make out why...)
+            if self.with_page_sep then
+                w = w + 1
+            end
+            if (not _mirroredUI and page == self.end_page) or
+                   (_mirroredUI and page == self.start_page) then
+                w = w - 1 -- needed visual tweak, to match appearance at start and end
+            end
+            local invisible_focusable_page_slot = FrameContainer:new{
+                overlap_offset = {x + 1 - self.focus_nav_border, self.pages_frame_height - self.span_height},
+                margin = 0,
+                padding = self.focus_nav_border,
+                bordersize = 0,
+                focusable = true,
+                focus_border_size = self.focus_nav_border,
+                focus_inner_border = true,
+                Widget:new{
+                    dimen = Geom:new{
+                        w = w,
+                        h = math.floor(1.2 * self.span_height) - 2*self.focus_nav_border,
+                    }
+                }
+            }
+            table.insert(self.pages_frame, invisible_focusable_page_slot)
+            table.insert(invisible_focusable_page_slots, invisible_focusable_page_slot)
+            if page == self.focus_nav_page then
+                invisible_focusable_page_slots.focused_widget_idx = #invisible_focusable_page_slots
+            end
+        end
         -- Extended separators below the baseline if requested (by PageBrowser
         -- to show the start of thumbnail rows)
         if self.extended_sep_pages and self.extended_sep_pages[page] then
@@ -602,9 +663,10 @@ function BookMapRow:paintTo(bb, x, y)
 end
 
 -- BookMapWidget: shows a map of content, including TOC, bookmarks, read pages, non-linear flows...
-local BookMapWidget = InputContainer:extend{
+local BookMapWidget = FocusManager:extend{
     -- Focus page: show the BookMapRow containing this page
-    -- in the middle of screen
+    -- in the middle of screen (despite its name, this has
+    -- nothing to do with FocusManager and focus navigation)
     focus_page = nil,
     -- Should only be nil on the first launch via ReaderThumbnail
     launcher = nil,
@@ -613,11 +675,19 @@ local BookMapWidget = InputContainer:extend{
     -- Restricted mode, as initial view (all on one screen), but allowing chapter levels changes
     overview_mode = false,
 
+    -- Border around focused items (page slots, chapter titles) on non-touch devices
+    -- (this needs to be wider than BookMapRow.toc_span_border or they won't show)
+    focus_nav_border = Size.border.thick,
+
     -- Make this local subwidget available for reuse by PageBrowser
     BookMapRow = BookMapRow,
 }
 
 function BookMapWidget:init()
+    -- On touch devices (with keys), we don't really need to navigate focus with keys,
+    -- so we should avoid allocating memory to huge data structures.
+    self.enable_focus_navigation = not Device:isTouchDevice() and Device:hasDPad() and Device:useDPadAsActionKeys()
+
     if self.ui.view:shouldInvertBiDiLayoutMirroring() then
         BD.invert()
     end
@@ -631,22 +701,7 @@ function BookMapWidget:init()
     }
     self.covers_fullscreen = true -- hint for UIManager:_repaint()
 
-    if Device:hasKeys() then
-        self.key_events.Close = { { Device.input.group.Back } }
-        self.key_events.ShowBookMapMenu = { { "Menu" } }
-        self.key_events.ScrollPageUp = { { Input.group.PgBack } }
-        self.key_events.ScrollPageDown = { { Input.group.PgFwd } }
-        if Device:hasSymKey() then
-            self.key_events.ScrollRowUp = { { "Shift", "Up" } }
-            self.key_events.ScrollRowDown = { { "Shift", "Down" } }
-        elseif Device:hasScreenKB() then
-            self.key_events.ScrollRowUp = { { "ScreenKB", "Up" } }
-            self.key_events.ScrollRowDown = { { "ScreenKB", "Down" } }
-        else
-            self.key_events.ScrollRowUp = { { "Up" } }
-            self.key_events.ScrollRowDown = { { "Down" } }
-        end
-    end
+    self:registerKeyEvents()
     if Device:isTouchDevice() then
         self.ges_events = {
             Swipe = {
@@ -692,6 +747,16 @@ function BookMapWidget:init()
         -- and allow us to get where we want.
         -- (Also, handling "hold" is a bit more complicated when we have our
         -- ScrollableContainer that would also like to handle it.)
+    else
+        -- NT: needed for selection
+        self.ges_events = {
+            Tap = {
+                GestureRange:new{
+                    ges = "tap",
+                    range = self.dimen,
+                }
+            }
+        }
     end
 
     -- No real need for any explicite edge and inter-row padding:
@@ -763,6 +828,12 @@ function BookMapWidget:init()
         ignore_events = {"swipe"},
         self.vgroup,
     }
+    -- Our event handlers are similarly named as those in ScrollableContainer, so even
+    -- if we add the key event to ignore_events above, registering them here with the
+    -- same names means they'll still be handled by ScrollableContainer's own handlers.
+    -- Therefore, we override its handlers to make them pass-through.
+    self.cropping_widget.onScrollPageUp = function() return false end
+    self.cropping_widget.onScrollPageDown = function() return false end
 
     self[1] = FrameContainer:new{
         width = self.dimen.w,
@@ -803,6 +874,28 @@ function BookMapWidget:init()
     self:update()
 end
 
+function BookMapWidget:registerKeyEvents()
+    if Device:hasKeys() then
+        if Device:isTouchDevice() then
+            -- Remove key handling by FocusManager (there is no ordering/priority
+            -- handling for key_events, unlike with touch zones)
+            self.key_events = {}
+            self.key_events.ScrollRowUp = { { "Up" } }
+            self.key_events.ScrollRowDown = { { "Down" } }
+        elseif Device:hasScreenKB() or Device:hasKeyboard() then
+            local modifier = Device:hasScreenKB() and "ScreenKB" or "Shift"
+            self.key_events.ScrollRowUp = { { modifier, "Up" } }
+            self.key_events.ScrollRowDown = { { modifier, "Down" } }
+            self.key_events.CloseAll = { { modifier, "Back" }, event = "Close", args = true }
+        end
+        self.key_events.Close = { { Device.input.group.Back } }
+        self.key_events.ShowBookMapMenu = { { "Menu" } }
+        self.key_events.ScrollPageUp = { { Input.group.PgBack } }
+        self.key_events.ScrollPageDown = { { Input.group.PgFwd } }
+    end
+end
+BookMapWidget.onPhysicalKeyboardConnected = BookMapWidget.registerKeyEvents
+
 function BookMapWidget:updateEditableStuff(update_view)
     -- Toc, bookmarks and hidden flows may be edited
     self.ui.toc:fillToc()
@@ -829,6 +922,9 @@ function BookMapWidget:updateEditableStuff(update_view)
 end
 
 function BookMapWidget:update()
+    self.layout = {}
+    self.cur_focused_widget = nil
+
     if not self.focus_page then -- Initial display
         -- Focus (show at the middle of screen) on the BookMapRow that contains
         -- current page
@@ -972,22 +1068,40 @@ function BookMapWidget:update()
                 if self.flat_map then
                     if item.page == p_start then
                         cur_left_spacing = self.row_left_spacing + self.flat_toc_level_indent * (item.depth-1)
-                        local txt_max_width = self.row_width - cur_left_spacing
-                        table.insert(self.vgroup, HorizontalGroup:new{
-                            HorizontalSpan:new{
-                                width = cur_left_spacing,
-                            },
+                        -- We'll display focus with inner borders, possibly drawn over the text.
+                        -- Adding top and bottom padding does not seem needed, but we need
+                        -- some left and right padding (for the border, and some thin one before the text)
+                        local h_padding = self.enable_focus_navigation and Size.border.default + Size.border.thin or 0
+                        local txt_max_width = self.row_width - cur_left_spacing - 2*h_padding
+                        local toc_title = FrameContainer:new{
+                            margin = 0,
+                            padding = 0,
+                            padding_left = h_padding,
+                            padding_right = h_padding,
+                            bordersize = 0,
+                            focusable = self.enable_focus_navigation,
+                            focus_border_size = self.focus_nav_border,
+                            focus_inner_border = true,
                             TextBoxWidget:new{
                                 text = self.ui.toc:cleanUpTocTitle(item.title, true),
                                 width = txt_max_width,
                                 face = self.flat_toc_depth_faces[item.depth],
+                            }
+                        }
+                        if self.enable_focus_navigation then
+                            table.insert(self.layout, {toc_title})
+                        end
+                        table.insert(self.vgroup, HorizontalGroup:new{
+                            HorizontalSpan:new{
+                                width = cur_left_spacing,
                             },
+                            toc_title,
                             -- Store this TOC item page, so we can tap on it to launch PageBrowser on its page
                             toc_item_page = item.page,
                         })
                         -- Add a bit more spacing for the BookMapRow(s) underneath this Toc item title
                         -- (so the page number painted in this spacing feels included in the indentation)
-                        cur_left_spacing = cur_left_spacing + Size.span.horizontal_default
+                        cur_left_spacing = cur_left_spacing + Size.span.horizontal_default + toc_title.padding_left
                         -- Note: this variable indentation may make the page slot widths variable across
                         -- rows from different levels (and self.fit_pages_per_row not really accurate) :/
                         -- Hopefully, it won't be noticeable.
@@ -1141,8 +1255,16 @@ function BookMapWidget:update()
             read_pages = self.read_pages,
             current_session_duration = self.current_session_duration,
             extended_sep_pages = extended_sep_pages,
+            enable_focus_navigation = self.enable_focus_navigation,
+            focus_nav_page = self.focus_page,
+            focus_nav_border = self.focus_nav_border,
         }
         table.insert(self.vgroup, row)
+        if self.enable_focus_navigation then
+            for _, focus_row in ipairs(row.focus_layout) do
+                table.insert(self.layout, focus_row)
+            end
+        end
         if not self.page_slot_width then
             self.page_slot_width = row.page_slot_width
         end
@@ -1201,7 +1323,7 @@ function BookMapWidget:onShowBookMapMenu()
             end,
         }},
         {{
-            text = _("Available gestures"),
+            text = Device:isTouchDevice() and _("Available gestures") or _("Controls"),
             align = "left",
             callback = function()
                 self:showGestures()
@@ -1490,6 +1612,7 @@ function BookMapWidget:onScrollPageUp()
         to_keep = row_h - (scroll_offset_y - row_y)
     end
     self.cropping_widget:_scrollBy(0, -(self.crop_height - to_keep))
+    self:updateFocusAfterScroll()
     return true
 end
 
@@ -1502,6 +1625,7 @@ function BookMapWidget:onScrollPageDown()
     else
         self.cropping_widget:_scrollBy(0, self.crop_height)
     end
+    self:updateFocusAfterScroll()
     return true
 end
 
@@ -1510,6 +1634,7 @@ function BookMapWidget:onScrollRowUp()
     local row, row_idx, row_y, row_h = self:getVGroupRowAtY(-1) -- luacheck: no unused
     if row then
         self.cropping_widget:_scrollBy(0, row_y - scroll_offset_y)
+        self:updateFocusAfterScroll()
     end
     return true
 end
@@ -1519,6 +1644,7 @@ function BookMapWidget:onScrollRowDown()
     local row, row_idx, row_y, row_h = self:getVGroupRowAtY(0) -- luacheck: no unused
     if row then
         self.cropping_widget:_scrollBy(0, row_y + row_h - scroll_offset_y)
+        self:updateFocusAfterScroll()
     end
     return true
 end
@@ -1795,6 +1921,92 @@ function BookMapWidget:onTap(arg, ges)
     return true
 end
 
+function BookMapWidget:updateFocusAfterScroll()
+    if not self.enable_focus_navigation then
+        return
+    end
+    -- Set this flag so the next call to updateFocus() from paintTo() will know
+    -- we've just scrolled and we don't have to force the current focused item
+    -- into view, but possibly change what is the current focus item.
+    self.update_focus_after_scroll = true
+end
+
+function BookMapWidget:updateFocus()
+    -- To work with up to date widget positions, this must be called after paintTo()
+    -- has done its job as it is it that updates all widget coordinates
+
+    if not self.enable_focus_navigation then return end
+
+    if not self.cur_focused_widget then -- first call after first paintTo()
+        for y, r in ipairs(self.layout) do
+            if r.focused_widget_idx then -- this row contains the focused widget
+                self:moveFocusTo(r.focused_widget_idx, y)
+                break
+            end
+        end
+        self.cur_focused_widget = self:getFocusItem()
+        -- This will cause a repaint and have the focus border appear
+        self:refocusWidget(FocusManager.RENDER_IN_NEXT_TICK, FocusManager.FORCED_FOCUS)
+        return
+    end
+
+    if not self.update_focus_after_scroll then -- regular painTo() not caused by scrolling
+        local cur_focused_widget = self:getFocusItem()
+        if cur_focused_widget ~= self.cur_focused_widget then
+            -- The focused widget has changed; this is expected to happen only
+            -- from the paintTo after the user has used keys to move the
+            -- focused item.
+            self.cur_focused_widget = cur_focused_widget
+        else
+            -- Nothing to do, this is a regular repaint without any move
+            return
+        end
+    end
+
+    local focused_widget_dimen = self.cur_focused_widget.dimen
+
+    if self.update_focus_after_scroll then
+        if focused_widget_dimen.y < self.cropping_widget.dimen.y
+                or focused_widget_dimen.y + focused_widget_dimen.h >= self.cropping_widget.dimen.y + self.crop_height then
+            -- The current focused widget is not fully in the viewport
+            -- The user has scrolled one page or one row, and the focused widget moved out
+            -- of the updated view: forget that focused widget and change it to a widget
+            -- in the middle of the new view.
+            for y, focus_row in ipairs(self.layout) do
+                if #focus_row > 0 then
+                    local dimen = focus_row[1].dimen
+                    if dimen.y + dimen.h > self.cropping_widget.dimen.y + self.crop_height/2 then
+                        self:moveFocusTo(1, y)
+                        break
+                    end
+                end
+            end
+            self.cur_focused_widget = self:getFocusItem()
+            -- This will trigger a repaint and cause us to be called again (at which point we should do nothing).
+            self:refocusWidget(FocusManager.RENDER_IN_NEXT_TICK, FocusManager.FORCED_FOCUS)
+        end
+    else
+        -- The focused widget was changed by the user (with keys), it may have moved out of view.
+        -- For a smooth experience, we can't move just the focused page slot into view and have
+        -- parts of its BookMapRow (chapter titles above in grid mode) truncated (borders, icons
+        -- below baseline): we need to move this BookMapRow fully into view.
+        local row, row_idx, row_y, row_h = self:getVGroupRowAtY(focused_widget_dimen.y - self.title_bar_h) -- luacheck: no unused
+        if row then
+            row_y = row_y - self.cropping_widget._scroll_offset_y
+            if row_y < 0 then
+                self.cropping_widget:_scrollBy(0, row_y)
+                -- This will trigger a repaint and cause us to be called again (at which point we should do nothing).
+                -- (We shouldn't need to refocus, but somehow, this works while a classic setDirty doesn't)
+                self:refocusWidget(FocusManager.RENDER_IN_NEXT_TICK, FocusManager.FORCED_FOCUS)
+            elseif row_y + row_h > self.crop_height then
+                self.cropping_widget:_scrollBy(0, row_y + row_h - self.crop_height)
+                self:refocusWidget(FocusManager.RENDER_IN_NEXT_TICK, FocusManager.FORCED_FOCUS)
+            end
+        end
+    end
+    self.update_focus_after_scroll = false
+end
+
 function BookMapWidget:paintTo(bb, x, y)
     -- Paint regular sub widgets the classic way
     InputContainer.paintTo(self, bb, x, y)
@@ -1803,6 +2015,7 @@ function BookMapWidget:paintTo(bb, x, y)
     if not self.overview_mode then
         self:paintBottomHorizontalSwipeHint(bb, x, y)
     end
+    self:updateFocus()
 end
 
 function BookMapWidget:paintLeftVerticalSwipeHint(bb, x, y)

frontend/ui/widget/container/framecontainer.lua

@@ -43,6 +43,7 @@ local FrameContainer = WidgetContainer:extend{
     focusable = false,
     focus_border_size = Size.border.window * 2,
     focus_border_color = Blitbuffer.COLOR_BLACK,
+    focus_inner_border = false, -- use inner border for focus style
     -- paint hatched background if provided
     stripe_color = nil,
     stripe_width = nil,
@@ -69,11 +70,18 @@ function FrameContainer:onFocus()
     if not self.focusable then
         return false
     end
-    self._origin_bordersize = self.bordersize
-    self._origin_border_color = self.color
-    self.bordersize = self.focus_border_size
-    self.color = self.focus_border_color
-    self._focused = true
+    if not self._focused then
+        if not self.focus_inner_border then
+            self._orig_bordersize = self.bordersize
+            self.bordersize = self.focus_border_size
+        else
+            self._orig_bordersize = self.inner_bordersize
+            self.inner_bordersize = self.focus_border_size
+        end
+        self._orig_border_color = self.color
+        self.color = self.focus_border_color
+        self._focused = true
+    end
     return true
 end
 
@@ -82,12 +90,15 @@ function FrameContainer:onUnfocus()
         return false
     end
     if self._focused then
-        self.bordersize = self._origin_bordersize
-        self.color = self._origin_border_color
+        if not self.focus_inner_border then
+            self.bordersize = self._orig_bordersize
+        else
+            self.inner_bordersize = self._orig_bordersize
+        end
+        self.color = self._orig_border_color
         self._focused = nil
-        return true
     end
-    return false
+    return true
 end
 
 
@@ -127,13 +138,6 @@ function FrameContainer:paintTo(bb, x, y)
                         container_width, container_height,
                         self.stripe_width, self.stripe_color)
     end
-    if self.inner_bordersize > 0 then
-        --- @warning This doesn't actually support radius, it'll always be a square.
-        bb:paintInnerBorder(x + self.margin, y + self.margin,
-            container_width - self.margin * 2,
-            container_height - self.margin * 2,
-            self.inner_bordersize, self.color, self.radius)
-    end
     if self.bordersize > 0 then
         local anti_alias = G_reader_settings:nilOrTrue("anti_alias_ui")
         bb:paintBorder(x + self.margin, y + self.margin,
@@ -146,6 +150,13 @@ function FrameContainer:paintTo(bb, x, y)
             x + self.margin + self.bordersize + self._padding_left + shift_x,
             y + self.margin + self.bordersize + self._padding_top)
     end
+    if self.inner_bordersize > 0 then
+        --- @warning This doesn't actually support radius, it'll always be a square.
+        bb:paintInnerBorder(x + self.margin, y + self.margin,
+            container_width - self.margin * 2,
+            container_height - self.margin * 2,
+            self.inner_bordersize, self.color, self.radius)
+    end
     if self.stripe_width and self.stripe_color and self.stripe_over then
         -- (No support for radius when hatched/stripe)
         -- We don't want to draw the stripes over any border

frontend/ui/widget/focusmanager.lua

@@ -432,7 +432,10 @@ function FocusManager:getFocusItem()
     if not self.layout then
         return nil
     end
-    return self.layout[self.selected.y][self.selected.x]
+    if self.layout[self.selected.y] then
+        return self.layout[self.selected.y][self.selected.x]
+    end
+    return nil
 end
 
 function FocusManager:_sendGestureEventToFocusedWidget(gesture)
@@ -543,4 +546,18 @@ function FocusManager:onKeyPress(key)
 end
 FocusManager.onKeyRepeat = FocusManager.onKeyPress
 
+function FocusManager:getFocusableWidgetXY(widget)
+    if not self.layout then
+        return
+    end
+    for y, row in ipairs(self.layout) do
+        for x, w in ipairs(row) do
+            if w == widget then
+                return x, y
+            end
+        end
+    end
+end
+
+
 return FocusManager

frontend/ui/widget/keyvaluepage.lua

@@ -190,6 +190,7 @@ function KeyValueItem:init()
         bordersize = 0,
         focusable = true,
         focus_border_size = Size.border.thin,
+        focus_inner_border = true,
         background = Blitbuffer.COLOR_WHITE,
         HorizontalGroup:new{
             dimen = content_dimen,

frontend/ui/widget/pagebrowserwidget.lua

@@ -4,6 +4,7 @@ local ButtonDialog = require("ui/widget/buttondialog")
 local CenterContainer = require("ui/widget/container/centercontainer")
 local Device = require("device")
 local Event = require("ui/event")
+local FocusManager = require("ui/widget/focusmanager")
 local Font = require("ui/font")
 local FrameContainer = require("ui/widget/container/framecontainer")
 local Geom = require("ui/geometry")
@@ -19,6 +20,7 @@ local TitleBar = require("ui/widget/titlebar")
 local UIManager = require("ui/uimanager")
 local VerticalGroup = require("ui/widget/verticalgroup")
 local VerticalSpan = require("ui/widget/verticalspan")
+local Widget = require("ui/widget/widget")
 local Input = Device.input
 local Screen = Device.screen
 local logger = require("logger")
@@ -31,7 +33,7 @@ local BookMapWidget = require("ui/widget/bookmapwidget")
 local BookMapRow = BookMapWidget.BookMapRow
 
 -- PageBrowserWidget: shows thumbnails of pages
-local PageBrowserWidget = InputContainer:extend{
+local PageBrowserWidget = FocusManager:extend{
     title = _("Page browser"),
     -- Focus page: will be put at the best place in the thumbnail grid
     -- (that is, the grid will pick thumbnails from pages before and
@@ -42,6 +44,8 @@ local PageBrowserWidget = InputContainer:extend{
 }
 
 function PageBrowserWidget:init()
+    self.layout = {}
+    self.build_focus_layout = not Device:isTouchDevice() and Device:hasDPad() and Device:useDPadAsActionKeys()
     if self.ui.view:shouldInvertBiDiLayoutMirroring() then
         BD.invert()
     end
@@ -53,15 +57,7 @@ function PageBrowserWidget:init()
     }
     self.covers_fullscreen = true -- hint for UIManager:_repaint()
 
-    if Device:hasKeys() then
-        self.key_events = {
-            Close = { { Device.input.group.Back } },
-            ScrollRowUp = { { "Up" } },
-            ScrollRowDown = { { "Down" } },
-            ScrollPageUp = { { Input.group.PgBack } },
-            ScrollPageDown = { { Input.group.PgFwd } },
-        }
-    end
+    self:registerKeyEvents()
     if Device:isTouchDevice() then
         self.ges_events = {
             Swipe = {
@@ -107,6 +103,16 @@ function PageBrowserWidget:init()
                 }
             },
         }
+    else
+        -- NT: needed for selection
+        self.ges_events = {
+            Tap = {
+                GestureRange:new{
+                    ges = "tap",
+                    range = self.dimen,
+                }
+            }
+        }
     end
 
     -- Put the BookMapRow left and right border outside of screen
@@ -116,7 +122,7 @@ function PageBrowserWidget:init()
         fullscreen = true,
         title = self.title,
         left_icon = "appbar.menu",
-        left_icon_tap_callback = function() self:showMenu() end,
+        left_icon_tap_callback = function() self:onShowMenu() end,
         left_icon_hold_callback = function()
             -- Cycle nb of toc span levels shown in bottom row
             if self:updateNbTocSpans(-1, true, true) then
@@ -185,6 +191,31 @@ function PageBrowserWidget:init()
     self:updateLayout()
 end
 
+function PageBrowserWidget:registerKeyEvents()
+    if Device:hasKeys() then
+        if Device:isTouchDevice() then
+            -- Remove key handling by FocusManager (there is no ordering/priority
+            -- handling for key_events, unlike with touch zones)
+            self.key_events = {}
+            self.key_events.ScrollRowUp = { { "Up" } }
+            self.key_events.ScrollRowDown = { { "Down" } }
+        elseif Device:hasScreenKB() or Device:hasKeyboard() then
+            local modifier = Device:hasScreenKB() and "ScreenKB" or "Shift"
+            self.key_events.ScrollRowUp = { { modifier, "Up" } }
+            self.key_events.ScrollRowDown = { { modifier, "Down" } }
+            -- same events as page-turn buttons for mod+left/right. it gives the impression of movement through the bottom ribbon
+            self.key_events.SwipeRibbonLeftNT = { { modifier, "Left" }, event = "ScrollPageUp" }
+            self.key_events.SwipeRibbonRightNT = { { modifier, "Right" }, event = "ScrollPageDown" }
+            self.key_events.CloseAll = { { modifier, "Back" }, event = "Close", args = true }
+        end
+        self.key_events.Close = { { Device.input.group.Back } }
+        self.key_events.ShowMenu = { { "Menu" } }
+        self.key_events.ScrollPageUp = { { Input.group.PgBack } }
+        self.key_events.ScrollPageDown = { { Input.group.PgFwd } }
+    end
+end
+PageBrowserWidget.onPhysicalKeyboardConnected = PageBrowserWidget.registerKeyEvents
+
 function PageBrowserWidget:updateEditableStuff(update_view)
     -- Toc, bookmarks and hidden flows may be edited
     -- Note: we update everything to keep things simpler, but we could provide flags to
@@ -245,6 +276,9 @@ function PageBrowserWidget:updateLayout()
     -- And put its bottom rounded corner outside of screen
     self.view_finder_h = self.row_height + 2*self.view_finder_bw + Size.radius.window
 
+    -- reset focus layout
+    self.layout = {}
+
     if self.grid then
         self.grid:free()
     end
@@ -316,7 +350,10 @@ function PageBrowserWidget:updateLayout()
     grid_item_outer_h_margin = math.floor((self.grid_width - self.nb_cols * self.grid_item_width - (self.nb_cols-1)*grid_item_inner_h_margin) / 2)
 
     self.grid:clear()
-
+    local focus_row = nil
+    local focus_row_index = 0
+    local focus_nav_border = Screen:scaleBySize(3)
+    local focus_grid_items = {}
     for idx = 1, self.nb_grid_items do
         local row = math.floor((idx-1)/self.nb_cols) -- start from 0
         local col = (idx-1) % self.nb_cols
@@ -343,6 +380,36 @@ function PageBrowserWidget:updateLayout()
             background = Blitbuffer.COLOR_WHITE,
             grid_item,
         })
+        if self.build_focus_layout then
+            local nav_item_frame = FrameContainer:new{
+                is_nav_item = true,
+                overlap_offset = {offset_x, offset_y},
+                initial_overlap_offset = {offset_x, offset_y},
+                margin = 0,
+                padding = 0,
+                bordersize = 0,
+                focusable = true,
+                focus_border_size = focus_nav_border,
+                focus_inner_border = true,
+                Widget:new{
+                    dimen = self.grid_item_dimen:copy()
+                }
+            }
+            table.insert(focus_grid_items, nav_item_frame)
+            if not focus_row or focus_row_index ~= row then
+                focus_row = {}
+                focus_row_index = row
+                table.insert(self.layout, focus_row)
+            end
+            table.insert(focus_row, nav_item_frame)
+        end
+    end
+    if self.build_focus_layout then
+        -- Append these just after the grid items, so we can access them
+        -- by tile index with just adding self.nb_grid_items
+        while #focus_grid_items > 0 do
+            table.insert(self.grid, table.remove(focus_grid_items, 1))
+        end
     end
 
     -- Put the focused (requested) page at some appropriate place in the grid
@@ -380,6 +447,15 @@ function PageBrowserWidget:update()
             local widget = table.remove(self.grid, i)
             widget:free()
         end
+        -- We could restore the original offset and dimen, as pages may not all have
+        -- the same dimensions on PDFs. But it feels best to keep using the previous
+        -- thumbnail size as most often, the new thumbnail will have that size.
+        --[[
+        if self.grid[i] and self.grid[i].is_nav_item then
+            self.grid[i].overlap_offset = self.grid[i].initial_overlap_offset
+            self.grid[i][1].dimen = self.grid_item_dimen:copy()
+        end
+        ]]--
     end
 
     if not self.focus_page then
@@ -661,6 +737,11 @@ function PageBrowserWidget:update()
             end
         end
     end
+    if self.build_focus_layout then
+        self.selected.x = math.min(self.selected.x, self.nb_cols)
+        self.selected.y = math.min(self.selected.y, self.nb_rows)
+        self:moveFocusTo(self.selected.x, self.selected.y, FocusManager.FOCUS_ONLY_ON_NT)
+    end
     UIManager:setDirty(self, function()
         return "ui", self.dimen
     end)
@@ -799,6 +880,23 @@ function PageBrowserWidget:showTile(grid_idx, page, tile, do_refresh)
         table.insert(self.grid, page_num_widget)
     end
 
+    local prev_nav_item_dimen
+    if self.build_focus_layout then
+        -- Resize the focus navigation frame from the grid item size to the page thumbnail size
+        local nav_item_frame = self.grid[self.nb_grid_items + grid_idx]
+        prev_nav_item_dimen = nav_item_frame.dimen and nav_item_frame.dimen:copy()
+        local thumb_frame_dimen = thumb_frame:getSize():copy()
+        local dw = self.grid_item_width - thumb_frame_dimen.w
+        local dh = self.grid_item_height - thumb_frame_dimen.h
+        local dx = math.floor(dw/2)
+        local dy = math.floor(dh/2)
+        local offset_x = nav_item_frame.initial_overlap_offset[1] + dx
+        local offset_y = nav_item_frame.initial_overlap_offset[2] + dy
+        nav_item_frame.overlap_offset = {offset_x, offset_y}
+        nav_item_frame[1].dimen = thumb_frame_dimen
+        nav_item_frame.dimen = nil
+    end
+
     if do_refresh then
         if self.wait_for_refresh_on_show_tile then
             self.wait_for_refresh_on_show_tile = nil
@@ -812,10 +910,14 @@ function PageBrowserWidget:showTile(grid_idx, page, tile, do_refresh)
                 -- by a BookMap launched from the ribbon: don't refresh.
                 return
             end
+            local dimen = thumb_frame.dimen
             if page_num_widget then
-                return "ui", thumb_frame.dimen:combine(page_num_widget.dimen)
+                dimen = dimen:combine(page_num_widget.dimen)
             end
-            return "ui", thumb_frame.dimen
+            if prev_nav_item_dimen then
+                dimen = dimen:combine(prev_nav_item_dimen)
+            end
+            return "ui", dimen
         end)
     end
 end
@@ -862,7 +964,7 @@ function PageBrowserWidget:preloadNextPrevScreenThumbnails()
     end
 end
 
-function PageBrowserWidget:showMenu()
+function PageBrowserWidget:onShowMenu()
     local button_dialog
     -- Width of our -/+ buttons, so it looks fine with Button's default font size of 20
     local plus_minus_width = Screen:scaleBySize(60)
@@ -875,7 +977,7 @@ function PageBrowserWidget:showMenu()
             end,
         }},
         {{
-            text = _("Available gestures"),
+            text = Device:isTouchDevice() and _("Available gestures") or _("Controls"),
             align = "left",
             callback = function()
                 self:showGestures()
@@ -1018,13 +1120,13 @@ end
 function PageBrowserWidget:showAbout()
     UIManager:show(InfoMessage:new{
         text = _([[
-Page browser shows thumbnails of pages.
+Page browser shows thumbnails of a book's pages.
 
 The bottom ribbon displays an extract of the book map around the pages displayed:
 
-If statistics are enabled, black bars are shown for already read pages (gray for pages read in the current reading session). Their heights vary depending on the time spent reading the page.
-Chapters are shown above the pages they encompass.
-Under the pages, these indicators may be shown:
+If statistics are enabled, black bars indicate pages that have already been read (gray bars for pages read in the current session). The height of these bars varies based on the time spent reading each page.
+Chapters are indicated above the pages they cover.
+Below the pages, the following indicators may appear:
 ▲ current page
 ❶ ❷ … previous locations
 ▒ highlighted text
@@ -1034,7 +1136,8 @@ Under the pages, these indicators may be shown:
 end
 
 function PageBrowserWidget:showGestures()
-    UIManager:show(InfoMessage:new{
+    local text
+    if Device:isTouchDevice() then
         text = _([[
 Swipe along the top or left screen edge to change the number of columns or rows of thumbnails.
 
@@ -1044,12 +1147,33 @@ Swipe horizontally in the bottom ribbon to move by the full stripe.
 
 Tap in the bottom ribbon on a page to focus thumbnails on this page.
 
-Tap on a thumbnail to read this page.
+Tap on a thumbnail to read that page.
 
 Long-press on ≡ to decrease or reset the number of chapter levels shown in the bottom ribbon.
 
-Any multiswipe will close the page browser.]]),
-    })
+Any multiswipe will close the page browser.]])
+    elseif Device:hasKeyboard() then
+        local lines = {
+            _("The settings (in this menu) can be used to change the number of rows and columns, whether to display page numbers, and to display different chapter-levels in the bottom ribbon."),
+            _("Press Shift+Up to move up by one row, or either previous-page-turn-button to move one screen."),
+            _("Press Shift+Down to move down by one row, or either next-page-turn-button to move one screen."),
+            _("Press Shift+Press on a thumbnail, to open more options."),
+            _("Press Shift+Back closes all instances of Page Browser and Book Map."),
+            _("Select a thumbnail to read that page.")
+        }
+        text = table.concat(lines, "\n\n")
+    elseif Device:hasScreenKB() then
+        local lines = {
+            _("The settings (in this menu) can be used to change the number of rows and columns, whether to display page numbers, and to display different chapter-levels in the bottom ribbon."),
+            _("Press ScreenKB+Up to move up by one row, or either previous-page-turn-button to move one screen."),
+            _("Press ScreenKB+Down to move down by one row, or either next-page-turn-button to move one screen."),
+            _("Press ScreenKB+Press on a thumbnail, to open more options."),
+            _("Press ScreenKB+Back closes all instances of Page Browser and Book Map."),
+            _("Select a thumbnail to read that page.")
+        }
+        text = table.concat(lines, "\n\n")
+    end
+    UIManager:show(InfoMessage:new{text = text})
 end
 
 function PageBrowserWidget:onClose(close_all_parents)
@@ -1275,6 +1399,47 @@ function PageBrowserWidget:onScrollRowDown()
     return true
 end
 
+-- Override FocusManager internal methods, so we can scroll the view instead of wrap around
+function PageBrowserWidget:_wrapAroundY(dy)
+    if dy > 0 then
+        self:onScrollRowDown()
+    elseif dy < 0 then
+        self:onScrollRowUp()
+    end
+    return true
+end
+
+function PageBrowserWidget:_wrapAroundX(dx)
+    if dx < 0 then
+        if self.nb_rows == 1 then
+            -- With a single row, it's best to slide one item rather
+            -- than the full row and getting a bit lost
+            if self:updateFocusPage(-1, true) then
+                self:update()
+            end
+        elseif self.selected.y == 1 then
+            self:onScrollRowUp()
+            self.selected.x = self.nb_cols
+        else
+            self.selected.y = self.selected.y - 1
+            self.selected.x = self.nb_cols
+        end
+    elseif dx > 0 then
+        if self.nb_rows == 1 then
+            if self:updateFocusPage(1, true) then
+                self:update()
+            end
+        elseif self.selected.y == self.nb_rows then
+            self:onScrollRowDown()
+            self.selected.x = 1
+        else
+            self.selected.y = self.selected.y + 1
+            self.selected.x = 1
+        end
+    end
+    return true
+end
+
 function PageBrowserWidget:onSwipe(arg, ges)
     local direction = BD.flipDirectionIfMirroredUILayout(ges.direction)
 
@@ -1460,6 +1625,21 @@ function PageBrowserWidget:onTap(arg, ges)
 end
 
 function PageBrowserWidget:onHold(arg, ges)
+    if not ges.pos then
+        if self:getFocusItem() then
+            -- emulator: triggered by ContextMenu key with focused widget, no pos information in event
+            -- set pos to center of widget
+            local pos = self:getFocusItem().dimen:copy()
+            pos.x = pos.x + math.floor(pos.w / 2)
+            pos.y = pos.y + math.floor(pos.h / 2)
+            pos.w = 0
+            pos.h = 0
+            ges.pos = pos;
+        else
+            return false
+        end
+    end
+
     -- If hold in the bottom BookMapRow, open a new BookMapWidget
     -- and focus on this page. We'll show a rounded square below
     -- our current focus_page to help locating where we were (it's
@@ -1469,14 +1649,7 @@ function PageBrowserWidget:onHold(arg, ges)
     if ges.pos.y > Screen:getHeight() - self.row_height then
         local page = self.row[1]:getPageAtX(ges.pos.x)
         if page then
-            local extra_symbols_pages = {}
-            extra_symbols_pages[self.focus_page] = 0x25A2 -- white square with rounder corners
-            UIManager:show(BookMapWidget:new{
-                launcher = self,
-                ui = self.ui,
-                focus_page = page,
-                extra_symbols_pages = extra_symbols_pages,
-            })
+            self:openBookMap(page)
         end
         return true
     end
@@ -1503,11 +1676,23 @@ function PageBrowserWidget:onHold(arg, ges)
     return true
 end
 
+function PageBrowserWidget:openBookMap(page)
+    local extra_symbols_pages = {}
+    extra_symbols_pages[self.focus_page] = 0x25A2 -- white square with rounder corners
+    UIManager:show(BookMapWidget:new{
+        launcher = self,
+        ui = self.ui,
+        focus_page = page,
+        extra_symbols_pages = extra_symbols_pages,
+    })
+end
+
 function PageBrowserWidget:onThumbnailHold(page, ges)
     local handmade_toc_edit_enabled = self.ui.handmade:isHandmadeTocEnabled() and self.ui.handmade:isHandmadeTocEditEnabled()
     local handmade_hidden_flows_edit_enabled = self.ui.handmade:isHandmadeHiddenFlowsEnabled() and self.ui.handmade:isHandmadeHiddenFlowsEditEnabled()
-    if not handmade_toc_edit_enabled and not handmade_hidden_flows_edit_enabled then
+    if Device:isTouchDevice() and not handmade_toc_edit_enabled and not handmade_hidden_flows_edit_enabled then
         -- No other feature enabled: we can toggle bookmark directly
+        -- On NT, we need to add "Go to book map" so we will never be here.
         self.ui.bookmark:toggleBookmark(page)
         self:updateEditableStuff(true)
         return
@@ -1524,6 +1709,16 @@ function PageBrowserWidget:onThumbnailHold(page, ges)
             end,
         }},
     }
+    if not Device:isTouchDevice() then
+        table.insert(buttons, {{
+            text = _("Go to book map"),
+            align = "left",
+            callback = function()
+                UIManager:close(button_dialog)
+                self:openBookMap(page)
+            end,
+        }})
+    end
     if handmade_toc_edit_enabled then
         local has_toc_item = self.ui.handmade:hasPageTocItem(page)
         table.insert(buttons, {{