feat: add 'full-native' displayCheck option, utilising Element#checkVisibility
#1818
Conversation
🦋 Changeset detectedLatest commit: 22d0f74 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds a new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Lib as tabbable.isHidden()
participant Browser as Element.checkVisibility
Caller->>Lib: isHidden(el, {displayCheck: "full-native"})
alt Browser supports checkVisibility
Lib->>Browser: el.checkVisibility({disableOpacityChecks:false, contentVisibility:true, visibility:true, cssAlias:true})
Browser-->>Lib: {visible: true/false}
Lib-->>Caller: return !visible
else Browser lacks checkVisibility
Lib->>Lib: run existing manual visibility checks (getComputedStyle, rects, etc.)
Lib-->>Caller: return manual-result
end
Note right of Lib #dfe7ff: 'full-native' is then treated alongside 'full'/'legacy-full' in subsequent logic
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes (Short, factual — and yes, browsers keep inventing new ways to hide buttons.) Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/isFocusable.cy.js (1)
510-557: Consider adding test cases specific tocheckVisibilitycapabilities.The test matrix expansion looks good and properly exercises the new
'full-native'option. However, as you noted in the PR description,checkVisibilitycan detect additional visibility cases likeopacityandcontent-visibilitythat the manual checks might miss.Consider adding a dedicated test suite that verifies these edge cases when
checkVisibilityis available. Something like:// Tests specifically for full-native's checkVisibility behavior describe('full-native with checkVisibility', () => { before(function() { // Skip if checkVisibility not supported if (!Element.prototype.checkVisibility) { this.skip(); } }); it('returns false for elements with opacity: 0', () => { const container = document.createElement('div'); container.innerHTML = '<div data-testid="invisible" tabindex="0" style="opacity: 0"></div>'; document.body.append(container); expect(isFocusable(getByTestId(container, 'invisible'), { displayCheck: 'full-native' })).to.eql(false); }); // Additional cases for content-visibility, filter: opacity(0), etc. });This would demonstrate the value of the native implementation and catch any regressions. Thoughts?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (11)
.changeset/late-tires-remain.md(1 hunks).github/workflows/ci.yml(1 hunks)README.md(2 hunks)index.d.ts(1 hunks)src/index.js(1 hunks)test/e2e/e2e.helpers.js(1 hunks)test/e2e/focusable.cy.js(3 hunks)test/e2e/isFocusable.cy.js(3 hunks)test/e2e/isTabbable.cy.js(3 hunks)test/e2e/shadow-dom.cy.js(2 hunks)test/e2e/tabbable.cy.js(3 hunks)
🔇 Additional comments (19)
.changeset/late-tires-remain.md (1)
1-6: LGTM!Clean changeset entry. Minor bump is appropriate for the new feature, and the description accurately captures the essence of the change.
.github/workflows/ci.yml (1)
54-57: Good documentation of browser version requirements.Clear note for maintainers about the minimum browser versions needed for the test suite. The existing container image (chrome106-ff106) already meets these requirements.
index.d.ts (1)
4-9: LGTM!Type definition cleanly extended to include the new
'full-native'option. Backward compatible since the property remains optional.README.md (3)
159-159: LGTM!Type definition updated to include the new option, with the default correctly documented as
full.
169-172: Clear documentation of the new option.Good description of
full-nativebehavior, with appropriate caveats about cross-browser differences and the fallback strategy. The note about potentially becoming the default in a future major version sets expectations nicely.
173-173: Good clarification.Adding "(default)" here prevents confusion since
full-nativeis listed first in the option list above.src/index.js (1)
402-415: checkVisibility options and compatibility are accurate
The option flags (opacityProperty/visibilityProperty and their historic aliases checkOpacity/checkVisibilityCSS) and corresponding browser support (aliases in Chrome 105+/Firefox 106+ vs. spec names arriving in Chrome 121+/Firefox 122+) match the MDN docs. No changes needed.test/e2e/e2e.helpers.js (1)
3-5: LGTM! Helper rename makes sense.The switch from
setupTestWindowtosetupTestDocumentwithcy.document()aligns with Cypress best practices and simplifies the test setup flow.test/e2e/isTabbable.cy.js (2)
3-3: Setup helper usage updated correctly.Import and usage of
setupTestDocumentalign with the helper rename. Clean transition.Also applies to: 17-19
496-541: Good coverage expansion for 'full-native'.The test matrix now includes 'full-native' alongside existing displayCheck options, ensuring the new native path gets exercised across in-document and detached-container scenarios. The expectations treat 'full-native' the same as 'full' and undefined (default), which makes sense—checkVisibility should match the manual checks for these cases.
test/e2e/shadow-dom.cy.js (1)
8-8: Setup flow updated consistently.The switch to
setupTestDocumentwith separate window retrieval viacy.window()works well for these shadow DOM tests. The reordering makes sense given the new helper API.Also applies to: 18-26
test/e2e/focusable.cy.js (3)
3-3: Setup helper migration complete.Import and usage of
setupTestDocumentconsistent with the broader test refactor.Also applies to: 18-20
28-109: Solid coverage for 'full-native' in basic example tests.The test matrix expansion ensures 'full-native' is tested across in-document and detached-container scenarios. The expectations correctly treat it the same as the default and 'full' options for these fixtures.
114-191: Inertness tests now cover 'full-native'.Extending the displayCheck variants to include 'full-native' in inertness tests ensures the native visibility path handles inert elements correctly. Good thoroughness! 💪
test/e2e/tabbable.cy.js (3)
3-3: Helper migration looks good.Consistent use of
setupTestDocumentfollowing the helper rename pattern.Also applies to: 18-20
28-107: 'full-native' integrated into basic example tests.The displayCheck matrix expansion ensures the new native visibility option is tested thoroughly. Expectations are consistent with existing options, which is what we'd expect for these test cases.
110-187: Inertness coverage extended to 'full-native'.The inertness test suite now exercises all displayCheck options including 'full-native', ensuring the native path correctly handles inert containers and nested scenarios. Nice and thorough! ✨
test/e2e/isFocusable.cy.js (2)
526-553: Test assertions correctly handle the new option.The conditional logic properly accounts for the different
displayCheckbehaviors:
'full-native','full', and default treat elements withdisplay: noneorvisibility: hiddenas unfocusable when in the document'legacy-full'has the quirky behavior of treating everything as focusable when not in the documentThe approach of treating
'full-native'the same as'full'in assertions makes sense—it's a progressive enhancement that falls back gracefully. Nice work keeping the test suite comprehensive without adding unnecessary complexity! 🎯
3-3: Helper rename is consistent across all E2E tests. setupTestWindow has officially retired; every E2E test now imports setupTestDocument.
|
@stefcameron, I opened this PR after discussing in #1800. The GitHub UI does not allow me to assign reviewers on this repository, so sending this ping just in case ✉️ |
|
@fpapado Thank for the PR, saw it! I will review within the next couple of days. |
|
@fpapado Had a look! Looks pretty solid. But the one thing I'm not seeing, or missing seeing, is at least one test that verifies the new I'd like to see one test that shows Is that possible? |
Yeah, these are both doable. I added a test for something that Testing the fallback in end-to-end proved slightly cumbersome, because both Chrome and Firefox in the existing CI docker image support Anyway, to work around that, I tested the fallback in unit tests, since JSDOM in this case conveniently does not support |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
README.md (1)
172-174: Consider clarifying the fallback behavior.The documentation mentions the fallback occurs "when Element#checkVisibility is not supported" but could be more explicit about what "fallback to full behavior" means in practice. Consider adding: "this strategy falls back to the manual visibility checks used by the 'full' option."
Current wording is clear enough for most users, but the extra clarity might help developers understand what happens under the hood.
- If Element#checkVisibility is not supported, this strategy falls back to the `full` behavior. + If Element#checkVisibility is not supported, this strategy falls back to the manual visibility checks used by the `full` option.test/unit/unit.test.js (1)
152-178: Check API support on the prototype, not the instanceChange the feature‐detection guard to verify the existence of the API on the prototype:
- if (typeof container.checkVisibility !== 'undefined') { + if (typeof Element.prototype.checkVisibility !== 'undefined') { // a tacit assumption in this test is that checkVisibility() is unsupported in JSDOM throw new Error( 'checkVisibility seems to be supported in this version of JSDOM, making the test invalid. Consider changing to an end-to-end test with sufficiently old browsers.' ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (7)
README.md(2 hunks)index.d.ts(1 hunks)src/index.js(2 hunks)test/e2e/focusable.cy.js(3 hunks)test/e2e/tabbable.cy.js(3 hunks)test/fixtures/basic.html(1 hunks)test/unit/unit.test.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- index.d.ts
🧰 Additional context used
🪛 markdownlint-cli2 (0.18.1)
README.md
170-170: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
171-171: Unordered list indentation
Expected: 2; Actual: 4
(MD007, ul-indent)
🔇 Additional comments (9)
README.md (1)
159-159: LGTM! Type signature correctly updated.The displayCheck type now includes 'full-native' alongside the existing options.
src/index.js (2)
401-419: LGTM! Clean implementation with appropriate fallback.The early branch for 'full-native' correctly:
- Checks for checkVisibility availability
- Uses comprehensive options (contentVisibilityAuto, visibilityProperty, checkVisibilityCSS alias)
- Returns the negated result to match isHidden's boolean semantics
- Falls through gracefully when checkVisibility is unavailable
The fallback is properly handled by including 'full-native' in the condition on line 441 (see next comment).
436-442: LGTM! Fallback condition correctly includes 'full-native'.This addresses the critical bug identified in past reviews. When checkVisibility is unavailable, 'full-native' now correctly executes the same manual visibility checks as 'full', ensuring the documented fallback behavior works as intended.
test/fixtures/basic.html (1)
56-65: LGTM! Well-chosen test fixtures for the new option.The new elements appropriately test visibility edge cases:
content-visibility: hiddenelements (which checkVisibility should detect)opacity: 0elements (which the comment correctly notes remain focusable/tabbable)These fixtures align with the conditional expectations in the e2e tests where
contentVisibilityHiddenParent-buttonis filtered out only for 'full-native'.test/unit/unit.test.js (1)
54-57: LGTM! Expected element IDs correctly updated.The new visibility-related element IDs align with the fixtures added in test/fixtures/basic.html.
test/e2e/tabbable.cy.js (2)
28-117: LGTM! Comprehensive test coverage for the new option.The parameterized tests now include 'full-native' alongside existing displayCheck options, with appropriate conditional expectations:
- Line 53-55: Correctly filters out
contentVisibilityHiddenParent-buttonfor 'full-native' since Element.checkVisibility can detect when a parent hascontent-visibility: hidden- Line 52: Keeps
contentVisibilityHidden-buttonin all cases, as the element itself is still in the tab order (only its content is hidden)This addresses one of stefcameron's requests for "a test demonstrating a behavioral difference between 'full-native' and 'full'". 🎯
121-196: LGTM! Inertness tests appropriately expanded.All displayCheck variants (including 'full-native') are now tested with inert containers, ensuring the new option doesn't break inertness detection.
test/e2e/focusable.cy.js (2)
28-119: LGTM! Mirrored test coverage for focusable elements.The test parameterization and conditional expectations mirror the tabbable tests, ensuring consistent behavior across both APIs. The filtering of
contentVisibilityHiddenParent-button(lines 55-57) when displayCheck is 'full-native' demonstrates the same behavioral difference between 'full-native' and 'full' that stefcameron requested.
124-200: LGTM! Complete inertness coverage.The inertness tests cover all displayCheck variants including 'full-native', ensuring the new option properly handles inert containers at various nesting levels.
| <button id="opacityZero-button" style="opacity: 0;">foo</button> | ||
| <div style="opacity: 0;"> | ||
| <button id="opacityZeroParent-button">foo</button> | ||
| </div> |
There was a problem hiding this comment.
@stefcameron, in my original description, I had mentioned opacity: 0 as something that checkVisibility can catch. While it can indeed catch that, I double-checked whether native browser behaviour (both for imperative HTMLElement.focus() and tabbing through with a keyboard) allows focusing/tabbing to an element with opacity: 0. It turns out that it does! Here's a relevant codepen.
CleanShot.2025-10-07.at.21.46.57.mp4
So in the interest of tracking the native focus/tab behaviour, I removed the opacityProperty option from the lines here https://github.com/focus-trap/tabbable/pull/1818/files#diff-bfe9874d239014961b1ae4e89875a6155667db834a410aaaa2ebe3cf89820556R406-R407, and added this test case as documentation.
Ultimately, I can imagine some libraries wanting to guard against developer error and checking opacity (ours at work does that, for example), but for tabbable I think matching the native behaviour is less surprising. The current README advice of using tabIndex (or even inert) to exclude unwanted elements can be used to exclude such invisible elements on demand.
There was a problem hiding this comment.
This makes sense, thank you for the explanation. I'm going to leave this comment unresolved so that it remains visible if I'm ever referring back to this PR wondering why we excluded that setting.
|
@fpapado Just one last small request, if that works with you. Also FF failed in CI: https://github.com/focus-trap/tabbable/actions/runs/18322680365/job/52302880471?pr=1818 |
The FF failure sent me down a small rabbit hole, when it comes to what Firefox considers visible and focusable 🔍 After some bisecting (full instructions in this repository, I used Playwright since I find it much easier to get a specific browser version on demand, compared to juggling docker image builds), it turns out that:
Since the logic is internally consistent, I don't think it is a problem, because |
Thank you for investigating, much appreciated!
Yes, I think that's OK. |
|
Thanks for trying to fix the test earlier. FF is still giving some grief... 😬 |
Hello! I took a small break due to work, but I think I figured it out now in 22d0f74. Turns out that |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
test/e2e/e2e.helpers.js (1)
3-5: Already reviewed and looks good.The switch from
cy.window()tocy.document()was discussed in past comments and approved. No new concerns here.
🧹 Nitpick comments (1)
test/e2e/e2e.helpers.js (1)
44-59: Smart workaround for Firefox version quirks.The helper correctly detects Firefox < 125 to handle the
content-visibility: hiddenbehavior difference, and the comment does a great job explaining why this exists (with a bonus link for the curious). The logic is clean and the use case is clear from the PR context about Firefox CI failures.One tiny optional suggestion: you could add a defensive check like
Cypress?.browser?.namein case the API shape changes, but given this is Cypress-specific test code and working fine in CI, it's probably overkill. 🤷If you're feeling extra defensive (totally optional):
export function isFirefoxLowerThan125() { return ( + Cypress.browser && // NOTE: `browser.name` is lowercase // @see https://docs.cypress.io/api/cypress-api/browser#Syntax Cypress.browser.name === 'firefox' && Cypress.browser.majorVersion < 125 ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (1)
test/e2e/e2e.helpers.js(2 hunks)
This PR closes #1800, by introducing a
'full-native'displayCheckoption. The option is done as a progressive enhancement on top of'full', meaning that ifElement#checkVisibilityis supported, then it is used and its value returned verbatim. Otherwise, we follow the existing codepaths with manual checks, that resemble the browser behavior.I parameterised the tests tests to include this option, and they pass the same as before, which gives me high confidence (very extensive test suite btw 😌).
Open questions
I have some open questions, that would be good to cover.
Tests for new cases
The nice thing about
Element#checkVisibilityis that it supports opacity checks, among other things. I could come up with some small fixtures to show case it. To add to that, maybe you have any ideas from past open issues? I imagine various visibility checks have come up in the past, and the 'full' displayCheck might have fallen short.Should this be the default in a future major?
For a minor version, I think it makes sense to do this additively, as a new option. However, for wider adoption, it might be good to make this the default in a future major. While folks who follow changelogs and want to get the closest native behaviour might discover this option through the docs, my hunch is that the default will drive the largest change.
Benchmarking?
I saw this line in the README:
Which is definitely correct; any advanced check for visibility will need to cause some reflow. Did you have any benchmarks for this, by any chance? We could see how the native version stacks against that (e.g. it might be causing style invalidations, but it's unclear to me if it causes layout reflow, per https://drafts.csswg.org/cssom-view-1/#dom-element-checkvisibility:~:text=Note%3A%20The%20checkVisibility()%20method%20provides%20a%20set%20of%20simple%20checks).
I imagine the native version to be similarly fast or faster, and at least more accurate. But good to check if we have the time 😅
CI / local E2E parity
At the moment, Cypress on CI runs fairly old browsers, and especially a very old version of node (18, when current LTS is 22). Locally, Cypress runs more contemporary browser versions (whatever is installed locally, and browsers make it quite hard to install specific versions). Node LTS is also used in other CI workflows, so there is some mismatch and relevant npm warnings about it. While all the tests pass in this PR, it took a bit of digging with browser versions, which I think could be streamlined (especially as a way to codify browser support!)
My suggestion would be to create a Dockerfile which includes a "approved" set of browsers to run local E2E tests in. Cypress has somewhat spotty support for Docker and ARM64, so to support all contributors we'd need some more work. I can pick this in a follow-up 🎁
PR Checklist
Please leave this checklist in your PR.
typeof document/window !== 'undefined'before using it in code that gets executed on load.npm run changesetlocally to add one, and follow the prompts).Summary by CodeRabbit
New Features
Documentation
Tests
Chores