Skip to content

๐ŸŽจ Palette: [UX improvement] HTML ๋ฆฌํฌํŠธ ์ ‘๊ทผ์„ฑ ๋ฐ ์‹œ๊ฐ์  ๊ฐœ์„  - #561

Closed
seonghobae wants to merge 1 commit into
mainfrom
palette-ux-html-report-a11y-12954787463374055146
Closed

๐ŸŽจ Palette: [UX improvement] HTML ๋ฆฌํฌํŠธ ์ ‘๊ทผ์„ฑ ๋ฐ ์‹œ๊ฐ์  ๊ฐœ์„ #561
seonghobae wants to merge 1 commit into
mainfrom
palette-ux-html-report-a11y-12954787463374055146

Conversation

@seonghobae

@seonghobae seonghobae commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

๐Ÿ’ก What:

  • _identifier_list์˜ empty state๋ฅผ <p> ํƒœ๊ทธ์—์„œ <div role="status">๋กœ ๋ณ€๊ฒฝํ•˜์—ฌ ARIA ๋ผ์ด๋ธŒ ์˜์—ญ์œผ๋กœ ์ง€์ •ํ–ˆ์Šต๋‹ˆ๋‹ค.
  • CSS์— hover-focus isolation (tbody:hover tr:not(:hover) { opacity: 0.5; })์„ ์ถ”๊ฐ€ํ•˜์—ฌ ํ‘œ์˜ ํŠน์ • ํ–‰์— ๋งˆ์šฐ์Šค๋ฅผ ์˜ฌ๋ ธ์„ ๋•Œ ๋‹ค๋ฅธ ํ–‰๋“ค์˜ ๋ถˆํˆฌ๋ช…๋„๋ฅผ ๋‚ฎ์ท„์Šต๋‹ˆ๋‹ค.
  • CSS์— main:focus { outline: none; }์„ ์ถ”๊ฐ€ํ•˜์—ฌ ๋งˆ์šฐ์Šค ํด๋ฆญ ์‹œ ๋ถˆํ•„์š”ํ•œ ํฌ์ปค์Šค ์•„์›ƒ๋ผ์ธ์ด ๋‚˜ํƒ€๋‚˜์ง€ ์•Š๋„๋ก ํ–ˆ์Šต๋‹ˆ๋‹ค (focus-visible์€ ์œ ์ง€๋˜์–ด ํ‚ค๋ณด๋“œ ์ ‘๊ทผ์„ฑ ๋ณด์žฅ).

๐ŸŽฏ Why:

  • ์Šคํฌ๋ฆฐ ๋ฆฌ๋” ์‚ฌ์šฉ์ž๊ฐ€ ๋ฐ์ดํ„ฐ๊ฐ€ ์—†๋Š” ์ƒํƒœ(empty state)๋ฅผ ์ฆ‰๊ฐ์ ์œผ๋กœ ์ธ์‹ํ•˜์ง€ ๋ชปํ•˜๋Š” ๋ฌธ์ œ๋ฅผ ํ•ด๊ฒฐํ•ฉ๋‹ˆ๋‹ค.
  • ๋ฐ์ดํ„ฐ๊ฐ€ ๋ฐ€์ง‘๋œ ํ‘œ(dense data table)์—์„œ ํŠน์ • ํ–‰์˜ ๋ฐ์ดํ„ฐ๋ฅผ ์ฝ๊ฑฐ๋‚˜ ๋ถ„์„ํ•  ๋•Œ ์‹œ๊ฐ์  ํ˜ผ๋ž€์„ ์ค„์ด๊ณ  ์ง‘์ค‘๋„๋ฅผ ๋†’์—ฌ์ค๋‹ˆ๋‹ค.
  • ๋งˆ์šฐ์Šค ํด๋ฆญ ์‹œ <main> ์˜์—ญ ์ „์ฒด์— ์•„์›ƒ๋ผ์ธ์ด ์ƒ๊ธฐ๋Š” ์‹œ๊ฐ์  ๋ฒ„๊ทธ๋ฅผ ํ•ด๊ฒฐํ•˜์—ฌ ๋” ๊น”๋”ํ•œ UI๋ฅผ ์ œ๊ณตํ•ฉ๋‹ˆ๋‹ค.

๐Ÿ“ธ Before/After:

  • Before: ๋นˆ ์ƒํƒœ ๋ฉ”์‹œ์ง€๊ฐ€ ๋‹จ์ˆœ ํ…์ŠคํŠธ๋กœ ๋ Œ๋”๋ง๋˜๋ฉฐ, ํ‘œ ํ–‰์„ ๊ฐ€๋ฆฌํ‚ฌ ๋•Œ ์‹œ๊ฐ์  ํ”ผ๋“œ๋ฐฑ์ด ๋‹จ์ˆœํ•œ ๋ฐฐ๊ฒฝ์ƒ‰ ๋ณ€๊ฒฝ์— ๊ทธ์ณค๊ณ , Skip to content ๋“ฑ์˜ ๊ธฐ๋Šฅ์œผ๋กœ <main> ์˜์—ญ ํด๋ฆญ ์‹œ ๊ตต์€ ์•„์›ƒ๋ผ์ธ์ด ํ‘œ์‹œ๋จ.
  • After: ๋นˆ ์ƒํƒœ ๋ฉ”์‹œ์ง€๊ฐ€ role="status"๋กœ ๋ Œ๋”๋ง๋˜์–ด ์Šคํฌ๋ฆฐ ๋ฆฌ๋”๊ฐ€ ์ฆ‰์‹œ ์ฝ์–ด์ฃผ๋ฉฐ, ํ‘œ์—์„œ ํŠน์ • ํ–‰์— ๋งˆ์šฐ์Šค๋ฅผ ์˜ฌ๋ฆฌ๋ฉด ๋‹ค๋ฅธ ํ–‰๋“ค์ด ํฌ๋ฏธํ•ด์ ธ ์„ ํƒํ•œ ๋ฐ์ดํ„ฐ์— ์˜จ์ „ํžˆ ์ง‘์ค‘ํ•  ์ˆ˜ ์žˆ์Œ. ๋˜ํ•œ ๋งˆ์šฐ์Šค ํด๋ฆญ ์‹œ ๋ถˆํ•„์š”ํ•œ ์•„์›ƒ๋ผ์ธ์ด ์ œ๊ฑฐ๋จ.

โ™ฟ Accessibility:

  • role="status"๋ฅผ ํ†ตํ•ด ๋นˆ ์ƒํƒœ ๋ฉ”์‹œ์ง€์— ๋Œ€ํ•œ ์Šคํฌ๋ฆฐ ๋ฆฌ๋”์˜ ๋ผ์ด๋ธŒ ์˜์—ญ ์ง€์› ๊ฐœ์„ 
  • :focus์™€ :focus-visible์„ ๋ถ„๋ฆฌํ•˜์—ฌ ๋งˆ์šฐ์Šค ์‚ฌ์šฉ์ž์˜ ์‹œ๊ฐ์  ๋ฐฉํ•ด ์š”์†Œ๋Š” ์ œ๊ฑฐํ•˜๋ฉด์„œ๋„ ํ‚ค๋ณด๋“œ ๋‚ด๋น„๊ฒŒ์ด์…˜ ์‚ฌ์šฉ์ž์˜ ์ ‘๊ทผ์„ฑ(์ดˆ์  ๊ฐ€์‹œ์„ฑ)์€ ๊ทธ๋Œ€๋กœ ๋ณด์žฅ

PR created automatically by Jules for task 12954787463374055146 started by @seonghobae

Summary by CodeRabbit

  • Accessibility

    • Improved keyboard focus visibility in standalone HTML reports.
    • Empty validation states now use a semantic status message.
  • Visual Improvements

    • Added smoother table-row opacity transitions.
    • Highlighted hovered rows while dimming non-hovered rows.
  • Tests

    • Updated report and validation checks to reflect the accessibility and styling improvements.

- ์Šคํฌ๋ฆฐ ๋ฆฌ๋”๊ฐ€ empty state๋ฅผ ์˜ฌ๋ฐ”๋ฅด๊ฒŒ ์ธ์‹ํ•˜๋„๋ก `<p>` ํƒœ๊ทธ ๋Œ€์‹  `role="status"`๊ฐ€ ํฌํ•จ๋œ `<div>` ํƒœ๊ทธ ์‚ฌ์šฉ
- ํ‘œ ํ–‰์— hover ์‹œ ๋‹ค๋ฅธ ํ–‰๋“ค์˜ ๋ถˆํˆฌ๋ช…๋„๋ฅผ ๋‚ฎ์ถฐ ์‹œ๊ฐ์  ์ง‘์ค‘๋„๋ฅผ ๋†’์ด๋Š” CSS ์ถ”๊ฐ€
- ๋งˆ์šฐ์Šค ํด๋ฆญ ์‹œ `<main>` ์š”์†Œ์˜ focus outline ์ˆจ๊น€ ์ฒ˜๋ฆฌ (ํ‚ค๋ณด๋“œ ๋„ค๋น„๊ฒŒ์ด์…˜ ์‹œ์—๋Š” ์œ ์ง€)
@google-labs-jules

Copy link
Copy Markdown

๐Ÿ‘‹ Jules, reporting for duty! I'm here to lend a hand with this pull request.

When you start a review, I'll add a ๐Ÿ‘€ emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down.

I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job!

For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with @jules. You can find this option in the Pull Request section of your global Jules UI settings. You can always switch back!

New to Jules? Learn more at jules.google/docs.


For security, I will only act on instructions from the user who triggered this task.

@coderabbitai

coderabbitai Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

๐Ÿ“ Walkthrough

Walkthrough

The change updates standalone report focus and table-row styling. It changes empty validation identifier output to a status-region <div>. Related tests now assert the updated HTML and CSS.

Changes

HTML report updates

Layer / File(s) Summary
Report styling and coverage
python/fast_mlsirm/scoring/essay/report_html.py, tests/test_scoring_essay_report_html.py
The report removes the default main focus outline, preserves keyboard-visible focus styling, and dims non-hovered table rows. Tests assert the updated CSS.
Validation report markup and formatting
python/fast_mlsirm/scoring/essay/validation_report_html.py, tests/test_scoring_essay_validation_report_html.py
Empty identifier lists render as <div class="empty-state" role="status">. Several expressions and strings are reformatted without behavior changes. Tests assert the new markup.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

๐Ÿšฅ Pre-merge checks | โœ… 5
โœ… Passed checks (5 passed)
Check name Status Explanation
Description Check โœ… Passed Check skipped - CodeRabbitโ€™s high-level summary is enabled.
Title check โœ… Passed The title clearly summarizes the pull request's accessibility and visual UX improvements to the HTML report.
Docstring Coverage โœ… Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check โœ… Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check โœ… Passed Check skipped because no linked issues were found for this pull request.
โœจ Finishing Touches
๐Ÿ“ Generate docstrings
  • Create stacked PR
  • Commit on current branch
๐Ÿงช Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch palette-ux-html-report-a11y-12954787463374055146

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

๐Ÿค– Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@python/fast_mlsirm/scoring/essay/report_html.py`:
- Around line 188-190: Update the table hover CSS rules in the report HTML
styling to remove the row-level opacity applied by tbody:hover tr:not(:hover),
keeping table-row contentโ€”including text, borders, and focus indicatorsโ€”at full
opacity while preserving the existing hover background behavior.
๐Ÿช„ Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

โ„น๏ธ Review info
โš™๏ธ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: c97e0daf-a4ea-4f75-b987-26e3bbc87abd

๐Ÿ“ฅ Commits

Reviewing files that changed from the base of the PR and between 84f3251 and bceede6.

๐Ÿ“’ Files selected for processing (4)
  • python/fast_mlsirm/scoring/essay/report_html.py
  • python/fast_mlsirm/scoring/essay/validation_report_html.py
  • tests/test_scoring_essay_report_html.py
  • tests/test_scoring_essay_validation_report_html.py

Comment on lines +188 to +190
tbody tr { transition: background-color 0.15s ease-in-out, opacity 0.15s ease-in-out; }
tbody tr:hover { background-color: rgba(128, 128, 128, 0.15); }
tbody:hover tr:not(:hover) { opacity: 0.5; }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

๐ŸŽฏ Functional Correctness | ๐ŸŸ  Major | โšก Quick win

Keep table-row content at full opacity.

opacity: 0.5 affects the complete row, including text, borders, and focus indicators. In the light color scheme, this can reduce normal text contrast below the accessible threshold. If a row contains a focusable control, the control can also remain dim while the pointer stays inside tbody. Remove row-level opacity or apply the effect only to non-text decoration.

๐Ÿค– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@python/fast_mlsirm/scoring/essay/report_html.py` around lines 188 - 190,
Update the table hover CSS rules in the report HTML styling to remove the
row-level opacity applied by tbody:hover tr:not(:hover), keeping table-row
contentโ€”including text, borders, and focus indicatorsโ€”at full opacity while
preserving the existing hover background behavior.

Copy link
Copy Markdown
Contributor Author

Closing this mixed-scope UX change rather than merging a regression. The role="status" empty-state improvement is independently useful, but this PR also adds tbody:hover tr:not(:hover) { opacity: 0.5; }, directly conflicting with the contrast-preservation decision in #550, and suppresses the generic main:focus outline. Those changes should not share one acceptance decision. Reintroduce the semantic empty-state change later as a focused, documented regression-tested PR after the canonical report accessibility branches settle; no source from this PR is being merged now.

@seonghobae seonghobae closed this Aug 6, 2026

Copy link
Copy Markdown
Contributor Author

Closing because the proposed bundle is not an accessibility improvement for this static standalone report. role="status" is a live-region contract for content updates/status changes, not ordinary empty content already present at page load. The added peer-opacity rule lowers the rendered contrast of still-active table data, and main:focus { outline: none; } can suppress the user-agent focus indicator outside the narrower :focus-visible case. These changes conflict with the repository's fail-safe focus/contrast policy and add no browser/assistive-technology evidence. The row-header semantics work remains separately governed in PR #556.

@google-labs-jules

Copy link
Copy Markdown

Closing this mixed-scope UX change rather than merging a regression. The role="status" empty-state improvement is independently useful, but this PR also adds tbody:hover tr:not(:hover) { opacity: 0.5; }, directly conflicting with the contrast-preservation decision in #550, and suppresses the generic main:focus outline. Those changes should not share one acceptance decision. Reintroduce the semantic empty-state change later as a focused, documented regression-tested PR after the canonical report accessibility branches settle; no source from this PR is being merged now.

Understood. Acknowledging that this work is now obsolete and stopping work on this task.

Copy link
Copy Markdown
Contributor Author

Closing as not mergeable in its current form. The patch reintroduces tbody:hover tr:not(:hover) { opacity: 0.5; }, which conflicts with the contrast-preserving direction and focused evidence in PR #550, and adds main:focus { outline: none; }, which suppresses an actual-focus indicator rather than preserving a clear keyboard/fallback boundary. The useful empty-state semantic change is separable, but this PR also lacks the required doctoring/changelog and combines it with contradictory presentation behavior. Reopen that semantic improvement as a focused, independently tested slice after #550 is resolved.

Copy link
Copy Markdown
Contributor Author

Closing this automation branch because it conflicts with the accepted accessibility direction and mixes an unrelated static empty-state change into the report styling repair. It reintroduces tbody:hover tr:not(:hover) { opacity: 0.5; }, which PR #550 deliberately removes to preserve peer data contrast, and adds main:focus { outline: none; }, which can suppress the only visible indicator when :focus-visible heuristics do not apply. The static role="status" change also needs a separate dynamic-announcement requirement and assistive-technology test rather than being bundled here. PR #550 remains the canonical focus/contrast repair; row-header semantics are handled separately by #556.

Copy link
Copy Markdown
Contributor Author

Closing rather than merging this mixed change. The peer-row opacity rule reverses the reviewed contrast-preservation contract in #550 by dimming unrelated table data, and main:focus { outline: none; } is an unnecessary global focus override. The independent empty-state semantic change should be proposed only as a narrow, standards-backed PR if it is still absent after current accessibility work merges. Keeping this PR open would reintroduce a known UX/accessibility regression and conflict with the canonical report direction.

Copy link
Copy Markdown
Contributor Author

Closing this automation PR rather than merging the mixed accessibility changes. The new tbody:hover tr:not(:hover) { opacity: 0.5; } deliberately reduces adjacent data contrast during comparison, conflicting with the maintained report-accessibility direction in #550, and main:focus { outline: none; } risks suppressing the focus destination reached through skip navigation. The independent empty-state role="status" idea can be reintroduced in a narrowly scoped, standards-reviewed PR without the contrast/focus regressions or unrelated formatting churn.

@google-labs-jules

Copy link
Copy Markdown

Closing because the proposed bundle is not an accessibility improvement for this static standalone report. role="status" is a live-region contract for content updates/status changes, not ordinary empty content already present at page load. The added peer-opacity rule lowers the rendered contrast of still-active table data, and main:focus { outline: none; } can suppress the user-agent focus indicator outside the narrower :focus-visible case. These changes conflict with the repository's fail-safe focus/contrast policy and add no browser/assistive-technology evidence. The row-header semantics work remains separately governed in PR #556.

Understood. Acknowledging that this work is now obsolete and stopping work on this task.

@google-labs-jules

Copy link
Copy Markdown

Closing as not mergeable in its current form. The patch reintroduces tbody:hover tr:not(:hover) { opacity: 0.5; }, which conflicts with the contrast-preserving direction and focused evidence in PR #550, and adds main:focus { outline: none; }, which suppresses an actual-focus indicator rather than preserving a clear keyboard/fallback boundary. The useful empty-state semantic change is separable, but this PR also lacks the required doctoring/changelog and combines it with contradictory presentation behavior. Reopen that semantic improvement as a focused, independently tested slice after #550 is resolved.

Understood. Acknowledging that this work is now obsolete and stopping work on this task.

@google-labs-jules

Copy link
Copy Markdown

Closing this automation branch because it conflicts with the accepted accessibility direction and mixes an unrelated static empty-state change into the report styling repair. It reintroduces tbody:hover tr:not(:hover) { opacity: 0.5; }, which PR #550 deliberately removes to preserve peer data contrast, and adds main:focus { outline: none; }, which can suppress the only visible indicator when :focus-visible heuristics do not apply. The static role="status" change also needs a separate dynamic-announcement requirement and assistive-technology test rather than being bundled here. PR #550 remains the canonical focus/contrast repair; row-header semantics are handled separately by #556.

Understood. Acknowledging that this work is now obsolete and stopping work on this task.

@google-labs-jules

Copy link
Copy Markdown

Closing rather than merging this mixed change. The peer-row opacity rule reverses the reviewed contrast-preservation contract in #550 by dimming unrelated table data, and main:focus { outline: none; } is an unnecessary global focus override. The independent empty-state semantic change should be proposed only as a narrow, standards-backed PR if it is still absent after current accessibility work merges. Keeping this PR open would reintroduce a known UX/accessibility regression and conflict with the canonical report direction.

Understood. Acknowledging that this work is now obsolete and stopping work on this task.

@google-labs-jules

Copy link
Copy Markdown

Closing this automation PR rather than merging the mixed accessibility changes. The new tbody:hover tr:not(:hover) { opacity: 0.5; } deliberately reduces adjacent data contrast during comparison, conflicting with the maintained report-accessibility direction in #550, and main:focus { outline: none; } risks suppressing the focus destination reached through skip navigation. The independent empty-state role="status" idea can be reintroduced in a narrowly scoped, standards-reviewed PR without the contrast/focus regressions or unrelated formatting churn.

Understood. Acknowledging that this work is now obsolete and stopping work on this task.

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.

1 participant