-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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 isDisplayed overflow-x/y bug #12856
base: main
Are you sure you want to change the base?
fix isDisplayed overflow-x/y bug #12856
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a stab at this. I have one question on the code changes. Regarding testing, you currently use the component test runner for this which is fine. An alternative is to make a PR to https://github.com/webdriverio/guinea-pig which is a simple website accessible at http://guinea-pig.webdriver.io/. This can be used to then to just keep this as e2e test, use the url
command instead of render
and move the test somewhere e2e/wdio/headless/test.e2e.ts
.
@@ -0,0 +1,36 @@ | |||
<head> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this fixture is needed if you use this already in the test directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I removed that file
|
||
describe('isDisplayed', () => { | ||
beforeEach(async () => { | ||
// TODO: this fails with "Failed to load test page […]" error |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you be more specific on the error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer a problem, I got rid of the fixture file. I render a document with Lit for now, and I can submit a PR to guinea pig if you prefer
beforeEach(async () => { | ||
// TODO: this fails with "Failed to load test page […]" error | ||
// while the browser actually appears able to load the page | ||
// await browser.url('http://localhost:8080/is-displayed.html') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a component tests so switching pages via url
is not possible.
e2e/package.json
Outdated
@@ -5,6 +5,9 @@ | |||
"private": true, | |||
"scripts": { | |||
"test:browser": "run-s test:browser:*", | |||
"test:browser:isDisplayed": "run-s test:browser:isDisplayed:*", | |||
"test:browser:isDisplayed:wdio": "cross-env WDIO_PRESET=isDisplayed node ../packages/wdio-cli/bin/wdio.js ./browser-runner/wdio.conf.js --spec isDisplayed.test.ts", | |||
"test:browser:isDisplayed:verifyCoverage": "run-s verifyCoverage", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need for coverage checks on this one since we don't test a custom component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got rid of this line
if (!elementSubtreeHasNonZeroDimension({ element, dimension: 'x' })) { | ||
return false | ||
} | ||
|
||
if (isElementSubtreeHiddenByOverflow(element) && !elementHasBoundingBox(element)) { | ||
if (!elementSubtreeHasNonZeroDimension({ element, dimension: 'y' })) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this also check when only overflow
is being used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, there are various tests in the test-suite I created that check the result of only using overflow
instead of overflow-x/y
+ chnages following feedback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although I like the changes to support the mentioned scenario, I do not feel confident in merging this (yet). Especially since changing this code potentially has A LOT of impact on our users, so getting it right is very important. I would like to run this on a big codebase first before merging this. Thank you very much for looking into this and contributing, great find overall 🙂
@@ -0,0 +1,108 @@ | |||
// @ts-expect-error resolved by vite | |||
import { expect, $ } from '@wdio/globals' | |||
// import { screen } from '@testing-library/jest-dom' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -114,25 +114,38 @@ export default function isElementDisplayed (element: Element): boolean { | |||
return cascadedStylePropertyForElement(parentElement, property) | |||
} | |||
|
|||
function elementHasBoundingBox(element: Element): boolean { | |||
function elementSubtreeHasNonZeroDimension({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's better to check for Zero dimensions than for non Zero dimensions for the function. Names like these are confusing especially when executed like so !nonZero
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff makes it seem that this is new code, while it isn't: this is an existing function that I modified to check dimensions one by one instead.
The function is used recursively. I agree that at the top level it would be better to write elementSubtreeHasZeroDimension()" instead of "!elementSubtreeHasNonZeroDimension()
. But in the recursive algorithm, the original name makes more sense. We could write a new const elementSubtreeHasZeroDimension = (...args) => !elemelementSubtreeHasNonZeroDimension(...args)
helper, but I feel that would be a bit overkill
return !!strokeWidth && (parseInt(strokeWidth, 10) > 0) | ||
if ( | ||
element.tagName.toUpperCase() === 'PATH' && | ||
(boundingBox.width > 0 || boundingBox.height > 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be && instead of ||?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This replaces boundingBox.width + boundingBox.height > 0
for clarity, so ||
is appropriate. The path will be considered displayed if just one of the dimension is > 0 and the stroke width is > 0
} | ||
|
||
// This element's subtree is hidden by overflow if all child subtrees are as well. | ||
return [].every.call(element.childNodes, function (childNode: Element) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We lose this functionality by removing this, the purpose of this is to check if any child elements of an element are not displayed. We need to keep this unless I missed something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a careful analysis, it seemed that this test isElementSubtreeHiddenByOverflow
was redundant with the test elementSubtreeHasNonZeroDimensions
.
I'm not sure why those two different tests existed in the first place, but isElementSubtreeHiddenByOverflow
seemed like an inferior version (not testing for path with > 0 stroke, weird recursive implementation). So I got rid of it.
But I agree this change should be carefully tested on a large code-base before being validated.
@erwinheitzman What would be your plan to test this change on a large codebase? We can volunteer testing it on our codebase at BedrockStreaming. This is where the regression in 8.2.22 was detected. We run a video streaming platform used by millions of users across Europe, on websites such as https://www.videoland.com/nl/ and https://www.6play.fr/ . And isn't there a plan to have release candidates for v9 to test potential regressions? |
@louisremi I would really appreciate it if you could do that but I do not expect you to go that far (perhaps nice to make sure the fix works on your end though). My plan is to run this against all the tests of the client I currently work for. It's scale is huge as well and it should give us a good indication of any impact on existing testcases. I do need to find time for it though |
Proposed changes
Fix regression inside isDisplay introduced with version 8.2.22: Webdriverio now incorrectly reports elements with style
overflow-x: hidden; width: 0
as displayed.This fixes #12781
Types of changes
Checklist
I have added the necessary documentation (if appropriate)I have added proper type definitions for new commands (if appropriate)Backport Request
[//]: # (The current
main
branch is the development branch for WebdriverIO v9. If your change should be released to the current major version of WebdriverIO (v8), please raise another PR with the same changes against thev8
branch.)v9
and doesn't need to be back-ported#XXXXX
Further comments
This PR is a Work In Progress:
opacity: 0
,visibility: hidden
anddisplay: none
to the test suiteReviewers: @webdriverio/project-committers