Fix visual bugs on AccessSecretStorageDialog

[luixxiul]

Close element-hq/element-web#19426

Previously I pushed #7996, which was not really a proper solution of this issue.

Current implementation:

• The buttons should not be wrapped.
• The link color should be red according to the line here, but the rule is not properly inherited.

This PR implements the layout as below, which should almost realize the design provided at element-hq/element-web#19426 (comment). To realize that, the buttons are needed to be wrapped with a span element, named as mx_Dialog_buttons_row.

type: defect

---

Here's what your changelog entry will look like:

🐛 Bug Fixes

• Fix visual bugs on AccessSecretStorageDialog (#8160). Fixes Buttons don't have enough padding in Security Key dialog element-hq/element-web#19426. Contributed by @luixxiul.

Preview: https://pr8160--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

[codecov]

Codecov Report

Merging #8160 (aa7bd70) into develop (3e31fdb) will decrease coverage by 0.44%.
The diff coverage is 100.00%.

❗ Current head aa7bd70 differs from pull request most recent head 3cf798d. Consider uploading reports for the commit 3cf798d to get more accurate results

@@             Coverage Diff             @@
##           develop    #8160      +/-   ##
===========================================
- Coverage    30.85%   30.40%   -0.45%     
===========================================
  Files          893      889       -4     
  Lines        50793    50507     -286     
  Branches     12928    12861      -67     
===========================================
- Hits         15671    15359     -312     
- Misses       35122    35148      +26     

Impacted Files	Coverage Δ
...s/views/dialogs/security/CreateKeyBackupDialog.tsx	0.00% <ø> (ø)
src/components/views/elements/DialogButtons.tsx	100.00% <100.00%> (ø)
...list/algorithms/tag-sorting/AlphabeticAlgorithm.ts	0.00% <0.00%> (-100.00%)	⬇️
src/components/views/rooms/SearchBar.tsx	0.00% <0.00%> (-89.29%)	⬇️
...c/stores/room-list/algorithms/tag-sorting/index.ts	14.28% <0.00%> (-71.43%)	⬇️
...list/algorithms/list-ordering/OrderingAlgorithm.ts	0.00% <0.00%> (-57.15%)	⬇️
...stores/room-list/algorithms/list-ordering/index.ts	14.28% <0.00%> (-57.15%)	⬇️
src/stores/ActiveWidgetStore.ts	38.70% <0.00%> (-40.46%)	⬇️
src/stores/room-list/algorithms/Algorithm.ts	4.03% <0.00%> (-29.22%)	⬇️
...-list/algorithms/list-ordering/NaturalAlgorithm.ts	0.00% <0.00%> (-21.06%)	⬇️
... and 62 more

[kerryarchibald]

Thanks for working on this! The changes that address the issue look good, I have left a few small recommendations.
Unless they are solving an issue related to the bug you're working on here, please remove the changes to the structure of the style files.

[kerryarchibald]

Is this change in nesting strictly necessary? To me, it looks like these selectors are sufficiently specific. Unnecessary nesting of the styles makes them harder to read and harder to maintain.

[luixxiul]

Selector's semantic specificity itself does not ensure correct styling, as you have seen on #8089 too. Nesting should protect you from possible and unnoticed visual regressions otherwise, which cannot be easily detected without automated tests. Without a policy it is a matter of preference to decide how strictly nesting should be, but this case if strict nesting had been applied, the bug of the link color of .mx_AccessSecretStorageDialog_reset_link unnoticed for a long time should have not happened in the first place. Nesting is not always required per se, but it is an effective insurance against visual regressions like above. You could loose the strictness later if needed, without causing unnoticed regressions. If rules are defined strictly, the necessity of maintaining them due to a visual bug is reduced from the start. It also should effectively ensure the independence of the components by disallowing unintentional references. This kind of proactive insurance must be what other preprocessors such as LESS also adopt nesting for, but I am afraid of it that it is just an amateur view of an outsider...

[kerryarchibald]

We have variables for standard spacings, please use them here: https://github.com/matrix-org/matrix-react-sdk/blob/develop/res/css/_spacing.scss

[luixxiul]

Please see: #8208 (comment)

[robintown]

Spacing variables are the current best practice, and so we're requiring their use in all new code, even though very little old code has been updated to use them yet.

[luixxiul]

I see, also there are some cases where spacing variables are not available, such as 3px/7px, etc.

[robintown]

Yes, in those cases, where reasonable, they should be changed to one of the more standardized values, or a different CSS approach should be found

[luixxiul]

@kerryarchibald Done. I would like to ask you to re-review. Thanks in advance.

[kerryarchibald]

Code looks good, I've left one small nitpick comment. Unfortunately now a few checks are failing, there have been some changes to the CI lately so you may need to merge develop and recheck.

Also, to make re-review easier please don't force push after the initial review. This makes it impossible to see which changes are new and what has already been reviewed.

Thanks for working on this and thanks for your patience!

[kerryarchibald]

this is the same style as common.scss, do you still need it?

[luixxiul]

@kerryarchibald Force pushing was to make sure for myself that absolutely nothing not related to the fix has been included. sorry