Skip to content

Conversation

@andyfeller
Copy link
Member

@andyfeller andyfeller commented Mar 10, 2025

Relates github/cli#830

These changes relate to a new experimental feature for rendering markdown in GitHub CLI and CLI extensions using accessible colors.

Out of the box, markdown package continues to render GFM using existing charmbracelet/glamour light and dark styles in 8-bit color.

By setting accessible_colors configuration setting or GH_ACCESSIBLE_COLORS environment variable, markdown package will begin using ANSI 4-bit colors that most modern terminals allow users to customize. This will allow users with low vision and color blindness to tailor their experiences more effectively.

Go version change

This pull request bumps the minimum Go version from 1.21 to 1.23 due to transitive dependencies from experimental Go module:

Demo

The following accessible colors have been aligned for consistency with Primer Design System within the limited palette of 4-bit colors.

Headers

Current default

headers, current release, dark appearance headers, current release, light apperance

Accessible

headers, accessible-colors, dark appearance headers, accessible-colors, light apperance

New default

headers, new default, dark appearance headers, new default, light appearance

Horizontal Rule

Current default

horizontal rule, current release, dark appearance horizontal rule, current release, light appearance

Accessible

horizontal rule, accessible-colors, dark appearance horizontal rule, accessible-colors, light appearance

New default

horizontal rule, new default, dark appearance horizontal rule, new default, light appearance

Images

Current default

image, current release, dark appearance image, current release, light appearance

Accessible

image, accessible-colors, dark appearance image, accessible-colors, light appearance

New default

image, new default, dark appearance image, new default, light appearance

Links

Current default

link, current release, dark appearance link, current release, light appearance

Accessible

link, accessible-colors, dark appearance link, accessible-colors, light appearance

New default

link, new default, dark appearance link, new default, light appearance

Code

Current default

code, current release, dark appearance code, current release, light appearance

Accessible

code, accessible-colors, dark appearance code, accessible-colors, light appearance

New default

code, new default, dark appearance code, new default, light appearance

Quotes

Current default

quotes, current release, dark appearance quotes, current release, light appearance

Accessible

quotes, accessible-colors, dark appearance quotes, accessible-colors, light appearance

New default

quotes, new default, dark appearance quotes, new default, light appearance

Tables

Current default

table, current release, dark appearance table, current release, light appearance

Accessible

table, accessible-colors, dark appearance table, accessible-colors, light appearance

New default

table, new default, dark appearance table, new default, light appearance

jtmcg and others added 28 commits February 6, 2025 16:44
Add accessibility package and isEnabled function
This commit leverages `cli` fork of `charmbracelet/glamour` with enhancement for configuring `chroma` formatter.

It includes simple tests that enabling `gh` accessible features causes codeblocks to be downsampled to base 16 ANSI colors.
This commit is v2 approach to testing for accessible colors and codeblock rendering using an in-house approach to identifying ANSI escape sequences and analyzing them for color depth.

After talking with @williammartin, this is likely going to be refactored a third time leveraging a module to help with parsing escape sequences from text.
This commit is focused on PR feedback around codeblock testing and simplifying related code:

1. Use of new `WithOptions(...TermRendererOption) TermRendererOption` to clean up `WithTheme()`

   The `glamour` TermRendererOption pattern has a limitation that users cannot compose multiple options without duplicating code or building one-off anonymous functions.

   However, this commit takes advantage of an enhancement in https://github.com/cli/glamour/pull/3 that allows users to leverage a helper to avoid building one-offs.

1. Use of new `leaanthony/go-ansi-parser` dependency for parsing ANSI escape sequences and display attributes

   In v1 of `markdown_test.go`, the codeblock tests were very simple, testing the result of output of markdown rendering against a string of ANSI escape sequences. The concern raised is that this was testing the result rather than behavior wanted.

   In v2 of `markdown_test.go`, the codeblock tests were refactored to use regex to extract and analyze ANSI escape sequences and display attributes. The concern raised was that this was a lot of logic to build and maintain and might benefit from a dependency that could do it.

   In v3 of `markdown_test.go`, a combination of v1 and v2 approaches for 1) testing that theme appropriate colors are used and 2) testing that ensures accessible display options are used when accessible experience is enabled
After discussing the changing nature of the tests with @jtmcg, this commit renames the test functions to make it apparent their purposes: testing we get the colors we want separate from checking for accessibility concerns.

In addition, there were a number of other improvements to be more idiomatic.
1. Introduce constant for `gh config` setting for accessible colors
2. Align existing constant for env var to match
3. Rename exported functions to distinguish use specifically for color
4. Minor enhancements to comments and tests
Following Go `x` convention seen across various projects, the former `accessibility` package has been relocated to communicate its experimental status more clearly to extension authors.

