quarto-dev / quarto-cli

Open-source scientific and technical publishing system built on Pandoc.
https://quarto.org
Other
3.93k stars 325 forks source link

Insufficient contrast with navbar-color (or navbar-fg) when using themes #238

Closed batpigandme closed 2 years ago

batpigandme commented 2 years ago

Following the quarto online docs to make a dark version of Cosmo, I'm getting a super faded navbar foreground color for the active links (screenshot below). Poking around at the HTML, it looks like the boostrap dark theme is using the navbar-light foreground color for the dark-theme navbar, even with navbar type set as dark in the YAML.

Using:

theme:
  light: cosmo
  dark: [cosmo, theme-dark.scss]

with theme-dark.scss

/*-- scss:defaults --*/
// Base document colors
$body-bg: #181818;
$body-color: white;
$link-color: #75AADB;

// Code blocks
$code-block-bg-alpha: -.8;

image

I run into a similar issue when applying other Bootswatch themes (the insufficient contrast between the navbar foreground and background)

Here's an example using sandstone with no dark theme set: image

I've tried adding and removing yaml parameters for the navbar, e.g.

  navbar:
    type: dark
    background: dark

I've also tried adding in the theme and bootstrap defaults for the navbar links in a custom.scss on top of the theme, but I still get the same result.

Let me know if you need more info—it might just be a documentation thing where I'm not understanding the layering of the files. But I've spent quite a bit of time playing around with theming at this point (decided to do a bit of a crash course over the holiday), so I'd be happy to help write something up so other folks don't stumble into the same error.

dragonstyle commented 2 years ago

Hi,

Thanks for reporting this- you are correct about the navbar behavior resulting in not nearly enough contrast! This can be customized using either

navbar:
   foreground: <hex color>

or by setting the variable navbar-fg In your dark theme. But you really shouldn’t need to set this to get a reasonable behavior- I will improve the default behavior!

I didn’t see this variable for customizing the navbar in our docs - I’ll add that as well!

batpigandme commented 2 years ago

Hi Charles, Thanks. Yeah, I saw the foreground color option, but it's definitely a tricky default—it looks like the rendered HTML keeps coming out to navbar-light, even when type: dark is set in the YAML, which is a bit counterintuitive.

https://github.com/quarto-dev/quarto-cli/blob/0cfedef2d7fccbc2e102bb996409639bbe43daba/src/resources/projects/website/templates/nav-before-body.ejs#L11

I think there will be folks (myself included) who will think that the styling issue comes from the scss/css if they're not using the YAML to configure most of their styling (i.e. if they apply a bootswatch theme that uses a dark navbar by default, they'll probably assume the elements in the navbar will be styled with the navbar-dark class).

All in all, the docs are great! Thanks so much for your help.

jjallaire commented 2 years ago

For documentation I think given that you can't really get anywhere with themes w/o sass that we shouldn't talk much (if at all) about the navbar : foreground YAML option (I think its super confusing to talk about both, esp. when YAML runs out of gas almost immediately).

@dragonstyle If you switch to dark mode on quarto.org you'll see that the link hover color for the navbar results in pretty much illegible text.

I also noticed that our own dark theme on quarto.org has diverged from the docs (we've added more variables):

https://github.com/quarto-dev/quarto-web/blob/main/theme-dark.scss

Compare that to the docs: https://quarto.org/docs/output-formats/html-themes.html#customizing-themes

Should we update the docs to have the complete set of variables?

batpigandme commented 2 years ago

The disconnect for me is coming with respect to the navbar's class (since the color applied on the links in the navbars is coming because it's always navbar-light) e.g. on quarto.org in dark mode: <nav class="navbar navbar-expand-lg navbar-light ">

Or in dark mode for the site I was playing around with here, the text color for the active nav element and brand comes in the from the CSS selector: .navbar-light .navbar-nav .show >.nav-link, .navbar-light .navbar-nav .nav-link.active{}

So (I think) to override it in a custom theme I'd have to specify the foreground color as being for navbar-light, even though it's a dark navbar (same holds for applying bootswatch themes like sandstone).

dragonstyle commented 2 years ago

@dragonstyle If you switch to dark mode on quarto.org you'll see that the link hover color for the navbar results in pretty much illegible text.

I agree- we intend to have better default behavior, but its clearly not doing a good job. I'll take a look at improving that behavior!

