[SM-223] Web: change <h2> modal title to <h1>, tweak styles to keep same look

[patrickhlauke]

Type of change

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

Objective

Change primary heading in modals from <h2> to <h1> - for consistency with the desktop app modals, and because logically in a modal (which is its own little document) the top-most heading is the primary heading. Not a critical issue, but mostly good practice. See #3323 (comment)

Code changes

• apps\web\src\app\ ... .component.html: swap out <h2 class="modal-title"> for <h1 class="modal-title"> in the modal dialogs
• apps\web\src\scss\modals.scss: add extra styling to keep the look and feel of the modals the same (set the same explicit font-size for .modal-title as regular h2s)

Screenshots

No change

Before you submit

- [x] I have checked for **linting** errors (`npm run lint`) (required)
- [ ] I have added **unit tests** where it makes sense to do so (encouraged but not required)
- [ ] This change requires a **documentation update** (notify the documentation team)
- [ ] 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-1324

[djsmith85]

@danielleflinn Could you please review/approve the styling changes done in apps\web\src\scss\modals.scss.
Would we prefer the normal size for h1's or go with the modified style?

[danielleflinn]

h1s look great and I agree with the style tweaks Patrick made to mimic existing header styles.

@patrickhlauke could you also update the h2 in this file to an h1?

[patrickhlauke]

@danielleflinn sure [edit: done]. should I be concerned that looking at that file, I don't see any role="dialog" etc? how/where is this used? also, is there some other matching CSS that is then used? (as my patch to the styles won't pick this up, wherever it is used)

[danielleflinn]

@patrickhlauke yes we will need to add role=dialog and other aria attributes typically used in dialogs to that component as well; good catch! If you'd like to while you are in there feel free to add these, otherwise I'll have someone internally update it.

That component is currently not in use but we have plans to update all modals in the web vault to utilize it as their base (that way to make 1 style or html tag change, we can do it in 1 place rather than the 40+ places you had to update the h2 in this PR).

Updating the h2 should not effect the styles for the linked component as it uses Tailwind utility classes rather than a default html heading style.

Hope this clarifies things! Let me know if you have additional thoughts

[patrickhlauke]

@patrickhlauke yes we will need to add role=dialog and other aria attributes typically used in dialogs to that component as well; good catch! If you'd like to while you are in there feel free to add these, otherwise I'll have someone internally update it.

as I'm not too hot on Angular, i'll add the role and aria-modal, but for aria-labelledby, it'll need some autogenerated unique id, and not sure what the best approach here will be - unless those modals will always only ever be shown one at a time, in which case can easily set a single general one hardcoded in the component itself...or go for an aria-label using the same text as that's injected into the heading.

tailwind...urgh. would not be my choice, but fair enough ;)

[danielleflinn]

Thanks Patrick; I'll work with the team to get a solution in for aria-labeledby vs aria-label. Thanks for your contribution.

[patrickhlauke]

@danielleflinn done the role/aria-modal (also updated the simple-dialog component just in case). hoping that when transitioning to these new components over time any improvements/approaches we made recently won't fall by the wayside again (e.g. the notifications/toasts, which finally announce correctly in AT after my upstream fix to scttcper/ngx-toastr#934)

[Hinton]

@patrickhlauke I've changed the titles to h1 in #3439 along with fixing labelledby for the new dialogs. To avoid conflicts should we remove those changes from this PR?

The role and aria-modal gets set by the new dialog service from Angular CDK. (Demo https://62a88a6de5b807fa98886113-mjjgrsvchd.chromatic.com/?path=/story/component-library-dialogs-service--default).

Are there any other improvements we could make to the new dialogs from an accessibility standpoint? I'm hoping we can begin transitioning over to these new dialogs soon which should make the modals much nicer to work with and ensure they work identical.

[Hinton]

@djsmith85 from a code perspective this looks good once the component library dialogs have been updated. Could you look into checking with QA when they have some capacity to test this?

[patrickhlauke]

@Hinton

Demo https://62a88a6de5b807fa98886113-mjjgrsvchd.chromatic.com/?path=/story/component-library-dialogs-service--default

That looks quite good. Note the "X" close control lacks an accessible name though (could do with an aria-label="Close" on the button)

[patrickhlauke]

@patrickhlauke I've changed the titles to h1 in #3439 along with fixing labelledby for the new dialogs. To avoid conflicts should we remove those changes from this PR?

I've just copied those changed headings from #3439 verbatim into this PR now, so shouldn't cause any conflicts (unless by doing so I messed things up somehow)

[Hinton]

Thanks for the feedback on the close button. I added both title and aria-label.

[Hinton]

I think it should work, we'll just have to merge #3439 first since the dialogs will stop working until both are merged.

[Hinton]

Code change looks good.

[djsmith85]

@Hinton As discussed please continue championing this.

[Hinton]

@patrickhlauke I've merged #3439, could you please resolve the merge conflicts, and I'll look into getting some QA capacity queued up to test this?

[patrickhlauke]

@Hinton done, chief

[Hinton]

Merged, thanks @patrickhlauke.