fix: honour per-screen tab-bar hiding on the custom-Column chrome path - #347
Open
shanerbaner82 wants to merge 1 commit into
Open
fix: honour per-screen tab-bar hiding on the custom-Column chrome path#347shanerbaner82 wants to merge 1 commit into
shanerbaner82 wants to merge 1 commit into
Conversation
#250) `tabBarOptions()->hidden()` and the `$hidesTabBar` shortcut had no effect unless the layout opted into native chrome. `NativeLayout::usesNativeChrome()` returns false by default, so a layout that defines `tabBar()` without also overriding it landed on the custom-Column path — the common case, and the one reported. `shouldHideTabBar()` had exactly one caller, inside wrapWithNativeChrome(), so nothing on the Column path could ever remove the bar: buildChromeColumn() appends `$tabBar->toElement()` unconditionally. NavBar already had the equivalent guard 24 lines earlier, which is what made the gap visible — `$hidesNavBar` worked where `$hidesTabBar` did not. This mirrors it: null the TabBar on the Column path, which is also what buildChromeColumn() needs to hand the bottom safe-area edge back to the wrapper. The native-chrome path is untouched — it keeps the bar config and folds `hide_tab_bar` onto the sentinel so the TabView survives for tab switching. The wire path was never at fault: NativeRootTabs maps `hideTabBar` → `hide_tab_bar`, and both NativeRootTabsRenderer.kt and .swift honour it. Coverage is why this shipped: the only existing test used ChromeTabsLayout (usesNativeChrome() = true), exercising just the working path, and only via the `$hidesTabBar` boolean — never the builder. Added: - tab hiding on the Column path, both spellings, plus a visible control - tab hiding via the tabBarOptions() builder on the native path assertTabBarHidden()/assertTabBarVisible() carried the same asymmetry — they only understood the native sentinel and hard-failed on the Column path. They now handle both, matching assertNavBarHidden(). Fixes #250 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Will this be merged soon? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #250.
The bug
No effect. Same for the
protected bool $hidesTabBar = true;shortcut.Cause
There are two chrome paths (
NativeComponent.php:539), and per-screen tab hiding was only wired into one of them:wrapWithNativeChrome()checksshouldHideTabBar()and foldshideTabBaronto the sentinel. Works.buildChromeColumn()appends$tabBar->toElement()unconditionally.shouldHideTabBar()had exactly one caller in the codebase — insidewrapWithNativeChrome()— so nothing on this path could ever remove the bar.Which path you get is the crux:
NativeLayout::usesNativeChrome()returnsfalseby default. A layout that definestabBar()without also overriding it lands on the broken path, which is the common case and matches the report.The asymmetry that makes this clearly a bug rather than a design choice: NavBar already had the equivalent guard, 24 lines earlier —
so
$hidesNavBar/navigationOptions()->hidden()worked where the TabBar equivalents did not.Fix
Mirror that guard in the TabBar branch. Nulling the bar (rather than passing a flag) is deliberate and matches the NavBar comment — "On the custom-Column path hiding is identical to the layout returning null" — and it's also what
buildChromeColumn()needs: it picks its safe-area variant from which bars exist (lines 726–730), so a null tab bar correctly hands the bottom edge back to the wrapper.The native-chrome path is untouched: it keeps the bar config and folds
hide_tab_baronto the sentinel, because the TabView has to survive for tab switching.Not the cause
The wire path was intact end to end, so despite the report being Android-only, neither renderer was at fault:
NativeRootTabs.php:109mapshideTabBar→hide_tab_barNativeRootTabsRenderer.kt:169,305reads it and skips the bottom barNativeRootTabsRenderer.swift:463does the sameTabBarOptions::hidden()/isHidden()were also correct.Why it shipped
The only test covering this (
TestingSuiteV2Test.php:223) usedChromeTabsLayout, whoseusesNativeChrome()returnstrue— so it exercised only the working path, and only through the$hidesTabBarboolean, never the builder. The Column path had zero coverage for tab hiding.Added:
tabBarOptions()builder on the native pathThe new Column-path test fails on
mainand passes with the fix. The new native-path builder test passes onmain— it's pinning down behavior that already worked but was untested, and it's what confirms the builder itself was never broken.assertTabBarHidden()/assertTabBarVisible()carried the same asymmetry as the source — they only understood the native sentinel and hard-failed with "No native tab chrome rendered" on the Column path. They now handle both, matchingassertNavBarHidden().Test run
Before: 901 tests / 3021 assertions / 0 failures. After: 903 / 3029 / 0 failures.
One caveat
The reporter didn't share their layout, so I can't be certain theirs doesn't override
usesNativeChrome(). If it does, their symptom has a different cause and this is a separate bug found while investigating. The default-falsebehavior makes this by far the likelier explanation, and the missing guard is a real bug regardless.🤖 Generated with Claude Code