diff --git a/.claude/context/architecture.md b/.claude/context/architecture.md index bc60a55..cb533ce 100644 --- a/.claude/context/architecture.md +++ b/.claude/context/architecture.md @@ -297,13 +297,21 @@ bare system prompt gives no context on its own. matching `Scaffold.appBar`'s old `pickRequest == null` guard - see `buildAppTabView`'s doc comment. It sits above each tab's own pinned in-content header (the sort/filter controls row, or Files/Photos' - selection bar - see below); the two float/scroll independently. - `BottomNavBar` (`widgets/bottom_nav_bar.dart`) adapts - the pinned `AppTab`s onto `NooBottomBar`. `AppDrawer` + selection bar - see below); the two float/scroll independently - each + tab also wraps its whole `CustomScrollView` in `SafeArea(top: true, + bottom: false, ...)` so that pinned header stays clear of the status bar + once the floating top bar above it fully collapses (see + `topBarSliver`'s own doc comment for why that reservation can't live + inside the top bar itself). `BottomNavBar` (`widgets/bottom_nav_bar.dart`) + adapts the pinned `AppTab`s onto `NooBottomBar`. `AppDrawer` (`widgets/app_drawer.dart`) builds a `NooDrawer`: account block, storage meter, a "More" list of the hidden tabs, Settings, and an "Edit tabs" link (opens Settings - there's no in-page anchor to scroll to its Tabs - section yet). + section yet). `SettingsController.navMenuStyle` (Settings → Appearance → + "Navigation menu") offers an alternative to the hamburger/drawer pair: + the avatar button opens `showAvatarMenu` (`widgets/avatar_menu.dart`) + instead, a dropdown holding the same hidden-tabs + Settings content - + see `styling.md`'s Gotchas for the wiring. - **Desktop:** a `NooSidebar` (account card, pinned tabs, divider, remaining tabs, storage meter, Settings) sits beside a `NooToolbar` (tab title, search, an "Upload" action on Files/Photos) over the same diff --git a/.claude/context/styling.md b/.claude/context/styling.md index c72088c..2e7ac7a 100644 --- a/.claude/context/styling.md +++ b/.claude/context/styling.md @@ -236,7 +236,16 @@ blocks are noted where they matter: not a modal flow) - there's no existing anchored-popup primitive here (`PopupMenuButton`'s own width doesn't stretch to a full content column), so don't reach for `showNooSheet`/`showNooDialog` for something shaped - like this. Also added `NooTopBar.androidTitleReplacement`: Android has no + like this. It's positioned just past the status bar (`SafeArea`'s own + inset, not the top bar's full height on top of that) so it covers the + top bar - including the tab title - rather than sitting below it, and + its card carries two stacked `boxShadow`s rather than just + `nooDialogShadow` alone: that one shadow's blur is wide and soft enough + to read as basically invisible on a small card over a dark theme's + near-black `bg` (a dark, diffuse shadow needs real density close to the + edge to be visible against an already-dark backdrop), so a second, + tighter, more opaque contact shadow underneath it gives real elevation + in both themes. Also added `NooTopBar.androidTitleReplacement`: Android has no large title to put a second search row under the way iOS's `search:` slot does, so an inline search bar (`AppTopBar` passes a plain `ShellSearchLauncher()` when search isn't in the bottom bar) replaces the diff --git a/lib/views/activity_view.dart b/lib/views/activity_view.dart index d29eff1..ea91537 100644 --- a/lib/views/activity_view.dart +++ b/lib/views/activity_view.dart @@ -100,12 +100,19 @@ class ActivityView extends StatelessWidget { ...tabBottomInsetSlivers(context), ]; - final scrollView = CustomScrollView( - controller: scrollController, - // See files_view.dart's identical fix - without this, pull-to- - // refresh can't be triggered on an empty or single-item feed. - physics: const AlwaysScrollableScrollPhysics(), - slivers: contentSlivers, + // See files_view.dart's identical fix - without this, the sticky + // controls row rides up under the status bar once the floating top bar + // above it fully collapses. + final scrollView = SafeArea( + top: true, + bottom: false, + child: CustomScrollView( + controller: scrollController, + // See files_view.dart's identical fix - without this, pull-to- + // refresh can't be triggered on an empty or single-item feed. + physics: const AlwaysScrollableScrollPhysics(), + slivers: contentSlivers, + ), ); return ColoredBox( diff --git a/lib/views/favorites_view.dart b/lib/views/favorites_view.dart index 0608cb8..df68879 100644 --- a/lib/views/favorites_view.dart +++ b/lib/views/favorites_view.dart @@ -658,12 +658,19 @@ class _FavoritesViewState extends State { color: colors.accent, backgroundColor: colors.surface, onRefresh: favoritesController.fetchAll, - child: CustomScrollView( - controller: widget.scrollController, - // See files_view.dart's identical fix - without this, pull-to- - // refresh can't be triggered on an empty or single-item list. - physics: const AlwaysScrollableScrollPhysics(), - slivers: contentSlivers, + // See files_view.dart's identical fix - without this, the sticky + // controls row rides up under the status bar once the floating + // top bar above it fully collapses. + child: SafeArea( + top: true, + bottom: false, + child: CustomScrollView( + controller: widget.scrollController, + // See files_view.dart's identical fix - without this, pull-to- + // refresh can't be triggered on an empty or single-item list. + physics: const AlwaysScrollableScrollPhysics(), + slivers: contentSlivers, + ), ), ), ), diff --git a/lib/views/files_view.dart b/lib/views/files_view.dart index 0595328..02d2946 100644 --- a/lib/views/files_view.dart +++ b/lib/views/files_view.dart @@ -853,14 +853,30 @@ class _FilesViewState extends State { unawaited(sync.syncOnPull()); return browser.reload(); }, - child: CustomScrollView( - controller: widget.scrollController, - // Pull-to-refresh needs a scroll physics that allows dragging - // past the edge even when content doesn't fill the viewport - - // an empty or single-item list otherwise can't be pulled at all - // under the platform default physics. - physics: const AlwaysScrollableScrollPhysics(), - slivers: contentSlivers, + // `topBarSliver`'s floating header can collapse all the way to + // zero height (fully scrolled away), at which point the sticky + // controls row right below it in `contentSlivers` would otherwise + // ride up underneath the status bar instead of stopping below it + // - the floating top bar used to be the only thing reserving that + // space (via its own internal `SafeArea`), and that reservation + // disappears along with it once it's fully hidden. Wrapping the + // whole scroll view keeps the inset outside the scrolling region + // entirely, so it's never implicated in the floating header's own + // collapse/reveal math - safe to apply unconditionally, since + // desktop's `MediaQuery.padding.top` is 0 anyway (no topBar / no + // status bar there). + child: SafeArea( + top: true, + bottom: false, + child: CustomScrollView( + controller: widget.scrollController, + // Pull-to-refresh needs a scroll physics that allows dragging + // past the edge even when content doesn't fill the viewport - + // an empty or single-item list otherwise can't be pulled at + // all under the platform default physics. + physics: const AlwaysScrollableScrollPhysics(), + slivers: contentSlivers, + ), ), ), ), diff --git a/lib/views/photos_view.dart b/lib/views/photos_view.dart index b482446..7b02541 100644 --- a/lib/views/photos_view.dart +++ b/lib/views/photos_view.dart @@ -369,12 +369,19 @@ class _PhotosViewState extends State { color: colors.accent, backgroundColor: colors.surface, onRefresh: photosController.fetchAllMedia, - child: CustomScrollView( - controller: widget.scrollController, - // See files_view.dart's identical fix - without this, pull-to- - // refresh can't be triggered on an empty or single-item list. - physics: const AlwaysScrollableScrollPhysics(), - slivers: contentSlivers, + // See files_view.dart's identical fix - without this, the sticky + // controls row rides up under the status bar once the floating + // top bar above it fully collapses. + child: SafeArea( + top: true, + bottom: false, + child: CustomScrollView( + controller: widget.scrollController, + // See files_view.dart's identical fix - without this, pull-to- + // refresh can't be triggered on an empty or single-item list. + physics: const AlwaysScrollableScrollPhysics(), + slivers: contentSlivers, + ), ), ), ), diff --git a/lib/views/recent_view.dart b/lib/views/recent_view.dart index 7a6fa9f..ce2b82d 100644 --- a/lib/views/recent_view.dart +++ b/lib/views/recent_view.dart @@ -88,12 +88,19 @@ class _RecentViewState extends State { color: colors.accent, backgroundColor: colors.surface, onRefresh: recent.fetchAll, - child: CustomScrollView( - controller: widget.scrollController, - // See files_view.dart's identical fix - without this, pull-to- - // refresh can't be triggered on an empty or single-item list. - physics: const AlwaysScrollableScrollPhysics(), - slivers: contentSlivers, + // See files_view.dart's identical fix - without this, the sticky + // controls row rides up under the status bar once the floating top + // bar above it fully collapses. + child: SafeArea( + top: true, + bottom: false, + child: CustomScrollView( + controller: widget.scrollController, + // See files_view.dart's identical fix - without this, pull-to- + // refresh can't be triggered on an empty or single-item list. + physics: const AlwaysScrollableScrollPhysics(), + slivers: contentSlivers, + ), ), ), ); diff --git a/lib/views/shares_view.dart b/lib/views/shares_view.dart index 7c85a43..ecb7f52 100644 --- a/lib/views/shares_view.dart +++ b/lib/views/shares_view.dart @@ -134,12 +134,19 @@ class _SharesViewState extends State { color: colors.accent, backgroundColor: colors.surface, onRefresh: sharesController.fetchAll, - child: CustomScrollView( - controller: widget.scrollController, - // See files_view.dart's identical fix - without this, pull-to- - // refresh can't be triggered on an empty or single-item list. - physics: const AlwaysScrollableScrollPhysics(), - slivers: contentSlivers, + // See files_view.dart's identical fix - without this, the sticky + // controls row rides up under the status bar once the floating top + // bar above it fully collapses. + child: SafeArea( + top: true, + bottom: false, + child: CustomScrollView( + controller: widget.scrollController, + // See files_view.dart's identical fix - without this, pull-to- + // refresh can't be triggered on an empty or single-item list. + physics: const AlwaysScrollableScrollPhysics(), + slivers: contentSlivers, + ), ), ), ); diff --git a/lib/views/trash_view.dart b/lib/views/trash_view.dart index ffad901..c48df6c 100644 --- a/lib/views/trash_view.dart +++ b/lib/views/trash_view.dart @@ -103,12 +103,19 @@ class _TrashViewState extends State { color: colors.accent, backgroundColor: colors.surface, onRefresh: trashController.fetchAll, - child: CustomScrollView( - controller: widget.scrollController, - // See files_view.dart's identical fix - without this, pull-to- - // refresh can't be triggered on an empty or single-item list. - physics: const AlwaysScrollableScrollPhysics(), - slivers: contentSlivers, + // See files_view.dart's identical fix - without this, the sticky + // controls row rides up under the status bar once the floating top + // bar above it fully collapses. + child: SafeArea( + top: true, + bottom: false, + child: CustomScrollView( + controller: widget.scrollController, + // See files_view.dart's identical fix - without this, pull-to- + // refresh can't be triggered on an empty or single-item list. + physics: const AlwaysScrollableScrollPhysics(), + slivers: contentSlivers, + ), ), ), ); diff --git a/lib/widgets/avatar_menu.dart b/lib/widgets/avatar_menu.dart index dccd3b7..1f1f23a 100644 --- a/lib/widgets/avatar_menu.dart +++ b/lib/widgets/avatar_menu.dart @@ -9,8 +9,6 @@ import '../theme/design_tokens.dart'; import 'noo/core/noo_avatar.dart'; import 'noo/core/noo_badge.dart'; import 'noo/lists/noo_settings_row.dart'; -import 'noo/nav/noo_top_bar.dart'; -import 'noo/noo_layout.dart'; import 'shell/shell_common.dart'; /// The dropdown [ShellAvatarButton] opens when @@ -24,14 +22,13 @@ import 'shell/shell_common.dart'; /// app to reuse (`PopupMenuButton`'s own width doesn't stretch to the full /// content column the way this needs to). Future showAvatarMenu(BuildContext context) { - final navStyle = NooLayout.navStyle(context); return showGeneralDialog( context: context, barrierColor: Colors.transparent, barrierDismissible: true, barrierLabel: 'Close menu', transitionDuration: NooMotion.fast, - pageBuilder: (context, _, _) => _AvatarMenuContent(navStyle: navStyle), + pageBuilder: (context, _, _) => const _AvatarMenuContent(), transitionBuilder: (context, animation, _, child) => FadeTransition( opacity: animation, child: ScaleTransition( @@ -47,9 +44,7 @@ Future showAvatarMenu(BuildContext context) { } class _AvatarMenuContent extends StatelessWidget { - final NooNavStyle navStyle; - - const _AvatarMenuContent({required this.navStyle}); + const _AvatarMenuContent(); @override Widget build(BuildContext context) { @@ -60,15 +55,6 @@ class _AvatarMenuContent extends StatelessWidget { final hiddenTabs = settings.tabOrder .where((t) => settings.hiddenTabs.contains(t)) .toList(); - // The row/avatar it opens from is always this tall + the status bar - // above it (the button that opens this can't be tapped while its own - // top bar is scrolled away, so it's always on-screen at this exact - // position when that happens) - matches `NooTopBar.preferredSize` - // exactly rather than a guessed constant. - final topBarHeight = NooTopBar( - style: navStyle, - title: '', - ).preferredSize.height; void closeAndOpenSettings() { Navigator.pop(context); @@ -77,15 +63,15 @@ class _AvatarMenuContent extends StatelessWidget { return Align( alignment: Alignment.topCenter, + // Just the status-bar inset, not the top bar's own height on top of + // it - the card covers the top bar (title included) rather than + // sitting below it, so opening it reads as the avatar growing into + // this instead of a separate element appearing underneath the row + // it came from. child: SafeArea( bottom: false, child: Padding( - padding: EdgeInsets.fromLTRB( - NooSpace.md, - topBarHeight, - NooSpace.md, - 0, - ), + padding: const EdgeInsets.symmetric(horizontal: NooSpace.md), child: SizedBox( width: double.infinity, child: Container( @@ -94,7 +80,24 @@ class _AvatarMenuContent extends StatelessWidget { color: colors.surface, border: Border.all(color: colors.line), borderRadius: BorderRadius.circular(NooRadii.card), - boxShadow: const [nooDialogShadow], + // `nooDialogShadow` alone is a wide, soft, fairly faint + // shadow - built for a desktop dialog with plenty of room + // to fall off into. On a small card over a dark theme's + // near-black `bg`, that falloff is too gradual to read as + // elevation at all (a dark shadow needs real density close + // to the edge to be visible against an already-dark + // backdrop). A second, tighter, more opaque contact shadow + // underneath it gives an immediate value-step right at the + // card's edge in both themes, with the soft one still + // doing the wider ambient falloff on top. + boxShadow: const [ + BoxShadow( + color: Color(0x40000000), + blurRadius: 12, + offset: Offset(0, 4), + ), + nooDialogShadow, + ], ), child: Material( color: Colors.transparent, diff --git a/lib/widgets/tabs/tab_state_slivers.dart b/lib/widgets/tabs/tab_state_slivers.dart index 6211445..d1a4e89 100644 --- a/lib/widgets/tabs/tab_state_slivers.dart +++ b/lib/widgets/tabs/tab_state_slivers.dart @@ -133,14 +133,31 @@ List tabBottomInsetSlivers(BuildContext context) => [ /// [SliverFloatingHeader] sizes itself from [topBar]'s own natural layout /// (like `SliverToBoxAdapter`) rather than a fixed extent declared up /// front - so [topBar]'s own internal `SafeArea` (see `NooTopBar`'s doc -/// comment) already accounts for the status-bar inset correctly, with no -/// extra height math needed here (unlike building this on the general- -/// purpose `SliverPersistentHeader` would have required). +/// comment) already accounts for the status-bar inset correctly while +/// [topBar] itself is visible, with no extra height math needed here +/// (unlike building this on the general-purpose `SliverPersistentHeader` +/// would have required). /// /// Sits above a tab's own pinned in-content header (built with /// [StickyHeaderDelegate] - the sort/filter controls row, or the selection /// bar that replaces it) - put this sliver first in `contentSlivers` so /// that header stays exactly where it already is, independent of whether /// [topBar] is currently shown or scrolled away. +/// +/// That pinned header needs its OWN protection from the status bar too, +/// though: [topBar]'s `SafeArea` only reserves space while [topBar] has +/// some height to put it in - once it's fully collapsed (0 height, [topBar] +/// scrolled all the way away), that reservation disappears with it, and +/// the pinned header would ride up underneath the status bar instead of +/// stopping below it (a real bug this shipped with once already - caught +/// by `tab_state_slivers_test.dart`'s regression test for it). Every tab +/// view wraps its whole `CustomScrollView` (this sliver, the pinned header, +/// and everything else) in `SafeArea(top: true, bottom: false, ...)` to +/// fix this - that reserves the inset outside the scrolling/collapsing +/// region entirely, so it's never implicated in this sliver's own +/// collapse math regardless of [topBar]'s current state. Flutter's +/// `SafeArea` nesting means this doesn't double the inset: the outer one +/// zeroes `MediaQuery.padding.top` for everything below it, so [topBar]'s +/// own inner `SafeArea` sees nothing left to add. Widget topBarSliver(PreferredSizeWidget topBar) => SliverFloatingHeader(child: topBar); diff --git a/test/widgets/tabs/tab_state_slivers_test.dart b/test/widgets/tabs/tab_state_slivers_test.dart index 1d989ad..6810e2e 100644 --- a/test/widgets/tabs/tab_state_slivers_test.dart +++ b/test/widgets/tabs/tab_state_slivers_test.dart @@ -32,6 +32,38 @@ class _FakeTopBar extends StatelessWidget implements PreferredSizeWidget { /// tree is currently pumped. Finder _header() => find.byType(SliverFloatingHeader, skipOffstage: false); +/// A minimal stand-in for each tab's own pinned sort/filter row +/// (`StickyHeaderDelegate`) - just needs to be a `SliverPersistentHeader` +/// with `pinned: true` below `topBarSliver`, same contract every real tab +/// view uses. +class _FakeStickyHeader extends StatelessWidget { + const _FakeStickyHeader(); + + static const double height = 48; + + @override + Widget build(BuildContext context) => const SizedBox( + height: height, + child: ColoredBox(color: Colors.red), + ); +} + +class _FakeStickyHeaderDelegate extends SliverPersistentHeaderDelegate { + @override + double get minExtent => _FakeStickyHeader.height; + @override + double get maxExtent => _FakeStickyHeader.height; + @override + Widget build( + BuildContext context, + double shrinkOffset, + bool overlapsContent, + ) => const _FakeStickyHeader(); + @override + bool shouldRebuild(covariant SliverPersistentHeaderDelegate oldDelegate) => + false; +} + Future _pumpHost(WidgetTester tester, ScrollController controller) { return tester.pumpWidget( MaterialApp( @@ -140,4 +172,82 @@ void main() { ); }, ); + + testWidgets( + 'a pinned header below topBarSliver stays clear of the status bar once ' + 'the floating bar fully collapses (regression: it used to ride up ' + 'underneath the status bar once the only thing reserving that space ' + "disappeared along with the bar's own height)", + (tester) async { + final controller = ScrollController(); + addTearDown(controller.dispose); + const statusBarHeight = 40.0; + + await tester.pumpWidget( + MediaQuery( + data: const MediaQueryData( + padding: EdgeInsets.only(top: statusBarHeight), + ), + child: MaterialApp( + theme: nooTheme(Brightness.light), + home: Scaffold( + // The fix under test: wrapping the scroll view (not just the + // top bar) in `SafeArea(top: true)` reserves the status-bar + // inset outside the scrolling/collapsing region entirely, so + // it's never implicated in `topBarSliver`'s own collapse math. + body: SafeArea( + top: true, + bottom: false, + child: CustomScrollView( + controller: controller, + slivers: [ + topBarSliver(const _FakeTopBar()), + SliverPersistentHeader( + pinned: true, + delegate: _FakeStickyHeaderDelegate(), + ), + SliverList( + delegate: SliverChildBuilderDelegate( + (context, index) => + SizedBox(height: 60, child: Text('Item $index')), + childCount: 40, + ), + ), + ], + ), + ), + ), + ), + ), + ); + + // Fully visible before any scroll - right below the reserved inset. + expect( + tester.getTopLeft(find.byType(_FakeStickyHeader)).dy, + _FakeTopBar.height + statusBarHeight, + ); + + // Scroll well past the top bar's own height so it collapses fully. + final gesture = await tester.startGesture(const Offset(200, 300)); + await gesture.moveBy(const Offset(0, -300)); + await tester.pump(); + await gesture.up(); + await tester.pump(); + + expect( + tester.renderObject(_header()).geometry!.paintExtent, + 0, + reason: + 'the top bar should be fully collapsed for this check to ' + 'mean anything', + ); + // The regression: without the fix, this would be 0 (or negative, + // scrolled up under the status bar) instead of sitting right at the + // reserved inset. + expect( + tester.getTopLeft(find.byType(_FakeStickyHeader)).dy, + statusBarHeight, + ); + }, + ); }