Merged the VisibilityTrait into ElementTrait.
#386
Conversation
|
""" WalkthroughThis change consolidates all UI element visibility assertion methods from the now-removed Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant ElementTrait
participant Browser (JS)
Tester->>ElementTrait: elementAssertIsVisuallyVisible(selector)
ElementTrait->>Browser (JS): Execute JS to check visibility in viewport (with optional offset)
Browser (JS)-->>ElementTrait: Boolean result (visible or not)
ElementTrait-->>Tester: Pass or throw exception based on result
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (7)
💤 Files with no reviewable changes (3)
🔇 Additional comments (1)
🪧 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 (
|
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (1)
src/ElementTrait.php (1)
84-90: 🛠️ Refactor suggestionAdd strict
booltype-hints for the control flags
$is_exactand$is_invertedare currently untyped.
Becausedeclare(strict_types=1)is enabled, calling this helper with a non-boolean
literal (e.g.0,1,"false") will trigger a TypeError only at
run-time. Adding scalar type-hints makes the contract crystal-clear and
lets static analysers do their job.- protected function elementAssertAttributeWithValue(string $selector, strin 8000 g $attribute, mixed $value, $is_exact, $is_inverted): void { + protected function elementAssertAttributeWithValue( + string $selector, + string $attribute, + mixed $value, + bool $is_exact, + bool $is_inverted + ): void {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (6)
src/ElementTrait.php(2 hunks)src/VisibilityTrait.php(0 hunks)steps.md(1 hunks)tests/behat/bootstrap/FeatureContext.php(0 hunks)tests/behat/features/element.feature(2 hunks)tests/behat/features/visibility.feature(0 hunks)
💤 Files with no reviewable changes (3)
- tests/behat/bootstrap/FeatureContext.php
- tests/behat/features/visibility.feature
- src/VisibilityTrait.php
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
steps.md
523-523: Inline HTML
Element: details
(MD033, no-inline-html)
524-524: Inline HTML
Element: summary
(MD033, no-inline-html)
524-524: Inline HTML
Element: code
(MD033, no-inline-html)
529-529: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
532-532: Inline HTML
Element: details
(MD033, no-inline-html)
533-533: Inline HTML
Element: summary
(MD033, no-inline-html)
533-533: Inline HTML
Element: code
(MD033, no-inline-html)
538-538: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
541-541: Inline HTML
Element: details
(MD033, no-inline-html)
542-542: Inline HTML
Element: summary
(MD033, no-inline-html)
542-542: Inline HTML
Element: code
(MD033, no-inline-html)
547-547: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
550-550: Inline HTML
Element: details
(MD033, no-inline-html)
551-551: Inline HTML
Element: summary
(MD033, no-inline-html)
551-551: Inline HTML
Element: code
(MD033, no-inline-html)
556-556: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
559-559: Inline HTML
Element: details
(MD033, no-inline-html)
560-560: Inline HTML
Element: summary
(MD033, no-inline-html)
560-560: Inline HTML
Element: code
(MD033, no-inline-html)
565-565: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
568-568: Inline HTML
Element: details
(MD033, no-inline-html)
569-569: Inline HTML
Element: summary
(MD033, no-inline-html)
569-569: Inline HTML
Element: code
(MD033, no-inline-html)
574-574: Fenced code blocks should be surrounded by blank lines
null
(MD031, blanks-around-fences)
| public function elementAssertIsNotVisible(string $selector): void { | ||
| $element = $this->getSession()->getPage(); | ||
| $nodes = $element->findAll('css', $selector); | ||
|
|
||
| foreach ($nodes as $node) { | ||
| if ($node->isVisible()) { | ||
| throw new \Exception(sprintf('Element defined by "%s" selector is visible on the page, but should not be.', $selector)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
elementAssertIsNotVisible() should fail when the selector is missing
Currently the step passes when the element is not in the DOM at all.
For parity with “visible” assertions (which require presence), you may
want the negative step to first assert that the selector exists and
then check that all matches are hidden.
If the selector may truly be absent, clarify the behaviour in the PHPDoc
block and Gherkin docs.
I’ll verify how the positive elementAssertIsVisible() behaves when no elements are found, and check the existing PHPDoc for elementAssertIsNotVisible().
🏁 Script executed:
#!/bin/bash
set -e
echo "=== elementAssertIsVisible() ==="
rg -n -A 10 "function elementAssertIsVisible" src/ElementTrait.php || true
echo
echo "=== PHPDoc around elementAssertIsNotVisible() ==="
rg -n -B 3 -A 5 "elementAssertIsNotVisible" src/ElementTrait.php || trueLength of output: 1202
Require presence check in elementAssertIsNotVisible()
To mirror the “is visible” assertion (which fails when no elements are found) and avoid silently passing on a missing selector, add an explicit existence check before iterating over matched nodes. If you truly want to allow absence, update the PHPDoc/Gherkin to state that behaviour.
• File: src/ElementTrait.php around line 291
Insert immediately after fetching $nodes:
$nodes = $element->findAll('css', $selector);
+ if (empty($nodes)) {
+ throw new \Exception(
+ sprintf('Element defined by "%s" selector is not present on the page.', $selector)
+ );
+ }• Update the PHPDoc above elementAssertIsNotVisible() to reflect that it now throws when the selector is missing (or, if absence is acceptable, explicitly note that no-match = pass).
| @trait:ElementTrait @skipped | ||
| Scenario: Assert step definition "Then the element :selector should be displayed" fails as expected | ||
| Given some behat configuration | ||
| And scenario steps tagged with "@api @javascript @phpserver": | ||
| """ | ||
| When I am on the phpserver test page | ||
| Then the element "#hidden" should not be displayed | ||
| """ | ||
| When I run "behat --no-colors" | ||
| Then it should fail with an error: | ||
| """ | ||
| Element defined by "#hidden" selector is not visible on the page. | ||
| """ | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Scenarios marked @skipped risk becoming stale
Maintaining skipped tests for long periods hides regressions.
If Behat hangs:
- Investigate the root cause (often an unclosed modal or
an infinite JS loop). - Add an explicit timeout/
@javascriptwait step instead of skipping,
or move the flaky assertion to a separate, quarantined suite.
Reducing skipped scenarios keeps the suite trustworthy.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 3.x #386 +/- ##
==========================================
- Coverage 81.28% 81.03% -0.25%
==========================================
Files 32 31 -1
Lines 1881 1888 +7
==========================================
+ Hits 1529 1530 +1
- Misses 352 358 +6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/ElementTrait.php (1)
298-307: Absence vs. invisibility ambiguity is still unresolved
The step passes when the selector is missing entirely, which contradicts the presence-required behaviour ofelementAssertIsVisible()and the guidance given in the previous review.If absence should fail, add an existence check identical to the positive assertion; if absence should pass, document the difference explicitly in the PHPDoc/Gherkin docs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (1)
src/ElementTrait.php(3 hunks)
🔇 Additional comments (1)
src/ElementTrait.php (1)
258-288: Visibility assertion now behaves as requested – great job!The new implementation:
• Requires the selector to exist.
• Succeeds when at least one matched node is visible (avoids false-negatives with breakpoint-hidden duplicates).This aligns with the feedback in the previous round and makes the step far more robust.
| protected function elementIsVisuallyVisible(string $selector, int $offset) { | ||
| // The contents of this JS function should be copied as-is from the <script> | ||
| // section in the bottom of the tests/behat/fixtures/relative.html file. | ||
| $scriptFunction = <<<JS | ||
| function isElemVisible(selector, offset = 0) { | ||
| var failures = []; | ||
| document.querySelectorAll(selector).forEach(function (el) { | ||
| // Inject a style to disable scrollbars for more consistent results. | ||
| if (document.querySelectorAll('head #relative_style').length === 0) { | ||
| document.querySelector('head').insertAdjacentHTML( | ||
| 'beforeend', | ||
| '<style id="relative_style" type="text/css">::-webkit-scrollbar{display: none;}</style>' | ||
| ); | ||
| } | ||
|
|
||
| // Scroll to the element top, accounting for an offset. | ||
| window.scroll({ top: el.offsetTop + offset }); | ||
|
|
||
| // Gather visibility constraints. | ||
| const isVisible = !!(el.offsetWidth || el.offsetHeight || el.getClientRects().length); | ||
| const hasHeight = el.clientHeight > 1 || el.offsetHeight > 1; | ||
| const notClipped = !( | ||
| getComputedStyle(el).clip === 'rect(0px 0px 0px 0px)' && | ||
| getComputedStyle(el).position === 'absolute' | ||
| ); | ||
| const rect = el.getBoundingClientRect(); | ||
| > | ||
| rect.left + rect.width <= 0 || | ||
| rect.top + rect.height <= 0 || | ||
| rect.left >= window.innerWidth || | ||
| rect.top >= window.innerHeight | ||
| ); | ||
|
|
||
| if (!isVisible || !hasHeight || !notClipped || !onScreen) { | ||
| failures.push(el); | ||
| } | ||
| }); | ||
|
|
||
| return failures.length === 0; | ||
| } | ||
| JS; | ||
| // Include and call visibility assertion function. | ||
| $script = <<<JS | ||
| (function() { | ||
| {$scriptFunction} | ||
| return isElemVisible('{$selector}', {$offset}); | ||
| })(); | ||
| JS; | ||
|
|
||
| return $this->getSession()->evaluateScript($script); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
JS helper leaks globals & PHP-side string interpolation is injection-prone
onScreenis assigned withoutconst/let, polluting the page’s global scope and risking name clashes on repeated runs.- Embedding the raw
$selectorinto the JavaScript breaks when the selector contains quotes and opens the door for JS injection.
Proposed fix (PHP + JS):
- >
+ const >
@@
- // Include and call visibility assertion function.
- $script = <<<JS
- (function() {
- {$scriptFunction}
- return isElemVisible('{$selector}', {$offset});
- })();
- JS;
+ // Safely JSON-encode the selector to avoid quoting/injection issues.
+ $selectorJson = json_encode($selector, JSON_THROW_ON_ERROR);
+
+ $script = <<<JS
+ (function() {
+ {$scriptFunction}
+ return isElemVisible({$selectorJson}, {$offset});
+ })();
+ JS;Benefits
• Keeps the global namespace clean.
• Handles selectors like input[name="user[mail]"] without breaking the step.
• Eliminates a potential XSS vector if a malicious selector ever slips through.
📝 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.
| protected function elementIsVisuallyVisible(string $selector, int $offset) { | |
| // The contents of this JS function should be copied as-is from the <script> | |
| // section in the bottom of the tests/behat/fixtures/relative.html file. | |
| $scriptFunction = <<<JS | |
| function isElemVisible(selector, offset = 0) { | |
| var failures = []; | |
| document.querySelectorAll(selector).forEach(function (el) { | |
| // Inject a style to disable scrollbars for more consistent results. | |
| if (document.querySelectorAll('head #relative_style').length === 0) { | |
| document.querySelector('head').insertAdjacentHTML( | |
| 'beforeend', | |
| '<style id="relative_style" type="text/css">::-webkit-scrollbar{display: none;}</style>' | |
| ); | |
| } | |
| // Scroll to the element top, accounting for an offset. | |
| window.scroll({ top: el.offsetTop + offset }); | |
| // Gather visibility constraints. | |
| const isVisible = !!(el.offsetWidth || el.offsetHeight || el.getClientRects().length); | |
| const hasHeight = el.clientHeight > 1 || el.offsetHeight > 1; | |
| const notClipped = !( | |
| getComputedStyle(el).clip === 'rect(0px 0px 0px 0px)' && | |
| getComputedStyle(el).position === 'absolute' | |
| ); | |
| const rect = el.getBoundingClientRect(); | |
| > | |
| rect.left + rect.width <= 0 || | |
| rect.top + rect.height <= 0 || | |
| rect.left >= window.innerWidth || | |
| rect.top >= window.innerHeight | |
| ); | |
| if (!isVisible || !hasHeight || !notClipped || !onScreen) { | |
| failures.push(el); | |
| } | |
| }); | |
| return failures.length === 0; | |
| } | |
| JS; | |
| // Include and call visibility assertion function. | |
| $script = <<<JS | |
| (function() { | |
| {$scriptFunction} | |
| return isElemVisible('{$selector}', {$offset}); | |
| })(); | |
| JS; | |
| return $this->getSession()->evaluateScript($script); | |
| protected function elementIsVisuallyVisible(string $selector, int $offset) { | |
| // The contents of this JS function should be copied as-is from the <script> | |
| // section in the bottom of the tests/behat/fixtures/relative.html file. | |
| $scriptFunction = <<<JS | |
| function isElemVisible(selector, offset = 0) { | |
| var failures = []; | |
| document.querySelectorAll(selector).forEach(function (el) { | |
| // Inject a style to disable scrollbars for more consistent results. | |
| if (document.querySelectorAll('head #relative_style').length === 0) { | |
| document.querySelector('head').insertAdjacentHTML( | |
| 'beforeend', | |
| '<style id="relative_style" type="text/css">::-webkit-scrollbar{display: none;}</style>' | |
| ); | |
| } | |
| // Scroll to the element top, accounting for an offset. | |
| window.scroll({ top: el.offsetTop + offset }); | |
| // Gather visibility constraints. | |
| const isVisible = !!(el.offsetWidth || el.offsetHeight || el.getClientRects().length); | |
| const hasHeight = el.clientHeight > 1 || el.offsetHeight > 1; | |
| const notClipped = !( | |
| getComputedStyle(el).clip === 'rect(0px 0px 0px 0px)' && | |
| getComputedStyle(el).position === 'absolute' | |
| ); | |
| const rect = el.getBoundingClientRect(); | |
| const > | |
| rect.left + rect.width <= 0 || | |
| rect.top + rect.height <= 0 || | |
| rect.left >= window.innerWidth || | |
| rect.top >= window.innerHeight | |
| ); | |
| if (!isVisible || !hasHeight || !notClipped || !onScreen) { | |
| failures.push(el); | |
| } | |
| }); | |
| return failures.length === 0; | |
| } | |
| JS; | |
| // Safely JSON-encode the selector to avoid quoting/injection issues. | |
| $selectorJson = json_encode($selector, JSON_THROW_ON_ERROR); | |
| $script = <<<JS | |
| (function() { | |
| {$scriptFunction} | |
| return isElemVisible({$selectorJson}, {$offset}); | |
| })(); | |
| JS; | |
| return $this->getSession()->evaluateScript($script); | |
| } |
44adaf1 to
0204b86
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/ElementTrait.php (1)
138-146:⚠️ Potential issueEscape selector before embedding it into JavaScript to avoid breakage / injection
$selectoris interpolated directly into the JS string.
A selector containing a single quote or malicious payload will break the script or, worse, lead to code injection in the browser context.- var element = document.querySelector('{$selector}'); + const selector = %s; + var element = document.querySelector(selector);$selectorJson = json_encode($selector, JSON_THROW_ON_ERROR); $script = sprintf($script, $selectorJson);Using
json_encode()guarantees proper escaping and eliminates the injection vector.
♻️ Duplicate comments (5)
steps.md (1)
523-575: Markdown-lint findings from the previous review are still applicable hereThe newly added
<details>/ fenced-code blocks repeat the MD031/MD033 issues we discussed earlier (blank lines around fences and inline HTML).
Unless you have a renderer that requires the current format, please consider applying the earlier clean-up suggestion.tests/behat/features/element.feature (1)
248-262: 🧹 Nitpick (assertive)Large block of
@skippedscenarios – revisit or removeCarrying long-term skipped tests hides real regressions.
Investigate why Behat hangs (most often an unclosed modal or runaway JS), replace the skip with an explicit timeout or move the flaky cases to a quarantined suite.src/ElementTrait.php (3)
300-309:elementAssertIsNotVisible()still passes when the selector is absentPrevious feedback noted that the negative assertion succeeds when no element matches the selector, which is inconsistent with the positive assertion that fails on absence.
Either:
- Add an explicit presence check (mirroring
elementAssertIsVisible()), or- Document that “not visible” also covers “not present”.
416-421: 🧹 Nitpick (assertive)Typo in PHPDoc – “withing” → “within”
- * Assert that an element is displayed withing a viewport using different FE techniques. + * Assert that an element is displayed within a viewport using different FE techniques.
394-437:⚠️ Potential issueJavaScript helper still leaks a global and embeds raw selector
onScreenis assigned withoutlet/const, polluting the page’s global namespace.$selectoris inserted verbatim ('{$selector}') – same injection risk described above.Proposed patch:
- > + const > @@ - return isElemVisible('{$selector}', {$offset}); + const sel = %s; + return isElemVisible(sel, {$offset});And wrap the script with
sprintf($script, json_encode($selector, JSON_THROW_ON_ERROR));This keeps globals clean and selectors safe.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (6)
src/VisibilityTrait.php(0 hunks)steps.md(1 hunks)tests/behat/bootstrap/FeatureContext.php(0 hunks)tests/behat/features/element.feature(2 hunks)tests/behat/features/visibility.feature(0 hunks)
src/ElementTrait.php (3 hunks)
💤 Files with no reviewable changes (3)
- tests/behat/bootstrap/FeatureContext.php
- tests/behat/features/visibility.feature
- src/VisibilityTrait.php
🔇 Additional comments (1)
tests/behat/features/element.feature (1)
248-262: Error-message expectation no longer matches the implementation
ElementTrait::elementAssertIsVisible()now throws
None of the elements defined by "#hidden" selector are visible on the page.The skipped scenario still expects
Element defined by "#hidden" selector is not visible on the page.If you ever re-enable the scenario it will fail immediately.
Please update the expected text (or, better, assert on a regexp that is resilient to wording changes).
| public function elementAssertIsNotVisuallyVisibleWithOffset(string $selector, int $number): void { | ||
| if ($this->elementIsVisuallyVisible($selector, $number)) { | ||
| throw new \Exception(sprintf('Element(s) defined by "%s" selector is displayed within a viewport with a top offset of %d pixels, but should not be.', $selector, $number)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Missing-element edge-case causes false failure
elementIsVisuallyVisible() returns true when the selector matches zero elements.
Consequently elementAssertIsNotVisuallyVisibleWithOffset() treats a missing element as visible and throws, which is the opposite of what users typically expect.
Consider short-circuiting when querySelectorAll(selector).length === 0 and returning false.
0204b86 to
c2c8db9
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
src/ElementTrait.php (4)
300-309:⚠️ Potential issue
elementAssertIsNotVisible()still silently passes when the selector is missingIf no nodes match the selector the loop is skipped and the assertion reports success.
This is inconsistent withelementAssertIsVisible()(which throws when nothing is found) and can hide regressions.A one-liner at the presence check level is enough:
$nodes = $element->findAll('css', $selector); +if ($nodes === []) { + throw new \Exception(sprintf('Element defined by "%s" selector is not present on the page.', $selector)); +}
415-424:⚠️ Potential issue
isElemVisible()returns TRUE on zero matches – causes false positivesBecause
failures.length === 0when the selector matches nothing, “viewport” checks incorrectly pass for non-existent elements.
Change the early return to treat “no elements” as not visible, or document the behaviour explicitly.- return failures.length === 0; + return failures.length === 0 && document.querySelectorAll(selector).length > 0;
380-388: 🧹 Nitpick (assertive)Typo in PHPDoc – “withing” → “within”
Tiny but visible in IDE tool-tips and in generated docs.
- * Assert that an element is displayed withing a viewport using different FE techniques. + * Assert that an element is displayed within a viewport using different FE techniques.
395-437: 🛠️ Refactor suggestionJS helper still leaks globals and is vulnerable to selector injection
Previous feedback has not been addressed –
onScreenis a global and the raw selector is directly interpolated.
Replacing withconst+JSON.encodeboth hardens and tidies the code.- const rect = el.getBoundingClientRect(); - > + const rect = el.getBoundingClientRect(); + const > @@ - return isElemVisible('{$selector}', {$offset}); + const selector = JSON.parse({json_encode($selector, JSON_THROW_ON_ERROR)}); + return isElemVisible(selector, {$offset});(see also the variable rename above).
Failing to quote-escape selectors such asinput[name="user[mail]"]currently breaks the script and could be abused for JS-injection if an attacker controlled the selector string.tests/behat/features/element.feature (1)
248-262: 🧹 Nitpick (assertive)Large block of
@skippedscenarios – risk of silent regressionsTwenty-plus negative tests are permanently skipped. Long-term skips erode confidence: failures can creep in unnoticed and CI time is still consumed parsing them.
Consider:
• Investigating the root hang (often an uncaught JS alert or infinite loop).
• Moving flaky tests into a quarantined suite that still runs on demand.
• Converting repetitive cases into a Scenario Outline for readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Lite
📒 Files selected for processing (7)
src/ElementTrait.php(3 hunks)src/VisibilityTrait.php(0 hunks)steps.md(1 hunks)tests/behat/bootstrap/FeatureContext.php(0 hunks)tests/behat/features/element.feature(2 hunks)tests/behat/features/login.feature(1 hunks)tests/behat/features/visibility.feature(0 hunks)
💤 Files with no reviewable changes (3)
- tests/behat/bootstrap/FeatureContext.php
- tests/behat/features/visibility.feature
- src/VisibilityTrait.php
🔇 Additional comments (1)
tests/behat/features/login.feature (1)
8-12: Change from permission-based to role-based login looks correctThe step definition
I am logged in as a user with the "Administrator" roleexists inUserTrait, so scenarios should still pass. No further action required.Also applies to: 15-19
c2c8db9 to
182410e
Compare
Summary by CodeRabbit
New Features
Documentation
Tests
Chores