-
Notifications
You must be signed in to change notification settings - Fork 67
Introduce experimental accessible colors in markdown rendering #186
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
Conversation
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.
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.
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
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.
|
In addressing the lint errors, I've made minor changes to GitHub Actions workflows to pull Go version from Separately while troubleshooting lint job, I notice that the action used for Go linting is 3 major version behind; 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.
|
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
left a comment
There was a problem hiding this 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!
There was a problem hiding this 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.
williammartin
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@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.
From using 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
Indeed, when I run it through Which matches the |
williammartin
left a comment
There was a problem hiding this 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
Accessible colors should also be used when
|
| // 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-ipsumAs we see,
ghis using the custom accessible themes whenGLAMOUR_STYLEis unset: -
GLAMOUR_STYLE=dark GH_ACCESSIBLE_COLORS=1 bin/gh repo view andyfeller/markdown-lorem-ipsumHowever we see
ghwill fallback to the current markdown rendering experience even when thestyleandthemeshould be the same.
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.
Should tables be wrapped or not? 🤔One of the improvements in 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
|
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.
|
@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 |
|
Want to give a special shout out to our friends at |
williammartin
left a comment
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 🤦
There was a problem hiding this comment.
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.










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,
markdownpackage continues to render GFM using existingcharmbracelet/glamourlight and dark styles in 8-bit color.By setting
accessible_colorsconfiguration setting orGH_ACCESSIBLE_COLORSenvironment variable,markdownpackage 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
Accessible
New default
Horizontal Rule
Current default
Accessible
New default
Images
Current default
Accessible
New default
Links
Current default
Accessible
New default
Code
Current default
Accessible
New default
Quotes
Current default
Accessible
New default
Tables
Current default
Accessible
New default