mirror of
https://github.com/openai/codex.git
synced 2026-11-05 20:10:33 +00:00
Preserve prompt contrast when terminal colors are unavailable (#59257)
## Why Submitted prompts need readable foreground and background colors when the terminal does not report its default colors. ## What changed - Use a contrasting gray pair for truecolor and 256-color terminals when default colors are unavailable. Keep terminal defaults for 16-color terminals and unknown color support. - Preserve voice markers, inline element accents, and asynchronous reply code colors when applying prompt styles to native history. ## Testing Add contrast coverage for each color level and terminal rendering regressions for inline elements and asynchronous reply code colors. Update prompt snapshots to cover the fallback colors. GitOrigin-RevId: dcda0fc3e2b12a5dc9046322a4d82c61986e3ecd
This commit is contained in:
+3
-3
@@ -25,9 +25,9 @@ Buffer {
|
||||
x: 32, y: 1, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 2, fg: Reset, bg: Reset, underline: Reset, modifier: DIM,
|
||||
x: 34, y: 2, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 4, fg: Reset, bg: DarkGray, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 5, fg: Reset, bg: DarkGray, underline: Reset, modifier: BOLD | DIM,
|
||||
x: 2, y: 5, fg: Reset, bg: DarkGray, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 4, fg: Rgb(238, 238, 238), bg: Rgb(68, 68, 68), underline: Reset, modifier: NONE,
|
||||
x: 0, y: 5, fg: Rgb(238, 238, 238), bg: Rgb(68, 68, 68), underline: Reset, modifier: BOLD | DIM,
|
||||
x: 2, y: 5, fg: Rgb(238, 238, 238), bg: Rgb(68, 68, 68), underline: Reset, modifier: NONE,
|
||||
x: 0, y: 8, fg: Reset, bg: Reset, underline: Reset, modifier: BOLD,
|
||||
x: 6, y: 8, fg: Reset, bg: Reset, underline: Reset, modifier: DIM,
|
||||
x: 13, y: 8, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
|
||||
@@ -82,7 +82,9 @@ async fn deprecation_delivery_deduplicates_retained_transcript() -> Result<()> {
|
||||
let Some(Overlay::Transcript(overlay)) = app.overlay.as_mut() else {
|
||||
panic!("expected transcript")
|
||||
};
|
||||
overlay.render(area, &mut buffer);
|
||||
crate::terminal_palette::with_test_default_colors(/*colors*/ None, || {
|
||||
overlay.render(area, &mut buffer);
|
||||
});
|
||||
insta::assert_snapshot!("deduplicated_deprecation_transcript", format!("{buffer:?}"));
|
||||
|
||||
for (summary, details) in [
|
||||
|
||||
+2
-2
@@ -12,8 +12,8 @@ expression: persisted
|
||||
Line::default().bg(Color::Rgb(57, 57, 57)),
|
||||
Line::from_iter([
|
||||
Span::from(" "),
|
||||
Span::from("Answer from "),
|
||||
Span::from("desktop").fg(Color::Rgb(243, 139, 168)).bold(),
|
||||
Span::from("Answer from ").bg(Color::Rgb(57, 57, 57)),
|
||||
Span::from("desktop").fg(Color::Rgb(243, 139, 168)).bg(Color::Rgb(57, 57, 57)).bold(),
|
||||
]).bg(Color::Rgb(57, 57, 57)),
|
||||
Line::default().bg(Color::Rgb(57, 57, 57)),
|
||||
]
|
||||
|
||||
@@ -85,8 +85,9 @@ pub(super) fn render(
|
||||
.into_iter()
|
||||
.flat_map(|row| wrap(row, width, ""))
|
||||
.map(|row| {
|
||||
let style = row.line.style.patch(surface);
|
||||
row.style(style)
|
||||
let mut style = surface.patch(row.line.style);
|
||||
style.bg = surface.bg;
|
||||
row.style(style).resolve_span_styles()
|
||||
})
|
||||
.collect();
|
||||
lines.extend(prefix_hyperlink_lines(answer, " ".into(), " ".into()));
|
||||
|
||||
@@ -228,6 +228,20 @@ impl HistoryCell for UserHistoryCell {
|
||||
}
|
||||
|
||||
fn display_hyperlink_lines(&self, width: u16) -> Vec<HyperlinkLine> {
|
||||
let lines = self.transcript_hyperlink_lines(width);
|
||||
if self.is_spoken() || !self.text_elements.is_empty() {
|
||||
// Native history applies line styles over spans. Keep the prompt fill on the line
|
||||
// while resolving its foreground onto the text, marker, and inline elements.
|
||||
lines
|
||||
.into_iter()
|
||||
.map(HyperlinkLine::resolve_span_styles)
|
||||
.collect()
|
||||
} else {
|
||||
lines
|
||||
}
|
||||
}
|
||||
|
||||
fn transcript_hyperlink_lines(&self, width: u16) -> Vec<HyperlinkLine> {
|
||||
let sanitized_message = sanitize_user_text((&self.message).into());
|
||||
// Speech transcripts can begin with whitespace; the marker already supplies its separator.
|
||||
let message = if self.is_spoken() {
|
||||
@@ -256,7 +270,7 @@ impl HistoryCell for UserHistoryCell {
|
||||
.wrap_algorithm(textwrap::WrapAlgorithm::FirstFit),
|
||||
));
|
||||
|
||||
let mut wrapped_message =
|
||||
let wrapped_message =
|
||||
if let UserMessagePresentation::AsyncReplies { replies, cwd } = &self.presentation {
|
||||
super::async_replies::render(replies, cwd, usize::from(wrap_width))
|
||||
} else {
|
||||
@@ -285,13 +299,6 @@ impl HistoryCell for UserHistoryCell {
|
||||
lines.push(HyperlinkLine::new(Line::from("").style(style)));
|
||||
}
|
||||
}
|
||||
if self.is_spoken()
|
||||
&& let Some(first) = wrapped_message.first_mut()
|
||||
{
|
||||
// Wrapping already applied the foreground to the text. Native history
|
||||
// applies line styles over spans, so leave the voice marker's color intact.
|
||||
first.line.style.fg = None;
|
||||
}
|
||||
lines.extend(wrapped_message);
|
||||
lines.push(HyperlinkLine::new(Line::from("").style(style)));
|
||||
for source in lines.iter_mut().filter_map(|line| line.source.as_mut()) {
|
||||
@@ -308,18 +315,6 @@ impl HistoryCell for UserHistoryCell {
|
||||
self.transcript_hyperlink_lines(width)
|
||||
}
|
||||
|
||||
fn transcript_hyperlink_lines(&self, width: u16) -> Vec<HyperlinkLine> {
|
||||
let mut lines = self.display_hyperlink_lines(width);
|
||||
if self.is_spoken() {
|
||||
for line in &mut lines {
|
||||
if let Some(source) = &line.source {
|
||||
line.line.style.fg = source.line_style.fg;
|
||||
}
|
||||
}
|
||||
}
|
||||
lines
|
||||
}
|
||||
|
||||
fn raw_lines(&self) -> Vec<Line<'static>> {
|
||||
let message = sanitize_user_text((&self.message).into());
|
||||
let mut lines = raw_lines_from_source(message.as_ref().trim_end_matches(['\r', '\n']));
|
||||
|
||||
@@ -23,7 +23,14 @@ fn user_prompt_urls_keep_destinations_across_widths() {
|
||||
let mut snapshots = Vec::new();
|
||||
for width in [120, 60, 24] {
|
||||
let lines = cell.display_hyperlink_lines(width);
|
||||
assert_eq!(cell.transcript_hyperlink_lines(width), lines);
|
||||
let transcript = cell.transcript_hyperlink_lines(width);
|
||||
assert_eq!(
|
||||
transcript
|
||||
.into_iter()
|
||||
.map(HyperlinkLine::resolve_span_styles)
|
||||
.collect::<Vec<_>>(),
|
||||
lines
|
||||
);
|
||||
assert!(lines.iter().all(|line| line.width() <= usize::from(width)));
|
||||
for destination in [url, "https://example.test/a"] {
|
||||
let linked_text = lines
|
||||
@@ -182,7 +189,10 @@ fn spoken_user_messages_have_a_red_chevron_without_changing_raw_text() {
|
||||
/*x*/ 0, /*y*/ 0, /*width*/ 40, /*height*/ 3,
|
||||
);
|
||||
let mut buf = Buffer::empty(area);
|
||||
Paragraph::new(spoken.display_lines(area.width)).render(area, &mut buf);
|
||||
let lines = crate::terminal_palette::with_test_default_colors(/*colors*/ None, || {
|
||||
spoken.display_lines(area.width)
|
||||
});
|
||||
Paragraph::new(lines).render(area, &mut buf);
|
||||
insta::assert_snapshot!("spoken_user_prompt", format!("{buf:?}"));
|
||||
}
|
||||
|
||||
|
||||
+2
-2
@@ -11,8 +11,8 @@ Buffer {
|
||||
],
|
||||
styles: [
|
||||
x: 0, y: 0, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 1, fg: Red, bg: DarkGray, underline: Reset, modifier: BOLD,
|
||||
x: 2, y: 1, fg: Reset, bg: DarkGray, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 1, fg: Red, bg: Rgb(68, 68, 68), underline: Reset, modifier: BOLD,
|
||||
x: 2, y: 1, fg: Rgb(238, 238, 238), bg: Rgb(68, 68, 68), underline: Reset, modifier: NONE,
|
||||
x: 18, y: 1, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
]
|
||||
}
|
||||
|
||||
@@ -851,14 +851,72 @@ mod tests {
|
||||
}
|
||||
insta::assert_snapshot!(snapshots.join("\n"), @"
|
||||
PreWrap: › hello from voice
|
||||
marker: Idx(1), text: Default, background: Idx(8)
|
||||
marker: Idx(1), text: Rgb(238, 238, 238), background: Rgb(68, 68, 68)
|
||||
Terminal: › hello from voice
|
||||
marker: Idx(1), text: Default, background: Idx(8)
|
||||
marker: Idx(1), text: Rgb(238, 238, 238), background: Rgb(68, 68, 68)
|
||||
");
|
||||
},
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn vt100_prompt_keeps_text_element_accent_without_a_terminal_palette() {
|
||||
use crate::history_cell::HistoryCell;
|
||||
use crate::history_cell::new_user_prompt;
|
||||
use codex_protocol::user_input::TextElement;
|
||||
|
||||
crate::terminal_palette::with_test_default_colors(/*colors*/ None, || {
|
||||
let message = "[Image #1] details";
|
||||
let element = TextElement::new((0..10).into(), Some("[Image #1]".into()));
|
||||
let lines = new_user_prompt(message.into(), vec![element], vec![], vec![])
|
||||
.display_hyperlink_lines(/*width*/ 30);
|
||||
for policy in [
|
||||
HistoryLineWrapPolicy::PreWrap,
|
||||
HistoryLineWrapPolicy::Terminal,
|
||||
] {
|
||||
let (rows, _) =
|
||||
wrap_history_hyperlink_lines(&lines[1..2], /*wrap_width*/ 30, policy);
|
||||
let mut backend = VT100Backend::new(/*width*/ 30, /*height*/ 1);
|
||||
write_history_line(&mut backend, &rows[0], /*wrap_width*/ 30).unwrap();
|
||||
let screen = backend.vt100().screen();
|
||||
let element = screen.cell(/*row*/ 0, /*col*/ 2).unwrap();
|
||||
let plain = screen.cell(/*row*/ 0, /*col*/ 13).unwrap();
|
||||
assert_ne!(element.fgcolor(), plain.fgcolor());
|
||||
assert_eq!(element.bgcolor(), plain.bgcolor());
|
||||
}
|
||||
});
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn vt100_async_reply_keeps_inline_code_color_without_a_terminal_palette() {
|
||||
use crate::async_question_reply::AsyncQuestionReply;
|
||||
use crate::history_cell::HistoryCell;
|
||||
use crate::history_cell::new_user_prompt;
|
||||
|
||||
crate::terminal_palette::with_test_default_colors(/*colors*/ None, || {
|
||||
let answer = "Use `main`";
|
||||
let reply = AsyncQuestionReply {
|
||||
question_item_id: "ask".into(),
|
||||
question: "Which branch?".into(),
|
||||
answer: answer.into(),
|
||||
};
|
||||
let lines = new_user_prompt(answer.into(), vec![], vec![], vec![])
|
||||
.with_async_replies(vec![reply], std::path::Path::new("."))
|
||||
.display_hyperlink_lines(/*width*/ 30);
|
||||
let row = lines
|
||||
.iter()
|
||||
.find(|row| row.line.to_string().contains("Use main"))
|
||||
.unwrap();
|
||||
let mut backend = VT100Backend::new(/*width*/ 30, /*height*/ 1);
|
||||
write_history_line(&mut backend, row, /*wrap_width*/ 30).unwrap();
|
||||
let screen = backend.vt100().screen();
|
||||
let plain = screen.cell(/*row*/ 0, /*col*/ 2).unwrap();
|
||||
let code = screen.cell(/*row*/ 0, /*col*/ 6).unwrap();
|
||||
assert_eq!(plain.fgcolor(), vt100::Color::Rgb(238, 238, 238));
|
||||
assert_ne!(code.fgcolor(), plain.fgcolor());
|
||||
});
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn vt100_user_message_url_wrap_preserves_gutter_and_background() {
|
||||
use crate::history_cell::HistoryCell;
|
||||
|
||||
+6
-3
@@ -15,9 +15,12 @@ Buffer {
|
||||
" ",
|
||||
],
|
||||
styles: [
|
||||
x: 0, y: 0, fg: Reset, bg: DarkGray, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 3, fg: Reset, bg: DarkGray, underline: Reset, modifier: BOLD | DIM,
|
||||
x: 2, y: 3, fg: Reset, bg: DarkGray, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 0, fg: Rgb(238, 238, 238), bg: Rgb(68, 68, 68), underline: Reset, modifier: NONE,
|
||||
x: 0, y: 1, fg: Rgb(121, 180, 249), bg: Rgb(68, 68, 68), underline: Reset, modifier: NONE,
|
||||
x: 0, y: 2, fg: Rgb(238, 238, 238), bg: Rgb(68, 68, 68), underline: Reset, modifier: NONE,
|
||||
x: 0, y: 3, fg: Rgb(238, 238, 238), bg: Rgb(68, 68, 68), underline: Reset, modifier: BOLD | DIM,
|
||||
x: 2, y: 3, fg: Rgb(121, 180, 249), bg: Rgb(68, 68, 68), underline: Reset, modifier: NONE,
|
||||
x: 7, y: 3, fg: Rgb(238, 238, 238), bg: Rgb(68, 68, 68), underline: Reset, modifier: NONE,
|
||||
x: 0, y: 5, fg: Reset, bg: Reset, underline: Reset, modifier: NONE,
|
||||
]
|
||||
}
|
||||
|
||||
@@ -10,6 +10,7 @@ use crate::terminal_palette::best_color;
|
||||
use crate::terminal_palette::default_bg;
|
||||
use crate::terminal_palette::default_fg;
|
||||
use crate::terminal_palette::effective_stdout_color_level;
|
||||
use crate::terminal_palette::indexed_color;
|
||||
use crate::terminal_palette::rgb_color;
|
||||
use crate::terminal_palette::stdout_color_level;
|
||||
use ratatui::style::Color;
|
||||
@@ -84,12 +85,9 @@ pub fn user_message_style() -> Style {
|
||||
}
|
||||
|
||||
/// Submitted prompts use a lighter fill than the editable composer in either theme.
|
||||
///
|
||||
/// Some terminals do not answer the optional default-color probe. Keep submitted prompts shaded
|
||||
/// with an ANSI-safe fallback instead of dropping the surface.
|
||||
pub(crate) fn history_prompt_style() -> Style {
|
||||
let Some(background) = default_bg() else {
|
||||
return Style::default().bg(Color::DarkGray);
|
||||
return history_prompt_fallback(effective_stdout_color_level());
|
||||
};
|
||||
let (foreground, alpha) = if is_light(background) {
|
||||
((0, 0, 0), 0.02)
|
||||
@@ -99,6 +97,20 @@ pub(crate) fn history_prompt_style() -> Style {
|
||||
Style::default().bg(best_color(blend(foreground, background, alpha)))
|
||||
}
|
||||
|
||||
// These grays are exact entries in the xterm extended palette and have 8.4:1 contrast.
|
||||
// On terminals with only theme-defined colors, use the terminal's own foreground and background.
|
||||
fn history_prompt_fallback(level: StdoutColorLevel) -> Style {
|
||||
match level {
|
||||
StdoutColorLevel::TrueColor => Style::default()
|
||||
.fg(rgb_color((238, 238, 238)))
|
||||
.bg(rgb_color((68, 68, 68))),
|
||||
StdoutColorLevel::Ansi256 => Style::default()
|
||||
.fg(indexed_color(/*index*/ 255))
|
||||
.bg(indexed_color(/*index*/ 238)),
|
||||
StdoutColorLevel::Ansi16 | StdoutColorLevel::Unknown => Style::default(),
|
||||
}
|
||||
}
|
||||
|
||||
pub fn proposed_plan_style() -> Style {
|
||||
proposed_plan_style_for(default_bg())
|
||||
}
|
||||
|
||||
@@ -8,6 +8,27 @@ use ratatui::style::Modifier;
|
||||
use ratatui::text::Line;
|
||||
use ratatui::widgets::Widget;
|
||||
|
||||
#[test]
|
||||
fn prompt_fallback_preserves_contrast_at_each_color_level() {
|
||||
for level in [StdoutColorLevel::TrueColor, StdoutColorLevel::Ansi256] {
|
||||
let style = super::super::history_prompt_fallback(level);
|
||||
let resolve = |color| match (level, color) {
|
||||
(StdoutColorLevel::TrueColor, Some(Color::Rgb(r, g, b))) => (r, g, b),
|
||||
(StdoutColorLevel::Ansi256, Some(Color::Indexed(index))) if index >= 16 => {
|
||||
XTERM_COLORS[usize::from(index)]
|
||||
}
|
||||
_ => panic!("expected a palette-independent color for {level:?}, got {color:?}"),
|
||||
};
|
||||
assert!(ratio(resolve(style.fg), resolve(style.bg)) >= MIN_TEXT_CONTRAST);
|
||||
}
|
||||
for level in [StdoutColorLevel::Ansi16, StdoutColorLevel::Unknown] {
|
||||
assert_eq!(
|
||||
super::super::history_prompt_fallback(level),
|
||||
Style::default()
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn selection_text_stays_readable_after_palette_conversion() {
|
||||
for background in [
|
||||
|
||||
@@ -41,8 +41,9 @@ pub fn best_color_for_level(target: (u8, u8, u8), color_level: StdoutColorLevel)
|
||||
}
|
||||
|
||||
pub(crate) fn effective_stdout_color_level() -> StdoutColorLevel {
|
||||
// A scoped probe result fixes the test terminal at truecolor, including a failed probe.
|
||||
#[cfg(test)]
|
||||
if TEST_DEFAULT_COLORS.with(|colors| colors.get().flatten().is_some()) {
|
||||
if TEST_DEFAULT_COLORS.with(|colors| colors.get().is_some()) {
|
||||
return StdoutColorLevel::TrueColor;
|
||||
}
|
||||
|
||||
|
||||
@@ -52,9 +52,13 @@ fn prompt_margin_and_blank_background_survive_resize_without_changing_copy() {
|
||||
let message =
|
||||
"1234567890123456789012345678901234567890123456789012345678\n\n 界e\u{301}👩💻 trailing";
|
||||
let cell = new_user_prompt(message.into(), Vec::new(), Vec::new(), Vec::new());
|
||||
// Fix the inherited style without changing the process-wide terminal palette.
|
||||
let style = Style::default().on_dark_gray();
|
||||
let initial_lines = styled_prompt_lines(&cell, /*width*/ 60, style);
|
||||
let prompt_lines = |width| {
|
||||
crate::terminal_palette::with_test_default_colors(/*colors*/ None, || {
|
||||
cell.display_hyperlink_lines(width)
|
||||
})
|
||||
};
|
||||
let initial_lines = prompt_lines(/*width*/ 60);
|
||||
let style = initial_lines[0].line.style;
|
||||
let mut layout = TextLayout::new(initial_lines, /*width*/ 60);
|
||||
let expected_copy = format!("\n{message}\n");
|
||||
|
||||
@@ -64,7 +68,7 @@ fn prompt_margin_and_blank_background_survive_resize_without_changing_copy() {
|
||||
("prompt_margin_restored", 60),
|
||||
] {
|
||||
layout = layout.rewrap(width);
|
||||
let fresh_lines = styled_prompt_lines(&cell, width, style);
|
||||
let fresh_lines = prompt_lines(width);
|
||||
let area = Rect::new(
|
||||
/*x*/ 0,
|
||||
/*y*/ 0,
|
||||
@@ -129,13 +133,3 @@ fn reserved_margin_uses_the_same_alignment_for_painting_and_selection() {
|
||||
vec![0, 0, 0, 0, 0, 1, 2, 2],
|
||||
);
|
||||
}
|
||||
|
||||
fn styled_prompt_lines(cell: &dyn HistoryCell, width: u16, style: Style) -> Vec<HyperlinkLine> {
|
||||
cell.display_hyperlink_lines(width)
|
||||
.into_iter()
|
||||
.map(|mut line| {
|
||||
line.line.style = style;
|
||||
line
|
||||
})
|
||||
.collect()
|
||||
}
|
||||
|
||||
+3
-3
@@ -15,8 +15,8 @@ Buffer {
|
||||
" ",
|
||||
],
|
||||
styles: [
|
||||
x: 0, y: 0, fg: Reset, bg: DarkGray, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 1, fg: Reset, bg: DarkGray, underline: Reset, modifier: BOLD | DIM,
|
||||
x: 2, y: 1, fg: Reset, bg: DarkGray, underline: Reset, modifier: NONE,
|
||||
x: 0, y: 0, fg: Rgb(238, 238, 238), bg: Rgb(68, 68, 68), underline: Reset, modifier: NONE,
|
||||
x: 0, y: 1, fg: Rgb(238, 238, 238), bg: Rgb(68, 68, 68), underline: Reset, modifier: BOLD | DIM,
|
||||
x: 2, y: 1, fg: Rgb(238, 238, 238), bg: Rgb(68, 68, 68), underline: Reset, modifier: NONE,
|
||||
]
|
||||
}
|
||||
|
||||
@@ -1352,8 +1352,8 @@ fn spoken_prompt_selection_keeps_its_fallback_contrast() {
|
||||
(
|
||||
Some("one\n"),
|
||||
Color::Red,
|
||||
Color::Reset,
|
||||
Color::DarkGray,
|
||||
Color::Rgb(238, 238, 238),
|
||||
Color::Rgb(68, 68, 68),
|
||||
true
|
||||
)
|
||||
);
|
||||
|
||||
Reference in New Issue
Block a user