CityOfDetroit / COD-Design-System

Design system for the City of Detroit.
https://cityofdetroit.github.io/COD-Design-System/
MIT License
1 stars 3 forks source link

Fix focus visibility in theme settings #245

Closed sreidthomas closed 3 months ago

sreidthomas commented 4 months ago

The Problem

When using tab to focus light elements on a dark background (as someone might due using a keyboard to navigate a site), the focus visibility of elements is non-apparent.

The Solution

Override the $focus-ring-color SCSS variable and set it to blue instead of $primary in src/scss/_bootstrap-variables.scss.

Then regenerate the theme CSS using ./scripts/compile_styles.sh.

Additional Information

LINK TO ISSUE: https://github.com/CityOfDetroit/COD-Design-System/issues/244

sreidthomas commented 4 months ago

TO-DO AS OF JULY 23, 2024:

sreidthomas commented 4 months ago

--bs-btn-focus-box-shadow is not defined:

variablenotworking

0429a317-4501-4d9d-a2d8-2b20352091e9

sreidthomas commented 4 months ago

Any other help you can give for this PR? I am still not able to get the var(--bs-btn-focus-box-shadow) to show as defined @maxatdetroit

sreidthomas commented 4 months ago

NOTES ABOUT btn-focus-box-shadow

maxatdetroit commented 4 months ago

@sreidthomas

To set the box shadow for buttons on the :focus-visible state in Bootstrap 5.3, you should set the --bs-btn-focus-box-shadow CSS variable. Here's how you can properly set this property:

In src/scss/_host.scss, add the following under both the light and dark mode :host customization blocks:

  --bs-btn-focus-box-shadow: 0 0 0 0.25rem blue;

Note: This is how you would accomplish what you suggested:

This variable can be customized by overriding it in the custom SCSS file

sreidthomas commented 4 months ago

@sreidthomas

To set the box shadow for buttons on the :focus-visible state in Bootstrap 5.3, you should set the --bs-btn-focus-box-shadow CSS variable. Here's how you can properly set this property:

In src/scss/_host.scss, add the following under both the light and dark mode :host customization blocks:

  --bs-btn-focus-box-shadow: 0 0 0 0.25rem blue;

Note: This is how you would accomplish what you suggested:

This variable can be customized by overriding it in the custom SCSS file

So I'm testing this solution, and when that variable gets added, isn't the dropdown shadow supposed to be blue @maxatdetroit? Also, how did you know it should be the host.scss file?

maxatdetroit commented 4 months ago

So I'm testing this solution, and when that variable gets added, isn't the dropdown shadow supposed to be blue @maxatdetroit?

Yes, it should. If it isn't turning blue, make sure you've regenerated the CSS from the SCSS using your script: ./scripts/compile_styles.sh after updating any scss files.

Also, how did you know it should be the host.scss file?

This is the bootstrap documentation that suggests modifying the :root style selector to set bootstrap variables (which impacts all elements on the page in the light DOM), however since we're using the shadow DOM, we actually need to use the :host style selector style selector to target the shadow DOM styles specifically.

I learned this while reading all the code and reading those links I just shared a while back while getting up to speed on custom elements, bootstrap, and shadow DOM styling.

sreidthomas commented 4 months ago

So I'm testing this solution, and when that variable gets added, isn't the dropdown shadow supposed to be blue @maxatdetroit?

Yes, it should. If it isn't turning blue, make sure you've regenerated the CSS from the SCSS using your script: ./scripts/compile_styles.sh after updating any scss files.

Also, how did you know it should be the host.scss file?

This is the bootstrap documentation that suggests modifying the :root style selector to set bootstrap variables (which impacts all elements on the page in the light DOM), however since we're using the shadow DOM, we actually need to use the :host style selector style selector to target the shadow DOM styles specifically.

I learned this while reading all the code and reading those links I just shared a while back while getting up to speed on custom elements, bootstrap, and shadow DOM styling.

I did run the ./scripts/compile_styles.sh command but its still not blue. The links inside are black @maxatdetroit:

246
maxatdetroit commented 3 months ago

I did run the ./scripts/compile_styles.sh command but its still not blue. The links inside are black @maxatdetroit:

You'll have to take a look in the inspector and make sure the variable is set in the browser and that its impacting the stylesheets the way you'd expect (e.g. make sure that box-shadow is set correctly on .btn:focus-visible ruleset).

sreidthomas commented 3 months ago