It has also been renamed to `color` to align with adopting Primer Style color role functionality.
Realign accessible color logic around new env var
Due to errors in PR, this commit updates GitHub Actions workflows to use a newer version of actions/setup-go capable of reading Go version from go.mod.
@andyfeller
Copy link
Member Author

andyfeller commented Mar 10, 2025

In addressing the lint errors, I've made minor changes to GitHub Actions workflows to pull Go version from go.mod, which requires a newer version of actions/setup-go.

Separately while troubleshooting lint job, I notice that the action used for Go linting is 3 major version behind; v3.4.0 => v6.5.0.

For changes, see diff

Iterate on resolving lint job errors due to Go 1.23 version upgrade.

This should resolve most of the problems with 1 potential fix that I'll wait to confirm in the next run.
@andyfeller
Copy link
Member Author

andyfeller commented Mar 10, 2025

Note

In upping the Go version, there is a discrepancy between automation in the default branch which takes 1.21 into account whereas this branch bumps it up. 🤷

Meaning there will be some follow up work to the repository ruleset / branch protection rules afterwards.

Copy link
Member

@williammartin williammartin left a comment

Choose a reason for hiding this comment

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

@andyfeller I got a bit distracted with some other stuff so I didn't get into the good stuff yet in this PR review sorry. Mostly just cursory stuff!

Copy link
Member

@williammartin williammartin left a comment

Choose a reason for hiding this comment

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

Lots of good stuff in here, don't think there's too much work to resolve my comments.

I haven't yet pulled this into cli/cli to use it.

This commit is focused a handful of PR feedback:

1. Minor DRYing up with inlining logic
2. Update tests to hardcode env var and config setting rather than reuse constant names
3. Rework `GH_ACCESSIBLE_COLORS` evaluation (see below)

Previously, the env var logic had 2 concerns:

1. It couldn't distinguish between the env var being missing versus being empty
1. It didn't include `no` handling similar to `GH_DEBUG`

Now, `IsAccessibleColorsEnabled` will short circuit env var handling if unset.
Major change with this commit is relocating experimental logic into the `x`
package to communicate clearly expectations to users.  Additionally, the
package in-line docs have been updated to state experimental nature more
clearly.

There are a few other odd cleanups:

- test assertion improvement
- better in-line comments
The experimental markdown colors shouldn't be used for any arbitrary purpose outside of `x/markdown` package.  This commit unexports several types and functions, leaving `x/markdown` functions as the main use cases.

Additionally, there are a few additional changes to improve understanding and reduce the code involved:

1. Use of "ANSIColorCode" has been replaced in favor of "glamourStyleColor" to explain their purpose
2. Replace "ANSIColorCode.String()" and "ANSIColor.StrPtr()" for "glamourStyleColor.Code()" to simplify and clarify use over types
3. Add comments to explain importance of enumeration order
4. Expand test verifying color codes
This isn't really needed beyond this package, so unexporting it to avoid unintended use cases.
Copy link
Member

@williammartin williammartin left a comment

Choose a reason for hiding this comment

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

./bin/gh repo view with and without GH_ACCESSIBLE_COLORS=1

image showing markdown rendering

And on a white background with GH_ACCESSIBLE_COLORS=1

image

With config setting

➜  cli git:(trunk) ✗ gh config set accessible_colors enabled
! warning: 'accessible_colors' is not a known configuration key
image

@williammartin
Copy link
Member

williammartin commented Mar 13, 2025

@andyfeller There is text in the images above "Image: screenshot of gh pr status → https://user-images.githubusercontent.com/98482/84171218-327e7a80-aa40-11ea-8cd1-5177fc2d0e72.png" and I'm having trouble convincing myself that it is using ANSI 4-bit colors.

image

From using sequin it looks like the control code is:

 CSI 37m: Foreground color: White
 CSI 38;5;243m: Foreground color: 243

So this looks like a 256-bit color code. I think this is the relevant style bits are:


It also looks to me as if LinkText would not be 16-bit, but I could be mistaken.

Here's the green LinkText "hub" on my dark background:

image

Indeed, when I run it through sequin I see:

 CSI 0m: Reset style
 CSI 38;5;35;1m: Foreground color: 35, Bold
Text more detailed explanation

Which matches the DarkStyleConfig LinkTest

Copy link
Member

@williammartin williammartin left a comment

Choose a reason for hiding this comment

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

Requesting changes based on ^

This commit is focused on expanded accessibility testing and addressing discoveries from them:

1. `TestAccessibleDarkStyleConfigIs4Bit` and `TestAccessibleLightStyleConfigIs4Bit` will walk the Glamour StyleConfig type and check all elements
2. `Test_RenderAccessible` has been expanded with anchor and image elements

In addition, there are several fixes to logic and tests:

1. `Test_RenderAccessible` Go codeblock was missing replacement value, resulting in inaccurate test input
2. Dark and light accessible style logic was missing link text, image text, and H6 styling
3. Handling the `StyleBlock` of codeblocks which is unclear how it interacts with Chroma
@andyfeller
Copy link
Member Author

