Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Commit a27f05d

Browse files
committed
WIP - fix($anchorScroll): fix $digest lifecycle management in readystatechanged callback
Wrap the code inside of the listener callback in `$rootScope.$evalAsync()` to execute inside the Angular context (plus some minor modifications in the callback). Also, modified the tests accordingly (introducing `$browser.defer.flush()` where appropriate). (Note: The tests pass consistently on Chrome and IE, but sometimes some tests fail on Firefox (it could be 1 or more). Still working on it.)
1 parent 760070d commit a27f05d

File tree

2 files changed

+47
-25
lines changed

2 files changed

+47
-25
lines changed

src/ng/anchorScroll.js

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -217,10 +217,10 @@ function $AnchorScrollProvider() {
217217
if (offset) {
218218
// `offset` is the number of pixels we should scroll up in order to align `elem` properly.
219219
// This is true ONLY if the call to `elem.scrollIntoView()` initially aligns `elem` at the
220-
// top of the viewport. IF the number of pixels from the top of `elem` to the end of page's
221-
// content is less than the height of the viewport, then `elem.scrollIntoView()` will NOT
222-
// align the top of `elem` at the top of the viewport (but further down). This is often the
223-
// case for elements near the bottom of the page.
220+
// top of the viewport. IF the number of pixels from the top of `elem` to the end of the
221+
// page's content is less than the height of the viewport, then `elem.scrollIntoView()`
222+
// will NOT align the top of `elem` at the top of the viewport (but further down). This is
223+
// often the case for elements near the bottom of the page.
224224
// In such cases we do not need to scroll the whole `offset` up, just the fraction of the
225225
// offset that is necessary to align the top of `elem` at the desired position.
226226
var elemTop = elem.getBoundingClientRect().top;
@@ -241,10 +241,15 @@ function $AnchorScrollProvider() {
241241
} else if (!scrollScheduled) {
242242
scrollScheduled = true;
243243
document.addEventListener('readystatechange', function unbindAndScroll() {
244-
if (document.readyState === 'complete') {
245-
document.removeEventListener('readystatechange', unbindAndScroll);
246-
$rootScope.$evalAsync(scroll);
247-
}
244+
// When navigating to a page with a URL including a hash,
245+
// Firefox overwrites our `yOffset` if `$apply()` is used instead.
246+
$rootScope.$evalAsync(function() {
247+
if (document.readyState === 'complete') {
248+
scrollScheduled = false;
249+
document.removeEventListener('readystatechange', unbindAndScroll);
250+
scroll();
251+
}
252+
});
248253
});
249254
}
250255
}

test/ng/anchorScrollSpec.js

Lines changed: 34 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -40,10 +40,13 @@ describe('$anchorScroll', function() {
4040
}
4141

4242
function changeHashTo(hash) {
43-
return function($anchorScroll, $location, $rootScope) {
43+
return function($anchorScroll, $browser, $location, $rootScope) {
4444
$rootScope.$apply(function() {
4545
$location.hash(hash);
4646
});
47+
if ($browser.deferredFns.length) {
48+
$browser.defer.flush();
49+
}
4750
};
4851
}
4952

@@ -81,19 +84,12 @@ describe('$anchorScroll', function() {
8184
}
8285

8386
function mockDocumentReadyState() {
84-
var docMock = {
85-
readyState: 'mocked',
86-
updateReadyState: function(newState) {
87-
this.readyState = newState;
88-
this.dispatchFakeReadyStateChangeEvent();
89-
}
90-
};
91-
87+
var docMock = {};
9288
docSpies = {};
9389

94-
return function($rootScope, $window) {
95-
var propsToMock = ['body', 'documentElement'];
96-
var methodsToMock = [
90+
return function($browser, $rootScope, $window) {
91+
var propsToPassThrough = ['body', 'documentElement'];
92+
var methodsToPassThrough = [
9793
'getElementById',
9894
'getElementsByName',
9995
'addEventListener',
@@ -102,19 +98,26 @@ describe('$anchorScroll', function() {
10298

10399
var document_ = $window.document;
104100

105-
propsToMock.forEach(function(prop) {
101+
propsToPassThrough.forEach(function(prop) {
106102
docMock[prop] = document_[prop];
107103
});
108-
methodsToMock.forEach(function(method) {
104+
methodsToPassThrough.forEach(function(method) {
109105
docMock[method] = document_[method].bind(document_);
110106
docSpies[method] = spyOn(docMock, method).andCallThrough();
111107
});
112108

109+
docMock.readyState = 'mocked';
113110
docMock.dispatchFakeReadyStateChangeEvent = function() {
114111
var evt = document.createEvent('Event');
115112
evt.initEvent('readystatechange', false, false);
116113
document_.dispatchEvent(evt);
117-
$rootScope.$digest();
114+
};
115+
docMock.updateReadyState = function(newState) {
116+
this.readyState = newState;
117+
this.dispatchFakeReadyStateChangeEvent();
118+
if ($browser.deferredFns.length) {
119+
$browser.defer.flush();
120+
}
118121
};
119122

120123
$window.document = docMock;
@@ -218,7 +221,7 @@ describe('$anchorScroll', function() {
218221
expectScrollingTo('id=some1')));
219222

220223

221-
it('should only register one execution for when `document.readyState === "complete"', inject(
224+
it('should only register once for execution when `document.readyState === "complete"', inject(
222225
mockDocumentReadyState(),
223226
addElements('id=some1', 'id=some2'),
224227

@@ -259,6 +262,20 @@ describe('$anchorScroll', function() {
259262
}));
260263

261264

265+
it('should scroll immediately if `readyState === "complete"`', inject(
266+
mockDocumentReadyState(),
267+
addElements('id=some1'),
268+
269+
updateMockReadyState('complete'),
270+
changeHashTo('some1'),
271+
272+
expectScrollingTo('id=some1'),
273+
function() {
274+
expect(docSpies.addEventListener.callCount).toBe(0);
275+
expect(docSpies.removeEventListener.callCount).toBe(0);
276+
}));
277+
278+
262279
describe('watcher', function() {
263280

264281
function initLocation(config) {
@@ -356,7 +373,7 @@ describe('$anchorScroll', function() {
356373
var list = isArray(offsetList) ? offsetList : [offsetList];
357374

358375
return function($window) {
359-
expectScrollingTo(identifierCountMap)($window);
376+
inject(expectScrollingTo(identifierCountMap));
360377
expect($window.scrollBy.callCount).toBe(list.length);
361378
forEach(list, function(offset, idx) {
362379
// Due to sub-pixel rendering, there is a +/-1 error margin in the actual offset

0 commit comments

Comments
 (0)