Even after adding it to that file, it still says undefined. Is there anything else I should check @maxatdetroit?

maxatdetroit commented 3 months ago

Even after adding it to that file, it still says undefined. Is there anything else I should check @maxatdetroit?

When we update something in the source code, then compile it, and serve the minified/aggregated version in the browser, we know that the breakdown is somewhere in that sequence of events:

  1. Updating the source code. Sounds like you've already checked this. ✅
  2. Compiling the source code. E.g. from SCSS into CSS. Have you checked the compiled CSS to make sure the variable is set in the style sheets there? ❓
  3. Making sure the right version of the file is served to the browser. E.g. downloading the CSS directly from the browser so you can get an exact copy of what the browser is using when applying the styles. Have you tried this? We'd want to check that the variable is set somewhere in the stylesheets. ❓
  4. Making sure there aren't any styling conflicts in the stylesheets the browser is using. E.g. maybe something later in the stylesheets is overwriting the variable and unsetting it? Or maybe the :host selector isn't the right selector due to specificity or something else? Have you checked this? We'd want to use the developer inspector to see where else the variable may be being set or looking at the computed styles to see if it's not specific enough or overridden by a conflicting stylesheet. ❓

I would check out the remaining suggestions above with a ❓ next to them. Let me know if you want some more details about how to go about checking those. My notes are sort of brief above.

sreidthomas commented 3 months ago

TO DO AS OF JULY 29, 2024:

var dcss dcss2 dcss3 dcss4
sreidthomas commented 3 months ago

Making sure the right version of the file is served to the browser. E.g. downloading the CSS directly from the browser so you can get an exact copy of what the browser is using when applying the styles. Have you tried this? We'd want to check that the variable is set somewhere in the stylesheets. ❓

How do I download the CSS file from the browser @maxatdetroit?

sreidthomas commented 3 months ago

Making sure the right version of the file is served to the browser. E.g. downloading the CSS directly from the browser so you can get an exact copy of what the browser is using when applying the styles. Have you tried this? We'd want to check that the variable is set somewhere in the stylesheets. ❓

How do I download the CSS file from the browser @maxatdetroit?

I followed steps from this: https://webdeveloper.com/bounties/how-to-download-css-file-from-a-website/

But I don't see the relevant CSS files

maxatdetroit commented 3 months ago

Here's a way to find the relevant stylesheets using a combination of the inspector element picker and the stylesheet tab:

https://github.com/user-attachments/assets/4fd4b5cc-40f1-4fca-971e-c6466f3718f3

Our stylesheets are a little different than most websites because they're all inline stylesheets in the design system (as opposed to external stylesheets with helpful names).

sreidthomas commented 3 months ago

We'd want to use the developer inspector to see where else the variable may be being set or looking at the computed styles to see if it's not specific enough or overridden by a conflicting stylesheet.

Is there an easy way to find where else the variable is being set @maxatdetroit? I only switched browsers. I'm using FireFox and now the dropdown links are blue. In Chrome, they were black. I'm using the same code but getting different results in different browsers:

FIREFOX:

bluelinkdd

CHROME:

ddblack
maxatdetroit commented 3 months ago

Regarding Different Styles in Different Browsers

That is expected.

Default browser stylesheets, also known as user agent stylesheets, are a set of CSS rules that browsers apply to HTML elements by default. These stylesheets provide basic styling and layout for web pages even when no custom CSS is specified by the web developer.

The purpose of default stylesheets is to ensure that HTML documents have a consistent, readable appearance out of the box. They define things like:

  1. Basic text formatting (font sizes, line heights, etc.)
  2. Margins and paddings for various elements
  3. Default colors (e.g., blue for links)
  4. Basic layout rules (e.g., block vs. inline display)

However, these default styles can vary between browsers, which is one reason why web developers often use CSS reset or normalization techniques to create a more consistent cross-browser starting point.

Now, regarding the :focus-visible pseudo-class and why you might see different styles across browsers:

  1. Browser Implementation: The :focus-visible pseudo-class is a relatively new feature in CSS. It was introduced to provide a way to show focus indicators only when they're needed (e.g., when using keyboard navigation, but not when clicking with a mouse). Different browsers may implement this feature slightly differently or may be at different stages of implementation.

  2. Default Styles: Just like with other elements, browsers may have different default styles for the :focus-visible state. Some may apply a more prominent outline, while others might use a subtler indication.

  3. Heuristics: Browsers use different heuristics to determine when to apply :focus-visible styles. These heuristics can vary, leading to inconsistent behavior across browsers.

  4. Browser Versions: Older versions of browsers might not support :focus-visible at all, falling back to the standard :focus behavior or ignoring it entirely.

  5. Operating System Influence: The appearance of focus indicators can also be influenced by operating system settings, which can lead to differences even between the same browser on different OS platforms.

