Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix focus visibility in theme settings #245

Merged
merged 8 commits into from
Aug 5, 2024
Merged

Fix focus visibility in theme settings #245

merged 8 commits into from
Aug 5, 2024

Conversation

sreidthomas
Copy link
Contributor

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

…he color to blue. Compiled using compile_styles.sh
@sreidthomas sreidthomas requested a review from maxatdetroit July 18, 2024 15:36
Copy link
Member

@maxatdetroit maxatdetroit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sreidthomas would you test and provide a video of the test?

It looks this change doesn't fix the focus visibility of the dropdown button on the responsive offcanvas menu:

2024-07-22.10-29-24.mp4

We'll need to make this work for the dropdown button and test it before merge.

For fixing the the offcanvas dropdown button, I'd recommend looking at the bootstrap ruleset for .btn:focus-visible, figuring out why our blue focus configuration is not applying to that rule, then figuring out the way to fix that. Let me know if you want some additional ideas of what to check.

@sreidthomas
Copy link
Contributor Author

sreidthomas commented Jul 23, 2024

TO-DO AS OF JULY 23, 2024:

@sreidthomas
Copy link
Contributor Author

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

variablenotworking

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

@sreidthomas
Copy link
Contributor Author

sreidthomas commented Jul 25, 2024

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
Copy link
Contributor Author

sreidthomas commented Jul 25, 2024

NOTES ABOUT btn-focus-box-shadow

  • $btn-focus-box-shadow: This variable provides the box shadow of the button on focus. By default, it is 0.
  • This variable can be customized by overriding it in the custom SCSS file

@maxatdetroit
Copy link
Member

maxatdetroit commented Jul 25, 2024

@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
Copy link
Contributor Author

sreidthomas commented Jul 25, 2024

@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
Copy link
Member

maxatdetroit commented Jul 25, 2024

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

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

@maxatdetroit
Copy link
Member

maxatdetroit commented Jul 26, 2024

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
Copy link
Contributor Author

sreidthomas commented Jul 29, 2024

TO DO AS OF JULY 29, 2024:

  • Update Source Code
  • Check to make sure the variable is set in the compile CSS file (theme-bootstrap.css listed below)
var
  • Make sure the right version of the CSS file is being served to the browser: downloaded the file and the variable looks likes it being used in the file that is being served to the browser
dcss dcss2 dcss3 dcss4
  • Make sure there are no conflicting styles in the stylesheet

@sreidthomas
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Member

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

2024-07-29.12-51-29.mp4

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
Copy link
Contributor Author

sreidthomas commented Jul 29, 2024

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
Copy link
Member

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
Copy link
Member

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:

2024-07-29.16-25-47.mp4

@sreidthomas
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

NEW NOTES AND FINDINGS AS OF JULY 30, 2024:

  • The order of the rules (rules lower in the list override those above)
  • Any crossed-out properties (indicating they've been overridden)

@sreidthomas
Copy link
Contributor Author

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
Copy link
Member

@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
Copy link
Contributor Author

--bs-btn-focus-shadow-rgb

I set it to blue @maxatdetroit?

@maxatdetroit
Copy link
Member

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
Copy link
Contributor Author

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

@sreidthomas sreidthomas reopened this Jul 30, 2024
@maxatdetroit
Copy link
Member

maxatdetroit commented Jul 30, 2024

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
Copy link
Contributor Author

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
Copy link
Member

maxatdetroit commented Jul 31, 2024

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
Copy link
Member

Latest test:

Screen.Recording.2024-07-31.at.3.01.23.PM.mov

@maxatdetroit maxatdetroit self-requested a review August 5, 2024 15:14
@maxatdetroit maxatdetroit linked an issue Aug 5, 2024 that may be closed by this pull request
@maxatdetroit maxatdetroit merged commit b713f92 into dev Aug 5, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix focus visibility in theme settings
2 participants