Accessible colors should also be used when GLAMOUR_STYLE is dark or light

Problem

When determining how to render markdown, the code is concerned with 2 things:

  1. Is the terminal background dark or light? Or should none colors be used? (theme)

  2. How does the user want to stylize markdown? (style)

Currently, accessible colors are only applied if style has not been set, which can be confusing if the user sets GLAMOUR_STYLE to dark or light:

// WithTheme is a rendering option that sets the theme to use while rendering the markdown.
// It can be used in conjunction with [term.Theme].
// If the environment variable GLAMOUR_STYLE is set, it will take precedence over the provided theme.
func WithTheme(theme string) glamour.TermRendererOption {
style := os.Getenv("GLAMOUR_STYLE")
if style == "" || style == "auto" {
switch theme {
case "light", "dark":
if xcolor.IsAccessibleColorsEnabled() {
return glamour.WithOptions(
glamour.WithStyles(xmarkdown.AccessibleStyleConfig(theme)),
glamour.WithChromaFormatter("terminal16"),
)
}
style = theme
default:
style = "notty"
}
}
return glamour.WithStylePath(style)
}

  • GH_ACCESSIBLE_COLORS=1 bin/gh repo view andyfeller/markdown-lorem-ipsum

    As we see, gh is using the custom accessible themes when GLAMOUR_STYLE is unset:

    Screenshot of new accessible experience without glamour env variable

  • GLAMOUR_STYLE=dark GH_ACCESSIBLE_COLORS=1 bin/gh repo view andyfeller/markdown-lorem-ipsum

    However we see gh will fallback to the current markdown rendering experience even when the style and theme should be the same.

    Screenshot of new accessible experience with glamour env variable

Suggestion

This was honestly a bit confusing for me and I've been working on these changes for a while as I expected GLAMOUR_STYLE=dark or GLAMOUR_STYLE=light to also pick up accessible colors.

If this logic was enhanced to allow those 2 styles to fall into the same logic that applies accessible colors, I think we'll have fewer people surprised.

This branch was pinned to the commit SHA after our contribution was merged, however our friends at Charm have cut a new release, which we should use instead.
@andyfeller
Copy link
Member Author

andyfeller commented Mar 13, 2025

Should tables be wrapped or not? 🤔

One of the improvements in charmbracelet/glamour@v0.9.0 is greater control over table wrapping / overflow, which glamour now does for tables by default:

	ctx.table.lipgloss = table.New().Width(width).Wrap(true)

UPDATE: This PR has been updated with upstream changes to disable table content wrapping until we can understand its impact to screenreader accessibility.

Expand for full thoughts

I'm going to enhance https://github.com/andyfeller/markdown-lorem-ipsum to contain an example for discussion as I'm unsure how this affects accessible screenreader experience, which is outside the scope of the issue and PR here.

Current gh markdown table rendering experience

Screenshot 2025-03-13 at 5 54 04 PM

New gh experience table rendering experience

Screenshot 2025-03-13 at 5 53 14 PM Screenshot 2025-03-13 at 5 53 38 PM

cc: @williammartin

This commit updates the `charmbracelet/glamour` dependency to pick up an unreleased change around markdown table rendering and table wrapping.

The concern previously was that table wrapping would cause issues in the current `gh` experience as well as make it difficult for screen readers to understand content.
@andyfeller
Copy link
Member Author

andyfeller commented Mar 20, 2025

@williammartin : I'm going to spend some time later tonight putting together a demo of the changes in the PR just so we can confirm the experience meets expectations beyond the tests and code review. Though the fix for table wrapping hasn't been released, I think it's okay we pin to the commit for now to get this merged.

So re-re-requesting review and I'll follow up on demoing it later tonight.

UPDATE: See PR body above for updated screenshots

@andyfeller
Copy link
Member Author

Want to give a special shout out to our friends at charmbracelet who helped make this work possible with work on glamour and lipgloss in no specific order:

Copy link
Member

@williammartin williammartin left a comment

Choose a reason for hiding this comment

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

Approved with one non blocking comment! Well done on all this, I know it was a lot.

go.mod Outdated
go 1.21
go 1.23.0

toolchain go1.23.7
Copy link
Member

Choose a reason for hiding this comment

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

Is this really required? Would think it is unusual to have a toolchain entry for a library.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great question; go mod did this in upgrading dependencies 🤦

Copy link
Member Author

Choose a reason for hiding this comment

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

Removing for now

Discussing this with @williammartin, neither of us could reason why the `toolchain` directive was necessary for a library and choosing to remove.
@andyfeller andyfeller merged commit 61bf393 into trunk Mar 25, 2025
10 checks passed
@andyfeller andyfeller deleted the accessible-colors branch March 25, 2025 11:54
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.

4 participants