maxatdetroit commented 3 months ago

Regarding How to Find Where a CSS Variable is Set

Try using the 'Computed' tab of the style inspector in the dev tools. You can use the filter to filter down to a specific variable name, then expand the notes for that variable and it will tell you all the cascading styles and how they're overriding each other. Here's a video:

https://github.com/user-attachments/assets/eeb77691-b29c-46c6-ba9d-6b45f915e9b4

sreidthomas commented 3 months ago

Since Chrome and Firefox are behaving differently and that's normal, which browser should I focus on as far as this PR goes @maxatdetroit?

maxatdetroit commented 3 months ago

@sreidthomas , we should make sure the fix works on both browsers. Ideally, they have the same style behavior too (e.g. both are blue) but as long as there's a focus indicator with sufficient contrast on each browser, then it's not strictly necessary that they're the same exact styles.

sreidthomas commented 3 months ago

NEW NOTES AND FINDINGS AS OF JULY 30, 2024:

sreidthomas commented 3 months ago

Okay, so I think I found the issue. The .btn class has the btn-focus-box-shadow property set. When I remove it in Developer Tools, the blue highlight around dropdown shows @maxatdetroit, I'm just not sure what file to make the change in. I tried adding this line to _host.scss but it did not work:

.btn { --bs-btn-focus-box-shadow: none; } I shared the video via teams because it won't let me share here.

maxatdetroit commented 3 months ago

@sreidthomas very nice investigative work!

Based on the video, it looks to me like the --bs-btn-focus-shadow-rgb is not set in the --bs-btn-focus-box-shadow rule where it's referenced.

Any crossed-out properties (indicating they've been overridden)

This can also mean they've never been set in the first place.

Have you tried setting the --bs-btn-focus-shadow-rgb in the _host.scss file?

sreidthomas commented 3 months ago

--bs-btn-focus-shadow-rgb

I set it to blue @maxatdetroit?

maxatdetroit commented 3 months ago

I think it expects an RGB format. blue is a named color with a HEX (i.e. base 16) value of #0000FF. That converts to rgb(0,0,255) in RGB format.

sreidthomas commented 3 months ago

I set that variable in the file but it didn't work. It's only when I remove the whole thing like in the video that the blue highlight works @maxatdetroit

maxatdetroit commented 3 months ago

but it didn't work

Would you try using the debugging tools/workflow you've been using to figure out why? If you get stuck on something, let me know and I can look as well.

sreidthomas commented 3 months ago

WHERE I BECAME BLOCKED ON THIS ISSUE:

Found how the property --bs-btn-focus-shadow-rgb is being set twice but not sure where/how to fix the issue.

Currently right now, inside the .btn class if you disable the property, the blue shadow box shows but when it is enabled, the same property is being used but with lesser values so the blue shadow box is there but not as visible.

Video was posted on teams demonstrating this

maxatdetroit commented 3 months ago

After the most recent commits, this is now working for all elements, including buttons.

@sreidthomas for future reference, the problem with the buttons was that --bs-btn-focus-shadow-rgb was also being set in the bootstrap stylesheets after the :host ruleset, effectively overriding whatever we had put in the :host stylesheet.

How did I find that? With the help of the 'computed styles' pane in dev tools, I noticed all the styles were being set in the main bootstrap style sheet. So then I read through the compiled bootstrap styles in themed-bootstrap.css and noticed the variable is set multiple times.

How did I fix it? I moved the variable definition into _detroitmi-style-guide.scss which gets imported at the very end of the main bootstrap style sheet, and thus, overrides the previous variable definitions. We also could have defined the variable in button.css but if we did it there, then it wouldn't take effect for buttons unless they're cod-buttons and since others use our bootstrap stylesheets with basic button elements (not cod-buttons), we need to define it inside the bootstrap style sheets instead of the component style sheet to make sure it takes effect everywhere.

maxatdetroit commented 3 months ago

Latest test:

https://github.com/user-attachments/assets/2d326fc9-0453-4fef-b9f8-912c2918aa83