1964530 - Fix hardcoded 'transparent' values in test_moz_button.html

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P3)

Description

[Tim Giles [:tgiles]]

In test_moz_button.html, there are two instances of transparent that should be replaced with --button-background-color.

To help Mozilla out with this bug, here's the steps:

1. Comment here on the bug that you want to volunteer to help.
   • This will tell others that you're working on the next steps.
2. Download and build the Firefox source code
   • If you have any problems, please ask on Element/Matrix in the #introduction channel. They're there to help you get started.
   • You can also read the
     Firefox Contributors' Quick Reference,
     which has answers to most development questions.
3. Start working on this bug. In test_moz_button.html, there are two instances of transparent that should be replaced with --button-background-color.
   • If you have any problems with this bug, please comment on this bug and set the needinfo flag for me.
   • Also, you can find me and my teammates on the #reusable-components channel on Element/Matrix most hours of most days.
4. Build your change with mach build and test your change with mach test toolkit/content/tests/widgets/test_moz_button.html.
   • Also check your changes for adherence to our style guidelines by using mach lint
5. Submit the patch (including an automated test, if applicable) for review. Mark me as a reviewer so I'll get an email to come look at your code.
   • Getting your code reviewed
   • This is when the bug will be assigned to you.
6. After a series of reviews and changes to your patch, I'll push it to autoland.
   • Your code will soon be shipping to Firefox users worldwide!
7. ...now you get to think about what kind of bug you'd like to work on next.
   • Let me know what you're interested in and I can help you find your next contribution.

Comment 1

[Matthias Riffard]

This is my first time contributing, I thought it would be an ideal task to introduce myself to Firefox development.
I successfully onboarded, and could apply the modification.
However, this causes the tests concerned to fail:

FAIL backgroundColor should be --button-background-color - got "rgba(0, 0, 0, 0)", expected "rgba(0, 0, 0, 0.07000000029802322)"

Should I investigate this, or is it outside the scope of this task?

Comment 2

[Tim Giles [:tgiles]]

Hi Matthias, thanks for taking on this task! I'm going to go ahead and assign this to you as well, just for record-keeping.

You should continue to investigate this, but please submit your current work in progress so I can take a look at exactly what is going on with your patch and help you investigate the issue! You can submit your work in progress by:

1. Committing your work, git add . and git commit -m "<replace with your commit message>"
2. Installing moz-phab, our tool for managing revisions and reviews, via this guide on how to submit a patch
3. Submitting your patch using moz-phab submit -s --wip
   4. Using -s will only submit a single commit, as I'm assuming the change you made will be in one commit
   1. Using --wip will allow you to submit a patch without notifying other potential reviewers and what not, as our system may automatically assign reviewers based on the modified files paths in our repository.
4. Feel free to needinfo if you need help with the work in progress submission or to let me know that you've successfully submitted the patch so I can take a look at it.

As for my initial hunch to the issue, I think it's because I incorrectly assumed --button-background-color was the right design token for those transparent values. I believe the token we want to use is actually --button-background-color-ghost. I saw where the --button-background-color-ghost token gets reassigned to --button-background-color and it seems I made an false leap that we could just use that token, my mistake 😅

You can skim/read/skip this if you want, but I just wanted to explain how I got to that conclusion as a learning opportunity. First, I saw that the two buttons in question for this bug have type="icon ghost" or type="ghost icon". Next, I went to our Storybook instance (or you could run Storybook locally with ./mach storybook) and went to the "Button" folder under UI Widgets and specifically to the "Icon Ghost" story. There I inspected the <moz-button> element in the story and looked for the "background-color" CSS rule, which then led me to noticing ghost buttons use --button-background-color-ghost instead of --button-background-color.

Comment 3

[Matthias Riffard]

Attachment: Bug 1964530 - Replace hardcoded values in test_moz_button.html. r=tgiles

Comment 4

[Matthias Riffard]

Thank you for this instructive answer, I just discovered storybook! I successfully applied the correct token and submitted the patch, which is waiting for your review.

Comment 5

[Tim Giles [:tgiles]]

Thanks for the update! The patch looks good to me, but I can't approve it until you take the patch out of the Draft state. You can do this by navigating to the bottom of the page on your Phabricator revision and search for the "Add Action" dropdown. In this dropdown there should be an option called "Request Review" or something similar. Go ahead and do that and we can keep this moving!

Comment 6

[Matthias Riffard]

My bad, it should be ready for review now!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9e05d2e8997e
Replace hardcoded values in test_moz_button.html. r=tgiles

Comment 8

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/9e05d2e8997e