Add Plus Button (2/3): declarative plus-button option for TabbedLocation [devtools/devtools-frontend : main]
Guangyue Xu (Gerrit)
Guangyue Xu added 8 comments![]( "Open in Gerrit")
Root.Runtime.experiments.register(
Root.ExperimentNames.ExperimentName.PLUS_BUTTON, 'Show "+" button on the tab strip for adding tools');
Guangyue XuPlease don't add new Runtime.experiments
Done. Replaced the legacy `Root.Runtime.experiments.register('plus-button', …)` with a `base::Feature`-backed gate per the doc:
Added `devToolsPlusButton: {enabled: boolean}` to `HostConfig` in `Runtime.ts`.
`ViewManager.createTabbedLocation` gates on `Root.Runtime.hostConfig.devToolsPlusButton?.enabled` before installing the button.
The `Settings → Experiments` toggle is wired via `Root.Runtime.experiments.registerHostExperiment({…, aboutFlag: 'devtools-plus-button', isEnabled: hostConfig.devToolsPlusButton?.enabled ?? false}) (mirrors durable-messages / jpeg-xl).`
The matching` kDevToolsPlusButton base::Feature` entry land in the Chromium CLs linked in the commit message (crrev.com/c/7853628, crrev.com/c/7857037). Once they land, the button can be enabled either via `chrome.exe --enable-features=DevToolsPlusButton` or via the `DevTools Settings → Experiments` toggle.
// Compare against the same exported constant the implementation
// uses, not a hard-coded English string.
assert.strictEqual(button.title, UI.PlusButton.DEFAULT_TITLE());Guangyue XuThis is not worth the complexity. You can test against the English string, test only run against the English locale
Guangyue XuDone. The default-tooltip assertion in the `installPlusButton` describe block now compares against the literal 'More tools' (no `i18nString` round-trip), matching the convention used elsewhere.
Done
export function populatePlusButtonMenu(Guangyue XuThis logic for gathering, filtering, and deduplicating views feels quite imperative. To follow our Model-View-Presenter (MVP) architecture, this business logic should be moved into a Presenter class. This separates the logic of 'what to show' from the legacy UI logic.
Guangyue XuDone. Extracted `PlusButtonPresenter` with a `buildModel(): PlusButtonMenuModel` method (lines ~138-235). All gathering / filtering / dedup / sort lives there; `populatePlusButtonMenu` is now a thin "view" that takes the model and pushes it into `ContextMenu`. Presenter is exported and unit-tested directly (`describe('PlusButtonPresenter')`).
Done
if (view.viewId() === 'issues-pane') {Guangyue XuYou can avoid the duplication here and correctly preserve `view.isPreviewFeature()` instead of hardcoding `false` by merging these two blocks:
```typescript
const isIssuesPane = view.viewId() === 'issues-pane';
addToolEntries.push({
title: view.title(),
jslogContext: view.viewId(),
isPreviewFeature: view.isPreviewFeature(),
action: () => {
if (isIssuesPane) {
Host.userMetrics.issuesPanelOpenedFrom(Host.UserMetrics.IssueOpener.HAMBURGER_MENU);
}
showView(view);
},
});
```Also, a quick question: is `HAMBURGER_MENU` the correct metric enum to use here, or should the Plus Button get its own enum value (like `MORE_TOOLS_MENU` or similar) to avoid mixing this data with the main DevTools three-dot menu?
Guangyue XuDone — merged the two branches; the issues-pane case now reuses `view.isPreviewFeature()` and only the UMA call is conditional on `isIssuesPane`.
On the `enum:` agreed, added a new `IssueOpener.MORE_TOOLS_MENU = 6` in `UserMetrics.ts:749-758` (with a JSDoc comment pointing to the matching Chromium `enum DevToolsIssuesPanelOpenedFrom` in `tools/metrics/histograms/metadata/dev/enums.xml`). The plus-button issues entry now records `MORE_TOOLS_MENU` instead of `HAMBURGER_MENU` so the dashboard doesn't conflate the two surfaces. Filing the matching Chromium-side `enums.xml` CL as a follow-up.
Done
// Skip views that are already shown as a visible tab — the user canGuangyue Xu`hasTab` returns true for both visible and hidden tabs. You might want to update this comment to clarify that hidden tabs are skipped here because they are already handled in the `overflowTabs` section above.
For example:
```typescript
// Skip views that already have a tab. Hidden tabs are listed in the overflow section,
// and visible tabs are accessible directly in the tab strip.
```
Done. Reworded to make the dedup intent explicit for both cases.
if (otherLocation) {Guangyue XuRemoved the `includeOtherMainLocation` opt-in: cross-location is the design intent, not a per-caller knob. Per the design doc, the plus button replaces the "More tools" ≫ button to make the experience symmetric across the main panel and drawer — "the context menu for the '+' button in the main panel would show the tools not already available in the main panel, and the same holds for the drawer panel." The presenter still gates the loop on `location ∈ {PANEL, DRAWER_VIEW}`, so non-main tabbed locations are unaffected.
Done
context.tabbedPane.element.appendChild(button);Guangyue XuWe should avoid using imperative APIs like `appendChild` to update the DOM. Instead, we should rely on orchestrated rendering of lit-html templates. The plus button should ideally be rendered via a declarative view function.
Guangyue XuDone. `installPlusButton` now uses a single `tabbedPane.element`). No more `document.createElement` / `setAttribute` / `appendChild`. The `Directives.ref` capture replaces the previous `querySelector` round-trip so we still hand the caller back the `MenuButton` instance.
Done
if (options?.plusButton) {Guangyue XuThis new feature must be guarded by an experiment flag. Please ensure that the plus button is only installed if the corresponding experiment is enabled, for example: `if (options?.plusButton && Root.Runtime.experiments.isEnabled('plus-button'))`.
Guangyue XuDone. Registered a `plus-button` experiment in `ExperimentNames.ts` (`PLUS_BUTTON = 'plus-button'`) and `MainImpl.ts`, and added it to the `DevtoolsExperiments` UMA enum (value 112, MAX bumped to 113). The install site in `ViewManager.ts:724` now reads:
```
if (options?.plusButton && Root.Runtime.experiments.isEnabled(Root.ExperimentNames.ExperimentName.PLUS_BUTTON)) { PlusButton.installPlusButton(...);}
```Also added a regression test in `ViewManager.test.ts` that flips the experiment and asserts the button mounts before `appendApplicableItems`. Test-only experiment registration mirrored in `RuntimeHelpers.ts`.
Done
Related details
- Danil Somsikov
- Piotr Paulski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Danil Somsikov (Gerrit)
Danil Somsikov voted Code-Review+1![]( "Open in Gerrit")
- Guangyue Xu
- Piotr Paulski
- Wolfgang Beyer
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guangyue Xu (Gerrit)
Guangyue Xu added 2 comments![]( "Open in Gerrit")
Root.Runtime.experiments.register(
Root.ExperimentNames.ExperimentName.PLUS_BUTTON, 'Show "+" button on the tab strip for adding tools');
Guangyue XuPlease don't add new Runtime.experiments
Done. Replaced the legacy `Root.Runtime.experiments.register('plus-button', …)` with a `base::Feature`-backed gate per the doc:
Added `devToolsPlusButton: {enabled: boolean}` to `HostConfig` in `Runtime.ts`.
`ViewManager.createTabbedLocation` gates on `Root.Runtime.hostConfig.devToolsPlusButton?.enabled` before installing the button.
The `Settings → Experiments` toggle is wired via `Root.Runtime.experiments.registerHostExperiment({…, aboutFlag: 'devtools-plus-button', isEnabled: hostConfig.devToolsPlusButton?.enabled ?? false}) (mirrors durable-messages / jpeg-xl).`
The matching` kDevToolsPlusButton base::Feature` entry land in the Chromium CLs linked in the commit message (crrev.com/c/7853628, crrev.com/c/7857037). Once they land, the button can be enabled either via `chrome.exe --enable-features=DevToolsPlusButton` or via the `DevTools Settings → Experiments` toggle.
Done
// Skip views that are already shown as a visible tab — the user canGuangyue Xu`hasTab` returns true for both visible and hidden tabs. You might want to update this comment to clarify that hidden tabs are skipped here because they are already handled in the `overflowTabs` section above.
For example:
```typescript
// Skip views that already have a tab. Hidden tabs are listed in the overflow section,
// and visible tabs are accessible directly in the tab strip.
```
Done. Reworded to make the dedup intent explicit for both cases.
- Piotr Paulski
- Wolfgang Beyer
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Wolfgang Beyer (Gerrit)
Wolfgang Beyer voted and added 3 comments![]( "Open in Gerrit")
Votes added by Wolfgang Beyer
| Code-Review | +1 |
3 comments
Thanks for adding the base::Feature gate!
Regarding the CL's main content, my opinion is that it's using comments a bit too much. I like having comments explaining the code, but I think they should focus on explaining the reasons 'why' something is done. The 'what' is being done, doesn't really need an explanation, it's already in the code itself.
/**
* Creates a `TabbedViewLocation`. The first four parameters are
* positional because they are common to almost every call site; the
* rarely-used trailing knobs (default tab, custom visibility predicate,
* custom `TabbedPane` factory, plus-button installation) are bundled
* into a single `options` bag so adding new optional knobs in the
* future does not bloat the positional signature.
*/nit: This is a common pattern, I don't think it needs a comment
/**
* Trailing options bag for {@link ViewManager.createTabbedLocation}.
* All fields are optional; the bag itself is optional too.
*/nit: I feel this is unnecessary
Related details
- Guangyue Xu
- Piotr Paulski
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guangyue Xu (Gerrit)
Guangyue Xu added 2 comments![]( "Open in Gerrit")
* Creates a `TabbedViewLocation`. The first four parameters are
* positional because they are common to almost every call site; the
* rarely-used trailing knobs (default tab, custom visibility predicate,
* custom `TabbedPane` factory, plus-button installation) are bundled
* into a single `options` bag so adding new optional knobs in the
* future does not bloat the positional signature.
*/nit: This is a common pattern, I don't think it needs a comment
Thanks — fair point. Did a pass on `PlusButton.ts` and `ViewManager.ts`: dropped JSDocs that just restate the type or parameter name, tightened the rest to focus on the "why". Specifically removed the `createTabbedLocation` and `TabbedLocationOptions` JSDocs you flagged.
* Trailing options bag for {@link ViewManager.createTabbedLocation}.
* All fields are optional; the bag itself is optional too.
*/nit: I feel this is unnecessary
- Piotr Paulski
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guangyue Xu (Gerrit)
Guangyue Xu voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
devtools-frontend-scoped@luci-project-accounts.iam.gserviceaccount.com (Gerrit)
devtools-fro...@luci-project-accounts.iam.gserviceaccount.com submitted the change with unreviewed changes![]( "Open in Gerrit")
Unreviewed changes
11 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: front_end/ui/legacy/PlusButton.ts
Insertions: 47, Deletions: 157.
@@ -26,28 +26,16 @@
const str_ = i18n.i18n.registerUIStrings('ui/legacy/PlusButton.ts', UIStrings);
const i18nString = i18n.i18n.getLocalizedString.bind(undefined, str_);
-/**
- * Configuration for the plus button installed at the trailing end of a
- * `TabbedPane`. Passed declaratively to
- * {@link ViewManager.createTabbedLocation} so the button is mounted before
- * the first layout pass and its width is reserved during the very first
- * overflow detection.
- */
+/** Declarative configuration for the plus button. */
export interface PlusButtonOptions {
- /** Tooltip and accessible name. Defaults to "More tools". */
title?: Platform.UIString.LocalizedString;
- /** Visual logging context. */
jslogContext?: string;
}
/**
- * Minimal `TabbedPane` surface read by {@link populatePlusButtonMenu} and
- * {@link installPlusButton}. Defined as an interface so test doubles can
- * satisfy it without the `as unknown as TabbedPane` double-cast and so
- * the populator's dependency on `TabbedPane` is documented at the type
- * level. `element` is included only because {@link installPlusButton}
- * uses it as the parent for the slotted plus button; the populator
- * itself never reads it.
+ * Minimal `TabbedPane` surface read by the populator. Defined as an
+ * interface so test doubles can satisfy it without an `as unknown as
+ * TabbedPane` double-cast.
*/
export interface PlusButtonTabbedPane {
element: HTMLElement;
@@ -58,43 +46,20 @@
selectTab(tabId: string, userGesture?: boolean, forceFocus?: boolean): boolean;
}
-/**
- * Inputs for {@link populatePlusButtonMenu}. The populator queries
- * `tabbedPane.hiddenTabs()` and `tabbedPane.hasTab()` lazily on each
- * invocation, so callers do not need to mirror state in response to
- * `overflow-tabs-changed`.
- */
export interface PlusButtonMenuContext {
- /** TabbedPane the plus button belongs to. */
tabbedPane: PlusButtonTabbedPane;
- /** Location name of `tabbedPane` (e.g. `'panel'`, `'drawer-view'`). */
location: string;
/**
- * Returns the views the parent `TabbedLocation` knows about. Production
- * callers pass `() => location.views.values()` (the location's local
- * view registry, which includes views moved in via `appendView`) and
- * NOT `manager.viewsForLocation(location)`. The two are usually the
- * same set but diverge for cross-location moves: a view moved to this
- * location appears in `location.views` immediately while
- * `manager.viewsForLocation` reflects the original registration.
- * Called fresh on every menu open so newly-registered views (or views
- * that were just removed from the visible tab strip) are reflected
- * immediately without the caller having to re-install the button.
+ * Production callers pass `() => location.views.values()` (NOT
+ * `manager.viewsForLocation(location)`) so views moved in via
+ * `appendView` are reflected immediately. Called fresh on every
+ * menu open.
*/
views: () => Iterable<View>;
- /**
- * Used for cross-location lookups and moves. Inlined to avoid coupling
- * to the full `ViewManager` surface and to keep test doubles small.
- */
manager: {
viewsForLocation(location: string): View[],
moveView(viewId: string, locationName: string): void,
};
- /**
- * Reveals a view in this location. Wraps `TabbedLocation.showView` so the
- * populator does not need to know about `insertBefore` / `userGesture` —
- * the populator always invokes it as a user gesture.
- */
showView: (view: View) => void;
}
@@ -105,40 +70,21 @@
action: () => void;
}
-/**
- * Read-only view of a hidden tab as exposed by the {@link PlusButtonPresenter}
- * model — `id`, display `title`, and the optional jslog context. Exported
- * because tests assert against the presenter output directly.
- */
export interface OverflowTabModel {
id: string;
title: string;
jslogContext?: string;
}
-/**
- * The output of {@link PlusButtonPresenter.buildModel}. Captures the two
- * sections of the plus-button menu (overflow tabs in tab order, and
- * "add tool" entries already filtered, deduplicated and sorted) without
- * referencing any UI primitives.
- */
export interface PlusButtonMenuModel {
overflowTabs: readonly OverflowTabModel[];
addToolEntries: readonly AddToolEntry[];
}
/**
- * Presenter (in the Model-View-Presenter sense) for the plus-button menu.
- *
- * Stateless apart from the context / options bound at construction;
- * every {@link buildModel} call re-queries the source of truth via the
- * bound context (so newly-registered views or views that just left the
- * visible tab strip are reflected immediately). The output of
- * `buildModel` is the "model" — a plain data structure describing what
- * to show. Rendering that model into a {@link ContextMenu} is the
- * responsibility of {@link populatePlusButtonMenu} (the "view" side),
- * keeping the filtering / dedup / sort business logic out of the
- * legacy UI layer.
+ * Presenter (MVP) for the plus-button menu. {@link buildModel} is called
+ * fresh on every menu open so newly-registered views — or views that
+ * just left the visible tab strip — are reflected immediately.
*/
export class PlusButtonPresenter {
readonly #context: PlusButtonMenuContext;
@@ -147,21 +93,15 @@
this.#context = context;
}
- /**
- * Computes the menu model. Called fresh on every menu open via
- * {@link populatePlusButtonMenu} so newly-registered views — or views
- * that just left the visible tab strip — are reflected immediately
- * without the caller having to re-install the button.
- */
buildModel(): PlusButtonMenuModel {
const {tabbedPane, location, views, manager} = this.#context;
const overflowTabs: readonly OverflowTabModel[] =
tabbedPane.hiddenTabs().map(tab => ({id: tab.id, title: tab.title, jslogContext: tab.jslogContext}));
const addToolEntries: AddToolEntry[] = [];
- // Seed dedup sets from the overflowed tabs above so an addable entry
- // (e.g. a closeable view in the other main location) sharing an id or
- // title with an overflowed tab does not get listed twice.
+ // Seed dedup sets from the overflowed tabs so an addable entry
+ // (e.g. a closeable view in the other main location) sharing an id
+ // or title with an overflowed tab is not listed twice.
const seenIds = new Set<string>(overflowTabs.map(tab => tab.id));
const seenTitles = new Set<string>(overflowTabs.map(tab => tab.title));
@@ -172,9 +112,7 @@
if (tabbedPane.hasTab(view.viewId())) {
continue;
}
- // Mirror the cross-location filters: transient views are not
- // user-addable, so they should never appear as an "add tool" entry
- // even when they happen to be registered in the local view set.
+ // Transient views are not user-addable.
if (view.isTransient()) {
continue;
}
@@ -191,9 +129,7 @@
action: () => {
if (isIssuesPane) {
// Distinct from `HAMBURGER_MENU` so plus-button opens are
- // not mixed with three-dot-menu opens in the dashboard.
- // The matching `enums.xml` label is a follow-up Chromium
- // CL; until it lands the bucket shows as the raw integer.
+ // not conflated with three-dot-menu opens in the dashboard.
Host.userMetrics.issuesPanelOpenedFrom(Host.UserMetrics.IssueOpener.MORE_TOOLS_MENU);
}
this.#context.showView(view);
@@ -201,19 +137,16 @@
});
}
- // Always offer cross-location moves between the two main surfaces:
- // the panel plus button lists drawer views, and the drawer plus button
- // lists panel views. Other tabbed locations (e.g. nested sidebars)
- // have no "other main location" and therefore skip this step.
+ // Offer cross-location moves between the two main surfaces: the
+ // panel plus button lists drawer views and vice versa.
const otherLocation = location === ViewLocationValues.PANEL ? ViewLocationValues.DRAWER_VIEW :
location === ViewLocationValues.DRAWER_VIEW ? ViewLocationValues.PANEL :
null;
if (otherLocation) {
for (const view of manager.viewsForLocation(otherLocation)) {
// Non-closeable views (e.g. Console) cannot be moved between
- // locations, so don't offer them as "add tool" entries from the
- // other location. They can still appear in the overflow section
- // when their own location's tab strip overflows.
+ // locations, so they're excluded here. They still appear in the
+ // overflow section when their own location's tab strip overflows.
if (view.isTransient() || !view.isCloseable() || seenIds.has(view.viewId()) || seenTitles.has(view.title())) {
continue;
}
@@ -236,32 +169,17 @@
}
/**
- * Populates `contextMenu` with the plus-button menu entries:
- *
- * 1. Top section: tabs that are currently hidden because of overflow, in
- * tab order. Selecting one re-orders it into the visible region (so the
- * persisted tab order itself keeps it visible after a reload) and then
- * selects it.
- * 2. Lower section (separated when overflow is present, top-level
- * otherwise): views in this location that are not currently shown as a
- * tab, plus — for the two main locations (PANEL / DRAWER_VIEW) —
- * closeable, non-transient views from the other main location.
- * Entries are deduplicated by view id and by title (also against any
- * overflowed tabs above), then sorted alphabetically.
- *
- * This is the "view" half of the MVP split: it asks the
- * {@link PlusButtonPresenter} for a model, then renders that model into
- * the legacy `ContextMenu`. All filtering / dedup / sort logic lives in
- * the presenter; this function only handles the rendering decisions
- * (which section to use, how to wire up overflow reveals).
+ * Renders the plus-button menu by asking {@link PlusButtonPresenter}
+ * for a model and pushing it into `contextMenu`. Overflowed tabs (in
+ * tab order) come first, followed by deduplicated "add tool" entries
+ * sorted alphabetically.
*/
export function populatePlusButtonMenu(contextMenu: ContextMenu, context: PlusButtonMenuContext): void {
const model = new PlusButtonPresenter(context).buildModel();
const hasOverflow = model.overflowTabs.length > 0;
- // Overflowed tabs (in tab order) go into the default section. When
- // there are no overflowed tabs, the add-tool entries take the default
- // section instead so they are not visually demoted to a footer.
+ // When there are no overflowed tabs, surface the add-tool entries in
+ // the default section so they are not visually demoted to a footer.
for (const tab of model.overflowTabs) {
contextMenu.defaultSection().appendItem(
tab.title, () => revealOverflowTab(context.tabbedPane, tab.id), {jslogContext: tab.jslogContext ?? tab.id});
@@ -274,26 +192,13 @@
}
/**
- * Reveals an overflowed tab and persists its new position so that, on
- * the next DevTools reopen, it stays in the visible region instead of
- * falling back into the overflow menu. This mutates the tab order
- * setting via `tabbedPane.moveTab` (which fires `TabOrderChanged`),
- * hence the more emphatic name.
- *
- * The tab is reordered into the position of the *last currently-visible*
- * tab. This guarantees:
- * 1. The persisted tab order itself places the selected tab in the
- * visible region of the tab strip — independent of any runtime
- * `currentTab` / `lastSelectedOverflowTab` priority logic. So the
- * selection survives a DevTools window close+reopen even if the
- * persisted "last selected tab" is somehow unavailable.
- * 2. The previously-last-visible tab is pushed to the start of the
- * overflow region, which matches the user's intuition that the tab
- * they just opened replaces the tab they implicitly stopped using.
- *
- * When `tabbedPane.firstHiddenTabIndex()` is `-1` (no overflowed tabs)
- * or `0` (no visible tabs to reorder past), the reorder step is
- * skipped and the function falls back to a plain `selectTab`.
+ * Reveals an overflowed tab and persists its new position via
+ * `moveTab(firstHidden - 1)` so the tab stays in the visible region
+ * after a reload — independent of any runtime `currentTab` /
+ * `lastSelectedOverflowTab` priority logic. The previously-last-visible
+ * tab is pushed to the start of the overflow region, matching the
+ * intuition that the newly opened tab replaces the one the user
+ * implicitly stopped using.
*
* Exported only for testing.
*/
@@ -301,18 +206,11 @@
const firstHidden = tabbedPane.firstHiddenTabIndex();
if (firstHidden > 0) {
// `firstHidden - 1` is the index of the last currently-visible tab.
- // Move the chosen overflow tab there so the persisted order itself
- // places it inside the visible region.
tabbedPane.moveTab(tabId, firstHidden - 1);
}
tabbedPane.selectTab(tabId, /* userGesture */ true, /* forceFocus */ true);
}
-/**
- * Inputs for the {@link PLUS_BUTTON_VIEW} view function. Captures the
- * configuration of a single rendered `<devtools-menu-button>` without
- * referencing the surrounding `PlusButtonMenuContext`.
- */
interface PlusButtonViewInput {
title: string;
jslogContext: string;
@@ -320,18 +218,15 @@
}
/**
- * View function for the plus button. Follows the standard
- * `(input, output, target)` signature so `Lit.render` is called inside a
- * view function (per `@devtools/no-lit-render-outside-of-view`). The
- * `output` parameter captures the rendered `<devtools-menu-button>`
- * host via a `ref` directive so {@link installPlusButton} can return
- * it without a `querySelector` round-trip.
+ * Standard `(input, output, target)` view function so `Lit.render` is
+ * called inside a view (per `@devtools/no-lit-render-outside-of-view`).
+ * `output.button` is captured via `ref` to avoid a `querySelector`
+ * round-trip in {@link installPlusButton}.
*
- * Setting `slot` declaratively in the template guarantees the attribute
- * is present on the very first connection — so the first `slotchange`
- * already sees the button as the trailing-slot target and no extra
- * layout pass is needed to re-evaluate overflow once the attribute
- * settles.
+ * `slot` is set declaratively in the template so the attribute is
+ * present on the very first connection — the first `slotchange` then
+ * sees the button as the trailing-slot target and no extra layout pass
+ * is needed.
*/
export const PLUS_BUTTON_VIEW =
(input: PlusButtonViewInput, output: {button?: MenuButton}, target: HTMLElement): void => {
@@ -351,20 +246,15 @@
};
/**
- * Renders a `<devtools-menu-button>` configured as a "plus" button into
- * `tabbedPane`'s `trailing-button` slot using a declarative lit-html
- * template. The slotted element suppresses the legacy overflow chevron
- * (see `TabbedPane`'s slot handling) and its width is reserved during
- * overflow detection so the last visible tab never gets clipped.
- *
- * The returned `MenuButton` is the slotted host; the next CL will use
- * it to toggle visibility (e.g. when the drawer is minimized).
+ * Renders a `<devtools-menu-button>` configured as the plus button into
+ * `tabbedPane`'s `trailing-button` slot and returns the slotted host.
+ * The returned `MenuButton` is used by the next CL to toggle visibility
+ * (e.g. when the drawer is minimized).
*/
export function installPlusButton(context: PlusButtonMenuContext, options: PlusButtonOptions = {}): MenuButton {
const output: {button?: MenuButton} = {};
- // `render` is synchronous and the ref directive in PLUS_BUTTON_VIEW
- // fires during render, so `output.button` is guaranteed to be
- // assigned by the time the view returns.
+ // `render` is synchronous and the `ref` directive fires during render,
+ // so `output.button` is assigned by the time the view returns.
PLUS_BUTTON_VIEW(
{
title: options.title ?? i18nString(UIStrings.moreTools),
```
```
The name of the file: front_end/ui/legacy/ViewManager.ts
Insertions: 7, Deletions: 29.
@@ -396,14 +396,6 @@
throw new Error('Unresolved location: ' + location);
}
- /**
- * Creates a `TabbedViewLocation`. The first four parameters are
- * positional because they are common to almost every call site; the
- * rarely-used trailing knobs (default tab, custom visibility predicate,
- * custom `TabbedPane` factory, plus-button installation) are bundled
- * into a single `options` bag so adding new optional knobs in the
- * future does not bloat the positional signature.
- */
createTabbedLocation(
revealCallback: (() => void), location: string, restoreSelection?: boolean, allowReorder?: boolean,
options?: TabbedLocationOptions): TabbedViewLocation {
@@ -654,22 +646,13 @@
type TabOrderSetting = Record<string, number>;
-/**
- * Trailing options bag for {@link ViewManager.createTabbedLocation}.
- * All fields are optional; the bag itself is optional too.
- */
export interface TabbedLocationOptions {
- /** Tab id to select on first show, if present. */
defaultTab?: string|null;
- /** Predicate consulted by `isViewVisible` instead of `tabbedPane.isShowing()`. */
isLocationVisible?: () => boolean;
- /** Returns the `TabbedPane` instance to use; defaults to `new TabbedPane()`. */
tabbedPaneFactory?: TabbedPaneFactory;
/**
- * If provided, a `<devtools-menu-button>` configured as a plus button is
- * installed into the `TabbedPane`'s `trailing-button` slot before any
- * tabs are appended, so the very first layout pass reserves width for
- * it. See {@link PlusButton.PlusButtonOptions}.
+ * Installed into the `TabbedPane`'s `trailing-button` slot before any
+ * tabs are appended, so the very first layout pass reserves width for it.
*/
plusButton?: PlusButton.PlusButtonOptions;
}
@@ -715,21 +698,16 @@
this.defaultTab = options?.defaultTab;
this.isLocationVisible = options?.isLocationVisible;
- // Mount the plus button BEFORE appendApplicableItems so the very first
- // layout pass already reserves width for it. Otherwise the first paint
- // uses no reservation and a second paint snaps the last tab into the
- // overflow menu. The unit test
- // 'createTabbedLocation installs the plus button before appending tabs'
- // pins this ordering invariant.
+ // Install before `appendApplicableItems` so the very first layout pass
+ // reserves width for the button and we avoid a reflow that snaps the
+ // last tab into the overflow menu.
if (options?.plusButton && Root.Runtime.hostConfig.devToolsPlusButton?.enabled) {
PlusButton.installPlusButton(
{
tabbedPane: this.#tabbedPane,
location: this.location,
- // The local `views` map captures cross-location moves
- // (`appendView` may add views that aren't in
- // `manager.viewsForLocation(location)`); use it so the menu
- // reflects what the location is actually showing.
+ // Use the local `views` map (not `manager.viewsForLocation`) so
+ // cross-location moves added via `appendView` are reflected.
views: () => this.views.values(),
manager: this.manager,
showView: view => {
```
Change information
Add Plus Button (2/3): declarative plus-button option for TabbedLocation
Second of three CLs for the Plus Button feature. Adds the populator,
installer, and a declarative `plusButton` option on
`createTabbedLocation`. No user-visible change yet; CL3 opts the panel
and drawer locations in.
`front_end/ui/legacy/PlusButton.ts`:
- `populatePlusButtonMenu` lists overflowed tabs (in tab order) and
addable views, dedup'd across the local and other main location.
- `revealOverflowTab` persists the revealed tab's new position via
`moveTab(firstHidden - 1)` so it stays visible across reloads.
- `installPlusButton` mounts a `<devtools-menu-button>` into the
TabbedPane's `trailing-button` slot.
`ViewManager`:
- `createTabbedLocation` migrates its trailing knobs into a single
`TabbedLocationOptions` bag.
- When `plusButton` is set, the button mounts BEFORE
`appendApplicableItems` so the first overflow pass reserves its
width. Regression test in `ViewManager.test.ts`.
Gated on `Root.Runtime.hostConfig.devToolsPlusButton?.enabled` (the
matching `kDevToolsPlusButton` `base::Feature` and `chrome://flags`
entry land in the Chromium CLs below).
Call sites in `InspectorView.ts` / `InspectorDrawerView.ts` migrated to
the options bag (no `plusButton` opt-in yet — that's CL3).
Related:
Prototype: https://crrev.com/c/7792390
CL1: https://crrev.com/c/7814020
Chromium enum: https://crrev.com/c/7853628
Chromium feature param: https://crrev.com/c/7857037
- M config/gni/devtools_grd_files.gni
- M front_end/core/host/UserMetrics.ts
- M front_end/core/root/ExperimentNames.ts
- M front_end/core/root/Runtime.ts
- M front_end/entrypoints/main/MainImpl.ts
- M front_end/ui/legacy/BUILD.gn
- M front_end/ui/legacy/InspectorDrawerView.ts
- M front_end/ui/legacy/InspectorView.ts
- A front_end/ui/legacy/PlusButton.test.ts
- A front_end/ui/legacy/PlusButton.ts
- M front_end/ui/legacy/ViewManager.test.ts
- M front_end/ui/legacy/ViewManager.ts
- M front_end/ui/legacy/legacy.ts
- M front_end/ui/visual_logging/KnownContextValues.ts
Code-Review: +1 by Wolfgang Beyer, +1 by Danil Somsikov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |