1787934 - [Win][HCM] Bookmark This Page image button is not readable when it is active

Status: NEW

(Toolkit :: Themes, defect, P3)

Description

[Anna Yeddi [:ayeddi]]

Attachment: 2022-08-29 Win HCM Bookmark this page active button.png

STR:

1. Activate High Contrast Theme on Windows OS (i.e. Night Sky)
2. Open any page
3. Navigate to the address bar and activate Bookmark this page image button
4. Confirm the bookmark panel is opened and review the Bookmark this page image button's appearance.

Expected:

When the Bookmark this page image button is pressed and the corresponding panel is opened, the star icon which serves as a label is visible and discernable. Colors used communicate to the HCM user that this button is selected/active:

• For example, with "Night Sky" HCM theme, the button has purple background and gray foreground (the star icon is gray against purple).

Actual:

When the Bookmark this page image button is pressed and the corresponding panel is opened, the star icon which serves as a label is not visible as it uses the same text color as its background one. It is not clear what is the on-screen label of this control, thus the purpose is not communicated to users with low vision and/or cognitive difficulties:

• For example, with "Night Sky" HCM theme, the button has purple background and foreground (the star icon is purple against purple).

It can be resolved by converting the foreground color to the SelectedItemText for active/pressed state to provide the following combination of selected item colors:

• SelectedItem for Selected Background
• SelectedItemText for Selected Text / foreground

Comment 1

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

The severity field is not set for this bug.
:dao, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 2

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

The severity field for this bug is set to S4. However, the accessibility severity is higher, [access-s3].
:dao, could you consider increasing the severity?

For more information, please visit auto_nag documentation.

Comment 3

[ewere.njoagwu]

I would like to work on this bug if it is available, I am an outreachy intern.

Comment 4

[Tim Giles [:tgiles]]

Redirecting needinfo request to :morgan who is the mentor of this bug

Comment 5

[Pushpanjali]

I would like to work on this bug, I am an outreachy intern.

Comment 6

[Morgan Reschenberg [:morgan]]

Hi there :) Sorry I was out at a conference -- we'd love to have you work on this bug!
To start -- it'd be good to verify this issue still exists in the way the bug has captured it. Can you enable HCM and follow Anna's instructions in #c0? Once you've done that, please upload a screenshot of what you see.

Comment 7

[Pushpanjali]

yes. I will inform.

Comment 8

[Pushpanjali]

Attachment: Screenshot 2023-03-21 122804.png

yes, the issue still exits. Can I work on it?

Comment 9

[Morgan Reschenberg [:morgan]]

Yep, definitely, thank you for confirming. I'll assign this to you. Have you contributed to Firefox before?
If not, it's a good idea to read our contributing guide before you work on setting up a build. For this bug, you should only need an "Artifact" build, but if you'd like to build all of Firefox for desktop, that'll work too :)

Once you've got a build setup, I recommend using the browser toolbox inspector to help you identify the chunk of code responsible for this button. You may also find searchfox helpful for browsing and searching the codebase quickly.

Regarding High Contrast Mode and "expected" behaviour of this button, we have a design guide that walks you through our best practises.

Please let me know if you have any questions -- use the "need info" feature otherwise I won't be notified that you're waiting for an answer to something. Thanks for contributing!

Comment 10

[Pushpanjali]

Yes , Ma'am. I am working on it

Comment 11

[Pushpanjali]

I'm facing difficulty in understanding how to correct the issue, in night sky mode on hover bookmark background turns purple and as in active state bookmark icon is also purple. hence, we can see this bug. In order to correct I need to change any one colors to other. Can you guide me how to solve this?

Comment 12

[Pushpanjali]

Hello , ma'am?

Comment 13

[Morgan Reschenberg [:morgan]]

Attachment: toolbar_button_states_HCM_figma

Hello! I've attached a screenshot of the expected states of this button, as mocked in Figma. I've also annotated each state with the corresponding CSS system colour keywords (X on Y where X is the foreground colour and Y is the background colour. the foreground colour should also be used for the border unless otherwise specified). This differs slightly from the "expected" results in #c0 because we've updated our HCM guidance since this bug was filed. Please follow this reference instead.

Please need-info me if you have additional questions

Comment 14

[Morgan Reschenberg [:morgan]]

(In reply to Pushpanjali from comment #11)

I'm facing difficulty in understanding how to correct the issue, in night sky mode on hover bookmark background turns purple and as in active state bookmark icon is also purple. hence, we can see this bug. In order to correct I need to change any one colors to other. Can you guide me how to solve this?

In order to find which files to edit, I recommend using the browser toolbox (hamburger menu > tools > browser toolbox -- note you'll have to enable this option in devtools, follow the instructions at the link). This will launch an inspector like the devtools inspector. Click on the bookmarks button with the inspector. In the righthand inspector pane, select the "computed" tab -- this allows you to see which styles from the CSS cascade are being applied to the element, and where those styles are defined.

In my view of this button, I see color is defined in urlbar-searchbar.css. If you click on the file name, it'll take you to the exact declaration. You can edit this file and watch firefox update in realtime :) this is a great way to test that you've identified the correct style.

To update this button, you'll need to change color, background-color, and border-color. The values of these properties should be the CSS System Color Keyword I've identified in the figma screenshot above. You'll need to make sure the changes you make are within a @media (forced-colors) block so they don't alter the non-HCM style of this button.

To find hover/active/etc styles, you can use the "rules" pane of the browser toolbox. Next to the search input, there's a :hov button -- click that. In the panel that pops up, you can filter styles for only those that apply on :hover or only those that apply when :active. Checking the box next to one of these pseudoclasses will apply that pseudoclass to the component. You can then check the computed pane once again to find the files that the relevant rules exist in :)

Comment 15

[Siya]

Hi Morgan! Will it be okay if I work on this bug?

Comment 16

[Siya]

Hi Morgan! I'm sorry for pinging again, but I've gone through the previous comments and have been able to resolve the bug on my local machine, I'm waiting for a nod from your end considering this bug was assigned but the mentee has been inactive the last few days. Please do consider assigning this to me. Thank you!

Comment 17

[Siya]

Hi Morgan! Will it be okay if I work on this bug? I've gone through the previous comments and have been able to resolve the bug on my local machine, I'm waiting for a nod from your end considering this bug was assigned but the mentee has been inactive the last few days. Please do consider assigning this to me or if you'd prefer I could just directly submit a patch from my end. Thank you!
P.S: extremely sorry for commenting here the third time, I got confused with the needinfo feature a bit there.

Comment 18

[Morgan Reschenberg [:morgan]]

(In reply to Siya from comment #17)

Hi Morgan! Will it be okay if I work on this bug? I've gone through the previous comments and have been able to resolve the bug on my local machine, I'm waiting for a nod from your end considering this bug was assigned but the mentee has been inactive the last few days. Please do consider assigning this to me or if you'd prefer I could just directly submit a patch from my end. Thank you!
P.S: extremely sorry for commenting here the third time, I got confused with the needinfo feature a bit there.

Hi! No worries, I was out sick sorry for the late response :)
We typically wait for two weeks of inactivity on a bug before we consider reassigning from an assignee.

Pushpanjali, are you still working on this? You don't need to have it done now or soon, I don't want to reassign it out from under you if its still a current project. Please let me know within the next few days. Thank you!

Comment 19

[Morgan Reschenberg [:morgan]]

Siya, are you still interested in this bug ?

Comment 20

[Siya]

(In reply to Morgan Reschenberg [:morgan] from comment #19)

Siya, are you still interested in this bug ?
Yes Morgan. I'll update you soon with my progress :)

Comment 21

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

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Comment 22

[Anna Yeddi [:ayeddi]]

Attachment: Bug 1787934 - Adding support to HCM for toolbarbutton icons in active state. r=emilio,dao

Allowing the starred icon to be filled with the currentColor which
should be SelectedItemText on HCM so it is visibile when the panel is
expanded and when the expanded panel button is hovered as well.

Comment 23

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

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit BugBot documentation.

Comment 24

[Dão Gottwald [:dao]]

Given the discussion in https://phabricator.services.mozilla.com/D187301, this doesn't really seem like a good first bug. The final patch may end up looking small and straightforward, but getting there can be tricky...

Anna, are you going to keep working on this?

Comment 25

[Anna Yeddi [:ayeddi]]

With the larger work that has happened for chrome HCM and button components, the WIP patch attached to this bug would need to be reviewed. I won’t have cycles for it but would be happy to guide anyone who could work on the fix.