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

Add node-scroll-info component #217

Merged
merged 7 commits into from Sep 18, 2012

Conversation

Projects
None yet
9 participants
@rgrove
Contributor

rgrove commented Aug 8, 2012

ScrollInfo is a Y.Node plugin that provides convenient events and methods related to scrolling. This could be used, for example, to implement infinite scrolling, or to lazy-load content based on the current scroll position. Performance is a key concern when executing code in response to scroll events, so ScrollInfo takes great care to be as efficient as possible.

ImageLoader could be refactored to use ScrollInfo, and it could be useful for ScrollView as well. It's currently used in production on SmugMug's search page, and Flickr has also expressed interest in using it.

Running Locally

The built node-scroll-info module and updated yui and loader build files are intentionally not included in order to keep the diffs clean. Before testing these changes, you'll need to build them:

$ cd src/node-scroll-info && ant all
$ cd ../yui && ant all
@rgrove

This comment has been minimized.

Show comment
Hide comment
@rgrove

rgrove Aug 8, 2012

Owner

Lulz. Thanks for making that a mention, GitHub.

Owner

rgrove commented on 9aaeacd Aug 8, 2012

Lulz. Thanks for making that a mention, GitHub.

This comment has been minimized.

Show comment
Hide comment
@ericf

ericf Aug 8, 2012

This happened to me before, same reason :)

ericf replied Aug 8, 2012

This happened to me before, same reason :)

// browsers don't, so we have to use the documentElement.
return this._hostIsBody && !Y.UA.webkit ? Y.config.doc.documentElement :
Y.Node.getDOMNode(this._host);
},

This comment has been minimized.

@ericf

ericf Aug 8, 2012

Member

Is there a feature detection alternative to determining this, instead of looking at Y.UA?

@ericf

ericf Aug 8, 2012

Member

Is there a feature detection alternative to determining this, instead of looking at Y.UA?

This comment has been minimized.

@rgrove

rgrove Aug 8, 2012

Contributor

Not without actually scrolling the viewport, which A) may not be possible if there's not enough content to make it scrollable, and B) would suck for the user.

For example, in Firefox, document.body.scrollTop will always return 0 even if the page is scrolled down. There's no way to tell that this is incorrect unless we explicitly scroll the page down and then verify that scrollTop returns the expected value, but this would be really annoying to the user, and impossible if the page isn't scrollable.

@rgrove

rgrove Aug 8, 2012

Contributor

Not without actually scrolling the viewport, which A) may not be possible if there's not enough content to make it scrollable, and B) would suck for the user.

For example, in Firefox, document.body.scrollTop will always return 0 even if the page is scrolled down. There's no way to tell that this is incorrect unless we explicitly scroll the page down and then verify that scrollTop returns the expected value, but this would be really annoying to the user, and impossible if the page isn't scrollable.

This comment has been minimized.

@ericf

ericf Aug 8, 2012

Member

Ah yes, the DOM and all its glory :)

I just wanted to be sure there wasn't a suitable way without having to sniff the browser.

@ericf

ericf Aug 8, 2012

Member

Ah yes, the DOM and all its glory :)

I just wanted to be sure there wasn't a suitable way without having to sniff the browser.

This comment has been minimized.

@msweeney

msweeney Aug 8, 2012

Contributor

Unless I'm misunderstanding the use-case, calling Y.DOM.getDocScrollX() ("dom-screen" module) or node.get('docScrollX') ("node-screen" module) will return a normalized scrollTop value for the document.

@msweeney

msweeney Aug 8, 2012

Contributor

Unless I'm misunderstanding the use-case, calling Y.DOM.getDocScrollX() ("dom-screen" module) or node.get('docScrollX') ("node-screen" module) will return a normalized scrollTop value for the document.

Show outdated Hide outdated src/node-scroll-info/js/node-scroll-info.js
this._left = el.offsetLeft;
this._top = el.offsetTop;
this._width = el.clientWidth;
},

This comment has been minimized.

@ericf

ericf Aug 8, 2012

Member

This may not be accurate in IE, which does some weird thing with offsets where it adds an extra 2px or something. @msweeney would know better than me if this could be an actual issue; I remember him accounting for it in Y.Node.

@ericf

ericf Aug 8, 2012

Member

This may not be accurate in IE, which does some weird thing with offsets where it adds an extra 2px or something. @msweeney would know better than me if this could be an actual issue; I remember him accounting for it in Y.Node.

This comment has been minimized.

@rgrove

rgrove Aug 8, 2012

Contributor

Seems to work in my testing. I don't want to go through Y.DOM here for performance reasons, but I guess I could if it's really a problem.

@rgrove

rgrove Aug 8, 2012

Contributor

Seems to work in my testing. I don't want to go through Y.DOM here for performance reasons, but I guess I could if it's really a problem.

This comment has been minimized.

@ericf

ericf Aug 8, 2012

Member

I was remembering this commit: 8abfde4 which looks like a switch to using clientTop and clientLeft which gets around the IE documentElement border issue.

@ericf

ericf Aug 8, 2012

Member

I was remembering this commit: 8abfde4 which looks like a switch to using clientTop and clientLeft which gets around the IE documentElement border issue.

@msweeney

This comment has been minimized.

Show comment
Hide comment
@msweeney

msweeney Aug 8, 2012

Contributor

Hey Ryan, thanks for the pull request.

Let's start with adding this to the gallery and go from there. If/when we want to add these features to ScrollView we can consider moving to core. I think ImageLoader should be moved to gallery in the near future.

Contributor

msweeney commented Aug 8, 2012

Hey Ryan, thanks for the pull request.

Let's start with adding this to the gallery and go from there. If/when we want to add these features to ScrollView we can consider moving to core. I think ImageLoader should be moved to gallery in the near future.

@rgrove

This comment has been minimized.

Show comment
Hide comment
@rgrove

rgrove Aug 8, 2012

Contributor

Hey Matt. I understand the desire to be selective about what goes into core. That said, I'm not interested in maintaining this (or other modules) as gallery modules right now for the following reasons:

  1. The current workflow for adding and maintaining a module in the gallery requires much more effort than adding and maintaining a module in core. This adds a lot of overhead for me and for my employer, who graciously allows me to spend company time contributing to YUI.
  2. Modules in the gallery are not currently easy to discover. Having been involved in the creation of yuilibrary.com, I know that there have long been plans to improve this, but until those improvements actually occur the likelihood of anyone finding and using a gallery module like this is slim unless the author actively evangelizes it. While I know the YUI team doesn't see it this way, this effectively makes gallery modules second-class citizens and less worth a contributor's time and effort.
  3. If I want to provide easily browsable online documentation and issue-reporting functionality for a gallery module, I currently have to set this up and manage it all myself. GitHub repos are ideal for this, except that getting my gallery module on the CDN requires me to maintain it in a fork of the yui3-gallery repo, forcing me to either double-edit all my changes or set up build scripts to do this work for me, and complicating the process of releasing updates. Yet another time-sink.

I know the team is working on improving these things, but in the meantime, please understand that these are serious barriers for people who either contribute during their own free time or are lucky enough to be able to spend a portion of their working hours contributing to YUI, and in some cases (like this one), a contributor may not feel the effort and time required are worthwhile.

Contributor

rgrove commented Aug 8, 2012

Hey Matt. I understand the desire to be selective about what goes into core. That said, I'm not interested in maintaining this (or other modules) as gallery modules right now for the following reasons:

  1. The current workflow for adding and maintaining a module in the gallery requires much more effort than adding and maintaining a module in core. This adds a lot of overhead for me and for my employer, who graciously allows me to spend company time contributing to YUI.
  2. Modules in the gallery are not currently easy to discover. Having been involved in the creation of yuilibrary.com, I know that there have long been plans to improve this, but until those improvements actually occur the likelihood of anyone finding and using a gallery module like this is slim unless the author actively evangelizes it. While I know the YUI team doesn't see it this way, this effectively makes gallery modules second-class citizens and less worth a contributor's time and effort.
  3. If I want to provide easily browsable online documentation and issue-reporting functionality for a gallery module, I currently have to set this up and manage it all myself. GitHub repos are ideal for this, except that getting my gallery module on the CDN requires me to maintain it in a fork of the yui3-gallery repo, forcing me to either double-edit all my changes or set up build scripts to do this work for me, and complicating the process of releasing updates. Yet another time-sink.

