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

[REGRESSION] Custom selectors no longer able to be last in group #4774

Closed
DavertMik opened this issue Dec 19, 2020 · 4 comments
Closed

[REGRESSION] Custom selectors no longer able to be last in group #4774

DavertMik opened this issue Dec 19, 2020 · 4 comments
Assignees

Comments

@DavertMik
Copy link
Contributor

DavertMik commented Dec 19, 2020

Context:

  • Playwright Version: 1.7
  • Operating System: Not related
  • Node.js version: 12
  • Browser: Chromium
  • Extra: [any specific details about your environment]

Code Snippet

In CodeceptJS we had this custom selector engine which worked fine since... Playwright 0.13, I think.
The purpose of it was to filter elements by value:

module.exports.createValueEngine = () => {
  return {
    // Creates a selector that matches given target when queried at the root.
    // Can return undefined if unable to create one.
    // eslint-disable-next-line no-unused-vars
    create(root, target) {
      return null;
    },

    // Returns the first element matching given selector in the root's subtree.
    query(root, selector) {
      if (!root) {
        return null;
      }
      return `${root.value}`.includes(selector) ? root : null;
    },

    // Returns all elements matching given selector in the root's subtree.
    queryAll(root, selector) {
      if (!root) {
        return null;
      }
      return `${root.value}`.includes(selector) ? root : null;
    },
  };
};

Usage example:

playwright.selectors.register('__value', createValueEngine);
// wait for the field with value
page.waitForSelector('css=input[type=text] >> __value=Hello');

This worked till 1.7.0.

I tried to debug and seems that in 1.7 in query and queryAll calls I am receiving document object, and not Node object.

@aslushnikov aslushnikov changed the title [BUG] Custom selectors no longer able to be last in group [REGRESSION] Custom selectors no longer able to be last in group Dec 19, 2020
@dgozman
Copy link
Contributor

dgozman commented Dec 19, 2020

This has indeed regressed in 1.7, and will be fixed in the next patch by #4754.

Some suggestions for writing robust selector engines:

  • The comment says "Returns the first element matching given selector in the root's subtree" and this engine clearly does not follow it 😄 To return the first element from the subtree, this engine has to iterate over the subtree elements, not just check the root itself.
  • For the engine to ever work with page.$$() and other methods querying multiple elements, queryAll should return an array, not an element: return `${root.value}`.includes(selector) ? [root] : [];

@Georgegriff
Copy link
Contributor

I suspect I copied and pasted the comments into the CodeceptJS code and forgot to change, which would explain why they don't make sense. These selectors are implemented to enable us to do arbitrary selectors on properties, in this case value, but we have ones for disabled too, without forcing our users to use any specific selector strat, E.g. xpath/CSS, I suppose you could think of them as filters
So we can enable I.waitForValue(selector, value) in the framework

@DavertMik
Copy link
Contributor Author

For the engine to ever work with page.$$() and other methods querying multiple elements, queryAll should return an array, not an element: return ${root.value}.includes(selector) ? [root] : [];

Yes, I found that during my debugging. To my surprise changing that didn't help to solve the issue 😄 . But sure that part should be updated.

The comment says "Returns the first element matching given selector in the root's subtree" and this engine clearly does not follow it smile To return the first element from the subtree, this engine has to iterate over the subtree elements, not just check the root itself.

Yeah, the use case for this engine is very specific and probably it is a bit of misuse of selector engine as we don't go deeper by the tree but we filter the elements according to some rules. I would say this is a quite smart idea by @Georgegriff . At least we won't need to create various pseudo-CSS selectors (jQuery style) for additional filtration of elements.

Anyway, for further usage, we should to stick to your guidelines.

@dgozman
Copy link
Contributor

dgozman commented Dec 22, 2020

This will be fixed in v1.7.1.

@dgozman dgozman closed this as completed Dec 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants