fix isDisplayed overflow-x/y bug #12856

louisremi · 2024-05-10T15:40:35Z

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

Polish (an improvement for an existing feature)
Bugfix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update (improvements to the project's docs)
Specification changes (updates to WebDriver command specifications)
Internal updates (everything related to internal scripts, governance documentation and CI files)

Checklist

I have read the CONTRIBUTING doc
I have added tests that prove my fix is effective or that my feature works
~~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 the v8 branch.)

This change is solely for v9 and doesn't need to be back-ported
Back-ported PR at #XXXXX

Further comments

This PR is a Work In Progress:

Add more tests for opacity: 0, visibility: hidden and display: none to the test suite
Decide whether to keep fixture inside test file as Lit component, or to add them to guinea-pig
Backport to v8

Reviewers: @webdriverio/project-committers

linux-foundation-easycla · 2024-05-10T15:40:41Z

The committers listed above are authorized under a signed CLA.

✅ login: louisremi / name: Louis-Rémi Babé (9f77bbf, 7e0f12e, d550988, 5e84abf)

christian-bromann

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.

christian-bromann · 2024-05-10T15:57:13Z

e2e/browser-runner/__fixtures__/is-displayed.html

@@ -0,0 +1,36 @@
+<head>


I don't think this fixture is needed if you use this already in the test directly

Alright, I removed that file

christian-bromann · 2024-05-10T15:57:34Z

e2e/browser-runner/commands/element/isDisplayed.test.ts

+
+describe('isDisplayed', () => {
+    beforeEach(async () => {
+        // TODO: this fails with "Failed to load test page […]" error


can you be more specific on the error?

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

christian-bromann · 2024-05-10T15:57:57Z

e2e/browser-runner/commands/element/isDisplayed.test.ts

+    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')


This is a component tests so switching pages via url is not possible.

christian-bromann · 2024-05-10T15:58:22Z

e2e/package.json

@@ -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",


no need for coverage checks on this one since we don't test a custom component

I got rid of this line

christian-bromann · 2024-05-10T15:59:12Z

packages/webdriverio/src/scripts/isElementDisplayed.ts

+    if (!elementSubtreeHasNonZeroDimension({ element, dimension: 'x' })) {
        return false
    }

-    if (isElementSubtreeHiddenByOverflow(element) && !elementHasBoundingBox(element)) {
+    if (!elementSubtreeHasNonZeroDimension({ element, dimension: 'y' })) {


Does this also check when only overflow is being used?

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

erwinheitzman

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 🙂

erwinheitzman · 2024-05-21T09:10:03Z

e2e/browser-runner/commands/element/isDisplayed.test.ts

@@ -0,0 +1,108 @@
+// @ts-expect-error resolved by vite
+import { expect, $ } from '@wdio/globals'
+// import { screen } from '@testing-library/jest-dom'


Comments can be removed

erwinheitzman · 2024-05-21T10:30:11Z

packages/webdriverio/src/scripts/isElementDisplayed.ts

@@ -114,25 +114,38 @@ export default function isElementDisplayed (element: Element): boolean {
        return cascadedStylePropertyForElement(parentElement, property)
    }

-    function elementHasBoundingBox(element: Element): boolean {
+    function elementSubtreeHasNonZeroDimension({


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

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

erwinheitzman · 2024-05-21T10:31:17Z

packages/webdriverio/src/scripts/isElementDisplayed.ts

-            return !!strokeWidth && (parseInt(strokeWidth, 10) > 0)
+        if (
+            element.tagName.toUpperCase() === 'PATH' &&
+            (boundingBox.width > 0 || boundingBox.height > 0)


Shouldn't this be && instead of ||?

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

erwinheitzman · 2024-05-21T10:38:44Z

packages/webdriverio/src/scripts/isElementDisplayed.ts

-        }
-
-        // This element's subtree is hidden by overflow if all child subtrees are as well.
-        return [].every.call(element.childNodes, function (childNode: Element) {


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

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.

louisremi · 2024-05-22T12:24:39Z

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.

@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?

erwinheitzman · 2024-05-22T12:59:50Z

@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

fix isDisplayed overflow-x/y bug

7e0f12e

christian-bromann reviewed May 10, 2024

View reviewed changes

louisremi added 2 commits May 20, 2024 16:49

add tests for other isDisplayed cases

5e84abf

+ chnages following feedback

remove unnecessary modifications to wdio conf and project dependencies

9f77bbf

erwinheitzman reviewed May 21, 2024

View reviewed changes

remove comment and changes to pnpm lock

d550988

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix isDisplayed overflow-x/y bug #12856

fix isDisplayed overflow-x/y bug #12856

louisremi commented May 10, 2024 •

edited

linux-foundation-easycla bot commented May 10, 2024 •

edited

christian-bromann left a comment

christian-bromann May 10, 2024

louisremi May 20, 2024

christian-bromann May 10, 2024

louisremi May 20, 2024

christian-bromann May 10, 2024

christian-bromann May 10, 2024

louisremi May 20, 2024

christian-bromann May 10, 2024

louisremi May 20, 2024

erwinheitzman left a comment •

edited

erwinheitzman May 21, 2024

louisremi May 22, 2024

erwinheitzman May 21, 2024

louisremi May 22, 2024

erwinheitzman May 21, 2024

louisremi May 22, 2024 •

edited

erwinheitzman May 21, 2024

louisremi May 22, 2024

louisremi commented May 22, 2024

erwinheitzman commented May 22, 2024

fix isDisplayed overflow-x/y bug #12856

Are you sure you want to change the base?

fix isDisplayed overflow-x/y bug #12856

Conversation

louisremi commented May 10, 2024 • edited

Proposed changes

Types of changes

Checklist

Backport Request

Further comments

Reviewers: @webdriverio/project-committers

linux-foundation-easycla bot commented May 10, 2024 • edited

christian-bromann left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erwinheitzman left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louisremi May 22, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

louisremi commented May 22, 2024

erwinheitzman commented May 22, 2024

louisremi commented May 10, 2024 •

edited

linux-foundation-easycla bot commented May 10, 2024 •

edited

erwinheitzman left a comment •

edited

louisremi May 22, 2024 •

edited