Skip to content

Bk/bug/5511 fix copy icon btn#5552

Open
Bartan89 wants to merge 2 commits into
cloudfoundry:developfrom
Bartan89:bk/bug/5511-fix-copy-icon-btn
Open

Bk/bug/5511 fix copy icon btn#5552
Bartan89 wants to merge 2 commits into
cloudfoundry:developfrom
Bartan89:bk/bug/5511-fix-copy-icon-btn

Conversation

@Bartan89

@Bartan89 Bartan89 commented Jul 3, 2026

Copy link
Copy Markdown

Description

Fix copy-to-clipboard icon behavior and state transitions

Motivation and Context

This PR refactors the copy-to-clipboard component to use a clear state-driven UI flow.

How Has This Been Tested?

Test the temporal nature of the component as well as the error state imagined for the off change that corporate's (clients) that somehow block copying ability. Followed by the fact that the
(method) Clipboard.writeText(data: string): Promise<void>
is a promise hence has a catch implied by its implementation by the browser(s)

besides automated unit test (with integration towards virtual dom nodes)
There is a manual test instruction in the form of addinga runtime reassignment of a method to mock an error state of the clipboard. Beyond that it has been tested on all modern browsers.

Types of changes

It uses modern angular approach instead of conditionally hiding elements based on opacity. This is proved fragile with bubbling events. This clean approach can also be extended into the realm of lazy loading so that chunks are reeled in async however (defer syntax) for now that is kept out of scope due to the limited code.

Also the green iconography has been added with animation. New animation was created where a key approach was to keep the scale equal or under 1 to assure it never clips into its parent div.

Also a conditional animation was added to the clipboard icon to satisfyingly come back from it all.

also the @input decorator was used with required: true to give compile time type safety in the components. However due to the strictness level actual the compilation is waived for now. There is a ticket with more info and some scripts: #5505

Lastly the parent element has been shifted to more natural fit the baseline of the text. Also it has been moved ever so slightly and the width is now determined by its content.

There are a couple of side effects in the code which can be avoided if rewritten entirely with RxJs however it was deemed more readable to rely on promises and signal state.

Since zone.js was removed no more need for on push setting

  • Bug fix (non-breaking change which fixes an issue)
  • Docs update
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have followed the guidelines in CONTRIBUTING.md, including the required formatting of the commit message
functionality error placement alignment

Bartan89 added 2 commits July 3, 2026 14:12
Signed-off-by: Bart Kuijper <bart.kuijper89@gmail.com>
Signed-off-by: Bart Kuijper <bart.kuijper89@gmail.com>
@norman-abramovitz

Copy link
Copy Markdown
Contributor

Thanks for this change. It is the difference between getting something in place and making it look good.

There are three behavior regressions I'd like fixed before merge, plus two smaller ones. All are small changes.

1. Click propagation needs to be stopped again.
The old handler took $event and called stopPropagation(). That wasn't decoration — this component gets embedded inside clickable containers. On the Endpoints card view it sits inside an app-meta-card with a [routerLink], and the address cell renders inside routerLink'd table rows. With the new (click)="copyToClipboard(text)" the click bubbles, so copying an address also navigates to the endpoint detail page, and the component is destroyed before your success indicator ever shows. Pass the event back through (or $event.stopPropagation() inline in the binding) so the component stays safe to embed anywhere.

2. The failure state is terminal — give it a way back.
Once didUserPressCopy hits 'yes but failed', nothing ever resets it: the error icon has no click handler and there's no timeout, so one failed write removes the copy button until the user leaves the page. Clipboard failures are often transient (NotAllowedError when the document briefly loses focus is the classic), so a single blip shouldn't permanently kill the affordance. You already have the 700ms reset timer for success — running the same reset after showing the error gives retry for free.

3. Guard against navigator.clipboard being undefined.
The deleted code gated the button on clipboard availability and fell back to execCommand. Outside a secure context (plain-HTTP deployments, which Stratos evaluation setups do use), navigator.clipboard is undefined, so every click throws straight into the catch — and with point 2, that's a permanent error icon on a page where copy used to work. I'm fine with dropping the execCommand fallback as legacy, but then the button should be gated on navigator.clipboard?.writeText being present rather than rendering something that can only ever fail. The deleted spec for this case should come back in whatever form matches the approach you pick.

4. Space key: keep the preventDefault().
The old (keydown.space) binding called $event.preventDefault(). Since this is a span[role="button"] and not a native button, Space keeps its default scroll behavior — a keyboard user activating the control now also scrolls the page a viewport. One-liner to restore.

5. Restore the aria-label.
The old template had [attr.aria-label]="tooltip || 'Copy to clipboard'". Without it, screen readers announce the icon's ligature text ("content copy"), and title alone isn't a reliable accessible name. Related: tooltip is now required: true, but code-block.component.html doesn't pass it — the build tolerates that (our template strictness is relaxed, as you noted via #5505), so it ships with title=undefined rather than failing. I'd actually prefer the component default the string — your own doc comment says to use "Copy to clipboard" unless there's a strong reason, and a default encodes that better than making every caller repeat it. If you keep it required, please update code-block too.

There's a second, project-specific reason to keep ARIA attributes healthy here. The theme-builder tool under tools/stb/ (work in progress) treats the DOM as its source of truth and reads real aria-*/role attributes as the fallback naming source when its own stba-* instrumentation isn't present — see the prefix-scheme section of tools/stb/docs/architecture.md. So an element's aria-label isn't just for screen readers in this codebase; it's how components eventually get sensible names in the theming model. Worth having on your radar as you touch more of the shared components.

Smaller things, take or leave:

  • Please keep ChangeDetectionStrategy.OnPush. We run zoneless, and this component renders per-row in the endpoint tables — with Default strategy every CD cycle anywhere in the app re-executes all those templates. Your state is already a signal, so OnPush works with no other changes.
  • The 700ms timers stack if the icon is clicked twice quickly (the first timer flips the state early and restarts the animation), and a pending timer will fire on a destroyed component. Tracking one timer id and clearing it per click / on destroy fixes both, and is the same mechanism point 2 wants.
  • File hygiene: trailing whitespace on the 'yes but failed' line, missing newline at EOF (both are in our .editorconfig), and the manual-testing console snippet comment appears twice — once in the catch and again after the class.
  • With showSuccessText, "Copied to clipboard" now shows for 0.7s (was 2s) — hard to actually read. Consider a longer reset for the text case, or just bumping the constant.

None of this is big — the direction is good and I'd like to merge it once the propagation, error-reset, and clipboard-guard points are addressed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants