[PS-1452] Desktop/Browser: use readonly/disabled fields in cipher view, add copy buttons to all fields

[patrickhlauke]

Type of change

- [x] Bug fix
- [x] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Currently, in both the desktop and browser versions, when viewing a cipher, most fields lack any underlying structure - the label and value are simply <div>s and <span>s. Structurally/semantically, this is a bit "loose". And for screen reader users, it means that they can only get to/hear the actual values by navigating using reading keys/cursor keys.

This PR switches the view to use readonly (and in the case of boolean custom fields, disabled, as you can't have a readonly checkbox currently) form fields with a proper <label>. This provides an unambiguous structure.

While initially in this PR I tried to provide some focus styling for when readonly fields receive focus, I've pivoted to a different approach: making the readonly fields explicitly non-focusable using tabindex="-1".

To improve the keyboard experience, this PR now also adds explicit copy buttons for all fields (including <textarea> for notes/addresses). For screen reader users, the copy buttons now also refer back to their original field / value using aria-describedby, making for a much more immediate experience; in the case of secret/hidden fields, the value is only announced as part of the accessible description if the field value is also visible, providing the same level of privacy consideration.

The only fields that still don't receive focus and don't have a copy button are custom field checkboxes and linked fields, but for these it makes no sense to include a copy button.

Code changes

• apps/browser/src/_locales/en/messages.json, apps/desktop/src/locales/en/messages.json, apps/browser/src/_locales/en/messages.json: add new strings for the various specific copy buttons
• apps/browser/src/popup/scss/box.scss and apps/desktop/src/scss/box.scss: added a new class .box-content-row-textarea for the textarea containers, and a new .action-buttons-absolute class to position the copy button for textareas absolutely over the corner of the textarea, rather than awkwardly off to the right and making the <textarea> smaller
• apps/web/src/scss/forms.scss: added new class .input-group-append-absolute and tweaked .input-group to allow for textarea/notes copy button to be absolutely positioned over the corner of the textarea
• apps/browser/src/popup/vault/view.component.html: Remove redundant aria-readonly="true" if there is already a readonly attribute ... the aria one is superfluous
• apps/desktop/src/app/vault/view.component.html, apps/desktop/src/app/vault/view-custom-fields.component.html, apps/browser/src/popup/vault/view.component.html, apps/browser/src/popup/vault/view-custom-fields.component.html: change the generic <div>/<span>s used to show a fake label and value to use actual <label> and <input>/<textarea> elements, but set to tabindex="-1" so they're not in focus order. Added "Copy..." buttons to all fields. Tweaked/fixed some of the existing aria-label definitions. Tied action buttons explicitly to their field/value using aria-describedby so that the value is announced by screen readers when the action button receives focus (with extra handling for password/hidden fields to only do this when the field is visually presented in clear text). As an aside, this also harmonises the desktop markup to be more in line with the the popup one - moving towards what I assume would be a desirable ultimate goal of having the exact same code for both? The change here also fixes a small inconsistency: copy buttons for websites incorrectly announce as "Copy URI" at the moment - now, website ones correctly announce as "Copy Website".
• apps/web/src/app/vault/add-edit.component.html: add copy buttons to all item view/edit fields in web vault

Edit: Split out the change originally made to ::selection into its own PR #3675. Backed out the change to the copyVerificationCodeTotp string on desktop, as it would cause problems with already translated versions (per #3485 (comment))

Screenshots

Video recording on https://www.youtube.com/watch?v=xLTCkh12Er4 (too big for github inline video)

Video shows the browser extension before/after in Windows/Chrome/NVDA and Windows/NVDA/desktop app. In both cases, we navigate using Tab/Shift+Tab (with a tiny digression at 0:47 - 1:00 to show how screen reader users can still get to the values using cursor keys/reading keys, but it's cumbersome)

---

Current desktop app showing an identity item (which currently is the one with the least/no copy buttons or anything)

After PR applied, desktop app showing an identity item - now with added copy buttons

---

Comparison of current and modified browser extension

A card item (note also the copy button on the textarea)

As above for desktop, comparion of an identity item (now with added copy buttons)

---

After screenshot of an item view in the web vault

Before you submit

• Please add unit tests where it makes sense to do so (encouraged but not required)
• If this change requires a documentation update - notify the documentation team
• If this change has particular deployment requirements - notify the DevOps team

[bitwarden-bot]

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PS-1452

[patrickhlauke]

/cc @danielleflinn because of the visible change with the focusable readonly fields

EDIT: as I now changed this PR considerably (not going for a visual change with focus indication for readonly fields, but instead making all fields non-focusable and instead adding copy buttons to all fields), adding the original screenshot / video comparisons from the PR description here, for context for the following comments about the original approach:

The visual change introduced by this PR is the border bottom on readonly fields when they receive focus.

A (fairly low res) video of navigating through the current view of a test item in the browser popup using chrome/nvda (using cursor navigation first, then using Tab/Shift+Tab), compared to the experience (at about 1 minute in) after this PR is applied. Possibly a bit more verbose, but particularly when tabbing, things are more explicitly announced. Note that the browser popup already makes some use of readonly inputs, while the desktop version doesn't at the moment. Again, this PR harmonises the two.

bitwarden-pr-item-view-readonly.mp4

[patrickhlauke]

Note that depending on when things get hopefully merged, this will need to be harmonised with #3321, #2660, and #3347

[danielleflinn]

Hi @patrickhlauke! Thanks for another great accessibility contribution :)

Can you provide more context around the WCAG requirement for a visible focus indicator? I'm assuming this relates to WCAG SC 2.4.7. Does the existing View Item focus state (highlight text) not meet the focus visible requirement?
cc: @bitwarden/dept-design

[patrickhlauke]

@danielleflinn yes, it's 2.4.7. looking at the current situation (let's just take the browser popup as an example), it's fine except for the <textarea> (e.g. a note) in the view - it takes focus, but has no focus indication. so that needed to be addressed.

