Anchor scroll offset #9517

petebacondarwin · 2014-10-09T13:52:51Z

No description provided.

tbosch · 2014-10-09T19:28:35Z

docs/app/src/app.js

 .config(['$locationProvider', function($locationProvider) {
  $locationProvider.html5Mode(true).hashPrefix('!');
-}]);
+}]);


Missing newline

tbosch · 2014-10-09T19:38:54Z

test/ng/anchorScrollSpec.js

+  });
+
+
+  // TODO: Add tests for <body> with:


Will you add those tests? Is there a problem?

petebacondarwin · 2014-10-09T20:02:46Z

test/ng/anchorScrollSpec.js

+    var mockedDoc = {};
+    docSpies = {};
+
+    initialReadyState = initialReadyState || 'complete';


I don't think we need this line as it is covered below when assigning the mockedDoc.readyState, no?

True. Will remove it.

tbosch · 2014-10-09T21:11:19Z

src/ng/anchorScroll.js

+          var rect = elem.getBoundingClientRect();
+          var top = rect.top;
+          var height = rect.height;
+          offset = top + height;


Why not use rect.bottom?

Why indeed ?! (Changed it.)

tbosch · 2014-10-09T21:31:47Z

test/ng/anchorScrollSpec.js


        elmSpy[identifier] = spyOn(elm, 'scrollIntoView');
-        jqLite(document.body).append(jqElm);
+        jqLite($window.document.body).append(jqElm);


Do we remove those elements from the document at the end of the tests?

I don't think so. We do dealoc($document), but that doesn't remove them if I am not mistaken.
Do we need to remove them ? They don't seem to stay in the DOM anyway (and the original "pre-yOffset" tests didn't remove them either).

So then dealoc($document) seems to work...

tbosch · 2014-10-09T22:11:46Z

test/ng/anchorScrollSpec.js

+          addElements('id=some1', 'id=some2'),
+          function($window) {
+            // Make sure the elements are just a little shorter than the viewport height
+            var viewportHeight = $window.document.documentElement.clientHeight;


Could we just mock out getBoundingClientRect so that we don't have to deal with css sizing here? This would make the test more robust...

Now that the expression for calculating necessaryOffset only depends on getBoundingClientRect(), we can indeed mock that. But what's the fun in it ?

I believe at some point it would make sense to have some e2e tests for this (but putting it in the docs app is kind of too much). Maybe when this #9527 gets implemented, we could add some e2e tests and make these unit tests more "unit"-y (and robust).
For now I think it's a good thing that we have some minimal interaction with the actual functions.

WDYT ?

Let's keep it simple for now and don't do an integration test, as this would also make the test simpler. We did the same for $browser, and so far it turned out to be a good thing.

But yes, we should create a new issue to add e2e tests (reference #9527 in it) for the whole $anchorScroll service, where we also test the scrolling itself, and not just the calculation of the scroll position.

Sorry, I am not sure what yu mean: Shall we mock getBoundingClientRect() or keep the current approach (for now at least) ?

Please mock it.

tbosch · 2014-10-09T22:14:40Z

@petebacondarwin I left all of my ideas as comments. Could you ping me when they are resolved?
Btw, where is the up to date PR?

petebacondarwin · 2014-10-10T12:58:43Z

test/ng/anchorScrollSpec.js


+        expect(windowSpies.removeEventListener.callCount).toBe(1);
+        expect(windowSpies.removeEventListener).
+            toHaveBeenCalledWith('load', jasmine.any(Function), false);



These checks are not really valid since jqLite actually passes its own callback which it then uses to iterates through all the handlers it knows about so if another party had added an event handler too then removeEventListener would not be called.

Indeed :(
Right now this is not the case (I tried it and it works (with Igor's fix), but we wouldn't want to rely on this as it might break any time.

Personally, I believe that switching to the load event (instead of readystatechange) compicates things with no tangible benefit (so far).
But I am not too familiar with jqLite, so there is a good chance I am missing something.
Suggestions very welcome :)

tbosch · 2014-10-10T16:08:58Z

test/ng/anchorScrollSpec.js

+          scrollTo: (windowSpies.scrollTo = jasmine.createSpy('$window.scrollTo')),
+          scrollBy: (windowSpies.scrollBy = jasmine.createSpy('$window.scrollBy')),
+          document: createMockDocument(initialReadyState),
+          navigator: {},


I don't think we need this

This was added in 7b739c9.
Not sure if it is still needed.

tbosch · 2014-10-10T22:04:59Z

test/ng/anchorScrollSpec.js

-      document: document,
-      navigator: {}
-    });
+  function simulateWindowLoadEvent() {


Those two simulate functions don't do the same. Can we inline this function, as it is only used in one place?

I pulled them out so that their function names made the tests easier to read rather than to DRY up the code. Are you sure it is better to put it inline?

So I only meant the second one. If we leave it as a separate function we should rename it though. E.g. fireWindowLoadEvent...

This helper function can be used to execute a callback only after the document has completed its loading, i.e. after the `load` event fires or immediately if the page has already loaded and `document.readyState === 'complete'`.

Add support for a configurable vertical scroll offset to `$anchorScroll`. The offset can be defined by a specific number of pixels, a callback function that returns the number of pixels on demand or a jqLite/JQuery wrapped DOM element whose height and position are used if it has fixed position. The offset algorithm takes into account items that are near the bottom of the page preventing over-zealous offset correction. Closes angular#9368 Closes angular#2070 Closes angular#9360

Closes angular#9360

btford added the In Progress label Oct 9, 2014

mary-poppins added cla: yes and removed cla: no labels Oct 9, 2014

tbosch self-assigned this Oct 9, 2014