1858374 - Improve HCM styles for the accordion cards to follow Figma spec closely

Status: VERIFIED FIXED

(Firefox :: Shopping, defect)

Description

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Attachment: image.png

Found in

• 119.0b7

Affected versions

• 120.0a1 (2023-10-10)
• 119.0b7

Tested platforms

• Affected platforms: Windows 11x64
• Unaffected platforms: Ubuntu 22.1, macOS 12, Windows 10x64

Preconditions

• Contrast theme set to Night Sky

Steps to reproduce

1. Open a random Amazon product and observe the review checker.
2. Compare the review checker with Figma

Expected result

• The how we determine review quality and Settings strings are yellow.

Actual result

• The how we determine review quality and Settings strings are white.

Regression

• Not applicable

Additional notes

• Attached a screenshot.
• This applies to all review checker states.

Comment 1

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Attachment: image.png

Adding the screenshot again for better visibility. Sorry for the spam.

Comment 2

[kpatenio]

From debugging locally, it seems that shopping card accordion variants use --in-content-text-color for the header (which I believe eventually ends up being CanvasText), rather than ButtonText like expected in the Figma.

Comment 3

[kpatenio]

Attachment: Bug 1858374 - improve HCM accessibility for accordion shopping cards

Comment 4

[kpatenio]

Context: original bug was focused on inconsistencies in string colours between the Figma spec and actual implementation for the accordion cards. The Figma spec shows the accordion cards resembling dropdowns in HCM, thus using ButtonText for the actual card header and chevron.

Changing solely the string colour would've made text harder to read in some cases due to differing backgrounds, etc., so I implemented changes that improve HCM styling for the accordion card overall. But in doing so, I ended up doing all the work for Bug 1856714. I talked with emcminn, and we're thinking of marking that bug as a dupe and re-purposing this one instead.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fcd7d7c4456f
improve HCM accessibility for accordion shopping cards r=shopping-reviewers,desktop-theme-reviewers,ayeddi,niklas,reusable-components-reviewers,tgiles,dao

Comment 7

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/fcd7d7c4456f

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

Since nightly and release are affected, beta will likely be affected too.
For more information, please visit BugBot documentation.

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:kpatenio, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox121 to wontfix.

For more information, please visit BugBot documentation.

Comment 10

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Attachment: image.png

Hello! I looked over this issue with Firefox 122.0a1 (2023-12-06) on Windows 11x64 with Night Sky High contrast and it seems that there are still some discrepancies between Figma and actual behavior and I don't know if this is expected or not:

• the Not now button has a yellow square around it in the latest nightly and it does not in Figma
• the Settings and How we determine review quality cards are surrounded by a yellow square as well in the latest nightly and in Figma by a grey one

Are these behaviors expected or should we file a separate issue? Thank you!

Comment 11

[kpatenio]

(In reply to Alexandru Trif, Desktop QA [:atrif] from comment #10)

Created attachment 9367421 [details]
image.png

Hello! I looked over this issue with Firefox 122.0a1 (2023-12-06) on Windows 11x64 with Night Sky High contrast and it seems that there are still some discrepancies between Figma and actual behavior and I don't know if this is expected or not:

• the Not now button has a yellow square around it in the latest nightly and it does not in Figma
• the Settings and How we determine review quality cards are surrounded by a yellow square as well in the latest nightly and in Figma by a grey one

Are these behaviors expected or should we file a separate issue? Thank you!

Thanks for verifying the changes atrif! Could you please file a new issue for the inconsistency with the "Yes try it"/"Not now" card? I think I forgot to update the buttons for that card in my patch.

As for the Settings and How we determine review quality cards, @ayeddi do you know if this expected?

Comment 12

[Alexandru Trif, Desktop Test Engineering [:atrif]]

(In reply to kpatenio from comment #11)

Are these behaviors expected or should we file a separate issue? Thank you!

Thanks for verifying the changes atrif! Could you please file a new issue for the inconsistency with the "Yes try it"/"Not now" card? I think I forgot to update the buttons for that card in my patch.

Sure! I filled in bug 1868941. Thank you!

Comment 13

[Anna Yeddi [:ayeddi]]

(In reply to kpatenio from comment #11)

(In reply to Alexandru Trif, Desktop QA [:atrif] from comment #10)

Created attachment 9367421 [details]
image.png
(...)
As for the Settings and How we determine review quality cards, @ayeddi do you know if this expected?

Since these cards are somehow active UI, thus having the border in ButtonText (appears yellow in Win11 Dark Sky HCM) would be better than in CanvasText (appears white), thus the current Nightly behavior would work.

Ideally, we'd want to have only the <summary> to have the ButtonText and, when the card is opened/expanded, the left-bottom-right border under it (the border for the rest of <details> to have CanvasText color), but since the expected CSS changes did not work as expected for this details/summary pattern, this does not have to change. ButtonText would be a good solution atm.

Thank you for checking it!

Comment 14

[Alexandru Trif, Desktop Test Engineering [:atrif]]

Verified fixed with Firefox 122.0b7 on Windows 11 x64 with Night Sky high contrast theme and per comment 13. The Review Checker state from comment 1 is now displayed as in Figma spec.