I know the team is working on improving these things, but in the meantime, please understand that these are serious barriers for people who either contribute during their own free time or are lucky enough to be able to spend a portion of their working hours contributing to YUI, and in some cases (like this one), a contributor may not feel the effort and time required are worthwhile.

@rgrove

This comment has been minimized.

Show comment
Hide comment
@rgrove

rgrove Aug 10, 2012

Contributor

More about how we're using this here: http://sorcery.smugmug.com/2012/08/09/speeding-up-smugmug-search/

The more I think about this, the more I think there's a strong argument for adding this to core (beyond just my dissatisfaction with the gallery). Infinite scrolling and content lazy-loading are interactions that are becoming more and more common, particularly in mobile-friendly sites. Scroll metrics are relatively easy to gather, but not so easy to gather efficiently, so this is a problem that tends to result in lots of inefficient one-off implementations (even within YUI itself!).

I think there's value in YUI having a well-tested, efficient plugin for gathering scroll metrics. If it's right there in the core and super easy to use, there's no reason for any YUI user to roll their own implementation.

Contributor

rgrove commented Aug 10, 2012

More about how we're using this here: http://sorcery.smugmug.com/2012/08/09/speeding-up-smugmug-search/

The more I think about this, the more I think there's a strong argument for adding this to core (beyond just my dissatisfaction with the gallery). Infinite scrolling and content lazy-loading are interactions that are becoming more and more common, particularly in mobile-friendly sites. Scroll metrics are relatively easy to gather, but not so easy to gather efficiently, so this is a problem that tends to result in lots of inefficient one-off implementations (even within YUI itself!).

I think there's value in YUI having a well-tested, efficient plugin for gathering scroll metrics. If it's right there in the core and super easy to use, there's no reason for any YUI user to roll their own implementation.

@davglass

This comment has been minimized.

Show comment
Hide comment
@davglass

davglass Aug 10, 2012

Member

I agree with @rgrove on this one. This should be in core, I can see it's use in a few core modules already. It's small, well tested and useful out of the box.

Member

davglass commented Aug 10, 2012

I agree with @rgrove on this one. This should be in core, I can see it's use in a few core modules already. It's small, well tested and useful out of the box.

@ericf

This comment has been minimized.

Show comment
Hide comment
@ericf

ericf Aug 10, 2012

Member

+1 for that, I imagine using this in a lot of apps. The two that I'm already planning to use this in are the YUI website, and PNM. In fact in almost any web site/app I've done in the last few years, I have some form of this.

Member

ericf commented Aug 10, 2012

+1 for that, I imagine using this in a lot of apps. The two that I'm already planning to use this in are the YUI website, and PNM. In fact in almost any web site/app I've done in the last few years, I have some form of this.

@jconniff

This comment has been minimized.

Show comment
Hide comment
@jconniff

jconniff Aug 10, 2012

Contributor

Has anyone tested this at different zoom levels? I've had some problems with that on different browsers and on iPad.

Contributor

jconniff commented Aug 10, 2012

Has anyone tested this at different zoom levels? I've had some problems with that on different browsers and on iPad.

@rgrove

This comment has been minimized.

Show comment
Hide comment
@rgrove

rgrove Aug 10, 2012

Contributor

@jconniff: Yep, I've used it at various zoom levels and it seems to work fine. Do you remember what kinds of issues you were seeing? If there are specific things to look for, I can test those and verify that everything works.

Contributor

rgrove commented Aug 10, 2012

@jconniff: Yep, I've used it at various zoom levels and it seems to work fine. Do you remember what kinds of issues you were seeing? If there are specific things to look for, I can test those and verify that everything works.

