Fix multiple accessibility icons stacking by implementing inline positioning#1123
Conversation
Co-authored-by: pattonwebz <3902039+pattonwebz@users.noreply.github.com>
…tioning Co-authored-by: pattonwebz <3902039+pattonwebz@users.noreply.github.com>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the WalkthroughThe update modifies the tooltip positioning logic within the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AccessibilityCheckerHighlight
participant DOM
User->>AccessibilityCheckerHighlight: Triggers addTooltip(targetElement)
AccessibilityCheckerHighlight->>DOM: Query for existing tooltips with matching data-target-element
AccessibilityCheckerHighlight->>AccessibilityCheckerHighlight: Compute tooltipOffset based on count
AccessibilityCheckerHighlight->>DOM: Create new tooltip with data-target-element attribute
AccessibilityCheckerHighlight->>DOM: Position tooltip horizontally (left) using offset
DOM-->>User: Tooltip(s) appear inline, not stacked
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Poem
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Join our Discord community for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an accessibility issue where multiple accessibility icons for the same DOM element would stack on top of each other, making only the frontmost icon clickable. The solution implements inline horizontal positioning for multiple tooltips targeting the same element.
- Modified tooltip positioning logic to track existing tooltips for the same target element
- Added horizontal offset calculation to position multiple tooltips inline with a 5px gap
- Maintained existing functionality while ensuring all accessibility issues remain accessible
|
@copilot tackle the code review comments. Decide yourself if you want to replace the magic number with a named const |
Co-authored-by: pattonwebz <3902039+pattonwebz@users.noreply.github.com>
I've replaced the magic number with a named constant Warning Firewall rules blocked me from connecting to one or more addressesI tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (1)
src/frontendHighlighterApp/index.js(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: SteveJonesDev
PR: equalizedigital/accessibility-checker#921
File: src/pageScanner/checks/linked-image-alt-present.js:43-50
Timestamp: 2025-04-15T02:30:38.897Z
Learning: In the Accessibility Checker plugin, accessibility rules are separated by specific concerns. For example, linked images have separate rules for checking: (1) missing alt attributes and (2) empty alt attributes. The rule `linked_image_alt_present` specifically checks for the presence of alt attributes on linked images, while a separate rule `img_linked_alt_empty` handles validation of empty alt attributes. Suggestions should respect this separation of concerns.
Learnt from: pattonwebz
PR: equalizedigital/accessibility-checker#881
File: src/pageScanner/checks/duplicate-form-label-check.js:51-55
Timestamp: 2025-04-08T21:45:57.372Z
Learning: The Accessibility Checker intentionally flags multiple IDs in aria-labelledby as a failure, even though it's technically allowed in the ARIA spec. This design decision was made because multiple IDs can cause confusion with some screen readers, and the tool aims to discourage any form of duplicate labelling to ensure maximum compatibility.
Learnt from: pattonwebz
PR: equalizedigital/accessibility-checker#927
File: src/pageScanner/checks/img-alt-missing-check.js:35-37
Timestamp: 2025-04-18T14:27:18.140Z
Learning: In the Accessibility Checker plugin, the img_alt_missing rule specifically checks for missing alt attributes on images and image inputs, while empty alt attributes are handled by a separate rule. Each accessibility concern is deliberately separated into individual rules.
Learnt from: SteveJonesDev
PR: equalizedigital/accessibility-checker#1088
File: includes/classes/Rules/Rule/VideoPresentRule.php:41-42
Timestamp: 2025-07-18T01:18:27.812Z
Learning: In the Accessibility Checker plugin, the 'wcag' field can use custom values like '0.3' to represent best practice rules and manual testing requirements that don't map directly to specific WCAG success criteria but are still important for accessibility compliance.
Learnt from: SteveJonesDev
PR: equalizedigital/accessibility-checker#1088
File: includes/classes/Rules/Rule/EmptyParagraphTagRule.php:50-50
Timestamp: 2025-07-18T01:29:35.548Z
Learning: In the Accessibility Checker plugin, the 'wcag' field can use custom values like '0.1' to represent best practice rules and manual testing requirements that don't map directly to specific WCAG success criteria but are still important for accessibility compliance.
Learnt from: pattonwebz
PR: equalizedigital/accessibility-checker#927
File: src/pageScanner/checks/img-alt-missing-check.js:32-32
Timestamp: 2025-04-18T14:27:49.512Z
Learning: In the Accessibility Checker plugin, the img_alt_missing rule specifically checks for missing alt attributes (whether the attribute exists at all), while empty alt attributes (alt="") are handled by a separate rule. This separation of concerns is by design.
src/frontendHighlighterApp/index.js (1)
Learnt from: pattonwebz
PR: #927
File: src/pageScanner/checks/img-alt-missing-check.js:97-99
Timestamp: 2025-04-18T14:26:19.552Z
Learning: The minimum length check of > 5 characters for caption text in hasValidCaption() is intentional and should be preserved to maintain consistency with the original PHP implementation and to encourage more descriptive captions.
🪛 GitHub Actions: Lint JS
src/frontendHighlighterApp/index.js
[error] 349-349: ESLint: 'hashString' is not defined (no-undef)
[error] 349-349: ESLint: There must be a space after this paren (space-in-parens)
[error] 349-349: ESLint: There must be a space before this paren (space-in-parens)
🔇 Additional comments (2)
src/frontendHighlighterApp/index.js (2)
305-306: Good improvement with the named constant.Extracting the magic number
5toTOOLTIP_GAPconstant improves maintainability and addresses the previous review feedback. The offset calculation logic is sound, but it depends on the tooltip detection logic working correctly (see previous comment).
319-319: LGTM! Correct horizontal positioning calculation.The formula
tooltipOffset * (tooltipWidth + TOOLTIP_GAP)correctly implements the horizontal inline positioning by spacing each tooltip by its width plus the defined gap.
This function computes a hash value from a given string, ensuring a positive result.
Updated the tooltip positioning logic to ensure that tooltips created before the current one are considered when calculating their position.
|
After a little tweaking, this is working Peek.2025-07-28.22-54.mp4 |
pattonwebz
left a comment
There was a problem hiding this comment.
This worked when I tested it
SteveJonesDev
left a comment
There was a problem hiding this comment.
This seems like a pretty smart fix. Approved.
When multiple accessibility issues were detected for the same DOM element, the icon buttons would stack on top of each other, making only the front-most button clickable. This created a poor user experience where users couldn't access all the issues for an element.
Root Cause:
The
addTooltipfunction insrc/frontendHighlighterApp/index.jswas positioning all tooltips for the same target element at identical coordinates using thecomputePositionfunction from floating-ui, causing them to overlap completely.Solution:
Modified the tooltip positioning logic to:
data-targetElementattributeBefore: Icons stacked and only the front one was usable
After: Icons appear horizontally inline and all are clickable
The fix maintains all existing functionality while ensuring users can interact with all accessibility issues for any given element.
Fixes #951.
Warning
Firewall rules blocked me from connecting to one or more addresses
I tried to connect to the following addresses, but was blocked by firewall rules:
download.cypress.ionode index.js --exec install(dns block)https://api.github.com/repos/Dealerdirect/phpcodesniffer-composer-installer/zipball/1c968e542d8843d7cd71de3c5c9c3ff3ad71a1db/usr/bin/php8.3 -n -c /tmp/f4u4Xf /usr/bin/composer install(http block)https://api.github.com/repos/PHPCSStandards/PHPCSExtra/zipball/fa4b8d051e278072928e32d817456a7fdb57b6ca/usr/bin/php8.3 -n -c /tmp/f4u4Xf /usr/bin/composer install(http block)https://api.github.com/repos/PHPCSStandards/PHP_CodeSniffer/zipball/5b5e3821314f947dd040c70f7992a64eac89025c/usr/bin/php8.3 -n -c /tmp/f4u4Xf /usr/bin/composer install(http block)https://api.github.com/repos/antecedent/patchwork/zipball/1bf183a3e1bd094f231a2128b9ecc5363c269245/usr/bin/php8.3 -n -c /tmp/f4u4Xf /usr/bin/composer install(http block)https://api.github.com/repos/phpstan/phpstan/zipball/3a6e423c076ab39dfedc307e2ac627ef579db162/usr/bin/php8.3 -n -c /tmp/f4u4Xf /usr/bin/composer install(http block)If you need me to access, download, or install something from one of these locations, you can either:
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Summary by CodeRabbit
Fixes: https://linear.app/equalize-digital/issue/PRO-72/when-an-element-has-multiple-errors-or-warnings-associated-inline-the
Fixes: #951