inViewport falsely returns true

pugson commented 8 years ago

I attached verge.inViewport to a scroll event to test this.

When I start scrolling I keep getting true returned even if the element is not in the viewport. Only after I scroll past the desired element, false is returned.

If I scroll back up past that element, it's still true.

ryanve commented 8 years ago

Huh, can you paste the code you used?

ghost commented 8 years ago

@dubstrike Make a jsfiddle/codepen and show the code please.

ramtinsoltani commented 7 years ago

I have the same issue. I have the following JavaScript code:

window.addEventListener('scroll', function(e) {

  let test = $('#testme').get(0);

  if ( verge.inViewport(test) ) {

    test.style.backgroundColor = "blue";
    console.log('TRUE');

  } else {

    test.style.backgroundColor = "red";
    console.log('FALSE');

  }

});

and the following HTML:

<!--A few elements here-->
<div style="height:2000px;"></div> 
<div id="testme" style="width: 100%; height: 500px; background-color: red;"></div>

The result is always true, the console never logs FALSE, and the test element's background color is always blue. Same results with verge.inY(test)!

ryanve commented 7 years ago

I think I was able to reproduce on codepen and I commented what seems potential workarounds there.

Does verge.inViewport(test, -1) works as a solution for you?

ramtinsoltani commented 7 years ago

verge.inViewport(test, -1) does not fix anything and the output is still the same. Console logs TRUE until I scroll down past the element. Scrolling back up past the element would make the console to log TRUE again (no change in behavior with -1).

ryanve commented 7 years ago

@ramtinsoltani Can you reproduce that in codepen please? Either by forking mine or creating new and loading verge there via https://unpkg.com/verge@1.10.2/verge.js

ramtinsoltani commented 7 years ago

@ryanve Well I've made this codepen using the link to load verge, and it's working! But using the same link in a local .html file (or downloading verge.js and loading it locally) still behaves as before! I'm using Chrome 62.0.3202.62 64-bit on Windows 10.

So to reproduce it, copy the code below and save them as local files. Then run the .html file and you should run into the issue I'm having when you look at the Dev Tools console. These are the exact code I'm using locally (same as codepen but the html contains a script tag to load the javascript after loading verge):

index.html

<html>
  <head>
    <script src="https://code.jquery.com/jquery-3.2.1.min.js"></script>
    <script src="https://unpkg.com/verge@1.10.2/verge.js"></script>
    <script src="./script.js"></script>
  </head>
  <body>
    <div style="height:2000px;"></div>
    <div id="testme" style="width: 100%; height: 500px; background-color: red;"></div>
    <div style="height:2000px;"></div>
  </body>
</html>

script.js

window.addEventListener('scroll', function(e) {

  let test = $('#testme').get(0);

  if ( verge.inViewport(test, -1) ) {

    test.style.backgroundColor = "blue";
    console.log('TRUE');

  } else {

    test.style.backgroundColor = "red";
    console.log('FALSE');

  }

});

When you scroll all the way down, these are the different console logs:

Codepen output (the correct output, only logs TRUE when the element is in view):

FALSE
TRUE
FALSE

Local output (the incorrect output, only logs FALSE if you scroll pass the element, the rest of the time it logs TRUE):

TRUE
FALSE

I hope this helps.

ryanve commented 7 years ago

Great repro @ramtinsoltani! I reproduced it now too locally in Windows 7 in Chrome and FF. I isolated it to viewportH() returning a value that's too high. viewportH() uses a max technique due to reasons in S/O and to workaround issues with zooming. But in this case the window.innerHeight is smaller and correct

Math.max(document.documentElement.clientHeight, window.innerHeight || 0)

I used actual to confirm the correct viewport height but it was obviously wrong in my window because it was the result was so huge that it was like the document height

actual("height", "px") // 986
window.innerHeight // 986
$(window).height() // 4516
document.documentElement.clientHeight // 4516

<script src="https://unpkg.com/actual@0.4.0/actual.js"></script>

We could use actual or a similar matchMedia technique optimized for our use case but I wonder if there is an easier solution. Here is one possible solution. Does this make sense?

function viewportH() {
  var docElem = document.documentElement
  var inner = window.innerHeight || 0
  var client = docElem.clientHeight
  var offset = docElem.offsetHeight
  if (inner >= client) return inner
  if (!inner) return client
  if (client < offset) return client
  return inner
}

ryanve commented 7 years ago

Whoa I just figured out why the local differs from the codepen. Local has no doctype, If you add one then it works as expected! Discovered via S/O

<!DOCTYPE html>

So we either could warn about this in the readme and/or attempt a solution that works regardless.

ryanve commented 7 years ago

document.doctype === null when doctype is missing. We could use that to console.warn about it.

ryanve / verge

inViewport falsely returns true #22