garris / BackstopJS

Catch CSS curve balls.
http://backstopjs.org
MIT License
6.78k stars 603 forks source link

Issues using vh units in CSS #785

Open justlevine opened 6 years ago

justlevine commented 6 years ago

How do I get backstopJS to respect height: [num]vh in my css?

Theyre getting comically stretched out.

Eg. (viewport: 1366x768, selectors: 'document')

Actual Chrome Browser (1366x768) - showing just the second section, for reference: capture

Those two stretched-out sections have a height:90vh.

garris commented 6 years ago

What happens if your selectors prop is set to body?

justlevine commented 6 years ago

@garris then I just get the viewport instead of the fullPage....

garris commented 6 years ago

please post your config. and version info.

matthew-dean commented 6 years ago

Getting the same issue. Viewport units (maybe just vh) are (more than?) twice what they should be.

matthew-dean commented 6 years ago

As far as I can tell this may be a Chromy issue: https://github.com/OnetapInc/chromy/issues/117

@justlevine are you also using Chromy?

I'd love to just switch to Puppeteer, but it causes a host of other capturing issues that the Chromy plugin doesn't have. (Puppeteer captures incorrect section of the page, rendering the correct dimensions of the element but for the completely wrong coordinates.)

garris commented 6 years ago

Has anyone tried bumping the puppeteer version in backstop? Perhaps there will be some improvements?

JamieMcNaught commented 6 years ago

See my bug #820 - this seems to be an issue with the way Chrome(y) takes the screenshot by resizing the page - you can reproduce the issue in desktop Chrome by using "Capture Full size screenshot" in the "Developer Tools + mobile view". What I think we need to do is screenshot, scroll, screenshot, scroll and then stitch together.

I've no idea of any current + working workaround, but without a work around unfortunately I can't use backstep :( :( :(

matthew-dean commented 6 years ago

@JamieMcNaught Why doesn't Puppeteer have this issue? Only Chromy does, but I can't use the Puppeteer Backstop engine because it has other bugs.

AdrienLemaire commented 5 years ago

I have the same issue, using latest versions of BackstopJS and Puppeteer. BackstopJS v3.8.5 with dependency puppeteer ^1.2.0-next.1523485686787

  "viewports": [
    {
      "label": "mobile",
      "width": 320,
      "height": 480
    },
  ],
  "engine": "puppeteer",
  "onBeforeScript": "puppet/onBefore.js",
  "onReadyScript": "puppet/onReady.js"

style.less

    @media @xs-only, @sm-only {
      margin: 0 -1rem;
      height: 61.5vh;
      border-radius: 0;
      …
    }
martyfmelb commented 5 years ago

I've made a just-good-enough fix for this so that I can snapshot toyota.com.au, which has CSS that heavily uses VH units:

https://github.com/ie/snapsite-vrt/blob/feature/client-agnostic-replace-toyota-com-au/engine_scripts/chromy/onReady.js

(See fixVhHeights for the vh unit fix, and fixPercentageHeights for the % height unit fix.)

Unfortunately this fix crashes on some other sites like lexus.com.au (i.e. no screenshot taken), so your mileage may vary.

offroadkev commented 3 years ago

This issue is from 2018, we're now in 2021, should we abandon hope that backstop can be used as a visual regression tool in real life projects, or is this going to be resolved soon? This whole project is super awesome, but this issue is a stopper for myself and countless others unfortunately.

If this was fixed, backstop would be an amazing tool for all of us front end engineers, however, with this vh issue, I don't see how anyone could use backstop.

Sure there are websites that don't use vh (but most do these days, I would imagine), but imagine you spent the time to add backstop to your first 20+ templates on a project and then you got to one that had vh, boom, you just realized that all of your efforts with backstop were wasted....which is what happened to me.

Could someone from the backstop project chime in please and let us know if this is going to be resolved? Just need to know so that we can decide if we should rip backstop out of our project, or continue to wait for a resolution.

Please advise.

[Edit] @garris I see that you are the maintainer. I personally don't have time to contribute code to the project, but I am happy to do PR reviews and locally test any branches you have laying around that might solve this. I know you work your butt off for this project and like I said it's awesome (except for this blocker), so if you need another set of hands to help review code, please let me know, I'm more than capable and willing to do so.

garris commented 3 years ago

@vegaskev thanks for the help.

This product is 100% free -- it depends on community for support. I would love if the community would self-organize and solve this problem in a non-breaking way.

Please review the PRs and provide feedback. The question I would have is... What solution is the best to resolve this issue and why?

kwaves commented 3 years ago

Here's how I get around this issue:

CSS variables to the rescue

Step 1: Set all vh values with a CSS variable

:root {
  --viewport-height: 100vh;
}

Example usage:

.full-viewport-height {
    min-height: var(--viewport-height, 100vh);
}

.half-viewport-height {
    min-height: calc(var(--viewport-height, 100vh) * .5);
}

Step 2: Hard-code a px-based --viewport-height for your tests

Update the top line of overrideCSS.js to something like this:

const BACKSTOP_TEST_CSS_OVERRIDE = `html { --viewport-height: 1000px !important; } @media screen and (max-width: 361px) { html { --viewport-height: 740px !important; } }`;

The values above should match the dimensions of your viewports in backstop.json. In that example, the viewports were:

{
  "id": "whatever",
  "viewports": [
    {
      "label": "desktop",
      "width": 1440,
      "height": 1000
    },
    {
      "label": "mobile",
      "width": 360,
      "height": 740
    }
  ]
}

The above also assumes you're using onReady.js with the default overrideCSS reference intact.

For Chromy folks, you'd need to adapt overrideCSS.js from the Puppeteer folder.

Side benefit for 100vh mobile UI patterns

This idea occurred to me when I was trying to fix another problem:

As you may know, there's a long-running "feature-not-a-bug" on iPhone (and now Android too) where 100vh ignores the top and bottom browser bars. Those bars jump around as you scroll (and is also directionally-dependent, to my memory). The result is this:

viewport-units-mobile-crop

(Graphic: CSS Tricks)

More example code of the fix here.

Side note: While most people seem to be calculating 1vh then multiplying with calc(), I've found it's usually cleaner to set 100vh and divide, at least for the sorts of problems I'm usually trying to solve.

This doesn't seem like a Backstop problem

Given we're seeing this vh behavior with Chrome's full page screenshot feature in DevTools (as a commenter above pointed out), perhaps this should be addressed in Chromium (or whatever's responsible for this feature)? I'd be surprised if this wasn't a deliberate choice–even off the top of my head I can think of some advantages to handling it the way they currently are–but maybe this could be a candidate for a flag.

offroadkev commented 3 years ago

Thank you @garris and @kwaves