I also noticed that our own dark theme on quarto.org has diverged from the docs (we've added more variables):

https://github.com/quarto-dev/quarto-web/blob/main/theme-dark.scss

Compare that to the docs: https://quarto.org/docs/output-formats/html-themes.html#customizing-themes

Should we update the docs to have the complete set of variables?

I've been trying to keep the example dark mode very example to make it easier to understand, but maybe that is not helping since it doesn't show everything that is required for a more complex implementation... What do you think about things like the rules in the theme file? I think that it will be a challenge to keep the two in sync as the quarto site becomes more complex and begins needs somehow custom rules or variable customization (e.g. that .layout-example rule) that don't necessarily apply to general dark themes.

dragonstyle commented 2 years ago

The disconnect for me is coming with respect to the navbar's class (since the color applied on the links in the navbars is coming because it's always navbar-light) e.g. on quarto.org in dark mode: <nav class="navbar navbar-expand-lg navbar-light ">

This is uncovering a challenge in the way that we approached navbar and sidebar theming. While Bootstrap has the idea of light/dark navbars, we moved away from that and replaced the concept of a light and dark navbar with a set of navbar variables that can be set for your light and dark theme. Since the navbar yaml element has no concept of which theme (light or dark) is being used, specifying light and dark there might be confusing or in conflict with what the light or dark theme would want.

Instead, we use the following variables (which can be provided for dark and light themes) to compose the navbar, theoretically providing a good default for the foreground color (though this is currently not doing a good job). You can provide these variables for your light and dark theme, and this would do the trick.

navbar-bg
navbar-fg
navbar-hl

The same consistent variables are available for sidebars (sidebar-<bg|hl|fg>). This does have the benefit of being very simple and consistent, and it plays very nicely with the concept of dark and light versions of the same site and navbar.

As I read your comment and struggles to understand how to accomplish what you want, however, I see that anyone with experience with Bootstrap navbars might very quickly find themselves lost trying to understand what we are doing, especially since we are are ignoring the Bootstrap concept of light and dark at the navbar level. (From an implementation perspective, we are always using the 'light' classes, but applying colors to the light classes based upon the active theme. In the case of our dark contrast color, we are applying a custom color for the dark themes, it just is a poor contrast - we're lightening the link color but that isn't working well at all).

I think we could reconsider this choice and re-introduce the light and dark variants and the weirdness that this may cause with light and dark themes (e.g. a light navbar with a dark theme?), or more likely, I think we should do a much better job documenting / explaining what we're doing.

Curious if this explanation helps you makes sense of what we're doing...

(edit- corrected variable name navbar-hl)

batpigandme commented 2 years ago

OK, this makes total sense, and was definitely the source of my confusion, since I was trying to figure out the difference in rendering between my navbar with compiled css, and the demo one from the Bootswatch site (where, when the primary navbar is dark, the rendered navbar element is also dark).

I definitely went deeper into Sass, Bootstrap, and Bootswatch than I think most will (the post I wrote literally says: you can do everything in quarto without learning any of this), but I’d be more than happy to make coherent and share my notes re. stumbling points (I’m at RStudio, too, and Hadley’s put learning quarto in my latest remit, so I’m happy to do a call or message in slack, etc).

batpigandme commented 2 years ago

Concrete example, applying Sandstone theme:

Rendered site here.

Navbar in rendered site: image

Me trying to figure out what's wrong by comparing the primary navbar from the Bootswatch theme on the bootswatch site:

<nav class="navbar navbar-expand-lg navbar-dark bg-primary">

with the same element when it's rendered via Quarto:

<nav class="navbar navbar-expand-lg navbar-light ">

Based on this, my thought was that I needed to figure out why the primary (default) navbar was being classed as light. Looked to the Quarto theme file for sandstone where:

https://github.com/quarto-dev/quarto-cli/blob/bdfb7c3c0101bcd2575af067dbed1b8c3aaef729/src/resources/formats/html/bootstrap/themes/sandstone.scss#L152-L164

This made me think that the issue was that the navbar was being classed as light, and, thus, using the wrong colors for the foreground text for active and hover, since this color was coming from (in the compiled bootstrap):

.navbar-light .navbar-nav .show>.nav-link, .navbar-light .navbar-nav .nav-link.active {
    color: #4f5a3d;
}

That color (in my very limited search—i.e. looking at the color schemes in the sandstone.scss that were already hex codes) didn't seem to be coming from sandstone, but from bootstrap (but that part was just a guess)

dragonstyle commented 2 years ago

That makes complete sense- sorry to have lead you on a wild good chase! Tracing the colors from the rules backwards can be a bear since they're generated through a series of layered variable declarations - very hard to follow where they're coming from! Makes it even more important that things work as they should...

The change I made was a combination of two things:

1) We are trying to compute a contrasting color for the navbar-hl if it isn't provided, but we weren't using an accessible contrast generating version. I switched us to using the accessible version.

