Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: #29093 for elements with length in rem #29224

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
94 commits
Select commit Hold shift + click to select a range
8b1182f
first working version
senpl Mar 29, 2024
da6825f
clean commented code
senpl Mar 29, 2024
f4278d0
clean
senpl Mar 29, 2024
7d08741
changelog update
senpl Mar 29, 2024
cc86762
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
senpl Mar 29, 2024
cf5353a
Merge branch 'issue-29093-fix-Visible-elements-are-"not-visible"-for-…
senpl Mar 29, 2024
c2e4f99
Merge pull request #2 from senpl/#29093-fix
senpl Mar 29, 2024
bd82c5d
fix pipeline bug
senpl Mar 29, 2024
b5c97d7
Merge pull request #3 from senpl/#29093-fix
senpl Mar 29, 2024
bbeb1bb
fix pipeline for line2
senpl Mar 29, 2024
6d48369
Merge pull request #4 from senpl/#29093-fix
senpl Mar 29, 2024
71cdb93
fix pipeline3
senpl Mar 29, 2024
f85f307
Merge pull request #5 from senpl/#29093-fix
senpl Mar 29, 2024
b15254c
error reproduction
senpl Apr 2, 2024
0e52c90
go back to previous commend for tests
senpl Apr 2, 2024
55ac75f
Merge pull request #6 from senpl/#29093-fix
senpl Apr 2, 2024
8ef291b
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
jennifer-shehane Apr 3, 2024
47d2948
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
jennifer-shehane Apr 11, 2024
9d84c25
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
jennifer-shehane Apr 11, 2024
2a4d554
fix changelog
jennifer-shehane Apr 11, 2024
347ca9e
fix for test that catched not correct span
senpl Apr 12, 2024
5d57f92
fix for visibility compatible backward
senpl Apr 14, 2024
4ad8136
tests pass without this check so probably not needed
senpl Apr 14, 2024
f484c50
only cleanup to take less on screen and be more visible
senpl Apr 14, 2024
7912583
Merge pull request #8 from senpl/#29093-fix
senpl Apr 14, 2024
0fcc3ea
fix for 28638
senpl Apr 14, 2024
537176c
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
senpl Apr 14, 2024
d13e552
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
senpl Apr 16, 2024
044d3ec
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
jennifer-shehane Apr 16, 2024
2d85cb3
adding id to make easier to find searched element
senpl Apr 17, 2024
2842d5e
visibility tests pass
senpl Apr 17, 2024
1d7d9ab
final fix for 29093
senpl Apr 17, 2024
023646b
general cleanup and comments update
senpl Apr 17, 2024
8dc2777
Merge pull request #10 from senpl/#29093-fix
senpl Apr 17, 2024
92629ad
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
senpl Apr 18, 2024
89dedb4
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
jennifer-shehane Apr 18, 2024
61749d7
not using jquery for this check
senpl Apr 19, 2024
03f07cb
Merge pull request #11 from senpl/#29093-fix
senpl Apr 19, 2024
ec3f9ab
another potential speed improvement
senpl Apr 19, 2024
1d9a335
Merge pull request #12 from senpl/#29093-fix
senpl Apr 19, 2024
5c57aed
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
jennifer-shehane Apr 19, 2024
5ac6fe5
Update cli/CHANGELOG.md
jennifer-shehane Apr 19, 2024
f8d4b3e
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
senpl Apr 19, 2024
ad39a62
speed improvements for ci
senpl Apr 20, 2024
8371a99
Merge pull request #13 from senpl/#29093-fix
senpl Apr 20, 2024
82a4058
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
jennifer-shehane Apr 22, 2024
bc2f850
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
senpl Apr 23, 2024
2a423cb
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
jennifer-shehane Apr 23, 2024
9f288ad
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
senpl Apr 24, 2024
4612dc1
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
jennifer-shehane Apr 25, 2024
606ca75
Update changelog entry
jennifer-shehane Apr 25, 2024
7aac0f9
add pending to changelog
jennifer-shehane Apr 25, 2024
be84785
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
senpl May 1, 2024
aa659ee
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
jennifer-shehane May 2, 2024
050ff5f
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
jennifer-shehane May 2, 2024
35022fc
filerow test fix
senpl May 7, 2024
f3b4081
make width and haight visible in fleaky test
senpl May 7, 2024
c13d19c
Merge pull request #16 from senpl/#29093-fix
senpl May 7, 2024
fc85dd5
mocha added to launchpad package
senpl May 8, 2024
d423dfc
Merge pull request #17 from senpl/#29093-fix
senpl May 8, 2024
6073f20
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
senpl May 8, 2024
8e49f91
Update CHANGELOG.md
senpl May 8, 2024
07b50b5
Update CHANGELOG.md
senpl May 8, 2024
0ada697
Update CHANGELOG.md
senpl May 8, 2024
85dbb8a
Update CHANGELOG.md
senpl May 8, 2024
f94e28e
make pipeline view visible
senpl May 9, 2024
a1133a2
Merge pull request #18 from senpl/#29093-fix
senpl May 9, 2024
47dfcf2
out of bound fix
senpl May 9, 2024
a5ad454
Merge pull request #19 from senpl/#29093-fix
senpl May 9, 2024
b281e84
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
jennifer-shehane May 10, 2024
461e6e7
Remove old changelog entry
jennifer-shehane May 10, 2024
46f5105
another changelog fix
jennifer-shehane May 10, 2024
c670a8e
fix for existing test
senpl May 13, 2024
03bc284
Merge pull request #20 from senpl/#29093-fix
senpl May 13, 2024
0d0d984
kitchensink fix
senpl May 13, 2024
e791a7b
Merge pull request #21 from senpl/#29093-fix
senpl May 13, 2024
eb598ee
go back to previously working
senpl May 13, 2024
52be351
Merge pull request #22 from senpl/#29093-fix
senpl May 13, 2024
7082a6b
pipeline speed improvement
senpl May 13, 2024
04efcf5
Merge pull request #23 from senpl/#29093-fix
senpl May 13, 2024
5a279d8
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
senpl May 15, 2024
95c624c
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
senpl May 16, 2024
093c8e0
id for precise locator gind
senpl May 22, 2024
98c56de
Merge pull request #24 from senpl/#29093-fix
senpl May 22, 2024
09fe08f
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
senpl May 22, 2024
53c7242
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
jennifer-shehane Jun 3, 2024
3862c48
removed unussed id
senpl Jun 4, 2024
bf76534
display contents fix
senpl Jun 4, 2024
bcea9bd
additional tests
senpl Jun 4, 2024
cad469d
move to correct place in canClip
senpl Jun 4, 2024
58f2e9d
28638 in correct place
senpl Jun 4, 2024
7cf025a
readability and performance improvements
senpl Jun 4, 2024
d6ad12d
Merge pull request #25 from senpl/#29093-fix
senpl Jun 4, 2024
9561099
Merge branch 'develop' into issue-29093-fix-Visible-elements-are-"not…
senpl Jun 4, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions cli/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ _Released 6/4/2024 (PENDING)_

