Replace ScrollableSegmentedControl with our own page tab bar - #122
Merged
Conversation
andiwand
force-pushed
the
modernize/09-cd
branch
from
July 26, 2026 09:11
1e59eef to
b4f5f38
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1334276e72
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
#113 vendored ScrollableSegmentedControl because its upstream was archived in 2022 and it never gained SPM support, and noted that the alternative was reimplementing the page tabs ourselves. This does that, so 832 lines of third-party UIKit leave the repository. PageTabBar is ~180 lines against the same UICollectionView the vendored control used, and implements only the four things the document view ever asked for: text tabs, an underline under the selection, a horizontal scroll once they no longer fit, and .valueChanged on a tap. Two behaviour changes fall out of it: - Tabs are only as wide as their own title once the row has to scroll. The old control gave every tab the width of the longest one, so a single long sheet name pushed everything else off screen. While they all fit they still share the width evenly, which is what the common case looked like before. - Labels use .label/.secondaryLabel and the bar uses secondarySystemBackground instead of hardcoded dark grey, black and #F9F9F9, so the tabs are legible in dark mode. Tab cells are also accessibility elements now, with the selected trait. Adjacent fixes the rewrite made obvious: - documentPagesChanged appended segments and activated a fresh height constraint every time it ran, so a second announcement would have listed every page twice under conflicting constraints. Titles are assigned wholesale and the height constraint is created once. - The .valueChanged target was added in viewWillAppear, so a second appearance parsed the document twice per tap. It moves to viewDidLoad. - initialSelect is gone: selecting a tab in code no longer sends .valueChanged, following UISegmentedControl, so there is nothing to suppress. - doc.pageNames was force-unwrapped. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011MhKU2kWm1cPW4GBq9gon5
Two findings from the Codex review on #122. Sharing the width evenly is only fair while an even share is wide enough for the longest title. With "Q4 Revenue Forecast" next to "A" and "B" the three fit a 320 point bar comfortably, but an even third would truncate the long one for no reason. Tabs now take an even share only while that fits every title; failing that they keep their own widths and share out whatever is left over, so the row still fills without truncating. The tab bar's height was pinned at 40 points while the label scaled with Dynamic Type, so accessibility text sizes were clipped. PageTabBar now reports the height it needs -- which works out to the same 40 points at the default text size -- and the document view follows it, including when the text size changes while a document is open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011MhKU2kWm1cPW4GBq9gon5
andiwand
force-pushed
the
modernize/11-page-tab-bar
branch
from
July 26, 2026 09:21
1334276 to
9a47f44
Compare
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.
#113 vendored ScrollableSegmentedControl rather than drop CocoaPods support for it, and said the alternative was reimplementing the page tabs on
UICollectionViewin its own PR. This is that PR, so the 832 lines of third-party UIKit leave the repository.PageTabBar
OpenDocumentReader/PageTabBar.swiftis ~200 lines against the sameUICollectionViewthe vendored control was built on, and implements only what the document view ever asked for: text tabs, an underline under the selection, horizontal scrolling once they no longer fit, and.valueChangedon a tap. The storyboard's custom class and thesegmentedControloutlet are renamed accordingly.Tab widths follow one rule in three cases:
That is the first behaviour change: the old control gave every tab the width of the longest title, so a single long sheet name pushed the rest off screen — with the eight-sheet document I tested against, barely one and a half tabs were visible.
The second is dark mode. Labels are
.label/.secondaryLabeland the bar issecondarySystemBackground, instead of hardcodeddarkGray,blackand#F9F9F9. Tab cells are accessibility elements now too, with the selected trait, and the row scales with Dynamic Type — it is the same 40 points at the default text size and grows from there.Adjacent fixes the rewrite made obvious
documentPagesChangedappended segments and activated a fresh height constraint on every call, so a second announcement would have listed every page twice under conflicting constraints. Titles are assigned wholesale and the height constraint is created once..valueChangedtarget was added inviewWillAppear, so a second appearance parsed the document twice for a single tap. It moves toviewDidLoad.initialSelectis gone. Selecting a tab in code no longer sends.valueChanged— followingUISegmentedControl— so there is nothing to suppress.doc.pageNameswas force-unwrapped.Verification
ODR Full/Debug andODR Lite/Debug Lite build green on the rebased tree, which includes the GoogleMobileAds 13.7 bump from Bump github.com/googleads/swift-package-manager-google-mobile-ads from 12.14.0 to 13.7.0 #120. Clean build, no new warnings.Main.storyboard— that the outlet and the custom class actually resolve..odsthroughDocument/odrcore: all eight sheet names reached the bar, and selecting the fourth tab setdocument.page = 3and scrolled that tab into view. Rendered the bar in both appearances to check the underline, the even-width case and the scrolling case.One thing I could not automate on this machine: tapping the tabs by hand in the running app. The simulator is scriptable but
osascripthas no assistive-access permission here, so the interaction is covered by the tests above driving the real view hierarchy rather than by a literal finger on the screen.