2) Our accessible contrast generating function also made some assumptions about the background color which I had to remedy. I fixed this and now the call explicitly provides the background color to use when testing contrast.

The combination of these two fixes (here https://github.com/quarto-dev/quarto-cli/commit/45e38ff37e2d689e8ef87a300757812ba427adf3) should give you out of the box correct behavior- for the navbar highlight color, we will shift the link color (if specified) until we achieve sufficient contrast with the navbar background.

This should be available in the latest version of Quarto and I've regenerated the quarto.org website using the fixed version, which shows a much more sane behavior!

Thanks again for finding this issue- if you have a moment to give it another try with the updated version of Quarto, I'd appreciate hearing how it works out.

batpigandme commented 2 years ago

Works great!

I definitely think that JJ's suggestion of cataloging the available Sass variables would help here, since this means that there are parts of the bootswatch themes that, in effect, aren't applied (iIncluding, presumably, their minimum contrast ratios). And since this means that they'll look different to the out-of-the-box bootswatch theme examples in their native docs.

I assume this means, then, that I'd specify a navbar-h1 color in custom.scss, and custom-dark.scss (or whatever I name them), respectively, if I want to have direct control over them.

Thanks for the quick fix.

dragonstyle commented 2 years ago

Yes you’re right, we will look very similar but perhaps not identical to the bootswatch themes. We do have the ability to provide custom defaults for themes, so I will see if I can match them exactly out of the box…

For documentation on variables, we have this:

https://quarto.org/docs/output-formats/html-themes.html#sass-variables

I’ve added a link to it from the website navigation section since otherwise I don’t know that you’d know to look there!

You are correct, just set that navbar-hl variable in your theme files to provide your own color…

batpigandme commented 2 years ago

Was just looking at that section again, and realized I somehow thought those were variables to be specified in the YAML 😱 (my bad, it doesn't actually read like that's the case), which I don't think would happen with fresh eyes.

jjallaire commented 2 years ago

@batpigandme, there was some sleight of hand on those docs! When I read your blog post it said that those variables were specifiable in YAML. I knew they weren't so I followed the link and sure enough the docs did read as if they could be used in YAML! So I changed the docs to be more clear (thanks for the assist in pointing this out :-)

batpigandme commented 2 years ago

Ah! Good to know. I'll update the post (definitely a living document, since I was just kind of taking notes as I learned)

batpigandme commented 2 years ago

Played around with this a little bit more this morning, and (if I understand correctly) and the navbar is always classed as navbar-light (from the ejs in this comment https://github.com/quarto-dev/quarto-cli/issues/238#issuecomment-1003716971), I think that's at the root of the disconnect with the Bootswatch theme defaults not matching up, since their primary is often dark.

Is there any way to get the navbar classed as navbar-dark or bg-dark?

For example, with Cyborg (a dark theme with a dark body background), the generated navbar is (naturally) navbar-light, which had me adding the following to my custom.scss just to get the default navbar look back (since the theme variables for things like hover assume navbar-dark)

// Body for Cyborg
$body-bg: #060606 !default;

// Navbar for Cyborg
$navbar-bg: $body-bg !default;
$navbar-brand: #fff !default;
$navbar-hl: #fff !default;

/*-- scss:rules --*/
.navbar {
  border-bottom: 1px solid #282828;
}

It seems that Bootstrap doesn't really have a notion of a navbar that isn't classed as light or dark (which is unfortunate), so it does beget a bit of a disconnect with respect to the quarto theme system and the external Bootstrap/Bootswatch documentation.

jjallaire commented 2 years ago

We had quite a bit of discussion w/ @cpsievert about this based on his experience w/ bslib (btw Quarto actually uses the bslib fork of bootstrap/bootswatch) and collectively decided that the bootstrap/bootswatch navbar-light/navbar-dark thing wasn't implemented well and that we should do our own thing there. @cpsievert and @dragonstyle do I have this right?

At a minimum it seems like we need more unambiguous docs about. navbar theming (letting users know that the traditional ways to theme navbars won't work 100% as expected).

batpigandme commented 2 years ago

Yeah, it makes sense to avoid it (I agree that it's confusing). I'm wondering if navbar-dark might actually be the preferable default, given that's the navbar class for most of the theme defaults (whether it's bg-primary or bg-dark...basically all but the lightest of navbars)

jjallaire commented 2 years ago

Good point! IIUC using navbar-dark as the default would better match the actual appearance of our default navbars AND better match examples of bootstrap navbar CSS in the wild?

batpigandme commented 2 years ago

I think that's the case, JJ.

dragonstyle commented 2 years ago

Should be an easy change - I'll make it!