Using .within() in custom commands/functions to return an inner DOM element found in .within() results in unexpected behavior

tripleequal commented 3 years ago

Current behavior

This command worked in 5.3, but in 5.4 when I chain a .type() command to it. It throws and error

Command:

Cypress.Commands.add('firstCellSearchFilter', () => {
  cy.get('.ag-header-row-floating-filter').within(() => {
    cy.get('.ag-header-cell')
      .first()
      .within(() => {
        cy.get('.ag-text-field-input')
      })
  })
})

Error: cy.type() can only be called on a single element. Your subject contained 3 elements.

Foo.firstCellSearchFilter().type('bar') fails because Foo.firstCellSearchFilter() returns ALL of the cy.get('.ag-text-field-input') elements. Its like the scope of cy.get('.ag-header-cell').first().within() gets reset or ignored

Desired behavior

I would expect the command to scope to the single input element, like it did in 5.3

Versions

Cypress: 5.4 Mac Mojave Chrome 86

jennifer-shehane commented 3 years ago

I can see a behavior changed in 5.4.0 in an unexpected way, not sure this is exactly your situation. I can recreate this with the following code. cc @sainthkh

index.html

<html>
<body>
  <div class="first">First
      <div class="second">Second
        <input class="third">
      </div>
      <div class="second">Second</div>
  </div>
  <input class="third">
  <input class="third">
</body>
</html>

spec.js

Cypress.Commands.add('getThird', () => {
  cy.get('.first').within(() => {
    cy.get('.second')
      .first()
      .within(() => {
        cy.get('.third')
      })
  })
})

it('test cy.route()', () => {
  cy.visit('index.html')
  cy.getThird().type('foo')
})

5.3.0

5.4.0

sainthkh commented 3 years ago

It's the intended behavior. Before 5.4, cy.within was permanently narrowing scope for the following commands and it's fixed in #8699.

Here're the workarounds:

// Use CSS selector pseudo class
Cypress.Commands.add('getThird2', () => {
  cy.get('.first .second:first-child .third')
})

// Use jQuery commands + cy.get() withinSubject option.
Cypress.Commands.add('getThird3', () => {
  cy.get('.third', {
    withinSubject: Cypress.$('.first').find('.second').first(),
  })
})

jennifer-shehane commented 3 years ago

I don't think this is really expected behavior. Why should this cause Cypress to type in the element within the first within - .second? I would expect this custom command to return .first or .third, but never .second.

Cypress.Commands.add('getThird', () => {
  cy.get('.first').within(() => {
    cy.get('.second')
      .first()
      .within(() => {
        cy.get('.third')
      })
  })
})

it('test cy.route()', () => {
  cy.visit('index.html')
  cy.getThird().type('foo')
})

sainthkh commented 3 years ago

@jennifer-shehane I'll look into this more thoroughly tomorrow.

sainthkh commented 3 years ago

I checked it and concluded that it's the intended behavior.

First of all, I checked should command correctly works with the <div.first>.

cy.visit('fixtures/a.html')
cy.getThird().should('have.class', 'first')

This works fine. I concluded that the > <div class="second a">...</div> message has nothing to do with the bug in cy.within.

But it makes me curious why it shows div.second rather than div.first.

It happened because the node used to throw an error in type.js is not the selected node but the node at the coordinate.

Below is the location where the error is thrown (line 431).

https://github.com/cypress-io/cypress/blob/e02abb178f054427b8ebc4452fcda57d8e046205/packages/driver/src/cy/commands/actions/type.js#L420-L435

And $elToCheck is calculated below (line 359):

https://github.com/cypress-io/cypress/blob/e02abb178f054427b8ebc4452fcda57d8e046205/packages/driver/src/cy/actionability.js#L343-L366

That's why the element at the center of the div.first, the first div.second, was returned.

But this makes me curious about what happens if I extend the first input.third to make it located at the center of the div.first.

And I wanted to be sure if this is a bug or not.

I changed the html code like below:

<html>
<body>
  <div class="first">First
      <div class="second">Second
        <input class="third" style="width: 80%">
      </div>
      <div class="second">Second</div>
  </div>
  <input class="third">
  <input class="third">
</body>
</html>

The only change is to add style="width: 80%".

But in this case, the problematic test passes. This makes me think whether we should call this behavior a bug.

But I concluded that it's not. Because in modern web applications, there are a lot of components like below is possible:

<div class="awesome-component" id="abc">
  <div class="another-div">
    <input />
  </div>
</div>

In this case, not allowing cy.get('#abc').type('foo') and only accepting cy.get('#abc .another-div input').type('foo') is too inconvenient.

In conclusion, this is not a bug and there is nothing to be changed.

tripleequal commented 3 years ago

It is absolutely a bug. If its not then what is the point of within()? the within() is supposed to narrow the search scope. The BUG is the when you attach cy.get to an element that is being found in the within() is attaches the action, type() or click() to the element that was found at the top level within() scope instead of the element found in the within() scope.

referring to my original comment

Cypress.Commands.add('firstCellSearchFilter', () => {
  cy.get('.ag-header-row-floating-filter').within(() => {
    cy.get('.ag-header-cell')
      .first()
      .within(() => {
        cy.get('.ag-text-field-input')
      })
  })
})

So you're telling me that the intended behavior of Foo.firstCellSearchFilter().type() is to type or click on 'ag-header-row-floating-filter' (the outer scope) and not ag-text-field-input?

This worked in 5.3 and broke in 5.4

sainthkh commented 3 years ago

Actually, that's what the document says:

.within() yields the same subject it was given from the previous command.

Before 5.4, cy.within() didn't work as documented. And with the old behavior, there are no differences in the 2 code examples below:

cy.get('article').within(() => {
  cy.get('h1').should('contain.text', 'Blog')
})
.should('have.class', 'post')

cy.get('article').within(() => {
  cy.get('h1').should('contain.text', 'Blog').should('have.class', 'post')
})

jennifer-shehane commented 3 years ago

Another example of unexpected behavior from https://github.com/cypress-io/cypress/issues/9064

@sainthkh I do agree that this is working as documented, but I think it may be helpful to think about how to provide the wanted behavior here. People want to traverse within some DOM and return the inner DOM element they've found, so that they can return it to be chained as a command/function.

So, either we can provide an example of how they can do this or we change the behavior of .within()