**Bugfixes:**

- Fixed an issue where Cypress did not detect visible elements with width or height in rem as visible. Fixes [#29224](https://github.com/cypress-io/cypress/issues/29093) and [#28638](https://github.com/cypress-io/cypress/issues/28638).
- Fixed a situation where the Launchpad would hang if the project config had not been loaded when the Launchpad first queries the current project. Fixes [#29486](https://github.com/cypress-io/cypress/issues/29486).
- Pre-emptively fix behavior with Chrome for when `unload` events are forcefully deprecated by using `pagehide` as a proxy. Fixes [#29241](https://github.com/cypress-io/cypress/issues/29241).

Expand Down
22 changes: 17 additions & 5 deletions packages/driver/cypress/e2e/dom/visibility.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -247,7 +247,14 @@ describe('src/cypress/dom/visibility', () => {
this.$parentNoWidth = add(`\
<div style='width: 0; height: 100px; overflow: hidden;'>
<div style='height: 500px; width: 500px;'>
<span>parent width: 0</span>
<span id='parentNoWidth' >parent width: 0</span>
cacieprins marked this conversation as resolved.
Show resolved Hide resolved
</div>
</div>`)

this.$parentOfDivNoWidth = add(`\
<div style='width: 0; height: 100px; overflow: hidden;'>
<div style='height: 500px; width: 500px;'>
<div id='parentOfDivNoWidth' >parent width: 0</div>
</div>
</div>`)

Expand Down Expand Up @@ -368,7 +375,7 @@ describe('src/cypress/dom/visibility', () => {

this.$elOutOfParentBoundsAbove = add(`\
<div style='width: 100px; height: 100px; overflow: hidden; position: relative;'>
<span style='position: absolute; width: 100px; height: 100px; left: 0px; top: -100px;'>position: absolute, out of bounds above</span>
<span id='outOfParentBoundsAbove' style='position: absolute; width: 100px; height: 100px; left: 0px; top: -100px;'>position: absolute, out of bounds above</span>
</div>\
`)

Expand Down Expand Up @@ -733,8 +740,13 @@ describe('src/cypress/dom/visibility', () => {
})

it('is hidden if parent has overflow: hidden and no width', function () {
expect(this.$parentNoWidth.find('span')).to.be.hidden
expect(this.$parentNoWidth.find('span')).to.not.be.visible
expect(this.$parentNoWidth.find('#parentNoWidth')).to.be.hidden
expect(this.$parentNoWidth.find('#parentNoWidth')).to.not.be.visible
})

it('is hidden if parent has overflow: hidden and no width', function () {
expect(this.$parentOfDivNoWidth.find('#parentOfDivNoWidth')).to.be.hidden
expect(this.$parentOfDivNoWidth.find('#parentOfDivNoWidth')).to.not.be.visible
})

it('is hidden if parent has overflow: hidden and no height', function () {
Expand Down Expand Up @@ -833,7 +845,7 @@ describe('src/cypress/dom/visibility', () => {
})

it('is hidden when parent overflow hidden and out of bounds above', function () {
expect(this.$elOutOfParentBoundsAbove.find('span')).to.be.hidden
expect(this.$elOutOfParentBoundsAbove.find('#outOfParentBoundsAbove'), `expected '<span>' to be 'hidden'`).to.be.hidden
})

it('is hidden when parent overflow hidden and out of bounds below', function () {
Expand Down
12 changes: 12 additions & 0 deletions packages/driver/cypress/e2e/issues/29093.cy.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
// https://github.com/cypress-io/cypress/issues/29093
describe('issue 29093', () => {
before(() => {
cy
.viewport('macbook-16')
.visit('/fixtures/issue-29093.html')
})

it('can click selection when rem width used', () => {
cy.get('#sidebar-left > section').click()
})
})
37 changes: 37 additions & 0 deletions packages/driver/cypress/fixtures/issue-29093.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
<!DOCTYPE html>
<html lang="en" class="dark">

<head>
<meta charset="utf-8" />
<title>29093 repro</title>
<style id="compiled-css" type="text/css">
.big {
font-size: 128px;
}

.contents {
display: contents
Copy link
Contributor

Choose a reason for hiding this comment

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

This is what is causing Cypress to not properly determine that #sidebar-left is not hidden in the fixture. display: contents causes the element to not generate its own box, which prevents overflow:hidden from being applied as expected - the element is effectively replaced by its children, and its box model (and overflow settings) should be disregarded. See: https://drafts.csswg.org/css-display/#valdef-display-contents.

Copy link
Author

Choose a reason for hiding this comment

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

It's true. Check moved to canClip.

}

.overflow-hidden {
overflow: hidden
}

@media (min-width: 1024px) {
.lg\:grid {
display: grid
}
}
</style>
</head>

<body>
<div class="overflow-hidden contents">
<aside id="sidebar-left" class="lg:w-auto">
<section class="b-4">
<p class="big">IN SECTION</p>
</section>
</aside>
</div>
</body>
</html>
108 changes: 72 additions & 36 deletions packages/driver/src/dom/visibility.ts
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ const isStrictlyHidden = (el, methodName = 'isStrictlyHidden()', options = { che
}

// in Cypress-land we consider the element hidden if
// either its offsetHeight or offsetWidth is 0 because
// either its clientHeight or clientWidth is 0 because
// it is impossible for the user to interact with this element
if (elHasNoEffectiveWidthOrHeight($el)) {
// https://github.com/cypress-io/cypress/issues/6183
Expand Down Expand Up @@ -121,32 +121,38 @@ const elHasNoEffectiveWidthOrHeight = ($el) => {
// Is the element's CSS width OR height, including any borders,
// padding, and vertical scrollbars (if rendered) less than 0?
//
// elOffsetWidth:
// elClientWidth:
// If the element is hidden (for example, by setting style.display
// on the element or one of its ancestors to "none"), then 0 is returned.

// $el[0].getClientRects().length:
// For HTML <area> elements, SVG elements that do not render anything themselves,
// display:none elements, and generally any elements that are not directly rendered,
// an empty list is returned.

const el = $el[0]
const style = getComputedStyle(el)
const transform = style.getPropertyValue('transform')
const width = elOffsetWidth($el)
const height = elOffsetHeight($el)
const overflowHidden = elHasOverflowHidden($el)
let transform

if ($el[0].style.transform) {
const style = getComputedStyle(el)

transform = style.getPropertyValue('transform')
} else {
transform = 'none'
}

const width = elClientWidth($el)
const height = elClientHeight($el)

return isZeroLengthAndTransformNone(width, height, transform) ||
isZeroLengthAndOverflowHidden(width, height, overflowHidden) ||
isZeroLengthAndOverflowHidden(width, height, elHasOverflowHidden($el)) ||
(el.getClientRects().length <= 0)
}

const isZeroLengthAndTransformNone = (width, height, transform) => {
// From https://github.com/cypress-io/cypress/issues/5974,
// we learned that when an element has non-'none' transform style value like "translate(0, 0)",
// it is visible even with `height: 0` or `width: 0`.
// That's why we're checking `transform === 'none'` together with elOffsetWidth/Height.
// That's why we're checking `transform === 'none'` together with elClientWidth/Height.

return (width <= 0 && transform === 'none') ||
(height <= 0 && transform === 'none')
Expand All @@ -157,22 +163,28 @@ const isZeroLengthAndOverflowHidden = (width, height, overflowHidden) => {
(height <= 0 && overflowHidden)
}

const elHasNoOffsetWidthOrHeight = ($el) => {
return (elOffsetWidth($el) <= 0) || (elOffsetHeight($el) <= 0)
const elHasNoClientWidthOrHeight = ($el) => {
return (elClientWidth($el) <= 0) || (elClientHeight($el) <= 0)
}

const elOffsetWidth = ($el) => {
return $el[0].offsetWidth
const elementBoundingRect = ($el) => $el[0].getBoundingClientRect()

const elClientHeight = ($el) => {
return elementBoundingRect($el).height
}

const elOffsetHeight = ($el) => {
return $el[0].offsetHeight
const elClientWidth = ($el) => {
return elementBoundingRect($el).width
}

const elHasVisibilityHiddenOrCollapse = ($el) => {
return elHasVisibilityHidden($el) || elHasVisibilityCollapse($el)
}

const elHasVisibilityVisible = ($el) => {
return $el.css('visibility') === 'visible'
}

const elHasVisibilityHidden = ($el) => {
return $el.css('visibility') === 'hidden'
}
Expand All @@ -189,6 +201,10 @@ const elHasDisplayNone = ($el) => {
return $el.css('display') === 'none'
}

const elHasDisplayContents = ($el) => {
return $el.css('display') === 'contents'
}

const elHasDisplayInline = ($el) => {
return $el.css('display') === 'inline'
}
Expand Down Expand Up @@ -219,13 +235,24 @@ const canClipContent = function ($el, $ancestor) {
return false
}

// fix for 29093
if (elHasDisplayContents($ancestor)) {
return false
}

// the closest parent with position relative, absolute, or fixed
const $offsetParent = $el.offsetParent()
const isClosestAncsestor = isAncestor($ancestor, $offsetParent)

// fix for 28638 - when element postion is relative and it's parent absolute
if (elHasPositionRelative($el) && isClosestAncsestor && elHasPositionAbsolute($ancestor)) {
return false
}

// even if ancestors' overflow is clippable, if the element's offset parent
// is a parent of the ancestor, the ancestor will not clip the element
// unless the element is position relative
if (!elHasPositionRelative($el) && isAncestor($ancestor, $offsetParent)) {
if (!elHasPositionRelative($el) && isClosestAncsestor) {
return false
}

Expand Down Expand Up @@ -308,35 +335,40 @@ const elIsNotElementFromPoint = function ($el) {
return true
}

const elIsOutOfBoundsOfAncestorsOverflow = function ($el, $ancestor = getParent($el)) {
const elIsOutOfBoundsOfAncestorsOverflow = function ($el: JQuery<any>, $ancestor = getParent($el)) {
// no ancestor, not out of bounds!
// if we've reached the top parent, which is not a normal DOM el
// then we're in bounds all the way up, return false
if (isUndefinedOrHTMLBodyDoc($ancestor)) {
return false
}

if (elHasPositionRelative($el) && elHasPositionAbsolute($ancestor)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Relative elements can be clipped by absolute ancestors that have height or width of 0 and overflow:hidden. canClipContent (L#230) checks for various edge cases with offset parents before comparing the dimensions of an element and their ancestor to determine if the element is actually obscured by clipping.

Copy link
Author

Choose a reason for hiding this comment

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

moved to canClipContent. This is just first working fix. Assumption is: when El has position relative and it's ancestor Absolute than this can not be clipped. But if this assumption is not exactly correct then more complicated check could be done. It's just better then existing version for this case.

return false
}

if (canClipContent($el, $ancestor)) {
const elProps = $coordinates.getElementPositioning($el)
const ancestorProps = $coordinates.getElementPositioning($ancestor)
const ancestorProps = $ancestor.get(0).getBoundingClientRect()

if (elHasPositionAbsolute($el) && (ancestorProps.width === 0 || ancestorProps.height === 0)) {
return elIsOutOfBoundsOfAncestorsOverflow($el, getParent($ancestor))
}

const elProps = $el.get(0).getBoundingClientRect()

// target el is out of bounds
if (
// target el is to the right of the ancestor's visible area
(elProps.fromElWindow.left >= (ancestorProps.width + ancestorProps.fromElWindow.left)) ||
(elProps.left >= (ancestorProps.width + ancestorProps.left)) ||

// target el is to the left of the ancestor's visible area
((elProps.fromElWindow.left + elProps.width) <= ancestorProps.fromElWindow.left) ||
((elProps.left + elProps.width) <= ancestorProps.left) ||

// target el is under the ancestor's visible area
(elProps.fromElWindow.top >= (ancestorProps.height + ancestorProps.fromElWindow.top)) ||
(elProps.top >= (ancestorProps.height + ancestorProps.top)) ||

// target el is above the ancestor's visible area
((elProps.fromElWindow.top + elProps.height) <= ancestorProps.fromElWindow.top)
((elProps.top + elProps.height) <= ancestorProps.top)
) {
return true
}
Expand All @@ -348,7 +380,7 @@ const elIsOutOfBoundsOfAncestorsOverflow = function ($el, $ancestor = getParent(
const elIsHiddenByAncestors = function ($el, checkOpacity, $origEl = $el) {
// walk up to each parent until we reach the body
// if any parent has opacity: 0
// or has an effective offsetHeight of 0
// or has an effective clientHeight of 0
// and its set overflow: hidden then our child element
// is effectively hidden
// -----UNLESS------
Expand Down Expand Up @@ -376,11 +408,15 @@ const elIsHiddenByAncestors = function ($el, checkOpacity, $origEl = $el) {
return !elDescendentsHavePositionFixedOrAbsolute($parent, $origEl)
}

if (elHasVisibilityVisible($parent)) {
return false
}

// continue to recursively walk up the chain until we reach body or html
return elIsHiddenByAncestors($parent, checkOpacity, $origEl)
}

const parentHasNoOffsetWidthOrHeightAndOverflowHidden = function ($el) {
const parentHasNoClientWidthOrHeightAndOverflowHidden = function ($el) {
// if we've walked all the way up to body or html then return false
if (isUndefinedOrHTMLBodyDoc($el)) {
return false
Expand All @@ -392,7 +428,7 @@ const parentHasNoOffsetWidthOrHeightAndOverflowHidden = function ($el) {
}

// continue walking
return parentHasNoOffsetWidthOrHeightAndOverflowHidden(getParent($el))
return parentHasNoClientWidthOrHeightAndOverflowHidden(getParent($el))
}

const parentHasDisplayNone = function ($el) {
Expand Down Expand Up @@ -463,8 +499,8 @@ export const getReasonIsHidden = function ($el, options = { checkOpacity: true }
// either being covered or there is no el

const node = stringifyElement($el, 'short')
let width = elOffsetWidth($el)
let height = elOffsetHeight($el)
let width = elClientWidth($el)
let height = elClientHeight($el)
let $parent
let parentNode

Expand Down Expand Up @@ -513,24 +549,24 @@ export const getReasonIsHidden = function ($el, options = { checkOpacity: true }
return `This element \`${node}\` is not visible because its parent \`${parentNode}\` has CSS property: \`opacity: 0\``
}

if (elHasNoOffsetWidthOrHeight($el)) {
return `This element \`${node}\` is not visible because it has an effective width and height of: \`${width} x ${height}\` pixels.`
}

const transformResult = $transform.detectVisibility($el)

if (transformResult === 'transformed') {
return `This element \`${node}\` is not visible because it is hidden by transform.`
}

if (elHasNoClientWidthOrHeight($el)) {
return `This element \`${node}\` is not visible because it has an effective width and height of: \`${width} x ${height}\` pixels.`
}

if (transformResult === 'backface') {
return `This element \`${node}\` is not visible because it is rotated and its backface is hidden.`
}

if ($parent = parentHasNoOffsetWidthOrHeightAndOverflowHidden(getParent($el))) {
if ($parent = parentHasNoClientWidthOrHeightAndOverflowHidden(getParent($el))) {
parentNode = stringifyElement($parent, 'short')
width = elOffsetWidth($parent)
height = elOffsetHeight($parent)
width = elClientWidth($parent)
height = elClientHeight($parent)

return `This element \`${node}\` is not visible because its parent \`${parentNode}\` has CSS property: \`overflow: hidden\` and an effective width and height of: \`${width} x ${height}\` pixels.`
}
Expand Down
1 change: 1 addition & 0 deletions packages/launchpad/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
"gravatar": "1.8.0",
"javascript-time-ago": "2.3.8",
"markdown-it": "13.0.1",
"mocha": "10.4.0",
"rollup-plugin-polyfill-node": "^0.7.0",
"type-fest": "^2.3.4",
"unplugin-vue-components": "0.24.1",
Expand Down
4 changes: 2 additions & 2 deletions packages/launchpad/src/components/code/FileRow.cy.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -117,10 +117,10 @@ describe('FileRow', () => {
cy.contains(changesRequiredDescription).should('be.visible')
cy.get('pre').should('have.length', 2)

cy.get('.shiki').should('be.visible')
cy.get('div.rounded > div.text-left.cursor-text').should('be.visible')
Copy link
Member

Choose a reason for hiding this comment

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

@senpl Could you explain the need for this change? If users are required to update their tests based on this change, this would be considered a breaking change.

Copy link
Author

Choose a reason for hiding this comment

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

.shiki is not precise selector. In moment of search in this test there are two elements with this locator. One berly visible but still possible to locate.
This selector better explain what test intended to search.

Copy link
Author

Choose a reason for hiding this comment

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

It might be breaking change for poorly created locators. Still it's closer to what user can actually see in browser. So it's probably good change. If I could locate element in browser then it's visible, but there should be some rules to determine if something is visible or not.

Copy link
Contributor

@cacieprins cacieprins May 28, 2024

Choose a reason for hiding this comment

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

Hi @senpl ,

There are two .shiki elements in this component. One of them is always hidden, and one of them that should show/hide on click. The hidden state for this element is caused by a parent element with maxHeight of 0, and overflow:hidden.

I do agree that the .shiki selector here should be more specific. I edited it to assert the expected visibility state of both elements:

    // A FileRow with status=valid should not be initially open 
    cy.get('[data-cy=valid] .shiki').should('not.be.visible')
    // A FileRow with status=changes should be initially open
    cy.get('[data-cy=changes] .shiki').should('be.visible')

    // Clicking the header should collapse the shiki code view in the FileRow 
    cy.contains('cypress/integration/command.js').click()

    cy.get('[data-cy=valid] .shiki').should('not.be.visible')
    cy.get('[data-cy=changes] .shiki').should('not.be.visible')

Unfortunately, while this test passes on the develop, it does not pass with the visibility changes in this branch. Can you look into why?

Thank you!

Copy link
Author

Choose a reason for hiding this comment

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

I already did.
But this second locator is still visible. I could click it in browser and get it's locator. It's not big. It could to be very hard to click on it. Still it is visible. I do not see reason for Cypress to mark something as invisible when it is visible. It might produce warning that something is hardly visible, still if it's partly visible it should not expect that element to be not visible.

Copy link
Author

Choose a reason for hiding this comment

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

As in pictures below .shrik width is bigger then element code below in moment of checking. This is why something that is really invisible or not exist should be checked, not something that is visible in moment of click. Alternatively we could wait for this moment to become invisible, but this would make test slower and not as practical.
chrome_rufv04rfEp
chrome_tFds2UDHKr

cy.contains('cypress/integration/command.js').click()

cy.get('.shiki').should('not.be.visible')
cy.get('div.rounded > div.text-left.cursor-text').should('not.exist')
})

it('responds nice to small screens', { viewportWidth: 500 }, () => {
Expand Down