and then now with my change, since there's more things that receive focus (like the TOTP and the password / "hidden" fields), and they wouldn't show the highlight (since for some of these I visually hide the actual control that receives focus, so that it can still use the custom colourised version for the password when revealed, which you couldn't have if just using <input type="text">) ... so to sidestep the issue, decided to give a more consistent and explicit focus style for all these extra new scenarios (and the old case of the <textarea>), and add that to everything.

if you did have the highlight colour defined somewhere, I guess we could - instead of the bottom border focus style - try to mimic the same highlighting on the <textarea> and the "faked" password/colourised fields ...

[patrickhlauke]

Following #3321 being merged, I've updated this PR to incorporate the changes from there (making the <label> elements draggable)

[danielleflinn]

Thanks for the explanation @patrickhlauke . We definitely should provide a visible focus indicator for the fields you mentioned above that do not have the highlight. Rather than introducing a new focus UI pattern, could you use UI more consistent with existing patterns? Either the fake text-highlight you proposed or something similar to the focus outline that is used for the icon buttons should work (Design has a preference for the former).

[danielleflinn]

@patrickhlauke can you also provide a screen recording of the revised chrome/nvda screen reader functionality similar to the one you uploaded showing current screen reader functionality?

[patrickhlauke]

@patrickhlauke can you also provide a screen recording of the revised chrome/nvda screen reader functionality similar to the one you uploaded showing current screen reader functionality?

the video in the OP shows the before and (at about 1:00) after (but only for the popup, desktop app is pretty much the same)

[patrickhlauke]

Thanks for the explanation @patrickhlauke . We definitely should provide a visible focus indicator for the fields you mentioned above that do not have the highlight. Rather than introducing a new focus UI pattern, could you use UI more consistent with existing patterns? Either the fake text-highlight you proposed or something similar to the focus outline that is used for the icon buttons should work (Design has a preference for the former).

i'll have a look. not 100% i can fake text highlighting, as i don't think i can force the browser-native highlighting, so would need to pick some foreground and background colour explicitly. will have a try anyway.

[jtouchstonebw]

The design team has discussed the following options and is ok with either of these options for read-only focus states. Whichever one is easier to implement will work for us.

Using the border focus styles

Using a text highlight as a focus style (primary-500 at 25% opacity)

[patrickhlauke]

a heads-up that i'm actually taking some time to make a more radical change proposal here:

• making the readonly fields also non-focusable with tabindex="-1"
• making sure that all fields have an equivalent copy action button (including <textarea>, with a copy button floating in the top-right of the area itself)

this makes the keyboard experience more consistent (compared to current state, where some fields are already readonly inputs, and are focusable despite having an explicit copy button, while others are just static text with a copy button) and cleaner (no need for duplicate focus stops - once on the readonly input, and then on the copy button)

I should have something by end of this weekend for consideration

[patrickhlauke]

@Hinton @danielleflinn any more thoughts on this?

[patrickhlauke]

@Hinton @djsmith85 should I work on fixing the merge conflicts, or abandon this PR?

[patrickhlauke]

doing a merge on desktop (rather than relying on github's automatic merge/update) seems to have worked fine, without needing to do any manual conflict resolution (unless i somehow managed to mess things up locally)

[patrickhlauke]

i don't want to constantly push on this, but at this point i'm getting concerned that this PR will start going out of sync with other stuff being merged. luckily there's only been two recent cases where some merge conflicts were flagged by github, though then merging locally on desktop seemed to resolve things without the need for any intervention. but it would be good to know what's preventing this currently from being merged. is it just too much of a change? too many concerns with the use of readme inputs?

[patrickhlauke]

@djsmith85 @danielleflinn @Hinton ping?

[patrickhlauke]

@djsmith85 @Hinton is it worth me resolving the merge conflicts, or should I just close this PR now? any indication would be appreciated...

[patrickhlauke]

as we're coming up to 8 months that this PR has been left here, and the merge conflicts are now just piling up as a result... going to close it. shame that this didn't even deign a response though folks...not impressed.