laravel / dusk

Laravel Dusk provides simple end-to-end testing and browser automation.
https://laravel.com/docs/dusk
MIT License
1.87k stars 320 forks source link

Dusk @-selectors cannot be combined with other CSS selectors for the same element #1113

Closed pindab0ter closed 1 month ago

pindab0ter commented 1 month ago

Dusk Version

8.2.2

Laravel Version

11.11.1

PHP Version

8.2.22

PHPUnit Version

n/a

Database Driver & Version

n/a

Description

When using Dusk @-selectors, they cannot be combined with for other CSS selectors that are applicable to the same element.

For example: I have an HTML element with class="foo" dusk="bar". I want to match an element that has @bar as a Dusk selector and also .foo as a class. Also checking for a CSS class is useful for example when using Dusk Components to make sure the Dusk selector is applied to the correct element:

class TestComponent extends Component
{
    public function __construct(
        private readonly string $selector
    ) {
    }

    public function selector(): string
    {
        return "{$this->selector}.bar";
    }

   …
}

After parsing, the selector (here @foo) becomes [dusk="foo.test"] instead of [dusk="foo"].test. This is due to this regex only looking for spaces, not other CSS selector symbols like . and #.

A possible solution would be to change the regex to also look for CSS selector symbols in addition to spaces. This would break any existing Dusk selectors that contain periods (e.g. dusk="@foo.bar") or other CSS selectors. As of now that is valid, but it is my opinion that they should not be. In my opinion people should use commonly used separators like dashes or underscores in Dusk selectors, just like you would in id and class names.

Right now it is not possible to distinguish whether .bar in @foo.bar refers to a class named bar or is a part of the Dusk selector itself (e.g.: <div class="bar" dusk="foo.bar">).

Trying to work around this limitation by putting the Dusk selector after the other CSS selectors doesn't work either, as the parser looks for @ being the first character in the selector in the line above the previously linked line.

While I realise it might not be intended to use Dusk selectors in combination with other CSS selectors, I do believe this adds value and should be supported. To that end I suggest deprecating using reserved CSS symbols in Dusk selectors and updating the format regex accordingly.

Steps To Reproduce

  1. Apply a class name and a Dusk selector to an element (e.g. class="foo" dusk="bar")
  2. Write a selector that includes both (e.g. assertVisible('@bar.foo') or assertVisible('.foo@bar'))

https://github.com/pindab0ter/dusk-bug-report

crynobone commented 1 month ago

It is possible to define dusk selector as foo.bar

pindab0ter commented 1 month ago

Exactly. I think this should not be allowed, just like it isn't for id and class names on HTML elements.

github-actions[bot] commented 1 month ago

Thank you for reporting this issue!

As Laravel is an open source project, we rely on the community to help us diagnose and fix issues as it is not possible to research and fix every issue reported to us via GitHub.

If possible, please make a pull request fixing the issue you have described, along with corresponding tests. All pull requests are promptly reviewed by the Laravel team.

Thank you!

driesvints commented 1 month ago

Looks like for now, we're not going to make any changes, sorry.

pindab0ter commented 1 month ago

I would like to hear the considerations.

I'm describing a valid problem with undefined behaviour and I'm offering a possible solution. There's no conversation being had here. I'm just hearing "no".

JayBizzle commented 4 weeks ago

@pindab0ter If i understand what your are saying, I attempted this a while back, but there was no good solution i could find at the time - https://github.com/laravel/dusk/pull/1035

pindab0ter commented 2 weeks ago

That's because there is a fundamental problem, namely Dusk allowing characters in their selectors that are not allowed in other CSS selectors.

If that were to be disallowed, that would fix this issue.

It is a backwards compatible change, but this behaviour is not documented either (as mentioned earlier, every code, documentation and testing sample of Dusk selectors use kebab case). I suggest putting #1114 in the next major point release with a notice in the upgrade guide and/or release notes for that reason.

@driesvints what are your thoughts on this?

driesvints commented 1 week ago

@pindab0ter you can always re-attempt the PR with a more thorough explanation to see if Taylor would re-accept it.