Maybe the scope of .within() could change if there is a return statement, similar to how what is yielded from .then() changes based on the return statement. (This is just me thinking out loud, I haven't discussed this with the team).

function getTableCell(
  tableId,
  column,
  row,
) {
  return cy
    .get(tableId)
    .find('tbody>tr')
    .eq(row)
    .within(() => {
      cy.get(column);
    });
}

it('test', () => {
  cy.visit('index.html')
  getTableCell("#myTable", "#three", 0).should('have.text', 'foo')
})

5.4.0

5.5.0

sainthkh commented 3 years ago

@jennifer-shehane Actually, that's what I was thinking, too. I was wondering if I need to open a new issue about that. And you mentioned it.

I think it would meet the two groups of users if the new behavior works like below:

<div id="a">
  <div id="b">Hello</div>
  <div>World</div>
</div>

Old behavior:

cy.get('#a').within(() => cy.get('#b')).should('have.text', 'Hello')

Current behavior:

cy.get('#a').within(() => cy.get('#b')).should('have.text', 'HelloWorld')

Suggested behavior:

cy.get('#a').within(() => { cy.get('#b') }).should('have.text', 'HelloWorld');
cy.get('#a').within(() => { return cy.get('#b') }).should('have.text', 'Hello');

jennifer-shehane commented 3 years ago

Yeah, this would be a breaking change, but we intend to schedule breaking change releases more often (~ every 12 weeks). We have one scheduled for Nov 23 https://github.com/cypress-io/cypress/pull/8437

I'll try and bring this proposal up with the team and comment back.

RichardMatsen commented 3 years ago

I like the idea of using an explicit return to control the downstream subject.

@tripleequal, you can back-flip inside the custom command to work around the recent mod, something like this
(testing with the superb cypress-fiddle)

/// <reference types="@cypress/fiddle" />

Cypress.Commands.add('firstCellSearchFilter', () => {  
  let result;  
  cy.get('.ag-header-row-floating-filter')
    .within(() => {  
      cy.get('.ag-header-cell')  
        .first()  
        .within(() => {  
          cy.get('.ag-text-field-input')
            .invoke('val')
            .then(val => result = val);
        })
    })  
    .then(() => result); 
})  

const firstCellTest = {
  html: `
    <div class="ag-header-row-floating-filter">
      <div class="ag-header-cell">
        <input class="ag-text-field-input" value="1">
      </div>
      <div class="ag-header-cell">
        <input class="ag-text-field-input" value="2">
      </div>
    </div>
  `,
  test: `
    cy.firstCellSearchFilter().then((value) => {
      console.log(value);  // logs "1"
    });
  `
}

it('tests subject change from within()', () => {
  cy.runExample(firstCellTest)
})

I'd also like to see explicit return applied to .each(), which allow it to work like Array.map().
It could also behave like Array.reduce() if an accumulator parameter (updated on each iteration) was provided.

jennifer-shehane commented 3 years ago

@sainthkh After discussing this with the team, we are most concerned about there being consistency among all of the commands and how they work. So, .then() should work similarly to how .within() or .should() or any other command that accepts a function that includes other cypress comannds within it.

So, we'd like to not see new rules made up specifically for .within() - and make sure the logic matches more closely to how the other commands work.

sainthkh commented 3 years ago

I first thought of making a new command like inside, inward or simply in for the old within behavior.

But I realized that the problems could be solved with find command. It seems that there is no

I guess we can close this issue by creating a note in within command about searching elements in the narrowed scope with find.

It's short and looks good. Example:

it('test', () => {
  cy.visit('fixtures/dom.html')
  cy.get('#by-name').find('input:first').type('hello world')
})

Prashant-Kan commented 3 years ago

My Issue was closed as its duplicate to this. @jennifer-shehane I agree and will now keep an eye on this issue But any work around from the current version.

as we got stuck in here at 5.3.0 Now, we want to use the plugins introduce in 6.3.0 and later

For using Plugin to save the downloaded file to cypress folder and to assert the downloaded file content. we upgrade to v6.3.0 and found that this within function is broken so again degraded to v 5.3.0

It would be great if we can get some work around for both download file and within function .

Eagerly waiting for the solution

Prashant-Kan commented 3 years ago

@sainthkh and @jennifer-shehane

any solution for this issue. We have implemented Cypress in our application but not able to upgrade to latest version We are still at 5.3 as we have used implicit return at various places.

Please share if this issue is on planned for upcoming sprint

sainthkh commented 3 years ago

Doesn't changing within() to find() work? For example with the code above, we can change it like below:

cy.get('.ag-header-row-floating-filter')
   .find('.ag-header-cell')  
   .first()  
   .find('.ag-text-field-input')
   .invoke('val')
   .then(val => result = val)
   .then(() => result);

Prashant-Kan commented 3 years ago

Hi, @sainthkh Sorry for late reply

Actually, this works in few scenarios but in few scenarios its not working and also its getting problem in returning type

Each custom comman in our application has a return type of Cypress.Chainable such as

function CustomCommandName<Subject>(params: ParamModel): Cypress.Chainable<Subject> {
// our cusome command logic 
return cy.get<Subject>();
}

Now, if we use the find('targetelement') then it throws error and to overcome that error we have to do like

return cy.get('targetelement').find('targetchildelement') as any as Cypress.Chainable<Subject>

But, for few places its not feasible to return like this

any solution for this wehre we dont have to type cast twice (to any and then to Subject)

sainthkh commented 3 years ago

@Prashant-Kan How about this?

cy.get('targetelement').find<Subject>('targetchildelement')

Prashant-Kan commented 3 years ago

Hi, @sainthkh

Already tried, it throws error

Type 'Subject' does not satisfy the constraint 'Node'

shows red underline beneath Subject and when hover to it, shows the above mentioned error

sainthkh commented 3 years ago

I don't know the definition of Subject. But it's a bit hard for me to believe that cy.get<Subject>('...') works but cy.find<Subject>() doesn't work. Because their definitions are like below:

get<E extends Node = HTMLElement>(selector: string, options?: Partial<Loggable & Timeoutable & Withinable & Shadow>): Chainable<JQuery<E>>
find<E extends Node = HTMLElement>(selector: string, options?: Partial<Loggable & Timeoutable & Shadow>): Chainable<JQuery<E>>

They use the same constraint rule. How about extending Subject with HTMLElement?

Prashant-Kan commented 3 years ago

Hi @sainthkh

Find definitions

    find<K extends keyof HTMLElementTagNameMap>(selector: K, options?: Partial<Loggable & Timeoutable & Shadow>): Chainable<JQuery<HTMLElementTagNameMap[K]>>

    find<E extends Node = HTMLElement>(selector: string, options?: Partial<Loggable & Timeoutable & Shadow>): Chainable<JQuery<E>>

Get definitions

 get<K extends keyof HTMLElementTagNameMap>(selector: K, options?: Partial<Loggable & Timeoutable & Withinable & Shadow>): Chainable<JQuery<HTMLElementTagNameMap[K]>>

    get<E extends Node = HTMLElement>(selector: string, options?: Partial<Loggable & Timeoutable & Withinable & Shadow>): Chainable<JQuery<E>>

    get<S = any>(alias: string, options?: Partial<Loggable & Timeoutable & Withinable & Shadow>): Chainable<S>

The third definition of get accepts the cy.get while the same is missing in find and hence its not accepting.

Definition of Subject is

namespace Cypress {
        interface Chainable<Subject = any> {
                // define our custom commands
        }
}

sainthkh commented 3 years ago

Sorry for the late reply.

How about trying Generics like below?

function CustomCommandName<S extends HTMLElement>(params: ParamModel): Cypress.Chainable<S> {
  // our cusome command logic 
  return cy.get<S>();
}

If this doesn't work for you, can you narrow down the problematic code? I first thought that the Subject is a type defined in your project. But my guess was wrong.

As we all know, the debugging starts with reproducing the problem on the developer's computer. In the current situation, the clue is too little to solve the problem.

stevenlafl commented 3 years ago

Yeah, return would be a good solution here. Hours of work to use find() ultimately. cy.wrap didn't work in within either.

I did try return as an intuitive step, so I support that solution. +1

GrayedFox commented 1 year ago

I wanted to share a custom command here which (somewhat) works around this issue, since we are still waiting on a fix.

This custom command will allow you to pass in a parent and child subject (useful if using static selector objects, highly recommend Anton's excellent cypress selectors plugin for doing exactly this) - in my case I wanted to be able to pass in two subjects and have the behavior from 5.3. In this library a Selector is just an alias of Cypress.Chainable with an extra property and decorator capabilities.

Inside of the within block of the custom command, the reference isn't broken (it only changes given the return to an outer scope -- inside the closure of the within block things work as expected). We set our outer scope variable childWithin from within the enclosing scope of a then block to sidestep the weird within behavior.

The gotcha is that this little hack works well when chaining into Mocha and Chai expectations from within a test but will fail when passing the result into or from a different scope (your inner selector will suddenly match multiple elements, as if the parent scope has been forgotten, as OP describes). Same is true if trying to use the custom command from within a class method (which has a different calling scope to that of the test).

//commands.ts
import '@frsource/cypress-plugin-visual-regression-diff';
import { Selector } from 'cypress-selectors';

declare global {
  // eslint-disable-next-line @typescript-eslint/no-namespace
  namespace Cypress {
    interface Chainable {
      /**
       * Custom command that wraps a child element found within a parent
       * @param {Cypress.Chainable} parent - Parent to search within
       * @param {Selector} child - Child we wish to wrap and yield
       * @example cy.getChild(parent, child);
       */
      getChild(parent: Cypress.Chainable, child: Selector): Chainable<Element>;
    }
  }
}

function getChild(
  parent: Cypress.Chainable,
  child: Selector
): Cypress.Chainable {
  let childWithin: Selector;
  return parent
    .within(() => {
      child.then(($el) => {
        childWithin = $el;
      });
    })
    .then(() => {
      return cy.wrap(childWithin);
    });
}

Cypress.Commands.add('getChild', getChild);

Now from a page object or test you can use test data (if needed to identify an element on a page) and combine this with static selectors (below example uses property decorators from cypress-selectors to generate chainable selectors):

// segment of Page Object class with a getter
import { By, Selector } from 'cypress-selectors';
import { ExternalSelectorConfig } from 'cypress-selectors/dist/Selectors';

import { IProductCardDetails } from '../support/interfaces';
import { BasePage } from './base-page';

class ProductCardSelectors {
  @By.Attribute('product-title') static title: Selector;
  @By.Attribute('product-card-add') static addButton: Selector;
}

class ProductCard extends BasePage {
  constructor(
    readonly product: IProductCardDetails,
    readonly subdirectory: string = '/products/'
  ) {
    super();
  }

  get baseSelector(): string {
    return `product-card[product-url="${this.productUrlString}"]`;
  }

  get self(): Cypress.Chainable {
    return cy.get(this.baseSelector);
  }

  get productUrlString(): string {
    return `${this.product.domain}${this.subdirectory}${this.product.path}`;
  }

  get button() {
    return cy.getChild(this.self, ProductCardSelectors.addButton);
  }
  get title() {
    return cy.getChild(this.self, ProductCardSelectors.title);
  }
  // work around due to within() block behaving badly, see:
  // https://github.com/cypress-io/cypress/issues/8873
  get image() {
    return this.self.find('[data-testid="product-image-0"]');
  }

  addToCart = () => {
    cy.intercept('POST', BasePage.urls.api).as('graphql');
    // calling this.button from within a class function will exhibit the same bug, so we don't call cy.getChild here 
    this.self.within(() => ProductCardSelectors.addButton.click());
    cy.wait(['@graphql']);
    return this;
  };
}

export { ProductCard, ProductCardSelectors };

Finally, what it looks like in a test:

import { ProductCard } from '../page-objects/product-card';
import { adidasBackpack, darthToy, plushToy } from '../support/test-data';

const wcProduct = new ProductCard(adidasBackpack);

describe('Elements', () => {
  before(() => {
    wcProduct.open();
  });

  context('WebComponent Product', () => {
    it('displays the correct title', () => {
      wcProduct.title.should('have.text', adidasBackpack.title);
    });

    it('displays the correct product image', () => {
      // above trick won't work for this
      divProduct.image.matchImage();
    });
  });
});

Prashant-Kan commented 1 year ago

Any update on this?

cypress-io / cypress