Introduce experimental accessible colors in markdown rendering #186

andyfeller · 2025-03-10T14:17:04Z

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 screenshots demonstrate 3 terminal experiences, from left to right:

Current, default markdown behavior with 8-bit colors
New, environment variable configured 4-bit accessible colors
New, configuration setting configured 4-bit accessible colors

Add accessibility package and isEnabled function

Add accessible themes

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.

@williammartin

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 cli/glamour#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

@jtmcg

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.

Ensure codeblocks use accessible colors

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

Relates github/cli#830

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 · 2025-03-10T14:44:11Z

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 · 2025-03-10T16:35:20Z

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.

williammartin

@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!

.github/workflows/lint.yml

internal/yamlmap/yaml_map.go

williammartin · 2025-03-10T15:46:09Z

internal/testutils/config_stub.go

+// StubConfig replaces the config.Read function with a function that returns a config object
+// created from the given config string. It also sets up a cleanup function that restores the
+// original config.Read function.
+func StubConfig(t *testing.T, cfgStr string) {


Food For Thought

No concerns merging this as is because it's easy to change later, but maybe this should be config.Stub where this lives in package config_test would be better than a testutils package.

williammartin · 2025-03-10T15:48:45Z

pkg/markdown/accessibility.go

+	return strconv.Itoa(int(a))
+}
+
+func (a ANSIColorCode) StrPtr() *string {


Question

Did you take this pattern from somewhere?

Yeah, I believe it was based on 2 ideas:

charmbracelet/glamour default styles logic:

// DarkStyleConfig is the default dark style. DarkStyleConfig = ansi.StyleConfig{ Document: ansi.StyleBlock{ StylePrimitive: ansi.StylePrimitive{ BlockPrefix: "\n", BlockSuffix: "\n", Color: stringPtr("252"), }, Margin: uintPtr(defaultMargin), }, BlockQuote: ansi.StyleBlock{ StylePrimitive: ansi.StylePrimitive{}, Indent: uintPtr(1), IndentToken: stringPtr("│ "), },

which uses a handful of helper functions:

func boolPtr(b bool) *bool { return &b } func stringPtr(s string) *string { return &s } func uintPtr(u uint) *uint { return &u }

color.go in cli/cli and ansi.ColorFunc()

This was more inspirational about having a type where information about color could then be leveraged elsewhere rather than the unexported private functions above:

var ( magenta = ansi.ColorFunc("magenta") cyan = ansi.ColorFunc("cyan") red = ansi.ColorFunc("red") yellow = ansi.ColorFunc("yellow") blue = ansi.ColorFunc("blue") green = ansi.ColorFunc("green") gray = ansi.ColorFunc("black+h") lightGrayUnderline = ansi.ColorFunc("white+du") bold = ansi.ColorFunc("default+b") cyanBold = ansi.ColorFunc("cyan+b") greenBold = ansi.ColorFunc("green+b") highlightStart = ansi.ColorCode(highlightStyle) highlight = ansi.ColorFunc(highlightStyle) gray256 = func(t string) string { return fmt.Sprintf("\x1b[%d;5;%dm%s\x1b[m", 38, 242, t) } )

Alright, makes sense to me.

Maybe it doesn't need to be exported?

williammartin · 2025-03-10T15:50:59Z

pkg/markdown/accessibility.go

@@ -0,0 +1,106 @@
+package markdown


Question

Should this also live under x for the moment?

Great question! I can get behind that especially as we don't want anyone thinking its stable, yeah.

williammartin · 2025-03-10T15:55:09Z

pkg/x/color/accessibility.go

+}
+
+func isEnvVarEnabled(envVar string) bool {
+	return envVar != "" && envVar != "0" && envVar != "false"


Food For Thought

No specific request but this function looks a little sus in that the only place it is used is in this file and repeats the string check that wraps it on line 26.

I think I'd probably inline on 26 like if you wanted to keep the clarity:

if envVar != "" { isEnVarEnabled := envVar != "0" && envVar != "false" return isEnvVarEnabled }

Totally fair call out!

I was hoping we could tie into existing logic as this work evolved as this shouldn't be a new problem, but I suppose we don't really:

go-gh/pkg/api/http_client.go

Lines 80 to 88 in 7a6e554

ghDebug := os.Getenv("GH_DEBUG")

switch ghDebug {

case "", "0", "false", "no":

// no logging

default:

opts.Log = os.Stderr

opts.LogColorize = !term.IsColorDisabled() && term.IsTerminal(os.Stderr)

opts.LogVerboseHTTP = strings.Contains(ghDebug, "api")

}

We probably also want to add no, too

williammartin

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.

williammartin · 2025-03-11T14:20:27Z

pkg/x/color/accessibility_test.go

+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Setenv("GH_ACCESSIBLE_COLORS", tt.envVarValue)


Food For Thought

In practice I think this will work but I'm not sure it's totally correct in line with the descriptions e.g. the first test says:

When the accessibility configuration and env var are both unset, it should return false

But setting an env var to an empty string is not the same as it being unset. You can confirm this with:

_, set := os.LookupEnv("GH_ACCESSIBLE_COLORS") fmt.Println(set)

Actually if you look at this test:

{ name: "When the accessibility configuration is set to disabled and the env var is unset, it should return false", envVarValue: "", cfgStr: accessibilityDisabledConfig(), wantOut: false, },

You might expect that changing the implementation of IsAccessibleColorsEnabled with a refactor to a true "set" or "unset" check would be sage:

envVar, set := os.LookupEnv(AccessibleColorsEnv) if set { return isEnvVarEnabled(envVar) }

But it's not because "" is considered set. This refactor actually results in the config never being checked in the tests.

For me, this is enough to Request Changes to make it clear what an empty string vs unset should do.

williammartin · 2025-03-11T14:39:14Z

pkg/x/color/accessibility_test.go

+func accessibilityEnabledConfig() string {
+	return heredoc.Docf(`
+		%s: enabled
+	`, AccessibleColorsSetting)


Nit

I understand your desire to DRY out the use of this config but I have mixed feelings about it. The thing is, the tests are supposed to act as guardrails but changing the value of AccessibleColorsSetting would break users but wouldn't fail the tests.

I think it's generally better to duplicate in tests for this reason.

In fact you avoided this trap for the envvar by using the string directly in the test.

williammartin · 2025-03-11T14:54:28Z

pkg/markdown/accessibility_test.go

+	"github.com/stretchr/testify/assert"
+)
+
+func TestANSIColorCodeString(t *testing.T) {


Question

Maybe this is pedantic but it feels like we should test all the colors here. I swapped the order of some of them and no tests failed.

Maybe it's unnecessary, who's really going to swap the order? Maybe a comment would be fine. I don't think it's obvious to uninformed readers that the order of the enumeration is important.

An alternative would be to use a map to int.

What do you think?

williammartin · 2025-03-11T16:00:21Z

pkg/markdown/markdown.go

@@ -33,9 +34,16 @@ func WithWrap(w int) glamour.TermRendererOption {
 // 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")
+	accessible := color.IsAccessibleColorsEnabled()


Nit

If this is only used inside the light, dark case then maybe just inline it in the if there?

It would probably be a totally negligible performance improvement but generally I value the scoping being tight.

williammartin · 2025-03-11T16:00:44Z

pkg/markdown/markdown_test.go

+// Test_RenderAccessible tests rendered markdown for accessibility concerns such as color mode / depth and other display attributes.
+// It works by parsing the rendered markdown for ANSI escape sequences and checking their display attributes.
+// Test scenarios allow multiple color mode / depths because `ansi.Parse()` considers `\x1b[0m` sequence as part of `ansi.Default`.
+func Test_RenderAccessible(t *testing.T) {


Praise

This test is sick.

williammartin · 2025-03-11T16:03:34Z

pkg/markdown/markdown_test.go

-			_, err := Render(tt.text, WithTheme(tt.theme))
-			assert.NoError(t, err)
+			t.Cleanup(func() {
+				// Chroma caches charm style used to render codeblocks, it must be unregistered to avoid previously used style being reused.


Praise

Great comment.

williammartin · 2025-03-11T16:04:22Z

pkg/markdown/markdown_test.go

+				// Chroma caches charm style used to render codeblocks, it must be unregistered to avoid previously used style being reused.
+				delete(styles.Registry, "charm")
+			})
+			t.Setenv(color.AccessibleColorsEnv, tt.accessibleEnvVar)


Nit

Same issue as DRYing the config setting mentioned in another comment.

williammartin · 2025-03-11T16:05:02Z

pkg/markdown/markdown_test.go

+				path := filepath.Join(t.TempDir(), fmt.Sprintf("%s.json", tt.styleEnvVar))
+				err := os.WriteFile(path, []byte(customGlamourStyle(t)), 0644)
+				if err != nil {
+					t.Fatal(err)


Question

Is there a reason we're not using require.NoError(err) here? Suspect not.

jtmcg and others added 28 commits February 6, 2025 16:44

Move duplicated stubConfig function to a new internal/testutils dir

83e89a9

Add new accessibility package and IsEnabled() function

65cf7db

Address PR comments

794568b

Change GH_ACCESSIBLE env var to ACCESSIBLE

50d7ff4

Merge pull request #180 from cli/jtmcg/accessibility-isEnabled

a5c6b5e

Add accessibility package and isEnabled function

Add AnsiColor enum

1196ae7

Add accessible styles

897a7fe

Add accessible themes to markdown rendering

d2d3fcf

Cleaned up naming and comments for better understandability

dddc61a

Upgrade glamour version for access to styles package

5bd4f3e

Fix typo in env var name

6713a5d

Change link stylings so link text is correct color

2ef01cb

go mod tidy

b76f1c0

Update test to check correct config

bb3c0d2

Fix typo

06206cc

Merge pull request #182 from cli/jtmcg/add-accessible-themes

607ac5b

Add accessible themes

Ensure codeblocks use accessible colors

e61b6d6

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.

Merge pull request #183 from cli/andyfeller/accessible-colors-chroma

0136321

Ensure codeblocks use accessible colors

Realign accessible color logic around new env var

e05c340

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

Fix bugs from package renaming

33a6677

Merge pull request #185 from cli/andyfeller/gh_accessible_colors

44c18fe

Realign accessible color logic around new env var

Introduce experimental accessibile markdown colors

e9d3643

Relates github/cli#830

Workflows should use go.mod for Go version

d320dc9

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.

Reorder workflows to checkout first

87ac72f

Merge branch 'trunk' into accessible-colors

8931e96

andyfeller added 2 commits March 10, 2025 11:06

Bump golangci-lint action for failing lint job

3fdc103

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.

Address golangci-lint problem

3cba8b5

andyfeller requested review from williammartin and BagToad March 10, 2025 15:16

williammartin reviewed Mar 10, 2025

View reviewed changes

williammartin requested changes Mar 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce experimental accessible colors in markdown rendering #186

Introduce experimental accessible colors in markdown rendering #186

andyfeller commented Mar 10, 2025 •

edited

Loading

andyfeller commented Mar 10, 2025 •

edited

Loading

andyfeller commented Mar 10, 2025 •

edited

Loading

williammartin left a comment

williammartin Mar 10, 2025

williammartin Mar 10, 2025

andyfeller Mar 11, 2025

williammartin Mar 11, 2025

williammartin Mar 10, 2025

andyfeller Mar 11, 2025

williammartin Mar 10, 2025

andyfeller Mar 11, 2025

andyfeller Mar 11, 2025

williammartin left a comment •

edited

Loading

williammartin Mar 11, 2025

williammartin Mar 11, 2025

williammartin Mar 11, 2025

williammartin Mar 11, 2025

williammartin Mar 11, 2025

williammartin Mar 11, 2025

williammartin Mar 11, 2025

williammartin Mar 11, 2025

williammartin Mar 11, 2025

williammartin Mar 11, 2025

	ghDebug := os.Getenv("GH_DEBUG")
	switch ghDebug {
	case "", "0", "false", "no":
	// no logging
	default:
	opts.Log = os.Stderr
	opts.LogColorize = !term.IsColorDisabled() && term.IsTerminal(os.Stderr)
	opts.LogVerboseHTTP = strings.Contains(ghDebug, "api")
	}

Introduce experimental accessible colors in markdown rendering #186

Are you sure you want to change the base?

Introduce experimental accessible colors in markdown rendering #186

Conversation

andyfeller commented Mar 10, 2025 • edited Loading

Go version change

Demo

andyfeller commented Mar 10, 2025 • edited Loading

andyfeller commented Mar 10, 2025 • edited Loading

williammartin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williammartin left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyfeller commented Mar 10, 2025 •

edited

Loading

andyfeller commented Mar 10, 2025 •

edited

Loading

andyfeller commented Mar 10, 2025 •

edited

Loading

williammartin left a comment •

edited

Loading