fix(breadcrumbs): never submit a surrounding form (#DS-5597) by NikGurev · Pull Request #2081 · koobiq/angular-components

NikGurev · 2026-09-22T09:52:12Z

Summary

A breadcrumb navigates, but KbqBreadcrumbButton on a <button> host left the element at its implicit type="submit". Inside a form that turned every breadcrumb into a submit control — reported from a topbar, where breadcrumbs sit inside a form.

Two ways it showed up:

Click. Activating a breadcrumb button submitted the form. The overflow expand button (…) is the one the library itself renders, so this hit anyone using max.
Enter in a field. The first submit button in a form is its default button, the one the browser activates on implicit submission. That was the expand button, so pressing Enter in any field opened the collapsed-items dropdown and submitted the form.

KbqBreadcrumbButton now writes type="button" on a <button> host, over any type already there — a breadcrumb that submits is a mistake, not an intent. Anchors are untouched: on <a> the attribute hints at the MIME type of the link target, and the default breadcrumb template renders anchors.

The attribute is written once from the constructor rather than bound. A [attr.type] host binding was tried first and rejected: it needs a protected member to stay off anchors (public API surface), and it is order-dependent — it wins the first change detection and then goes quiet, so a consumer's own [attr.type] flip-flops. A static host: { type: 'button' } was also measured and fails outright: it lands on anchors, and a template-written type="submit" beats it.

List of notable changes:

fixed KbqBreadcrumbButton so a <button> breadcrumb never submits a surrounding form
added unit coverage for breadcrumbs inside a form, including a control case proving the assertions are not vacuous
added E2eBreadcrumbsInForm plus a Playwright case for Enter-in-a-field — jsdom does not implement implicit submission, so this only reproduces in a real browser
updated the public API golden file: KbqBreadcrumbButton now declares a constructor

What should reviewers focus on?

Overriding an author-set type rather than respecting it. type="submit" written on a breadcrumb is now silently replaced. Deliberate, and covered by should override a statically set type — but it is the one judgement call here.
The constructor write. Renderer2.setAttribute guarded on nodeName === 'BUTTON'. A consumer who binds [attr.type] still takes it over from the first change detection; nothing in the library re-asserts it, and no test claims that as a contract.
Both new suites were verified to fail without the fix. The Playwright one reproduces the original report exactly: the dropdown panel opens on Enter.

🤖 Generated with Claude Code

github-actions · 2026-09-22T09:57:55Z

Visit the preview URL for this PR (updated for commit 5663bef):

https://koobiq-next--prs-2081-h0440y0y.web.app

_{(expires Fri, 25 Sep 2026 12:34:39 GMT)}

_{🔥 via Firebase Hosting GitHub Action 🌎}

_{Sign: c9e37e518febda70d0317d07e8ceb35ac43c534c}

Copilot

Copilot review overview

🔵 Needs a closer look

It deliberately overrides a consumer-set type on a public component, a documented behavior-change judgment call that warrants human sign-off.

Review effort: Balanced
Findings: None

What changed in this PR

This PR fixes a bug where KbqBreadcrumbButton, when applied to a <button> host, left the element at its implicit type="submit". Inside a <form>, this turned every breadcrumb button (including the library-rendered overflow … expand button) into a submit control — activating a breadcrumb submitted the form, and pressing Enter in a field triggered implicit submission through the expand button (the form's default button). The fix writes type="button" on <button> hosts from the directive constructor, leaving anchors untouched.

Changes:

KbqBreadcrumbButton now writes type="button" on a <button> host via Renderer2.setAttribute, guarded on nodeName === 'BUTTON' so anchors are unaffected (SSR-safe).
Added Jest unit coverage (form-submission scenarios, static-type override, anchor exemption, and a non-vacuous control case) and an E2eBreadcrumbsInForm fixture with a Playwright test for Enter-in-field implicit submission.
Updated the breadcrumbs public API golden file to reflect the new constructor.

File	Description
`packages/components/breadcrumbs/breadcrumbs.ts`	Adds constructor that sets `type="button"` on `<button>` breadcrumb hosts; updates the directive JSDoc.
`packages/components/breadcrumbs/breadcrumbs.spec.ts`	Adds "inside a form" unit suite and the `BreadcrumbsInForm` test host with static-type and control cases.
`packages/components/breadcrumbs/e2e.ts`	Adds `E2eBreadcrumbsInForm` fixture with a form wrapping breadcrumbs and two input fields.
`packages/components/breadcrumbs/e2e.playwright-spec.ts`	Adds Playwright cases for Enter-in-field non-submission and `type="button"` on all breadcrumb buttons.
`packages/e2e/routes.ts`	Registers `E2eBreadcrumbsInForm` so it is reachable at `/E2eBreadcrumbsInForm`.
`tools/public_api_guard/components/breadcrumbs.api.md`	Records the new `constructor()` on `KbqBreadcrumbButton`.

The implementation is correct and SSR-safe: it uses Renderer2.setAttribute rather than direct DOM access, guards on nodeName === 'BUTTON' so anchors (where type hints at the link target MIME) are untouched, and does not conflict with KbqButton, which never manipulates type. The unit and Playwright suites cover the click and Enter-key paths, the anchor exemption, the static-type override, and a control case proving the assertions are meaningful. I found no objective defects to flag.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

A breadcrumb navigates, but `KbqBreadcrumbButton` on a `<button>` host left the element at its implicit `type="submit"`. Inside a form that turned every breadcrumb into a submit control: clicking one submitted the form, and pressing Enter in a field activated the first of them as the form's default button — in practice the overflow expand button, which opened its dropdown. `KbqBreadcrumbButton` now writes `type="button"` on a `<button>` host, over any type already there. Anchors are left alone: on `<a>` the attribute hints at the MIME type of the link target. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

artembelik · 2026-09-22T13:31:45Z

+
+        // A breadcrumb navigates, so it must never submit a surrounding form — but a `<button>`
+        // host defaults to `type="submit"`. Anchors take no `type`.
+        if (element.nodeName === 'BUTTON') {


почему просто для host элемента не установить type=button? решается 1 строкой, вместо 2 инжектов и доп проверок

перепроверю, но установка атрибута на хост элементе позволяет его перезаписать, это поведение не нужно

artembelik · 2026-09-22T13:33:04Z

+        await expect(buttons).not.toHaveCount(0);
+
+        for (const button of await buttons.all()) {
+            await expect(button).toHaveAttribute('type', 'button');


это уже проверяется в unit тестах

этот кейс вынесен отдельно

jsdom does not implement implicit submission

NikGurev requested a review from lskramarov as a code owner September 22, 2026 09:52

github-actions Bot added the bug Something isn't working label Sep 22, 2026

artembelik requested a balanced review from Copilot September 22, 2026 10:06

Copilot started reviewing on behalf of artembelik September 22, 2026 10:06 View session

artembelik reviewed Sep 22, 2026

View reviewed changes

Comment thread packages/components/breadcrumbs/breadcrumbs.ts Outdated

Copilot AI reviewed Sep 22, 2026

View reviewed changes

NikGurev force-pushed the fix/DS-5597 branch from 98a1a22 to 5663bef Compare September 22, 2026 12:29

NikGurev requested a review from artembelik September 22, 2026 12:32

artembelik reviewed Sep 22, 2026

View reviewed changes

artembelik approved these changes Sep 22, 2026

View reviewed changes

lskramarov assigned NikGurev Sep 22, 2026

Navigation Menu

fix(breadcrumbs): never submit a surrounding form (#DS-5597) - #2081

fix(breadcrumbs): never submit a surrounding form (#DS-5597)#2081
NikGurev wants to merge 1 commit into
mainfrom
fix/DS-5597

NikGurev commented Sep 22, 2026

Uh oh!

github-actions Bot commented Sep 22, 2026 •

edited

Loading

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

artembelik Sep 22, 2026 •

edited

Loading

Uh oh!

NikGurev Sep 22, 2026

Uh oh!

artembelik Sep 22, 2026

Uh oh!

NikGurev Sep 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

NikGurev commented Sep 22, 2026

Summary

List of notable changes:

What should reviewers focus on?

Uh oh!

github-actions Bot commented Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot review overview

🔵 Needs a closer look

Uh oh!

artembelik Sep 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NikGurev Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

artembelik Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

NikGurev Sep 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

github-actions Bot commented Sep 22, 2026 •

edited

Loading

artembelik Sep 22, 2026 •

edited

Loading