Change color on destructive button focus state

[KevinBatdorf]

What?

This is a minor change to the two destructive buttons' focus state

Why?

How?

Changes some of the styles in button.scss

Testing Instructions

Load the project, start up storybook (http://localhost:50240), view the buttons

Screenshots or screencast

Before:

before.mov

After:

after.mov

Closes #44426

[mirka]

Great fix, thank you!

The Primary Destructive looks good to me, though I'm not entirely convinced about the non-Primary Destructive. Maybe we should keep the non-darkened red for the text color? That seems to be more in line with what the focus styles do on the other variants.

If it's simple enough to also fix in this PR, I noticed that the text color for Destructive Link is also still blue:

CleanShot.2022-10-14.at.22.09.15.mp4

And lastly, if you could add a quick changelog, that would be great 👍 (I think we can file this under Bug Fix)

[KevinBatdorf]

Thanks @mirka for following up. I now attempted to normalize them with the least amount of changes, which hopefully is good enough for a merge (better than what's there now I think)

Link:

link.mov

Default:

default.mov

Primary:

primary.mov

Each button will hover to a 20% darker color, then when pressed show its natural color, and finally when unpressed/released, revert to the 20% darker again until it later loses focus.

I think the remaining button styles need some attention from someone more in tuned with the project to make some opinionated decision on overall styling (e.g. removing the gray from tertiary buttons would be an improvement, in my opinion).

[KevinBatdorf]

Tests failed but looks unrelated. I don't think I can restart them with my access without an arbitrary code change

[KevinBatdorf]

Noticed someone triggered the test. One thing I noticed just now is the focus border is missing. I'm going to tweak it now to include that. Not sure if it should be red or blue but it looks like in other places it was intended to be red.

[KevinBatdorf]

With the latest update (to include more uniform focus borders):

I think it's good for review now at your convenience, thanks.

Primary:

primary.mov

Link:

link.mov

Default:

default.mov

[ciampo]

While waiting for Lena to take another look at this, I'll just flag that PHP unit test failures are unrelated and should be fixed by rebasing (once #44507 is merged)

[mirka]

I agree that these component styles need some refactoring and normalization, and your changes do bring it closer to that!

However the challenge here is that Button is one of the most widely used components in this library, with tons of questionable style overrides everywhere in the Gutenberg app (not to mention other consumers) 😅 So any kind of normalization work is going to be a more high-effort endeavor: designer sign-off, splitting changes into the smallest increments possible, regression testing across the codebase, etc.

How about we keep this PR to the bug fix aspect of isDestructive specifically, so we can safely land the most important changes first?

(Any broader normalization should be discussed separately with the design team, and I imagine those PRs would need to be split by style variant so the surface area of regression testing might be manageable.)

[KevinBatdorf]

That make sense. I've updated it now to only include changes to the three destructive styles:

Screen.Recording.2022-10-25.at.2.58.25.PM.mov

Also, if you think it's easier to tweak something yourself feel free to just push a change directly.

[mirka]

I was trying to iron out the minor remaining inconsistencies and realized how convoluted this actually is 😂 I now think the cleanest way to accomplish this is to override the CSS variables. We can basically delete all the .is-destructive styles and replace it with three lines of CSS var overrides. The only exception is the "default" variant which is styled differently from the non-destructive "default".

@KevinBatdorf I can't push directly because as an external contributor your branch is not technically a part of this repo yet. But here's the commit that I would've pushed. What do you think?

cc @ciampo ^ if you have any concerns with this approach.

[KevinBatdorf]

That seems fine to me. I'm trying to figure out how to merge in that commit. git cherry-pick didn't work. I now realize I'm not on my fork of Gutenberg locally but it still PR'd from my fork magically.

[mirka]

Would the patch file be easier?

[KevinBatdorf]

Yeah I was able to apply it. Still ran into some problems with it though. I'll just leave a note here in case someone ends up here from Google trying to apply a patch like this:

1. I had to remove the merge from trunk I did yesterday -> git reset c264be0
2. Then I was able to apply the patch I saved locally -> git apply ~/Desktop/gb.patch
3. Add the file -> git add packages/components/src/button/style.scss
4. I expected signoff to work but it did not (no clue why!) -> git am --signoff < ~/Desktop/gb.patch
5. I aborted the am session (and grabbed a coffee) -> git am --abort
6. Co-signed the commmit -> git commit --author="Lena Morita <[email protected]>" -m "patch from Lena Morita"
7. Finally git push (if you had to do step 1 above then you may need --force, but double check your work first if so)

[KevinBatdorf]

About the gray you added, It still puts the active state at less than an AA rating, but I think the whole thing needs attention so maybe not necessary for this PR.

What do you think are the next steps if any? Is there somewhere to document that buttons need some ❤️?

[ciampo]

cc @ciampo ^ if you have any concerns with this approach.

The approach looks smart, I like it!

There may still be regressions, unfortunately — I quickly checked storybook and noticed that the text color for the link variant is still black when the button is is-pressed:

@mirka, could this PR be a good enough excuse to add viz reg tests to Button ? We could test for all possible permutations of the component (variant, disabled, icon, isDestructive, etc) and different interactions (idle, pressed, hover, focus, active)

We still probably do a good amount of manual testing for potential regressions in the editor, I'm afraid.

Is there somewhere to document that buttons need some ❤️?

That's a well-known fact! Unfortunately Button may be hardest component to maintain, given the overwhelming amount of style overrides which make it really hard for us to introduce changes without the risk or regressions.

We've been definitely trying to polish the component when possible, though! Apart from bug fixes, we've recently re-written the component's stories to use controls, and we're planning on refactoring the component to TypeScript too in the near future.

I guess the main thing that would make the work on component easier would be to clean up the style overrides, but that's not an easy task nor a quick one.

[KevinBatdorf]

I'm happy to work on any suggestions you have. I'm just worried I may not be the best person to make changes here since I don't have a ton of context of how it all fits together. It's also not clear what's a bug and what's not a bug (or as you mention, what could be a bug but still probably needs to stay as is for the time being)

I think there are two options at the moment?

1. Simply change the text color on the button pictured in the description (.is-primary.is-destructive) and that alone (from blue to white) using the same specificity (to not override any external user overrides). I think this is an obvious bug that shouldn't cause any issues changing it.
2. I can work on adding visual tests first, then changing all the is-destructive variants. That way regressions wouldn't slip through.

[mirka]

There may still be regressions, unfortunately — I quickly checked storybook and noticed that the text color for the link variant is still black when the button is is-pressed:

Is this about isPressed or :active? Your screenshot looks like it's for :active. The text being black is not a regression, it's in line with what happens for the non-destructive link variant when :active (i.e. clicked).

isPressed is another matter entirely. This prop makes no sense for a lot of states (all destructive states, link variants, etc), so it's hard to say what the behavior should even be for such nonsensical combinations 🙃 Similar issues with isBusy!

For this PR, I think our goal should be to correct the obvious bugs and leave the code in an incrementally better state (which we have both accomplished!) so we can continue addressing the wider problems. Good news is, isDestructive is only used in a handful of places in Gutenberg, so the surface area for testing is small. For now I would just focus on testing those, rather than start a deep dive on the state combos which are nonsensical or unlikely to be used by anyone.

[mirka]

For reference, here are the state combinations for isDestructive buttons that appear in Gutenberg (7 occurrences total):

• link
• secondary
• secondary & busy
• tertiary (via MenuItem)

And no obvious style overrides that would be relevant to this PR, so code-wise I feel pretty safe. (Assuming that third-party consumers are using it in similar ways and at a similarly low frequency.)

[KevinBatdorf]

@mirka just to confirm/clarify, you're suggesting this PR is good as it stands currently, or are you suggesting changes?

Also, if you would like to make a suggestion for a followup PR (e.g. visual tests) I'm happy to follow up as well.

[ciampo]

LGTM 🚀

As Lena mentioned above, this PR does a great job of improving incrementally the styles of the destructive button — and it does so cleverly using CSS variables.

There's a lot to do for Button, but this PR is definitely a (small) step in the right direction. Thank you to you both for working on this, @KevinBatdorf and @mirka !