ANCHOR SCROLL: WIP - feat($anchorScroll): add support for configurable scroll offset

[gkalpak]

Add support for a configurable scroll offset to $anchorScrollProvider.
The offset is expressed in pixels and can be specified either as a fixed value or as a function that returns the offset it pixels dynamically.
(This is a POC and a WIP: no docs, no tests)

Related to #9368

Closes #2070, #9360

[gkalpak]

Please, don't scream about this. I know :)
(It's ugly, it's not-Angular, it's hacky, it's hurting your eyes and I will remove it asap - just wanted to get some feedback on the offset approach.)

[petebacondarwin]

Or for POC you could have just done:

.config(['$anchorScrollProvider', function($anchorScrollProvider) {
  $anchorScrollProvider.setScrollOffset(function () { return 100; });
});

:-)

[petebacondarwin]

But I guess that you wanted to see it change as the header changed size...

[petebacondarwin]

I think this should be:

function scrollTo(elem) {
  if (elem) {
    elem.scrollIntoView();
    var offset = scrollOffsetGetter();
    if (offset) {
      $window.scrollBy(0, -1 * offset);
    }
  } else {
    $window.scrollTo(0, 0);
  }
}

Otherwise scrolling to the top never gets to 0, no?

[gkalpak]

You are right. I forgot about the .main-body-grid { margin-top: 120px; CSS style.

[petebacondarwin]

I kind of like this...

[petebacondarwin]

Actually the more I think about this, a finessed version of this seems like the right way to go

[gkalpak]

I updated the $anchorScroll docs and implemented the dynamic scroll offset in the docs app in a more proper way.

Unfortunately, there seems to be an issue (for a demo see the example I added in the $anchorScroll docs). In a nutshell, when the scrolled element is not the body, then scrollBy does not work.
(I haven't fully tracked down the problem, so there might be other things playing role.) :(

Possible solutions:

1. Document that in order for scroll offset to work, you need to have the body be scrolled (not some other element). - [Ugly and unconvincing, but easy to (not) implement]
2. Somehow detect the scrolled element and decrement its scrollTop. - [How to detect the scrolled element ? What if there are multiple scrolled elements]
3. Define an acompanying directive, that will designate the scrolled element (i.e. the element from whose scrollTop we should substract the offset). - [Might be the more reliable solution, but requires a new directive and possibly extending the $anchorScroll API]

@petebacondarwin any thoughts

[gkalpak]

I added some code to prevent elements near the bottom of the scrolled element to be scrolled further down than required.

[petebacondarwin]

I made some changes here: gkalpak#1

[gkalpak]

@petebacondarwin , a little update: fb71268

All the functionality should be in place (still missing updated docs and tests).

I tried to do a little research and I came to the conclusion that the most consistent and accurate (at least enough for our purpose) way of aquiring the element's height is using its offsetHeight property. It takes into account content, padding, scrollbars and border (so no box-sizing issues) and works reliably in Chrome 37, Firefox 32 and IE8-IE11.
I used this fiddle (should anyone find it useful).

I also added a helper to calculate the accumulated offsetTop to get the element's top position, because computedStyle.top was failing where top was set to auto.

I put back the fix to avoid scrolling more than it is necessary for elements near the end of the page.

I added a helper function to get an element's [computed]Style, with fallbacks for when getComputedStyle() is not available. More specifically, element.currentStyle (which is proprietary to IE) seems to work as expected in IE8 (where getComputedStyle() is missing) and if currentStyle is not available I fall back to element.style (so as a final resort, one could add inline styles to make the offset work in browsers that support neither getComputedStyle() nor currentStyle - not that I know of any such browser).
(I haven't tested it, but the code should be IE8 compatible, so maybe backportable to 1.2.x.)

Finally, I removed two redundant calls to $anchorScroll() from the docs' examples (since automatic scrolling is not disabled).

---

I have put a couple of large inline comments , which I am not a big fan of. If you have any idea how to make them more elegant or concise...

I will finish up with the docs tonight and write some tests tomorrow.

In the meantime, please feel free to tell me wdyt :)

[gkalpak]

@petebacondarwin
Right now we only need the "raw" DOM element (not the jqLite element), but still expect a jqLite element.
Should we modify the implementation so both raw and jqLite work (or do you think it complicates the "contract" for no real benefit) ?

[petebacondarwin]

Let's leave it as a wrapped element for now, it is easier to add public API that remove it.

[petebacondarwin]

I'll take a look tomorrow. Thanks

[petebacondarwin]

We are not supporting IE8 in 1.3.x so this can go away. Since this is a new feature it won't be backported to 1.2.x

ref: http://caniuse.com/#search=getComputedStyle

[gkalpak]

It never occurred me that 1.2.x only receives fixes (no new features). It makes sense.
No worries for IE8 then (yay) ! I'll remove the unnecessary code.

[gkalpak]

@caitp : Aha!

@petebacondarwin :
There is an issue with Firefox and IE (but not Chrome):
When loading a page with some hash in the URL, Firefox and IE seem to do the "native scrolling"
AFTER $anchorScroll has done its scrolling, thus ruining the yOffset.
E.g. if you navigate to http://localhost:8000/build/docs/api/ng/service/$anchorScroll#usage it will initially scroll correctly (accounting for the yOffset), but then instantly Firefox will scroll the element to the top (not aware of the yOffset). Subsequent calls to $anchorScroll() work as expected.
If you first navigate to http://localhost:8000/build/docs/api/ng/service/$anchorScroll, then append #usage to the URL and press ENTER, it works as expected.

It probably has something to do with Firefox (and IE) doing their native scrolling after something has happened (but I haven't found out what that something is - tried to wait for the load event but no luck).

The only workaround I have found thus far is changing the scrollYOffsetElement like this (but I am too reluctant to push this - it's ugly and unreliable):

.directive('scrollYOffsetElement', ['$anchorScroll', '$timeout', function($anchorScroll, $timeout) {
  return function(scope, element) {
    $anchorScroll.yOffset = element;
   $timeout($anchorScroll, 1000);
  };
}]);

Ideas anyone ?

[petebacondarwin]

I believe Igor has a fix for this. ..
On 7 Oct 2014 19:02, "Georgios Kalpakas" notifications@github.com wrote:

@caitp https://github.com/caitp : Aha!

@petebacondarwin https://github.com/petebacondarwin :
There is an issue with Firefox and IE (but not Chrome):
When loading a page with some hash in the URL, Firefox and IE seem to do
the "native scrolling"
AFTER $anchorScroll has done its scrolling, thus ruining the yOffset.
E.g. if you navigate to
http://localhost:8000/build/docs/api/ng/service/$anchorScroll#usage it
will initially scroll correctly (accounting for the yOffset), but then
instantly Firefox will scroll the element to the top (not aware of the
yOffset). Subsequent calls to $anchorScroll() work as expected.
If you first navigate to
http://localhost:8000/build/docs/api/ng/service/$anchorScroll, then
append #usage to the URL and press ENTER, it works as expected.

It probably has something to do with Firefox (and IE) doing their native
scrolling after something has happened (but I haven't found out what that
something is - tried to wait for the load event but no luck).

The only workaround I have found thus far is changing the
scrollYOffsetElement like this (but I am too reluctant to push this -
it's ugly and unreliable):

.directive('scrollYOffsetElement', ['$anchorScroll', '$timeout', function($anchorScroll, $timeout) {
return function(scope, element) {
$anchorScroll.yOffset = element;
$timeout($anchorScroll, 1000);
};
}]);

Ideas anyone ?

—
Reply to this email directly or view it on GitHub
#9371 (comment).

[gkalpak]

Little update on the Firefox/IE issue
Curiously enough I can't seem to be able to reproduce on IE any more. It's just a Firefox issue now.
Seems to be related to the readyState of the document.

[gkalpak]

Does Igor have a drawer with fixes for all sorts of stuff or something :P

[gkalpak]

@petebacondarwin : Delaying scrolling until readyState === 'complete' solves the problem on Firefox but not in IE (I was able to re-reproduce it on IE).
On IE, the issue appears when navigating to the page, but not when refreshing (while already on the page) although the page is reloaded anyway.

[petebacondarwin]

The fix I was thinking of was #9393 but it is probably not relevant to this problem. I will take a look this morning.

[gkalpak]

@petebacondarwin : I pushed gkalpak@8967fd7 that fixes the issue in Firefox.

(As mentioned earlier, applying a sufficient timeout works around the issue, but...)

[gkalpak]

@petebacondarwin : Yay, tests pass !
A couple of questions:

1. Is there any easy way to test that scrolling is "deferred" until after readyState === 'complete' ?
   
   E.g. somehow providing arbitrary values for document.readyState, but without having to mock all methods of document used by $anchorScroll[Provider].
2. Is there a more efficient way to test some change in the docs app than grunt package + wait until its packaged + wait some more + oh, I am still waiting...

[petebacondarwin]

1. ... I think mocking is the way forward
2. If you are only making changes to the app code and not regenerating the docs you can do

cd docs
gulp build-app

If you made changes to templates and need to rebuild docs you can do

grunt docs

Or

cd docs
gulp doc-gen

or to rebuld docs and generate the app

cd docs
gulp

You get the idea. It is good to run grunt package now and again any way to ensure that you have a clean full build.

[gkalpak]

BTW, I pushed a commit with some tests regarding document.readyState (although one of them might be flawed - looking into it).

(Thx, @petebacondarwin , for the docs-app-building info !)

[petebacondarwin]

So this function is called from within a $watch handler so it is already in a digest phase, but the readystatechange handler is not inside a $digest. So I think this should look like:

function scrollWhenReady() {
  if (document.readyState === 'complete') {
    $rootScope.$evalAsync(scroll);
  } else if (!scrollScheduled) {
    scrollScheduled = true;
    document.addEventListener('readystatechange', function unbindAndScroll() {
      $rootScope.$apply(function() {
        if (document.readyState === 'complete') {
          document.removeEventListener('readystatechange', unbindAndScroll);
          scroll();
        }
      });
    });
  }
}

[petebacondarwin]

For good measure it might be best to set scrollScheduled back to false in the handler just in case in the future we want to call it again.

[gkalpak]

WARNING: Although the tests passed, when I run them locally, I come across some inconsistent behaviour with Firefox:
Sometimes all tests pass, sometimes some tests fail (the number of failing tests varies from 1 to ~20).

[petebacondarwin]

I'll take a look tonight
On 8 Oct 2014 17:52, "Georgios Kalpakas" notifications@github.com wrote:

WARNING: Although the tests passed, when I run them locally, I come across
some inconsistent behaviour with Firefox:
Sometimes all tests pass, sometimes some tests fail (the number of failing
tests varies from 1 to ~20).

—
Reply to this email directly or view it on GitHub
#9371 (comment).

[gkalpak]

You are right :(

[gkalpak]

Closing this in favor of #9517.