Adaptive color not working

[dougmacknz]

I've forked the hosted example and changed the scrollable area to have a black background. I've also changed the --focus-primary var to black. https://codesandbox.io/s/bold-sun-lx1im?file=/public/index.html

The focus ring should be adapting and showing a visible focus ring in this case right? At the moment it's still just creating a black focus ring.

[faultyserver]

The code is actually working as intended, but this is a pathologically bad case for how the ring decides on a color to choose. You can see the simple logic here: https://github.com/discord/focus-rings/blob/a0ca56d7286c5b54cdf4e200ddb944be4564e52d/src/FocusColorUtils.tsx#L12-L22.

As you can see, it first checks for saturation, and if that's low enough, it will default to the PRIMARY color, which in your example is explicitly set to black. Since a black (or white) background has no saturation, this is the path that gets chosen there. Only after that does the code check the brightness to decide for a white or black ring.

This is logic that I honestly kind of skimped on because it was "good enough" for our use case with a bright blue ring. But it's something that can and should probably be done better. I don't have any immediate ideas for improvement, but it's something to look at for sure!

[Thisisjuke]

Hey, can we have a prop to disable this code snippet ? When I try to force another color, I think its prevent the change.

I would like to do something like :

.slider :global(.focus-rings-ring) {
   border: 0!important;
   border-color: transparent!important;
}

This css code is working because the content of my rings disappeared when I do :

.slider :global(.focus-rings-ring) {
   display:none!important;
}

The use-case is quite simple : I want to put focus-rings on card, and these cards in a slider (SwiperJS for exemple).

At the moment, I have the following behavior :

--> I move with tabs, then I drag my Slider. After Dragging / Swiping

I don't want to disable focus-rings, juste hide the color of the border onSwipe / onDrag, to prevent :

• Screen reader miss understanding
• Human miss understanding
• Ugly style / behavior :disappointed:

How can I do this ? Thx :sunglasses:

@faultyserver

[faultyserver]

@Thisisjuke I think the behavior you're seeing there, where the ring "detaches" from the element that it's supposed to be tracking, is likely because you're missing a FocusRingScope on the inside of the scrolling component. Without that, the ring renderer doesn't think that anything is moving when you scroll, so you get this static-looking ring while everything else moves.

Something like:

<ScrollingContainer ref={scrollingRef}>
  <FocusRingScope containerRef={scrollingRef}>
    {...yourScrollableChildren}
  </FocusRingScope>
</ScrollingContainer>

should make the rings track the elements properly.

To your original question, if you want to just disable the ring while the scrolling happens, you can also pass the disabled prop to the FocusRing around the element itself, which will prevent it from rendering at all when set.

We will probably add some flexibility for being able to inject a "best color selector" function or the like in the future, but hopefully one or both of those solutions is sufficient for your use case.

[Thisisjuke]

Hey thanks for your answer !

The screenshot was not the precise use-case we are looking for, just the global idea. I checked all the pages and we are now using a FocusRingScope as a child of each one of our absolute positioning element.

Perhaps, this colors behavior (getBestFocusColor) often gives us problems when the background of one of the elements of the page is of a certain color (blue) and the rest of the page is on a white background. The outline of the button is white on blue: perfect. But the outline of the elements triggered by this button is white (Menu or NavList from HeadlessUI or any absolute positionned list): we have white rings on white background.

The question is: can we choose the color for each FocusRing via a property of the FocusRing or the FocusRingScope? But above all to have the possibility of not passing by a check inside of getBestFocusColor ?

We will probably add some flexibility for being able to inject a "best color selector" function or the like in the future

If the above question is no, are you speaking about a close future or not at all ? If it become critical in our project (I hope not ahah) I might look for a dirty workaround or discuss to do a PR.

EDIT: discussed with my team, we can do the PR, just to be sure how you see this feature :smiley:

Thx for, your time :pray:

[faultyserver]

There are three (apparently undocumented) className props that FocusRing accepts, ringClassName, focusClassName, and focusWithinClassName that all let you apply additional styles to focused elements. ringClassName is probably what you want, to change the style of the ring when it is around a specific element.

However, I'd probably try to avoid doing that too much since it's hard to maintain and add to all of your focus rings across an app. Instead, I do think being able to inject your own color selector function would be good. If you have bandwidth to work on a PR, that would be great! Otherwise I'll probably try to get around to it soon.

we have white rings on white background.

This sounds like somehow the background color calculation isn't working, but I'm not really sure why without being able to see more of the context.

[Thisisjuke]

Hello there, After a bit of investigation with our team, we tough of an easy solution for us with no breaking changes for anyone.

Now, you are doing things like this :

enum ALLOWED_FOCUS_RING_COLORS {
  PRIMARY = "var(--focus-primary)",
  WHITE = "rgba(255,255,255,0.7)",
  BLACK = "rgba(0, 0, 0, 0.85)",
}

Maybe you can do something like that :

enum ALLOWED_FOCUS_RING_COLORS {
  PRIMARY = "var(--focus-primary)",
  LIGHT = "var(--focus-light, rgba(255,255,255,0.7))",
  DARK = "var(--focus-dark, rgba(0, 0, 0, 0.85))",
}

Now, if the saturation is below 0.4, we can chose on our own to force the same color for the 3 variables, or to set them to transparent in some contexts.

@faultyserver What do you think about this one ? Thx for your time :pray:

[Thisisjuke]

@faultyserver @dougmacknz Hello, there is a PR open since Dec 2, 2021. Can we have a status on this PR please ?