Conversation
tg666
added
enhancement
WalkthroughThis update reverts a prior removal of focus restoration on the settings modal trigger, improves accessibility and translation updates for modal buttons, reorders buttons in the settings modal DOM to match visual order, modifies cookie consent library imports and focus management logic, and injects internal modal focus utilities via Webpack string replacements. It also updates configuration defaults and adds a new development dependency. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ModalTrigger
participant CookieConsentWrapper
participant CookieConsentLib
participant ThirdButtonAppender
User->>ModalTrigger: Clicks settings modal trigger
ModalTrigger->>CookieConsentWrapper: showSettings(0)
CookieConsentWrapper->>CookieConsentLib: Show settings modal
CookieConsentWrapper->>CookieConsentLib: Reorder settings modal buttons (next frame)
CookieConsentLib->>CookieConsentLib: Update focusable elements and set focus
CookieConsentWrapper->>ThirdButtonAppender: Append third button (if enabled)
ThirdButtonAppender->>CookieConsentLib: Generate focusable span for label
ThirdButtonAppender->>CookieConsentLib: Listen for locale:change
ThirdButtonAppender->>ThirdButtonAppender: Update label on locale change
CookieConsentWrapper->>CookieConsentLib: On language update, update focus and re-translate third button
Possibly related PRs
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
- reverted change that removes a focus from an element created by the "Settings modal trigger selector" option after the settings modal is closed - fixed accessibility for third button in the consent modal - fixed accessibility in both modals after a language update - third button is now re-translated after a language update - buttons in the settings modal are physically reordered in the DOM if they are ordered visually using CSS flexbox property `order` to keep right tabindex flow
tg666
force-pushed
the
feature/accessibility
branch
from
f569598 to
390786d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 3
♻️ Duplicate comments (1)
webpack.config.prod.mjs (1)
55-120: Consistent implementation across environmentsThe string replacement configuration is identical to the development config, ensuring consistent behavior across environments.
🧹 Nitpick comments (1)
CHANGELOG.md (1)
12-12: Fix potential formatting duplication.The static analysis tool flagged a potential word repetition. Ensure there's no unintended duplication in the "### Fixed" section header.
🧰 Tools
🪛 LanguageTool
[duplication] ~12-~12: Possible typo: you repeated a word.
Context: ... violation of accessibility rules. ### Fixed - Fixed accessibility for third button in the c...(ENGLISH_WORD_REPEAT_RULE)
🛑 Comments failed to post (3)
webpack.config.dev.mjs (2)
86-86: 🛠️ Refactor suggestion
Hardcoded timeout value may cause focus issues
The 250ms timeout for setting focus might not be sufficient on slower devices or when animations are involved. This could lead to focus not being set correctly.
Consider making the timeout configurable or using a more reliable approach like
requestAnimationFrameor waiting for the transition end event:-setTimeout(() => setFocus(focusable_edges[0]), 250); +// Wait for modal transition to complete +const modal = focusable_edges[0]?.closest('.cc-modal'); +if (modal && modal.style.transition) { + modal.addEventListener('transitionend', () => { + setFocus(focusable_edges[0]); + }, { once: true }); +} else { + requestAnimationFrame(() => { + setFocus(focusable_edges[0]); + }); +}Also applies to: 97-97
🤖 Prompt for AI Agents
In webpack.config.dev.mjs at lines 86 and 97, the hardcoded 250ms timeout for setting focus may be unreliable on slower devices or during animations. Replace the setTimeout with a more robust approach such as using requestAnimationFrame to defer the focus setting until the next repaint, or listen for the relevant transition end event before setting focus. Alternatively, make the timeout duration configurable to allow adjustment based on device performance or animation timing.
68-73: 💡 Verification agent
🧩 Analysis chain
Potential race condition with focusable element filtering
The logic at line 68 checks both
'true' === focusableOnShowConsentand'false' === focusableOnShowConsent, but this comparison is string-based and may not handle edge cases properly. Consider using a more robust approach.
🌐 Web query:
What are the best practices for handling data attributes in JavaScript when dealing with boolean-like values?💡 Result:
When handling boolean-like values in JavaScript, following established best practices ensures your code is efficient, readable, and less prone to bugs. Here are comprehensive guidelines for working with boolean values in JavaScript.
Using Boolean Values Properly
Boolean Primitives vs Objects
Always use boolean primitives (
trueorfalse) instead of Boolean objects. Creating booleans as objects is unnecessary, adds overhead, and can lead to unexpected behavior[5][8].// DO this: let isActive = true; // DON'T do this: let isActive = new Boolean(true); // unnecessary object wrapperBoolean objects are always truthy in conditional statements, even when their wrapped value is
false[1]. This can lead to confusing behavior:if (new Boolean(false)) { console.log("This will always execute because Boolean objects are truthy!"); }Converting Values to Boolean
For converting non-boolean values to boolean, use the
Boolean()function as a function (not a constructor) or use the double NOT operator (!!)[1]:// Good approaches const boolValue1 = Boolean(expression); const boolValue2 = !!expression; // Avoid const badApproach = new Boolean(expression);The
Boolean()function converts values based on their "truthiness" in JavaScript. Non-empty strings, non-zero numbers, objects, and arrays are truthy, while empty strings, 0,null,undefined, andNaNare falsy[9].Coding Conventions
Variable Naming
Use descriptive names for boolean variables, preferably starting with verbs like "is", "has", or "can":
let isVisible = true; let hasPermission = false; let canEdit = user.role === 'admin';Default Values for Boolean Parameters
It's often helpful to define default values for boolean parameters in functions[2]:
function toggleFeature(featureId, enable = false) { // Implementation }This makes the function's behavior clearer and allows the parameter to be omitted when the default behavior is desired.
Boolean in Conditions
Explicit Comparisons
For maximum clarity, especially in complex conditions, use explicit comparisons:
// More explicit if (isActive === true) { // Code } // Shorter but still clear if (isActive) { // Code }Avoid Type Coercion Issues
Use strict equality operators (
===and!==) instead of loose equality operators (==and!=) to avoid unexpected type coercion[8]:// This evaluates to true because of type coercion if ("1" == true) { // This code will execute } // This evaluates to false with strict comparison if ("1" === true) { // This code won't execute }Common Patterns
Boolean Return Values with Additional Information
While a common pattern in some libraries like Modernizr is to return a truthy value with additional properties, this approach has significant limitations. In JavaScript, you cannot add properties to primitive boolean values that preserve their boolean behavior[4].
Instead, consider these alternatives:
Return an object with a boolean property and additional data:
function checkVideoSupport() { return { supported: true, formats: ['mp4', 'webm'] }; }Use
nullfor unsupported features and an object with details for supported ones:function checkSupport() { // Return null if not supported // Return object with details if supported }Avoiding Nulls in Boolean Fields
Don't use
nullfor boolean properties. Instead, assign default boolean values to avoidnullentirely[7]. If you genuinely need a third state (beyond true/false), consider using a different data type or approach.Best Practices Summary
- Use primitive boolean values (
true/false), not strings or Boolean objects[5][8].- Use the
Boolean()function or double NOT (!!) for type conversion, not the constructor[1].- Use strict equality operators (
===,!==) for boolean comparisons[8].- Provide default values for boolean function parameters[2].
- Use descriptive, action-oriented naming for boolean variables.
- Return objects with boolean properties rather than trying to add properties to boolean values[4].
- Don't use
nullin boolean fields; assign defaults instead[7].Following these practices will help you write cleaner, more maintainable JavaScript code when working with boolean values.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Boolean
- 2: https://softwareengineering.stackexchange.com/questions/425357/defining-default-values-for-boolean-arguments-in-javascript
- 3: https://www.w3schools.com/js/js_booleans.asp
- 4: https://softwareengineering.stackexchange.com/questions/339734/is-a-js-boolean-having-custom-properties-a-bad-practice
- 5: https://www.w3schools.com/js/js_best_practices.asp
- 6: https://www.javascript.com/learn/booleans
- 7: https://tag.tdwg.org/reference/boolean/
- 8: https://www.udacity.com/blog/2021/05/javascript-booleans.html
- 9: https://www.geeksforgeeks.org/javascript-boolean/
- 10: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Data_structures
Coerce and simplify boolean logic for focus management
- In webpack.config.dev.mjs around lines 68–73, avoid comparing the string value of
focusableOnShowConsenton each filter run.- Coerce the data attribute to a Boolean primitive once, then use a simple equality check.
Suggested update:
- if (('true' === focusableOnShowConsent && !consent_modal_visible) || - ('false' === focusableOnShowConsent && consent_modal_visible)) { - return false; - } - return true; + // Coerce once when reading the attribute + const focusableOnShowConsentBool = focusableOnShowConsent === 'true'; + + // Return true only when the consent modal visibility matches the desired focus behavior + return focusableOnShowConsentBool === consent_modal_visible;This ensures you use a Boolean primitive (per best practices) and makes the condition clearer and less error-prone.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.// Coerce once when reading the attribute const focusableOnShowConsentBool = focusableOnShowConsent === 'true'; // Return true only when the consent modal visibility matches the desired focus behavior return focusableOnShowConsentBool === consent_modal_visible; });🤖 Prompt for AI Agents
In webpack.config.dev.mjs around lines 68 to 73, the code compares the string value of focusableOnShowConsent directly, which is error-prone and inefficient. To fix this, convert focusableOnShowConsent to a Boolean primitive once before the filter logic, then use simple Boolean checks in the condition instead of string comparisons. This will simplify the logic, improve readability, and follow best practices for handling boolean-like values.src/CookieConsentWrapper.mjs (1)
279-285: 💡 Verification agent
🧩 Analysis chain
Verify autorun and delay configuration defaults
The code checks for
autorunand shows the consent modal after a delay. Ensure that the default values for these options are appropriate and documented.
🏁 Script executed:
#!/bin/bash # Search for default plugin options configuration ast-grep --pattern 'pluginOptions = { $$$ }' # Also search for any documentation about these options rg -A 5 -B 5 "autorun|delay" --glob "*.md" --glob "*.mjs"Length of output: 3308
Document
autorunanddelayPluginOptions in the READMEThe code defaults to
autorun = trueanddelay = 0(see src/Config/PluginOptions.mjs). These options control whether the consent modal is shown automatically and after how many milliseconds. Please update the project documentation (e.g. README.md) to include:
- Plugin option name and type
- Default value (
autorun:true,delay:0)- Brief description of behavior
Affected locations:
• src/Config/PluginOptions.mjs (constructor defaults)
• README.md (new “Plugin Options” section)🤖 Prompt for AI Agents
In src/CookieConsentWrapper.mjs around lines 279 to 285, the autorun and delay plugin options are used but not documented. Update the README.md to add a new "Plugin Options" section that lists these options with their names, types, default values (autorun: true, delay: 0), and brief descriptions of their behavior. Also verify and ensure the constructor defaults in src/Config/PluginOptions.mjs reflect these values and are clearly commented.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
CHANGELOG.md (2)
9-10: Refine phrasing for clarity in "Changed" entry.The bullet reads "removes a focus"; consider dropping the article for smoother language. For example:
- - Reverted change that removes a focus from the "Settings modal trigger selector" option after the settings modal is closed. + - Reverted change that removes focus from the "Settings modal trigger selector" option after the settings modal is closed.
16-16: Clarify tabindex flow description.The phrase "keep right tabindex flow" is unclear; rephrase to "maintain correct tabindex navigation flow." Suggested diff:
- - Buttons in the settings modal are physically reordered in the DOM if they are ordered visually using CSS flexbox property `order` to keep right tabindex flow. + - Buttons in the settings modal are physically reordered in the DOM if they are ordered visually using CSS flexbox property `order` to maintain correct tabindex navigation flow.
📜 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 (8)
CHANGELOG.md(1 hunks)package.json(1 hunks)src/Config/PluginOptions.mjs(1 hunks)src/CookieConsentWrapper.mjs(5 hunks)src/Ui/ModalTriggerFactory.mjs(0 hunks)src/Ui/ThirdButtonAppender.mjs(2 hunks)webpack.config.dev.mjs(1 hunks)webpack.config.prod.mjs(1 hunks)
💤 Files with no reviewable changes (1)
- src/Ui/ModalTriggerFactory.mjs
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- src/Config/PluginOptions.mjs
- src/Ui/ThirdButtonAppender.mjs
- webpack.config.dev.mjs
- webpack.config.prod.mjs
- src/CookieConsentWrapper.mjs
🧰 Additional context used
🪛 LanguageTool
CHANGELOG.md
[duplication] ~12-~12: Possible typo: you repeated a word.
Context: ... violation of accessibility rules. ### Fixed - Fixed accessibility for third button in the c...
(ENGLISH_WORD_REPEAT_RULE)
|
Pretty solid work! Can we mention other benefits in the introduction section?
|
Added |
- added util function `checkVisibility` - stylesheets are loaded sooner in the `CookieConsentWrapper.init` function - focusable elements are now filtered using `checkVisibility()` util instead of custom data attributes - focusing on the first accessible element in modals is now done via event listener on the event `transitionend` instead of timeout with fixed duration
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Ui/Utils.mjs (2)
8-19: Early-out when element is detached from DOMIf
element.isConnectedisfalse, all style queries will reflect the default user-agent sheet, yet the node is effectively invisible. Bail before the loop for a micro-optimisation:- let el = element; + if (!element.isConnected) { + return false; + } + + let el = element;
8-19: Loop termination condition is unboundedThe
while (el)loop climbs pastdocument.documentElementand ends only whenparentElementbecomesnull. Although harmless, explicitly stopping atdocument.documentElementsaves one unnecessary iteration and is more expressive.- while (el) { + while (el && el !== document.documentElement) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
dist/cookie-consent.min.jsis excluded by!**/dist/**,!**/*.min.jspackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (5)
package.json(2 hunks)src/CookieConsentWrapper.mjs(6 hunks)src/Ui/Utils.mjs(1 hunks)webpack.config.dev.mjs(1 hunks)webpack.config.prod.mjs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- package.json
- webpack.config.dev.mjs
- webpack.config.prod.mjs
- src/CookieConsentWrapper.mjs
2 participants
Summary by CodeRabbit
Bug Fixes
New Features
Chores
Documentation