Commit d0876bbd authored by Robert Knight's avatar Robert Knight

Scroll new annotations into view

Re-implement the behavior to scroll newly created annotations into view
in the sidebar so that the user can start editing them immediately.

A complication with doing this given that the annotation list is
virtualized is that if we want to scroll to the Nth visible annotation,
we may not know the actual heights of the N-1 annotations above it.

As a result of scrolling the list, some of the heights of those N-1
annotations may be updated from an estimate to an actual measured
height, changing the Y offset that we want to scroll to in order
to make the Nth annotation visible.

There are a few strategies that we could use for dealing with this:

 1. Forcibly render all N-1 annotations and measure their height.
    This is prohibitively expensive for long annotation lists.

 2. Use a cheaper method to calculate the exact height of an annotation.
    Unfortunately we don't have a way to do this at the moment.

 3. Use an iterative approach:

    a. Estimate the target position based on current known/guessed
       heights.

    b. Scroll to the estimated position, which will trigger re-rendering
       and possibly alter the known thread heights.

    c. Re-estimate the target position and if it changed, go to step b.

This commit implements the third method.
parent 488ebdc1
......@@ -154,4 +154,28 @@ describe('VirtualThreadList', function () {
event: 'scroll',
}]);
});
describe('#yOffsetOf', function () {
unroll('returns #offset as the Y offset of the #nth thread', function (testCase) {
var thread = generateRootThread(10);
threadList.setRootThread(thread);
idRange(0, 10).forEach(function (id) {
threadList.setThreadHeight(id, 100);
});
var id = idRange(testCase.index, testCase.index)[0];
assert.equal(threadList.yOffsetOf(id), testCase.offset);
}, [{
nth: 'first',
index: 0,
offset: 0,
},{
nth: 'second',
index: 1,
offset: 100,
},{
nth: 'last',
index: 9,
offset: 900,
}]);
});
});
......@@ -30,8 +30,6 @@ function FakeRootThread() {
this.thread = sinon.stub().returns({
totalChildren: 0,
});
this.setSearchQuery = sinon.stub();
this.sortBy = sinon.stub();
}
inherits(FakeRootThread, EventEmitter);
......@@ -39,6 +37,9 @@ function FakeVirtualThreadList() {
this.setRootThread = sinon.stub();
this.setThreadHeight = sinon.stub();
this.detach = sinon.stub();
this.yOffsetOf = function () {
return 100;
};
}
inherits(FakeVirtualThreadList, EventEmitter);
......@@ -284,6 +285,21 @@ describe('WidgetController', function () {
});
describe('when a new annotation is created', function () {
var windowScroll;
var cardListTopEl;
beforeEach(function () {
$scope.clearSelection = sinon.stub();
windowScroll = sinon.stub(window, 'scroll');
cardListTopEl = $('<div class="js-thread-list-top"></div>');
cardListTopEl.appendTo(document.body);
});
afterEach(function () {
windowScroll.restore();
cardListTopEl.remove();
});
/**
* It should clear any selection that exists in the sidebar before
* creating a new annotation. Otherwise the new annotation with its
......@@ -291,24 +307,26 @@ describe('WidgetController', function () {
* not part of the selection.
*/
it('clears the selection', function () {
$scope.clearSelection = sinon.stub();
$rootScope.$emit('beforeAnnotationCreated', {});
assert.called($scope.clearSelection);
});
it('does not clear the selection if the new annotation is a highlight', function () {
$scope.clearSelection = sinon.stub();
$rootScope.$emit('beforeAnnotationCreated', {$highlight: true});
assert.notCalled($scope.clearSelection);
});
it('does not clear the selection if the new annotation is a reply', function () {
$scope.clearSelection = sinon.stub();
$rootScope.$emit('beforeAnnotationCreated', {
references: ['parent-id']
});
assert.notCalled($scope.clearSelection);
});
it('scrolls the viewport to the new annotation', function () {
$rootScope.$emit('beforeAnnotationCreated', {$$tag: '123'});
assert.called(windowScroll);
});
});
describe('direct linking messages', function () {
......
......@@ -82,6 +82,28 @@ VirtualThreadList.prototype.setThreadHeight = function (id, height) {
this._heights[id] = height;
};
VirtualThreadList.prototype._height = function (id) {
// Default guess of the height required for a threads that have not been
// measured
var DEFAULT_HEIGHT = 200;
return this._heights[id] || DEFAULT_HEIGHT;
};
/** Return the vertical offset of an annotation card from the top of the list. */
VirtualThreadList.prototype.yOffsetOf = function (id) {
var self = this;
var allThreads = this._rootThread.children;
var matchIndex = allThreads.findIndex(function (thread) {
return thread.id === id;
});
if (matchIndex === -1) {
return 0;
}
return allThreads.slice(0, matchIndex).reduce(function (offset, thread) {
return offset + self._height(thread.id);
}, 0);
};
/**
* Recalculates the set of visible threads and estimates of the amount of space
* required for offscreen threads above and below the viewport.
......@@ -94,9 +116,6 @@ VirtualThreadList.prototype._updateVisibleThreads = function () {
var MARGIN_ABOVE = 800;
// Same as MARGIN_ABOVE but for the space below the viewport
var MARGIN_BELOW = 800;
// Default guess of the height required for a threads that have not been
// measured
var DEFAULT_HEIGHT = 200;
// Estimated height in pixels of annotation cards which are below the
// viewport and not actually created. This is used to create an empty spacer
......@@ -116,7 +135,7 @@ VirtualThreadList.prototype._updateVisibleThreads = function () {
for (var i = 0; i < allThreads.length; i++) {
thread = allThreads[i];
var threadHeight = this._heights[thread.id] || DEFAULT_HEIGHT;
var threadHeight = this._height(thread.id);
if (usedHeight + threadHeight < this.window.pageYOffset - MARGIN_ABOVE) {
// Thread is above viewport
......
......@@ -333,10 +333,41 @@ module.exports = function WidgetController(
return rootThread.thread().totalChildren;
};
/**
* Return the offset between the top of the window and the top of the
* first annotation card.
*/
function cardListYOffset() {
var cardListTopEl = document.querySelector('.js-thread-list-top');
return cardListTopEl.getBoundingClientRect().top + window.pageYOffset;
}
/** Scroll the annotation with a given ID or $$tag into view. */
function scrollIntoView(id) {
var estimatedYOffset = visibleThreads.yOffsetOf(id);
var estimatedPos = estimatedYOffset - cardListYOffset();
window.scroll(0, estimatedPos);
// As a result of scrolling the sidebar, the heights of some of the cards
// above `id` might change because the initial estimate will be replaced by
// the actual known height after a card is rendered.
//
// So we wait briefly after the view is scrolled then check whether the
// estimated Y offset changed and if so, trigger scrolling again.
scopeTimeout($scope, function () {
var newYOffset = visibleThreads.yOffsetOf(id);
if (newYOffset !== estimatedYOffset) {
scrollIntoView(id);
}
}, 200);
}
$rootScope.$on(events.BEFORE_ANNOTATION_CREATED, function (event, data) {
if (data.$highlight || (data.references && data.references.length > 0)) {
return;
}
$scope.clearSelection();
scrollIntoView(data.$$tag);
});
};
......@@ -30,7 +30,7 @@
You do not have permission to see this annotation
</p>
</li>
<li class="thread-list__spacer"
<li class="thread-list__spacer js-thread-list-top"
ng-style="{height: virtualThreadList.offscreenUpperHeight}"></li>
<li id="{{child.id}}"
class="annotation-card thread"
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment