Repository navigation
2403: fix pinned tab new tab placement bug - #2470
Open
the-nelsonator wants to merge 1 commit into
Open
the-nelsonator wants to merge 1 commit into
the-nelsonator wants to merge 1 commit into
Conversation
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.
Closes #2403
Problem
With a pinned tab active, a new tab created by the browser lands at the position of the general rule ("Place new tab (general rule)") instead of the position configured in its "If active tab is pinned" sub-option. Most visibly: middle-clicking a bookmark in the native bookmarks toolbar from a pinned tab opens the tab at panel end, even though both pinned-related settings default to
panel start.Root cause
In
getIndexForNewTab()(src/services/tabs.fg.create.ts), themoveNewTabActivePin/moveNewTabButtonActivePinoverride was gated behind two conditions that don't hold in this scenario:before/after/first_child/last_childbranches. With the default general rulepanel end, the function returnsnextIndexbefore reaching any of them, so the override was never read. The setup page also greys the field out forstart/end, while still showing its (default) value ofpanel start, so the setting looks like it should apply.activeTab.panelId === panel.id. For globally pinned tabs (pinnedTabsPosition: 'top'— now the default for new users) a pinned tab'spanelIdis just the last panel it happened to be associated with and carries no meaning. When it didn't match the target panel,if (!activeTab || activeTab.panelId !== panel.id) return nextIndexfired first and the tab went to panel end.The other pinned setting,
moveNewTabPin("Place new tab opened from pinned tab", defaultstart), is keyed onparent.pinned, and a tab middle-clicked from the bookmarks toolbar has noopenerTabId(Firefox opens places links withrelatedToCurrent: false, which is also why the tab appears at the end of the tab strip). So that rule is skipped as well and placement falls through to the general rule.How to reproduce
A — deterministic, stock settings, single panel:
panel end, "If active tab is pinned" =panel start, greyed out).→ The tab lands at panel end. Sidebery's own entry points (new-tab button, bookmarks/history panels, middle-click inside the sidebar) can't reproduce it, because they pass an explicit destination or an opener.
B — the intermittent variant (needs >= 2 tabs panels and a relative general rule such as
after), which explains why unpinning/repinning appears to fix it:panel start.panelIdisA; switch to panel B, activate the pinned tab, open a bookmark from the toolbar → it goes to B's end, override ignored.tab.panelId = <active panel>(tabs.fg.handlers.ts:1000) — and the override starts working for that tab.restoreTab()reassigns every global pinned tab to the first tabs panel (tabs.fg.ts:533-535), or simply switch to another panel, and the misplacement returns.Changes
src/services/tabs.fg.create.ts— lifted the pinned-active-tab check out of the four relative branches so it overrides every general rule exceptnone("do not move it"), and addedisPinnedTabRelatedToPanel()so thepanelIdcomparison is only applied when pinned tabs actually live in panels (pinnedTabsPosition === 'panel'). Covers bothmoveNewTabActivePinandmoveNewTabButtonActivePin. The now-unreachable per-branch copies of the check were removed.src/page.setup/components/settings.tabs.vue— the override field is no longer greyed out forstart/end, since it now applies there; it is inactive only fornone.newTabPosRelativeToActiveTab/newTabBtnPosRelativeToActiveTabbecamenewTabPosDependsOnActiveTab/newTabBtnPosDependsOnActiveTab.src/services/tabs.fg.create.test.ts(new) — 10 tests forgetIndexForNewTab().