Ensure codeblocks use accessible colors #183

andyfeller · 2025-02-19T15:21:47Z

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 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.

andyfeller · 2025-02-19T15:32:31Z

Looking back at the changes in the PR versus cli/cli#9821, I'm confirming the need to remove the replace and change all of the import statements to use our fork until we can get the changes accepted upstream.

jtmcg

Could you link the fork changes, @andyfeller? I'd find it helpful so we understand what this is introducing from the fork.

jtmcg · 2025-02-19T18:46:57Z

pkg/markdown/markdown_test.go

+			`, "`"),
+			theme:            "dark",
+			accessibleEnvVar: "true",
+			wantOut:          "\x1b[0m\x1b[30mfmt\x1b[0m\x1b[33m.\x1b[0m\x1b[36mPrintln\x1b[0m\x1b[33m(\x1b[0m\x1b[90m\"Hello, world!\"\x1b[0m\x1b[33m)\x1b[0m",


I find it really hard to tell what's different between this blob of output versus the one on line 96. That's why I added the human-grokkable format in the tests above these. I also find it a bit jarring that the output formats are different for these tests and would find value in their consistency. Perhaps codeblocks are different enough to justify it, but there may be value in pulling out a human-grokkable abstraction in these wantOuts

Completely get that 💯 as that was a consideration when I was writing the tests I choose to defer to seeing if someone would comment on. 👍

I can play around to see what this would look like for a more grokkable format but I suspect the extent of control characters is pretty significant. I don't know how much more grokkable it will be trying to reuse a bunch of placeholders but we can try.

Here's an example of what this would look like if we used the same approach of constants:

reset = "\x1b[0m" glamourLightCodeBlock_8bitRaisinBlackColorSeq = "\x1b[38;5;235m" glamourLightCodeBlock_8bitVividTangerineColorSeq = "\x1b[38;5;210m" glamourLightCodeBlock_8bitJadeColorSeq = "\x1b[38;5;35m" glamourLightCodeBlock_8bitCopperRoseColorSeq = "\x1b[38;5;95m"