@apm

This comment has been minimized.

Show comment
Hide comment
@apm

apm Aug 10, 2012

Contributor

Not really the same thing, but I see http://yuilibrary.com/projects/yui3/ticket/2531804 all of the time now. I'm always zooming my browser either because my screen has smaller pixels or because my eyesight is failing.

Contributor

apm commented Aug 10, 2012

Not really the same thing, but I see http://yuilibrary.com/projects/yui3/ticket/2531804 all of the time now. I'm always zooming my browser either because my screen has smaller pixels or because my eyesight is failing.

@jconniff

This comment has been minimized.

Show comment
Hide comment
@rgrove

This comment has been minimized.

Show comment
Hide comment
@rgrove

rgrove Aug 10, 2012

Contributor

Thanks @jconniff. I did some testing and it turns out metrics can be very slightly off when zoomed in (looks like it's due to rounding errors in the browser itself), but the error amounts are on the scale of a few pixels, which isn't enough to be a problem in normal usage. They're only really an issue in unit tests that expect exact values.

During this testing, I did notice that the way I was measuring viewport size (and also the way Y.DOM measures viewport size) is inaccurate in iOS, so I've fixed that.

Contributor

rgrove commented Aug 10, 2012

Thanks @jconniff. I did some testing and it turns out metrics can be very slightly off when zoomed in (looks like it's due to rounding errors in the browser itself), but the error amounts are on the scale of a few pixels, which isn't enough to be a problem in normal usage. They're only really an issue in unit tests that expect exact values.

During this testing, I did notice that the way I was measuring viewport size (and also the way Y.DOM measures viewport size) is inaccurate in iOS, so I've fixed that.

@see scrollMargin
**/
getOffscreenNodes: function (selector, margin) {
if (typeof margin === 'undefined') {

This comment has been minimized.

@msweeney

msweeney Aug 15, 2012

Contributor

In order to function as documented, the selector needs to fallback to "*" (asterisk), otherwise an empty Y.NodeList instance will be returned.

Ditto for getOnScreenNodes.

@msweeney

msweeney Aug 15, 2012

Contributor

In order to function as documented, the selector needs to fallback to "*" (asterisk), otherwise an empty Y.NodeList instance will be returned.

Ditto for getOnScreenNodes.

This comment has been minimized.

@msweeney

msweeney Aug 15, 2012

Contributor

The bulk of the implementations for both getOffScreenNodes and getOnScreenNodes seem like they could be replaced with Y.DOM.inRegion(). I'd like to at least capture any issues preventing it from being used in this context.

@msweeney

msweeney Aug 15, 2012

Contributor

The bulk of the implementations for both getOffScreenNodes and getOnScreenNodes seem like they could be replaced with Y.DOM.inRegion(). I'd like to at least capture any issues preventing it from being used in this context.

This comment has been minimized.

@rgrove

rgrove Aug 15, 2012

Contributor

Good catch, I'll have the selector fall back to *.

The only issue preventing me from using Y.DOM.inRegion() here is performance: http://jsperf.com/y-dom-inregion-vs-node-scroll-info

@rgrove

rgrove Aug 15, 2012

Contributor

Good catch, I'll have the selector fall back to *.

The only issue preventing me from using Y.DOM.inRegion() here is performance: http://jsperf.com/y-dom-inregion-vs-node-scroll-info

This comment has been minimized.

@msweeney

msweeney Aug 15, 2012

Contributor

Thanks for the pointer to the perf test. I don't think inRegion has been touched since it landed with the original DD drop. I'll see about optimizing.

Feel free to send along any additional performance issues that you've encountered so that we can address those with our ongoing core performance tuning efforts.

@msweeney

msweeney Aug 15, 2012

Contributor

Thanks for the pointer to the perf test. I don't think inRegion has been touched since it landed with the original DD drop. I'll see about optimizing.

Feel free to send along any additional performance issues that you've encountered so that we can address those with our ongoing core performance tuning efforts.

This comment has been minimized.

@rgrove

rgrove Aug 15, 2012

Contributor

Thanks for the pointer to the perf test. I don't think inRegion has been touched since it landed with the original DD drop. I'll see about optimizing.

Sweet!

Feel free to send along any additional performance issues that you've encountered so that we can address those with our ongoing core performance tuning efforts.

Already sent one perf pull request today, and there are more in the works. :)

@rgrove

rgrove Aug 15, 2012

Contributor

Thanks for the pointer to the perf test. I don't think inRegion has been touched since it landed with the original DD drop. I'll see about optimizing.

Sweet!

Feel free to send along any additional performance issues that you've encountered so that we can address those with our ongoing core performance tuning efforts.

Already sent one perf pull request today, and there are more in the works. :)

@travisbot

This comment has been minimized.

Show comment
Hide comment
@travisbot

travisbot Aug 15, 2012

This pull request fails (merged 45f5030 into d1b10fa).

This pull request fails (merged 45f5030 into d1b10fa).

@src-code

This comment has been minimized.

Show comment
Hide comment
@src-code

src-code Aug 20, 2012

I'd love to see this in core - I know of at least two teams (including my own) within Y! that have used or will be making use of this library already. Performant scroll bindings are something I've seen done poorly many times in the last few years, and it's only getting worse now with things like infinite scroll becoming popular, so the sooner this is made widely available, the better.

I'd love to see this in core - I know of at least two teams (including my own) within Y! that have used or will be making use of this library already. Performant scroll bindings are something I've seen done poorly many times in the last few years, and it's only getting worse now with things like infinite scroll becoming popular, so the sooner this is made widely available, the better.

@ghost ghost assigned msweeney Sep 6, 2012

@msweeney

This comment has been minimized.

Show comment
Hide comment
@msweeney

msweeney Sep 6, 2012

Contributor

While I disagree with the assumption that since Gallery sucks everything you contribute must land in core, I agree that this is a nice addition to the library, and would like to bring into core with some tweaks.

I've done some optimizing of Y.DOM.inRegion in a branch and would like to complete that and fix the viewport sizing rather than manage your one-offs.

Additionally, it seems like a utility for getting onscreen/offscreen nodes, something like Y.DOM.getOn(/Off)ScreenNodes() would be useful to drive the plugin and allow for other low-level usage.

Contributor

msweeney commented Sep 6, 2012

While I disagree with the assumption that since Gallery sucks everything you contribute must land in core, I agree that this is a nice addition to the library, and would like to bring into core with some tweaks.

I've done some optimizing of Y.DOM.inRegion in a branch and would like to complete that and fix the viewport sizing rather than manage your one-offs.

Additionally, it seems like a utility for getting onscreen/offscreen nodes, something like Y.DOM.getOn(/Off)ScreenNodes() would be useful to drive the plugin and allow for other low-level usage.

@rgrove

This comment has been minimized.

Show comment
Hide comment
@rgrove

rgrove Sep 6, 2012

Contributor

@msweeney: Thanks, but please re-read my comment above. I never asserted that everything I contribute must land in core. I only said I wasn't interested in putting this in the gallery, because I don't think it's worth my time. I think this should land in core because many people (again, see comments above) have agreed that it would be useful in core.

For the sake of clarity: are you saying you won't accept this pull request unless the functionality for getting onscreen/offscreen nodes is broken out into Y.DOM?

Contributor

rgrove commented Sep 6, 2012

@msweeney: Thanks, but please re-read my comment above. I never asserted that everything I contribute must land in core. I only said I wasn't interested in putting this in the gallery, because I don't think it's worth my time. I think this should land in core because many people (again, see comments above) have agreed that it would be useful in core.

For the sake of clarity: are you saying you won't accept this pull request unless the functionality for getting onscreen/offscreen nodes is broken out into Y.DOM?

@msweeney

This comment has been minimized.

Show comment
Hide comment
@msweeney

msweeney Sep 6, 2012

Contributor

The getOn/OffScreenNodes is nice to have, but I don't see that an intial requirement to land this in core.

But I'd prefer to fix what is already in core (inRegion, winHeight/Width) rather than manage one-offs. This seems like an ideal opportunity to address any issues you've encountered in Y.DOM.

Contributor

msweeney commented Sep 6, 2012

The getOn/OffScreenNodes is nice to have, but I don't see that an intial requirement to land this in core.

But I'd prefer to fix what is already in core (inRegion, winHeight/Width) rather than manage one-offs. This seems like an ideal opportunity to address any issues you've encountered in Y.DOM.

@rgrove

This comment has been minimized.

Show comment
Hide comment
@rgrove

rgrove Sep 6, 2012

Contributor

Agreed. If the iOS winHeight/Width bug can be fixed and if the performance of Y.DOM.inRegion() improves, I'll happily use those.

Contributor

rgrove commented Sep 6, 2012

Agreed. If the iOS winHeight/Width bug can be fixed and if the performance of Y.DOM.inRegion() improves, I'll happily use those.

@msweeney

This comment has been minimized.

Show comment
Hide comment
@msweeney

msweeney Sep 6, 2012

Contributor

DO you have tests that verify the accuracy of your winHeight/Width and inRegion one-offs? I'd like to add those to the dom-screen test suite to be sure all of your cases are covered.

Contributor

msweeney commented Sep 6, 2012

DO you have tests that verify the accuracy of your winHeight/Width and inRegion one-offs? I'd like to add those to the dom-screen test suite to be sure all of your cases are covered.

@rgrove

This comment has been minimized.

Show comment
Hide comment
@rgrove

rgrove Sep 6, 2012

Contributor

My inRegion() implementation is covered by node-scroll-info's unit tests. It's tested by placing marker elements inside and outside the onscreen region, scrolling to a given coordinate, and then verifying that the expected onscreen and offscreen nodes are returned.

I can't think of a reliable way to write an automated test for winHeight/Width, since it would require prior knowledge of the correct viewport dimensions for the device being tested. Short of using the same APIs being tested, there's no way to get this information at runtime. I suppose you could create a document 100,000 pixels tall/wide and then verify that the winHeight/Width numbers are more sane than 100,000, but that would only verify that they're not broken in one particular way. It wouldn't verify that they're actually accurate -- that would require manual verification.

Contributor

rgrove commented Sep 6, 2012

My inRegion() implementation is covered by node-scroll-info's unit tests. It's tested by placing marker elements inside and outside the onscreen region, scrolling to a given coordinate, and then verifying that the expected onscreen and offscreen nodes are returned.

I can't think of a reliable way to write an automated test for winHeight/Width, since it would require prior knowledge of the correct viewport dimensions for the device being tested. Short of using the same APIs being tested, there's no way to get this information at runtime. I suppose you could create a document 100,000 pixels tall/wide and then verify that the winHeight/Width numbers are more sane than 100,000, but that would only verify that they're not broken in one particular way. It wouldn't verify that they're actually accurate -- that would require manual verification.

@msweeney

This comment has been minimized.

Show comment
Hide comment
@msweeney

msweeney Sep 6, 2012

Contributor

No worries, I can add the winSize tests, I just wanted to make sure I didn't miss any of your cases, primarily regarding winHeight/Width (in)accuracy and zooming, which, from my testing, is where iOS goes awry.

Contributor

msweeney commented Sep 6, 2012

No worries, I can add the winSize tests, I just wanted to make sure I didn't miss any of your cases, primarily regarding winHeight/Width (in)accuracy and zooming, which, from my testing, is where iOS goes awry.

@msweeney

This comment has been minimized.

Show comment
Hide comment
@msweeney

msweeney Sep 8, 2012

Contributor

I've merged this into the 3.x branch so that it will be available in next week's preview release.

Contributor

msweeney commented Sep 8, 2012

I've merged this into the 3.x branch so that it will be available in next week's preview release.

@yuibuild yuibuild merged commit 45f5030 into yui:master Sep 18, 2012

1 check failed

default The Travis build failed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment