Wizard: only offer image mode on RHEL 10 hosts - #4740
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/Components/CreateImageWizard/steps/ImageOutput/components/BlueprintMode.tsx" line_range="108-117" />
<code_context>
- }}
- aria-describedby='blueprint-mode-description'
- />
+ {!isImageModeSupported && isHostDistroKnown ? (
+ // Disabled buttons don't emit hover events, so the tooltip
+ // needs a wrapper element as its trigger.
+ <Tooltip content='Image mode is currently available only on RHEL 10 hosts. Support for CentOS Stream and Fedora is coming soon.'>
+ <span
+ className='image-mode-toggle-wrapper'
+ data-testid='image-mode-toggle-wrapper'
+ >
+ {imageModeToggle}
+ </span>
+ </Tooltip>
+ ) : (
+ imageModeToggle
+ )}
</ToggleGroup>
</code_context>
<issue_to_address>
**suggestion:** Tooltip wrapper around a disabled button might benefit from explicit accessibility handling.
Because the tooltip trigger is a non-semantic `<span>` wrapping a disabled control, assistive tech may not expose the tooltip or the disabled state correctly. Consider either giving the wrapper an appropriate role/ARIA attributes, or keeping the inner button focusable (e.g., `aria-disabled` with click prevention) so the tooltip and disabled state are properly announced for keyboard and screen-reader users.
Suggested implementation:
```typescript
{!isImageModeSupported && isHostDistroKnown ? (
// Disabled buttons don't emit hover events, so the tooltip
// needs a wrapper element as its trigger.
// Make the wrapper a semantic, focusable control so the tooltip
// and disabled state are exposed to assistive technologies.
<Tooltip content='Image mode is currently available only on RHEL 10 hosts. Support for CentOS Stream and Fedora is coming soon.'>
<span
className='image-mode-toggle-wrapper'
data-testid='image-mode-toggle-wrapper'
role='button'
aria-disabled='true'
aria-label='Image mode toggle (unavailable for this host)'
tabIndex={0}
>
{imageModeToggle}
</span>
</Tooltip>
) : (
imageModeToggle
)}
```
Depending on how `imageModeToggle` is implemented elsewhere in this file, you may want to:
1. Ensure that the inner toggle is actually disabled *only* via `aria-disabled` (and click prevention) instead of the native `disabled` attribute, so that the control remains reachable by keyboard while still being announced as disabled.
2. If you switch to `aria-disabled`, make sure to prevent activation for unsupported hosts in the `onClick` handler (and optionally `onKeyDown` for Space/Enter) to preserve the disabled behavior while enabling consistent tooltip announcement for keyboard and screen-reader users.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| {!isImageModeSupported && isHostDistroKnown ? ( | ||
| // Disabled buttons don't emit hover events, so the tooltip | ||
| // needs a wrapper element as its trigger. | ||
| <Tooltip content='Image mode is currently available only on RHEL 10 hosts. Support for CentOS Stream and Fedora is coming soon.'> | ||
| <span | ||
| className='image-mode-toggle-wrapper' | ||
| data-testid='image-mode-toggle-wrapper' | ||
| > | ||
| {imageModeToggle} | ||
| </span> |
There was a problem hiding this comment.
suggestion: Tooltip wrapper around a disabled button might benefit from explicit accessibility handling.
Because the tooltip trigger is a non-semantic <span> wrapping a disabled control, assistive tech may not expose the tooltip or the disabled state correctly. Consider either giving the wrapper an appropriate role/ARIA attributes, or keeping the inner button focusable (e.g., aria-disabled with click prevention) so the tooltip and disabled state are properly announced for keyboard and screen-reader users.
Suggested implementation:
{!isImageModeSupported && isHostDistroKnown ? (
// Disabled buttons don't emit hover events, so the tooltip
// needs a wrapper element as its trigger.
// Make the wrapper a semantic, focusable control so the tooltip
// and disabled state are exposed to assistive technologies.
<Tooltip content='Image mode is currently available only on RHEL 10 hosts. Support for CentOS Stream and Fedora is coming soon.'>
<span
className='image-mode-toggle-wrapper'
data-testid='image-mode-toggle-wrapper'
role='button'
aria-disabled='true'
aria-label='Image mode toggle (unavailable for this host)'
tabIndex={0}
>
{imageModeToggle}
</span>
</Tooltip>
) : (
imageModeToggle
)}Depending on how imageModeToggle is implemented elsewhere in this file, you may want to:
- Ensure that the inner toggle is actually disabled only via
aria-disabled(and click prevention) instead of the nativedisabledattribute, so that the control remains reachable by keyboard while still being announced as disabled. - If you switch to
aria-disabled, make sure to prevent activation for unsupported hosts in theonClickhandler (and optionallyonKeyDownfor Space/Enter) to preserve the disabled behavior while enabling consistent tooltip announcement for keyboard and screen-reader users.
Codecov Report❌ Patch coverage is
@@ Coverage Diff @@
## lucas/cockpit/6-optional-users #4740 +/- ##
==================================================================
- Coverage 79.09% 79.01% -0.08%
==================================================================
Files 265 265
Lines 7021 7028 +7
Branches 2546 2588 +42
==================================================================
Hits 5553 5553
- Misses 1372 1376 +4
- Partials 96 99 +3
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 6 files with indirect coverage changes Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
2bca341 to
06e8e4d
Compare
06e8e4d to
1749109
Compare
1749109 to
8a40557
Compare
8a40557 to
f324e7d
Compare
06e8e4d to
c36c165
Compare
c36c165 to
7241c9d
Compare
On-prem image mode only ships official RHEL 10 images, so on other host distros it dead-ends at a registry login wall with nothing to build. Disable the image mode toggle on non-RHEL-10 hosts and explain why in a tooltip: image mode is currently available only on RHEL 10, with CentOS Stream and Fedora support coming soon. The hosted service is unaffected. The host distro is modeled as unknown until the check resolves: the toggle starts disabled without the tooltip, so RHEL 10 users don't see a flash of "coming soon" and other hosts don't get a brief window where an unsupported mode can be entered.
7241c9d to
eba1d52
Compare
|
Replaced by #4756 |
Image mode is disabled with an explanatory tooltip on non-RHEL-10 hosts, since only RHEL 10 official images ship today.
Stack created with GitHub Stacks CLI • Give Feedback 💬