In D5321#157310, @atul wrote:

Do you think it would be clearer if we renamed primary to filled and secondary to outline to help distinguish the two variants?

I think it makes sense. We might want to have success and danger buttons side by side in the future, and having them both be filled instead of primary makes more sense. Also, we now use this component in more places and not just as "Cancel", or "Continue" buttons.

Are you sure about this? Can you perhaps link some StackOverflow or GitHub discussions that talk about this? I wonder what would happen if you dynamically (in JS) render a <style> tag

Here and here. I couldn't find anything that explicitly states that it's impossible but it seems like it. I tried just including ':hover': {color: 'red'} in the inline styles but it didn't work for me.
I think rendering <style> is one of the workarounds mentioned in the linked posts but it doesn't seem that great to me.

Hmm... for the first, I strongly suspect there is a way to darken instead of brighten via a transform, no? For the second, I'm curious to know more about how complicated these are.

Yeah, the first point was a minor complication. You can just use values higher than 1.

For the second, I couldn't figure out how to implement it before, but I tried it once again after your comment and I have something that works (although I haven't tested it for all cases yet). It would look something like this (button.css):

.btn {
  position: relative;
  border-radius: 4px;
  font-size: var(--m-font-16);
  padding: 12px 24px;
  color: var(--fg);
  cursor: pointer;
}

.btn > * {
  z-index: 2;
}

.btn::before {
  content: "";
  border-radius: inherit;
  position: absolute;
  top: 0;
  left: 0;
  width: 100%;
  height: 100%;
  background: inherit;
  z-index: 1;
}

/* === These classes would be exclusive */
.btn-light:hover::before {
  filter: brightness(0.85);
}

.btn-dark:hover::before {
  filter: brightness(1.15);
}
/* === */

There is at least one problem with this. You can't use plain text in buttons (<Button> Cancel </Button>) because of the z-index manipulation. You have to first wrap the text in <div> or something else which might be unexpected for someone using this component. Also, I wonder if changing z-index could break some other thing, I can see we are changing it in a few other places in the code but I'm not familiar with that property very much.

For the second, I couldn't figure out how to implement it before, but I tried it once again after your comment and I have something that works (although I haven't tested it for all cases yet). It would look something like this (button.css):

.btn {
  position: relative;
  border-radius: 4px;
  font-size: var(--m-font-16);
  padding: 12px 24px;
  color: var(--fg);
  cursor: pointer;
}

.btn > * {
  z-index: 2;
}

.btn::before {
  content: "";
  border-radius: inherit;
  position: absolute;
  top: 0;
  left: 0;
  width: 100%;
  height: 100%;
  background: inherit;
  z-index: 1;
}

/* === These classes would be exclusive */
.btn-light:hover::before {
  filter: brightness(0.85);
}

.btn-dark:hover::before {
  filter: brightness(1.15);
}
/* === */

Can you explain more why the z-index is necessary? What are you trying to achieve with it?

There is at least one problem with this. You can't use plain text in buttons (<Button> Cancel </Button>) because of the z-index manipulation. You have to first wrap the text in <div> or something else which might be unexpected for someone using this component. Also, I wonder if changing z-index could break some other thing, I can see we are changing it in a few other places in the code but I'm not familiar with that property very much.

I think it would be pretty easy to check in the Button component if the children prop is a string, and if so to wrap it with a div. However I do agree with you that z-index is probably best to avoid if possible... still confused why it's necessary

Without the z-index the ::before pseudo-element covers the Button children and I couldn't find a way to make it work without it.

Sorry, I should have explained more and not just dropped the code in the comment.

The problem is that transform also applies to the Button children (text and icons) and it looks bad so we don’t want that. We need to create another element that will be the background of the button. We can do that in css with ::before pseudo-element that works as if there was an additional child of the <button>. It needs to fill the entire parent so that’s why I added the position, left, width etc. But because of that, it displays above the other siblings and that’s why we need to change the z-index.

I hope that makes it clearer.