{ name: "when the light theme is selected, the codeblock renders using 8-bit colors", text: heredoc.Docf(` %[1]s%[1]s%[1]sgo fmt.Println("Hello, world!") %[1]s%[1]s%[1]s `, "`"), theme: "light", // wantOut: "\x1b[0m\x1b[38;5;235mfmt\x1b[0m\x1b[38;5;210m.\x1b[0m\x1b[38;5;35mPrintln\x1b[0m\x1b[38;5;210m(\x1b[0m\x1b[38;5;95m\"Hello, world!\"\x1b[0m\x1b[38;5;210m)\x1b[0m", wantOut: fmt.Sprintf("%[1]s%[2]sfmt%[1]s%[3]s.%[1]s%[2]sPrintln%[1]s%[3]s(%[1]s%[4]s\"Hello, world!\"%[1]s%[3]s)%[1]s", reset, glamourLightCodeBlock_8bitRaisinBlackColorSeq, glamourLightCodeBlock_8bitVividTangerineColorSeq, glamourLightCodeBlock_8bitCopperRoseColorSeq), },

I also think there is something weird going on with the test as I originally implemented the dark tests and it used different escape codes, however I think there is some flakiness with chroma between tests that it isn't using the appropriate theme.

Before introducing light test case, this was the result of dark without and with accessible colors:

wantOut: "\x1b[0m\x1b[38;5;251mfmt\x1b[0m\x1b[38;5;187m.\x1b[0m\x1b[38;5;42mPrintln\x1b[0m\x1b[38;5;187m(\x1b[0m\x1b[38;5;173m\"Hello, world!\"\x1b[0m\x1b[38;5;187m)\x1b[0m",

wantOut: "\x1b[0m\x1b[37mfmt\x1b[0m\x1b[37m.\x1b[0m\x1b[36mPrintln\x1b[0m\x1b[37m(\x1b[0m\x1b[33m\"Hello, world!\"\x1b[0m\x1b[37m)\x1b[0m",

After adding light test case before dark, this is the result:

wantOut: "\x1b[0m\x1b[38;5;235mfmt\x1b[0m\x1b[38;5;210m.\x1b[0m\x1b[38;5;35mPrintln\x1b[0m\x1b[38;5;210m(\x1b[0m\x1b[38;5;95m\"Hello, world!\"\x1b[0m\x1b[38;5;210m)\x1b[0m",

wantOut: "\x1b[0m\x1b[30mfmt\x1b[0m\x1b[33m.\x1b[0m\x1b[36mPrintln\x1b[0m\x1b[33m(\x1b[0m\x1b[90m\"Hello, world!\"\x1b[0m\x1b[33m)\x1b[0m",

Going to step through the debugger a bit to see what's going on.

jtmcg · 2025-02-19T19:05:26Z

pkg/markdown/markdown.go

+				// Applying multiple glamour.TermRendererOption here requires a wrapper that applies each
+				// within glamour.NewTermRenderer() in Render() below.
+				stylesOption := glamour.WithStyles(AccessibleStyleConfig(theme))
+				chromaOption := glamour.WithChromaFormatter("terminal16")
+
+				return func(tr *glamour.TermRenderer) error {
+					if err := stylesOption(tr); err != nil {
+						return err
+					}
+					if err := chromaOption(tr); err != nil {
+						return err
+					}
+					return nil
+				}


Clever, but I really don't like how this reads... I find it pretty confusing given this is returning a glamour.TermRendererOption. I guess it's a function of this call signature, but that requires implementation level knowledge of glamour to understand this code.

I think I might prefer to abstract this closure out to something separate, so we end up with something like

Suggested change

// Applying multiple glamour.TermRendererOption here requires a wrapper that applies each

// within glamour.NewTermRenderer() in Render() below.

stylesOption := glamour.WithStyles(AccessibleStyleConfig(theme))

chromaOption := glamour.WithChromaFormatter("terminal16")

return func(tr *glamour.TermRenderer) error {

if err := stylesOption(tr); err != nil {

return err

}

if err := chromaOption(tr); err != nil {

return err

}

return nil

}

return AccessibleStyles(theme)

// in the accessibility dir

func AccessibleStyles(theme string) glamour.TermRendererOption {

// Applying multiple glamour.TermRendererOption here requires a wrapper that applies each

// within glamour.NewTermRenderer() in Render() below.

stylesOption := glamour.WithStyles(AccessibleStyleConfig(theme))

chromaOption := glamour.WithChromaFormatter("terminal16")

return func(tr *glamour.TermRenderer) error {

if err := stylesOption(tr); err != nil {

return err

}

if err := chromaOption(tr); err != nil {

return err

}

return nil

}

}

That way, we're separating the logic of "this is how you build the accessible theme using our implementation" from "select the right theme to use"

Clever, but I really don't like how this reads... I find it pretty confusing given this is returning a glamour.TermRendererOption. I guess it's a function of this call signature, but that requires implementation level knowledge of glamour to understand this code.

I think I might prefer to abstract this closure out to something separate, so we end up with something like

That way, we're separating the logic of "this is how you build the accessible theme using our implementation" from "select the right theme to use"

I was definitely conflicted in the implementation because of where / how we were applying the accessible theme in WithTheme versus in Render where this would allow us to have separate TermRendererOption that would be cleaner.

That said, I think at some level the reader digging into this has to understand glamour as long as its the dependency we use here.

Do you think further abstracting this could create a different type of confusion for reasoning about this behavior?

andyfeller · 2025-02-20T15:48:17Z

Going to close this PR for the moment because there is a bit more work to do with changing this to use our fork of glamour with import paths.

Will continue responding to comments in the mean time. 👍

@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

andyfeller · 2025-02-25T14:40:54Z

@jtmcg @williammartin : Thank you both for feedback while iterating on this PR! 🙇 I'm reopening it after iterating on feedback to make some improvements but also wanting to make the case for certain concerns.

Important

This PR is dependent on changes in cli/glamour#3 and has been updated to use the latest commit SHA from andyfeller/multi-options.

Changes

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 Update forked module name, paths, and docs to be imported; support for applying multiple options in a TermRendererOption glamour#3 that allows users to leverage a helper to avoid building one-offs. This results in a generic solution to the problem we face with WithTheme needing to set multiple options from a single place.
Use of new leaanthony/go-ansi-parser dependency for parsing ANSI escape sequences and display attributes

c5336a5 was my initial effort in refactoring the markdown codeblock tests to generically check ANSI escape codes using regular expressions to identify and parse escape sequences.

While discussing the test with @williammartin, he suggested leaanthony/go-ansi-parse as an alternative versus building and maintaining our own logic.
Expanding markdown_test.go to cover accessible color testing while still testing glamour and chroma output

There are 2 concerns I have around glamour and chroma as we implement accessible markdown rendering:
1. style-appropriate colors are used
2. 4-bit, base 16 ANSI colors are used when we ask for them.
In the original PR, only tests concerned with the former existed. While useful, I agree that they were very situational and maybe a little difficult to grok / maintain. However, they also highlighted important aspects of glamour and chroma around registry caching and how if we don't look at these colors we are likely to have low visibility colors used when not appropriate.

Now, we have a 2nd suite of tests that cover the latter that more generically test ANSI escape sequences for accessibility concerns.

andyfeller · 2025-02-25T15:24:52Z

Thoughts about testing codeblocks

One of the concerns originally raised around the codeblock color testing was that it was hard to grok and maintain:

go-gh/pkg/markdown/markdown_test.go

Lines 153 to 178 in 2b16a44

    
           { 
        
           	name:    "when the light theme is selected, the codeblock renders using 8-bit colors", 
        
           	text:    codeBlock, 
        
           	theme:   "light", 
        
           	wantOut: "\x1b[0m\x1b[38;5;235mfmt\x1b[0m\x1b[38;5;210m.\x1b[0m\x1b[38;5;35mPrintln\x1b[0m\x1b[38;5;210m(\x1b[0m\x1b[38;5;95m\"Hello, world!\"\x1b[0m\x1b[38;5;210m)\x1b[0m", 
        
           }, 
        
           { 
        
           	name:    "when the dark theme is selected, the codeblock renders using 8-bit colors", 
        
           	text:    codeBlock, 
        
           	theme:   "dark", 
        
           	wantOut: "\x1b[0m\x1b[38;5;251mfmt\x1b[0m\x1b[38;5;187m.\x1b[0m\x1b[38;5;42mPrintln\x1b[0m\x1b[38;5;187m(\x1b[0m\x1b[38;5;173m\"Hello, world!\"\x1b[0m\x1b[38;5;187m)\x1b[0m", 
        
           }, 
        
           { 
        
           	name:             "when the accessible env var is set and the light theme is selected, the codeblock renders using 4-bit colors", 
        
           	text:             codeBlock, 
        
           	theme:            "light", 
        
           	accessibleEnvVar: "true", 
        
           	wantOut:          "\x1b[0m\x1b[30mfmt\x1b[0m\x1b[33m.\x1b[0m\x1b[36mPrintln\x1b[0m\x1b[33m(\x1b[0m\x1b[90m\"Hello, world!\"\x1b[0m\x1b[33m)\x1b[0m", 
        
           }, 
        
           { 
        
           	name:             "when the accessible env var is set and the dark theme is selected, the codeblock renders using 4-bit colors", 
        
           	text:             codeBlock, 
        
           	theme:            "dark", 
        
           	accessibleEnvVar: "true", 
        
           	wantOut:          "\x1b[0m\x1b[37mfmt\x1b[0m\x1b[37m.\x1b[0m\x1b[36mPrintln\x1b[0m\x1b[37m(\x1b[0m\x1b[33m\"Hello, world!\"\x1b[0m\x1b[37m)\x1b[0m", 
        
           },

The concern being the difficulty of reading ANSI escape sequences like:

"\x1b[0m\x1b[38;5;235mfmt\x1b[0m\x1b[38;5;210m.\x1b[0m\x1b[38;5;35mPrintln\x1b[0m\x1b[38;5;210m(\x1b[0m\x1b[38;5;95m\"Hello, world!\"\x1b[0m\x1b[38;5;210m)\x1b[0m"

I 💯 agree this isn't ideal and thus experimented with a couple different forms which I'm unsure are legitimately better:

fmt.Sprintf(...)

From Ensure codeblocks use accessible colors #183 (comment):

	reset                                            = "\x1b[0m"
	glamourLightCodeBlock_8bitRaisinBlackColorSeq    = "\x1b[38;5;235m"
	glamourLightCodeBlock_8bitVividTangerineColorSeq = "\x1b[38;5;210m"
	glamourLightCodeBlock_8bitJadeColorSeq           = "\x1b[38;5;35m"
	glamourLightCodeBlock_8bitCopperRoseColorSeq     = "\x1b[38;5;95m"

fmt.Sprintf("%[1]s%[2]sfmt%[1]s%[3]s.%[1]s%[4]sPrintln%[1]s%[3]s(%[1]s%[5]s\"Hello, world!\"%[1]s%[3]s)%[1]s", reset, glamourLightCodeBlock_8bitRaisinBlackColorSeq, glamourLightCodeBlock_8bitVividTangerineColorSeq, glamourLightCodeBlock_8bitJadeColorSeq, glamourLightCodeBlock_8bitCopperRoseColorSeq)

text/template

t, err := template.New("codeblock").Parse(`{{.Reset}}{{.Package}}fmt{{.Reset}}{{.Operator}}.{{.Reset}}{{.Func}}Println{{.Reset}}{{.Operator}}({{.Reset}}{{.Literal}}\"Hello, world!\"{{.Reset}}{{.Operator}}){{.Reset}}`)

data := map[string]string{
    "Reset": "\x1b[0m",
    "Package": "\x1b[38;5;235m",
    "Operator": "\x1b[38;5;210m",
    "Func": "\x1b[38;5;35m",
    "Literal: "\x1b[38;5;95m",
}

var buf bytes.Buffer
err = t.Execute(&buf, data)
if err != nil {
    panic(err)
}

result := buf.String()

I have 2 concerns with the above alternatives:

There is significantly more code to understand and reason about
Some of these alternatives would affect the design of Test_Render, potentially adding to brittleness between test use cases

For these reasons, I kept the original v1 forms of the codeblock tests checking for specific color usage for confidence that reasonably contrasting colors relative to the theme are being displayed. If there is some other form these can take that is appropriate for testing without being a significant lift, then I appreciate suggestions.

jtmcg

A couple of questions and two suggestions, but otherwise this is looking good 👍 I have no further ideas about making the escape codes easier to grok other than what we already discussed, so 🤷

jtmcg · 2025-02-25T21:13:39Z

pkg/markdown/markdown.go

@@ -39,7 +39,10 @@ func WithTheme(theme string) glamour.TermRendererOption {
 		switch theme {
 		case "light", "dark":
 			if accessible {
-				return glamour.WithStyles(AccessibleStyleConfig(theme))
+				return glamour.WithOptions(


This is WAY better ❤️ I think it's super clear what's happening here and I love it. Thanks!

jtmcg · 2025-02-25T21:28:05Z

pkg/markdown/markdown_test.go

-				tmpDir := t.TempDir()
-				path := filepath.Join(tmpDir, fmt.Sprintf("%s.json", tt.styleEnvVar))
+			if tt.styleEnvVar == "" {
+				t.Setenv("GLAMOUR_STYLE", "")


Curious as to why this is necessary.

jtmcg · 2025-02-25T21:28:28Z

pkg/markdown/markdown_test.go

+					require.Equalf(t, st.ColourMode, ansi.Default, "Inaccessible color found in '%s' at %d", st, st.Offset)
+					require.Falsef(t, st.Faint(), "Inaccessible style found in '%s' at %d", st, st.Offset)


Can you explain what exactly these two checks are doing? The context for what this is asserting is hidden behind the dependency here. I'd prefer that context to not requiring spelunking into the ansi codebase, so maybe a comment explaining how this works would help?

Aside:
I think I have an emerging opinion that a table-test's struct should declare all inputs and outputs. As a corollary to that, I think that means I'm also think it's a bit of a test smell when there's no explicit thing you are expecting in the table test definition. This is not a strongly held opinion right now, but my intuition tells me that makes for easy to understand and easy to extend tests.

jtmcg · 2025-02-25T21:33:52Z

pkg/markdown/markdown_test.go

 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
+			// Unregister cached chroma style causing codeblock tests to fail based on previous theme
+			delete(styles.Registry, "charm")


Consider wrapping this in t.Cleanup. That should eliminate any artifacts hanging around from these tests after they run instead of cleaning them up before each test case. As it stands right now, I think the last test will leave the cached registry sitting around, which could cause a headache of flakiness down the line.

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.

andyfeller requested a review from jtmcg February 19, 2025 15:21

jtmcg reviewed Feb 19, 2025

View reviewed changes

andyfeller closed this Feb 20, 2025

andyfeller added 2 commits February 24, 2025 14:37

andyfeller reopened this Feb 25, 2025

jtmcg reviewed Feb 25, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure codeblocks use accessible colors #183

Ensure codeblocks use accessible colors #183

andyfeller commented Feb 19, 2025

andyfeller commented Feb 19, 2025 •

edited

Loading

jtmcg left a comment

jtmcg Feb 19, 2025

andyfeller Feb 20, 2025

andyfeller Feb 20, 2025

andyfeller Feb 20, 2025 •

edited

Loading

jtmcg Feb 19, 2025

andyfeller Feb 20, 2025

andyfeller commented Feb 20, 2025

andyfeller commented Feb 25, 2025

andyfeller commented Feb 25, 2025 •

edited

Loading

jtmcg left a comment

jtmcg Feb 25, 2025

jtmcg Feb 25, 2025

jtmcg Feb 25, 2025

jtmcg Feb 25, 2025

		require.Equalf(t, st.ColourMode, ansi.Default, "Inaccessible color found in '%s' at %d", st, st.Offset)
		require.Falsef(t, st.Faint(), "Inaccessible style found in '%s' at %d", st, st.Offset)

Ensure codeblocks use accessible colors #183

Are you sure you want to change the base?

Ensure codeblocks use accessible colors #183

Conversation

andyfeller commented Feb 19, 2025

andyfeller commented Feb 19, 2025 • edited Loading

jtmcg 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

andyfeller Feb 20, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andyfeller commented Feb 20, 2025

andyfeller commented Feb 25, 2025

Changes

andyfeller commented Feb 25, 2025 • edited Loading

Thoughts about testing codeblocks

jtmcg 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

andyfeller commented Feb 19, 2025 •

edited

Loading

andyfeller Feb 20, 2025 •

edited

Loading

andyfeller commented Feb 25, 2025 •

edited

Loading