history: ship review changes (#1522)

Author: shakyShane

special-pages/package.json

@@ -10,7 +10,7 @@
     "build": "node index.mjs",
     "build.dev": "npm run build -- --env development",
     "lint-fix": "cd ../ && npm run lint-fix",
-    "test-unit": "node --test unit-test/translations.mjs unit-test/color.spec.mjs pages/duckplayer/unit-tests/embed-settings.mjs pages/new-tab/app/freemium-pir-banner/unit-tests/utils.spec.mjs",
+    "test-unit": "node --test \"unit-test/*\" \"pages/history/unit-tests/*\" \"pages/duckplayer/unit-tests/*\" \"pages/new-tab/app/freemium-pir-banner/unit-tests/*\"",
     "test-int": "npm run test-unit && npm run build.dev && playwright test --grep-invert '@screenshots'",
     "test-int-x": "npm run test-int",
     "test.screenshots": "npm run test-unit && npm run build.dev && playwright test --grep '@screenshots'",

special-pages/pages/history/app/components/App.jsx

@@ -17,12 +17,16 @@ import { useSearchCommitForRange } from '../global/hooks/useSearchCommitForRange
 import { useURLReflection } from '../global/hooks/useURLReflection.js';
 import { useSearchCommit } from '../global/hooks/useSearchCommit.js';
 import { useRangesData } from '../global/Providers/HistoryServiceProvider.js';
+import { usePlatformName } from '../types.js';
+import { useLayoutMode } from '../global/hooks/useLayoutMode.js';
 
 export function App() {
+    const platformName = usePlatformName();
     const mainRef = useRef(/** @type {HTMLElement|null} */ (null));
     const { isDarkMode } = useEnv();
     const ranges = useRangesData();
     const query = useQueryContext();
+    const mode = useLayoutMode();
 
     /**
      * Handlers that are global in nature
@@ -58,7 +62,7 @@ export function App() {
     }, [onKeyDown, query]);
 
     return (
-        <div class={styles.layout} data-theme={isDarkMode ? 'dark' : 'light'}>
+        <div class={styles.layout} data-theme={isDarkMode ? 'dark' : 'light'} data-platform={platformName} data-layout-mode={mode}>
             <aside class={styles.aside}>
                 <Sidebar ranges={ranges} />
             </aside>

special-pages/pages/history/app/components/App.module.css

@@ -10,8 +10,19 @@ body {
 }
 
 .layout {
+    --sidebar-width: 250px;
+    --main-padding-left: 48px;
+    --main-padding-right: 76px;
+    --windows-scrollbar: 15px;
+
+    &[data-layout-mode="reduced"] {
+        --sidebar-width: 230px;
+        --main-padding-left: 28px;
+        --main-padding-right: 56px;
+    }
+
     display: grid;
-    grid-template-columns: 250px 1fr;
+    grid-template-columns: var(--sidebar-width) 1fr;
     grid-template-rows: max-content 1fr;
     grid-template-areas:
             'aside header'
@@ -22,8 +33,8 @@ body {
 }
 .header {
     grid-area: header;
-    padding-left: 48px;
-    padding-right: 76px;
+    padding-left: var(--main-padding-left);
+    padding-right: var(--main-padding-right);
 }
 .search {
     justify-self: flex-end;
@@ -36,24 +47,17 @@ body {
 .main {
     grid-area: main;
     overflow: auto;
-    padding-left: 48px;
-    padding-right: 76px;
+    padding-left: var(--main-padding-left);
+    padding-right: var(--main-padding-right);
     padding-top: 24px;
 }
-
 .customScroller {
     overflow-y: scroll;
-    &::-webkit-scrollbar {
-        width: 12px;
-    }
-
-    &::-webkit-scrollbar-track {
-        border-radius: 6px;
-    }
+    scrollbar-gutter: stable;
+}
 
-    &::-webkit-scrollbar-thumb {
-        background: rgb(108, 108, 108);
-        border: 4px solid var(--history-background-color);
-        border-radius: 6px;
+[data-platform="windows"] {
+    .customScroller {
+        padding-right: calc(var(--main-padding-right) - var(--windows-scrollbar));
     }
 }

special-pages/pages/history/app/components/Header.js

@@ -84,7 +84,7 @@ function Controls({ term, range, domain }) {
 
     return (
         <div class={styles.controls}>
-            <button class={styles.largeButton} onClick={onClick} aria-disabled={ariaDisabled} title={title}>
+            <button class={styles.largeButton} onClick={onClick} aria-disabled={ariaDisabled} title={title} tabindex={0}>
                 <Trash />
                 <span>{buttonTxt}</span>
             </button>

special-pages/pages/history/app/components/Item.js

@@ -53,11 +53,11 @@ export const Item = memo(
                     <a href={props.url} data-url={props.url} class={styles.entryLink} tabindex={0}>
                         {title}
                     </a>
-                    <span class={styles.domain} data-testid="Item.domain">
+                    <span class={styles.domain} data-testid="Item.domain" title={props.domain}>
                         {props.domain}
                     </span>
                     <span class={styles.time}>{dateTimeOfDay}</span>
-                    <button class={styles.dots} data-action={BTN_ACTION_ENTRIES_MENU} data-index={index} value={props.id} tabindex={0}>
+                    <button class={styles.dots} data-action={BTN_ACTION_ENTRIES_MENU} data-index={index} value={props.id} tabindex={-1}>
                         <Dots />
                     </button>
                 </div>

special-pages/pages/history/app/components/Item.module.css

@@ -1,8 +1,10 @@
 .item {
+
 }
 
 .title {
-    height: 32px;
+    --title-height: 32px;
+    height: var(--title-height);
     width: 100%;
     font-size: var(--title-3-em-font-size);
     font-weight: var(--title-3-em-font-weight);
@@ -17,7 +19,8 @@
 }
 
 .row {
-    height: 28px;
+    --row-height: 28px;
+    height: var(--row-height);
     display: flex;
     gap: 8px;
     align-items: center;
@@ -39,7 +42,7 @@
     color: var(--row-color);
     border-radius: var(--row-radius);
 
-    &:hover, &:focus-within {
+    &:hover {
         --dots-opacity: visible;
         --time-opacity: 0;
         --time-visibility: hidden;
@@ -63,28 +66,31 @@
     }
 }
 
-/**
- * The following code handles handles the multi-row selections. It's responsible
- * for ensuring only the first and last elements in a selection have rounded corners.
- */
-[data-is-selected='true'] .row {
-    border-radius: 0;
-}
+@supports selector(:has(*)) {
+    /**
+     * The following code handles handles the multi-row selections. It's responsible
+     * for ensuring only the first and last elements in a selection have rounded corners.
+     */
+    [data-is-selected='true'] .row {
+        border-radius: 0;
+    }
 
-[data-is-selected='true']:first-of-type .row,
-[data-is-selected='true']:not([data-is-selected='true'] + [data-is-selected='true']) .row
-{
-    border-top-left-radius: var(--row-radius);
-    border-top-right-radius: var(--row-radius);
-}
+    [data-is-selected='true']:first-of-type .row,
+    [data-is-selected='true']:not([data-is-selected='true'] + [data-is-selected='true']) .row
+    {
+        border-top-left-radius: var(--row-radius);
+        border-top-right-radius: var(--row-radius);
+    }
 
-[data-is-selected='true']:last-of-type .row,
-[data-is-selected='true']:not(:has(+ [data-is-selected='true'])) .row
-{
-    border-bottom-left-radius: var(--row-radius);
-    border-bottom-right-radius: var(--row-radius);
+    [data-is-selected='true']:last-of-type .row,
+    [data-is-selected='true']:not(:has(+ [data-is-selected='true'])) .row
+    {
+        border-bottom-left-radius: var(--row-radius);
+        border-bottom-right-radius: var(--row-radius);
+    }
 }
 
+
 .favicon {
     flex-shrink: 0;
     min-width: 0;
@@ -99,6 +105,7 @@
     text-overflow: ellipsis;
     text-decoration: none;
     color: inherit;
+    line-height: var(--row-height);
 }
 
 .domain {
@@ -110,7 +117,7 @@
     overflow: hidden;
     text-overflow: ellipsis;
 
-    @media screen and (min-width: 800px) {
+    [data-layout-mode="normal"] & {
         flex-shrink: 0;
     }
 
@@ -124,6 +131,8 @@
     flex-shrink: 0;
     opacity: var(--time-opacity);
     visibility: var(--time-visibility);
+    font-feature-settings: "tnum";
+    font-variant-numeric: tabular-nums;
 }
 
 .dots {
@@ -148,10 +157,6 @@
     &:hover {
         background: var(--dots-bg-hover);
     }
-
-    &:focus-visible {
-        opacity: 1;
-    }
 }
 
 .last {

special-pages/pages/history/app/components/Results.js

@@ -4,17 +4,31 @@ import { Item } from './Item.js';
 import styles from './VirtualizedList.module.css';
 import { VisibleItems } from './VirtualizedList.js';
 import { Empty } from './Empty.js';
-import { useSelected } from '../global/Providers/SelectionProvider.js';
+import { useSelected, useSelectionState } from '../global/Providers/SelectionProvider.js';
 import { useHistoryServiceDispatch, useResultsData } from '../global/Providers/HistoryServiceProvider.js';
-import { useCallback } from 'preact/hooks';
+import { useCallback, useEffect } from 'preact/hooks';
 
 /**
  * Access global state and render the results
  */
 export function ResultsContainer() {
     const results = useResultsData();
     const selected = useSelected();
+    const selectionState = useSelectionState();
     const dispatch = useHistoryServiceDispatch();
+
+    /**
+     * When the selection state changed in a way that might cause an element to be off-screen, re-focus it
+     */
+    useEffect(() => {
+        return selectionState.subscribe(({ lastAction, focusedIndex }) => {
+            if (lastAction === 'move-selection' || lastAction === 'inc-or-dec-selected') {
+                const match = document.querySelector(`[aria-selected][data-index="${focusedIndex}"]`);
+                match?.scrollIntoView({ block: 'nearest', inline: 'nearest' });
+            }
+        });
+    }, [selectionState]);
+
     /**
      * Let the history service know when it might want to load more
      */

special-pages/pages/history/app/components/SearchForm.js

@@ -101,7 +101,7 @@ function useSearchShortcut(platformName) {
  * @param {string} [message]
  * @return {asserts condition}
  */
-function invariant(condition, message) {
+export function invariant(condition, message) {
     if (condition) return;
     if (message) throw new Error('Invariant failed: ' + message);
     throw new Error('Invariant failed');

special-pages/pages/history/app/components/Sidebar.js

@@ -80,44 +80,53 @@ export function Sidebar({ ranges }) {
             <h1 class={styles.pageTitle}>{t('page_title')}</h1>
             <nav class={styles.nav}>
                 {ranges.value.map((range) => {
-                    const { buttonLabel, linkLabel } = labels(range, t);
-                    return (
-                        <div class={styles.item} key={range}>
-                            <RowLink onClick={() => onClick(range)} current={current} range={range} label={linkLabel}>
-                                {titleMap[range](t)}
-                            </RowLink>
-                            {range === 'all' && (
-                                <DeleteAllButton onClick={onDelete} ariaLabel={buttonLabel} range={range} ranges={ranges} count={count} />
-                            )}
-                            {range !== 'all' && <DeleteButton onClick={() => onDelete(range)} label={buttonLabel} range={range} />}
-                        </div>
-                    );
+                    return <Item onClick={onClick} onDelete={onDelete} current={current} range={range} ranges={ranges} count={count} />;
                 })}
             </nav>
         </div>
     );
 }
 
-function RowLink({ range, current, label, children, onClick }) {
+/**
+ * A component that renders a list item with optional delete actions and a link.
+ *
+ * @param {Object} props
+ * @param {import('@preact/signals').ReadonlySignal<Range|null>} props.current The current selection with a value property.
+ * @param {Range} props.range The range represented by this item.
+ * @param {(range: Range) => void} props.onClick Callback function triggered when the range is clicked.
+ * @param {(range: Range) => void} props.onDelete Callback function triggered when the delete action is clicked.
+ * @param {import("@preact/signals").Signal<Range[]>} props.ranges
+ * @param {import('@preact/signals').ReadonlySignal<number>} props.count The count value associated with the ranges.
+ */
+function Item({ current, range, onClick, onDelete, ranges, count }) {
+    const { t } = useTypedTranslation();
+    const { buttonLabel, linkLabel } = labels(range, t);
     const classNames = useComputed(() => {
-        return cn(styles.link, current.value === range && styles.active);
+        if (range === 'all' && current.value === null) {
+            return cn(styles.item, styles.active);
+        }
+        return cn(styles.item, current.value === range && styles.active);
     });
+
     return (
-        <a
-            href="#"
-            aria-label={label}
-            class={classNames}
-            tabindex={0}
-            onClick={(e) => {
-                e.preventDefault();
-                onClick(range);
-            }}
-        >
-            <span class={styles.icon}>
-                <img src={iconMap[range]} />
-            </span>
-            {children}
-        </a>
+        <div class={classNames} key={range}>
+            <button
+                aria-label={linkLabel}
+                className={styles.link}
+                tabIndex={0}
+                onClick={(e) => {
+                    e.preventDefault();
+                    onClick(range);
+                }}
+            >
+                <span className={styles.icon}>
+                    <img src={iconMap[range]} />
+                </span>
+                {titleMap[range](t)}
+            </button>
+            {range === 'all' && <DeleteAllButton onClick={onDelete} ariaLabel={buttonLabel} range={range} ranges={ranges} count={count} />}
+            {range !== 'all' && <DeleteButton onClick={() => onDelete(range)} label={buttonLabel} range={range} />}
+        </div>
     );
 }