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

Inability to determine reason for invisibility #5974

Open
badeball opened this issue Dec 16, 2019 · 13 comments
Open

Inability to determine reason for invisibility #5974

badeball opened this issue Dec 16, 2019 · 13 comments

Comments

@badeball
Copy link

@badeball badeball commented Dec 16, 2019

Current behavior:

Per v3.8.0, one might receive a message stating EG. Cypress could not determine why this element '<button>' is not visible.

This is non-sensical, as there's obviously some heuristic implemented that has determined the element to be invisible.

I understand that there's a method getReasonIsHidden that is somewhat replicating the behavior of isHidden, but returning a human-readable string instead of a boolean value. I am however having a very hard time understanding why you would implement it like this.

Since there should be one, and only one, heuristic-implemention, why not make one a function of the other?

const getReasonIsHidden = function ($el) {
  // Implementation ...
}

const isHidden = ($el) => {
  return !!getReasonIsHidden($el);
}

Desired behavior:

Never to see such a non-informative error message.

Steps to reproduce: (app code and test code)

Doesn't really matter here, but I can see if I can produce a minimal example.

Versions

Cypress v3.8.0, Arch Linux, Chrome 77.

@jennifer-shehane

This comment has been minimized.

Copy link
Member

@jennifer-shehane jennifer-shehane commented Dec 17, 2019

I agree, this is pretty awful. I wrote this code as a catchall if a specific error message was not written for a hidden check. #4421

This is complicated by the fact that the isHidden function is separate (returning a boolean) from the getReasonIsHidden (returning a string) function which is not ideal.

Giving a repro would actually be really helpful because there must be a case we are checking but not printing a message for.

The one thing I do suspect is this check because I can't find a corresponding error message for it. https://github.com/cypress-io/cypress/blob/develop/packages/driver/src/dom/visibility.js#L356:L356 This actually does have a specific error message here: https://github.com/cypress-io/cypress/blob/issue-5974/packages/driver/src/dom/visibility.js#L494:L494

@badeball

This comment has been minimized.

Copy link
Author

@badeball badeball commented Dec 17, 2019

I figured out why. One of the ancestors has a CSS property transform: translate(0px, 0px) * and a height of 0. It does not however have overflow: hidden, hence its children are visible, but determined by Cypress to be invisible (by elIsHiddenByAncestors, which recursively calls elIsHiddenByTransform). The method getReasonIsHidden invokes elIsHiddenByTransform, but not elIsHiddenByAncestors, hence the indeterminate error message.

* I know this is a completely non-sensical attribute for someone to give to an element. This is done by Google Maps.

@jennifer-shehane

This comment has been minimized.

Copy link
Member

@jennifer-shehane jennifer-shehane commented Dec 17, 2019

@badeball K, thanks for looking into this. I'll have to try to set up a test for this situation.

  • parent: transform: translate(0px, 0px); height: 0; (no overflow: hidden)
  • child: visible

Recommended Action: getReasonIsHidden should check elIsHiddenByAncestors for reason.

@jennifer-shehane

This comment has been minimized.

Copy link
Member

@jennifer-shehane jennifer-shehane commented Dec 18, 2019

Got a repro and also verified this is a regression introduced in 3.8.0, likely from this PR #5590. So this is actually incorrectly failing the visibility check also.

index.thml

<!DOCTYPE html>
<html>
<body>
  <div style="transform: translate(0, 0); height: 0;">
    <p>Aenean lacinia bibendum nulla sed consectetur.</p>
  </div>
</body>
</html>

spec.js

it('types', function () {
  cy.visit('index.html')
  cy.get('p').should('be.visible')
})

3.7.0

Screen Shot 2019-12-18 at 1 37 30 PM

3.8.0

Screen Shot 2019-12-18 at 1 34 44 PM

@sainthkh

This comment has been minimized.

Copy link
Contributor

@sainthkh sainthkh commented Dec 18, 2019

@jennifer-shehane Ill check it out.

@jennifer-shehane jennifer-shehane removed their assignment Dec 18, 2019
@badeball

This comment has been minimized.

Copy link
Author

@badeball badeball commented Dec 18, 2019

This is undoubtedly a very interesting case, along with the whole problem of programmatically determining visibility. You obviously made a great deal of effort here, which I am wholeheartedly appreciative of.

However, I'd like to emphasize that what I originally wanted to address was the disconnect between reasoning and actual determination.

As long as the two methods isHidden & getReasonIsHidden implements their own heuristics, there will always be a possibility for the last, indeterminate message to be returned.

This message is non-sensical at the very core. Cypress does determine visibility and I believe it should always be able to tell you why in case it determined something to be invisible.

@badeball

This comment has been minimized.

Copy link
Author

@badeball badeball commented Dec 18, 2019

Here's an example of what I mean by sharing heuristic, without changing the current interface.

function determineVisibility ($el) {
  if ( /* some condition */ ) {
    return {
      visible: false,
      reason: "foobar"
    };
  }

  return {
    visible: true
  };
}

export function isHidden ($el) {
  return !determineVisibility($el).visible;
}

export function getReasonIsHidden ($el) {
  const visibility = determineVisibility($el);

  if (visibility.visible) {
    throw new Error("Erroneously invoked on a visible element");
  }

  return visibility.reason;
}
@jennifer-shehane

This comment has been minimized.

Copy link
Member

@jennifer-shehane jennifer-shehane commented Dec 19, 2019

@sainthkh I also ran this, without a parent, where the <p> is still visible and it falsely fails.

<!DOCTYPE html>
<html>
<body>
  <p style="transform: translate(0, 0); height: 0;">
   Aenean lacinia bibendum nulla sed consectetur.
  </p>
</body>
</html>
@sainthkh

This comment has been minimized.

Copy link
Contributor

@sainthkh sainthkh commented Dec 19, 2019

@jennifer-shehane

Screenshot from 2019-12-19 17-16-58

As you can see in the screenshot, "without-a-parent" problem is not the problem of the #6000. It's the problem of effective width or height. It's not regression.

It's been fixed by checking the value of effective width/height and transform value together.

@sainthkh

This comment has been minimized.

Copy link
Contributor

@sainthkh sainthkh commented Dec 27, 2019

@badeball I realized that your idea can be used to fix #677, too.

But I cannot implement your idea now because #5916 and #6000 are not merged yet. After they're done.

I'll revisit this issue.

@janakdr

This comment has been minimized.

Copy link

@janakdr janakdr commented Jan 14, 2020

@sainthkh it looks like the PRs you mentioned are merged now. Is this issue fixed, or is there still more work to do? We're blocked on upgrading Cypress to >= 3.8 due to this issue. Thanks!

@sainthkh

This comment has been minimized.

Copy link
Contributor

@sainthkh sainthkh commented Jan 15, 2020

@janakdr I've been spending time on #5762. And my part is finished and I'm waiting for feedback.

If your problem is the test falsely failing, then it is solved in #6000. Please try 3.8.2.

But your idea about creating a visibility result object to determine the reason is not started yet.

Currently, I'm trying to make #619 and planning to fix that with #677.

@janakdr

This comment has been minimized.

Copy link

@janakdr janakdr commented Jan 15, 2020

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.