Snap aborting without proper dimensions

[rodneyrehm]

I'm using {snap: 'li'} on a horizontally scrolled <ul>. As wrapper and items are positioned absolutely, I've not cared to give the wrapper a height. This resulted in the snap mode not engaging.

If your requirements are not satisfied, tell the developer why. If you fail silently, you send them on a wild goose chase. Consider the following code in place of that silent return:

if ( !this.wrapperWidth || !this.wrapperHeight || !this.scrollerWidth || !this.scrollerHeight ) {
  throw new Error("snap requires wrapper and scroller to have a defined width and height");
}

Alternatively you could log to the console:

if ( !this.wrapperWidth || !this.wrapperHeight || !this.scrollerWidth || !this.scrollerHeight ) {
  window.console && console.warn("snap requires wrapper and scroller to have a defined width and height");
  return;
}

I haven't sent a PR because you haven't yet defined any error feedback handling. I didn't want to impose a certain style, so pick what you like better.

Throwing an error has the advantage that your code doesn't have to handle faulty configuration (like resetting this.options.snap, or making sure that next() doesn't call goToPage() with NaN). It has the downside of working only in synchronous code. As all your event's are executed synchronously, the initialization is safe to throw errors. An error thrown in an event handler might be slightly more confusing, as showing a usable stack trace is up to the browser.

If you go with the console.warn option make sure to clean the configuration. In the case above you'd want to delete this.options.snap;.

I also recommend adding safeguards to next(), prev(), goToPage() to abort if the instance is not using snap:

{
  next: function (time, easing) {
    if (!this.pages.length) {
      throw new Error("No pages available to navigate, forgot to specify snap?");
    }
    // actual next() handling
  }
}

[cubiq]

the wrapper should have a width also if absolute positioned. the problem is probably with the LIs. Could you provide a demo?

The error reporting would be very problematic since it's very hard to foresee CSS issues from javascript. Anyway I've never been asked about it before, if there's interest I'll work on it.

[rodneyrehm]

Example on JSBin - when you try to scroll the list, the console will tell you TypeError: Cannot read property 'length' of undefined on line 1291. This is not immediately helping unless you know what iScroll does internally (or you go examine the code).

I don't see how this is a "CSS issue". Your code expects and element to have both dimensions set. it doesn't care about the exact values at this point, only that they're set. If they're not set, your code doesn't function properly (or as the developer expected). That is what you're guarding against. It's all about pointing the developer in the right direction if your code detected that something is wrong. See Handling Errors.

[cubiq]

the same error can be triggered by many CSS configurations and it's hard to foresee which is the exact cause. For example it's common that people use float left on LIs but not on the wrapper. The end result is that the wrapper is larger than the scroller but the problem is not actually an undefined width/height.

This is what I mean when I say that it's hard to find the exact CSS issue that caused the problem from the code.

[rodneyrehm]

I see what you're getting at. I agree, you don't want to test all these things. You shouldn't. What you should explain, though, are your expectations.

Your code doesn't care how I got these elements in line. (float, nowrap, flexbox, absolute position, …) - it just cares about the dimensions of two elements. And that is what you report on. It's up to the developer to figure out why a dimension is not set, it is ob to you to complain about the missing dimension.

[cubiq]

Cannot read property 'length' of undefined is a very bad error message and should be fixed. Also I think I've found a solution to 99% of use cases by using scrollwidth but I have to make extensive tests on the usual 200 devices.

Re error reporting, it is very hard to give proper feedback. "snap requires wrapper and scroller to have a defined width and height" might be misleading if one has actually defined the width of the wrapper. I could say "wrapper width/height must be bigger smaller than scroller width/height" which is pretty generic if when looking at the browser you see the scroller being actually bigger.

I hope to address these situations in the documentation. I agree proper error handling would be desirable.

[rodneyrehm]

wrapper width/height must be bigger than scroller width/height which is pretty generic if when looking at the browser you see the scroller being actually bigger.

a) it's not what your code is testing for b) it doesn't say that scroller needs a width > 0

The scroller is the content of the wrapper, right? in that case doesn't the scroller have to be bigger than the wrapper? I believe "scroller" and "wrapper" are ambiguous terms. How about "scrollContainer" and "scrollElement"?

[cubiq]

sorry pal, I meant "smaller"