Re-implement bucket generation and usage

Restructure the way that buckets are generated and represented. Simplify
what `BucketBar` needs to do with the buckets. Fix a bug in which
anchors can be occasionally included in multiple buckets.

Fixes #397
Fixes #2618

src/annotator/plugin/bucket-bar.js

@@ -2,21 +2,12 @@ import Delegator from '../delegator';
 import scrollIntoView from 'scroll-into-view';
 
 import { setHighlightsFocused } from '../highlighter';
-import {
-  findClosestOffscreenAnchor,
-  constructPositionPoints,
-  buildBuckets,
-} from '../util/buckets';
+import { findClosestOffscreenAnchor, anchorBuckets } from '../util/buckets';
 
 /**
  * @typedef {import('../util/buckets').Bucket} Bucket
- * @typedef {import('../util/buckets').PositionPoints} PositionPoints
  */
 
-const BUCKET_SIZE = 16; // Regular bucket size
-const BUCKET_NAV_SIZE = BUCKET_SIZE + 6; // Bucket plus arrow (up/down)
-const BUCKET_TOP_THRESHOLD = 115 + BUCKET_NAV_SIZE; // Toolbar
-
 // Scroll to the next closest anchor off screen in the given direction.
 function scrollToClosest(anchors, direction) {
   const closest = findClosestOffscreenAnchor(anchors, direction);
@@ -117,30 +108,7 @@ export default class BucketBar extends Delegator {
   }
 
   _update() {
-    /** @type {PositionPoints} */
-    const { above, below, points } = constructPositionPoints(
-      this.annotator.anchors
-    );
-
-    this.buckets = buildBuckets(points);
-
-    // Add a bucket to the top of the bar that, when clicked, will scroll up
-    // to the nearest bucket offscreen above, an upper navigation bucket
-    // TODO: This should be part of building the buckets
-    this.buckets.unshift(
-      { anchors: [], position: 0 },
-      { anchors: above, position: BUCKET_TOP_THRESHOLD - 1 },
-      { anchors: [], position: BUCKET_TOP_THRESHOLD }
-    );
-
-    // Add a bucket to the bottom of the bar that, when clicked, will scroll down
-    // to the nearest bucket offscreen below, a lower navigation bucket
-    // TODO: This should be part of building the buckets
-    this.buckets.push(
-      { anchors: [], position: window.innerHeight - BUCKET_NAV_SIZE },
-      { anchors: below, position: window.innerHeight - BUCKET_NAV_SIZE + 1 },
-      { anchors: [], position: window.innerHeight }
-    );
+    this.buckets = anchorBuckets(this.annotator.anchors);
 
     // The following affordances attempt to reuse existing DOM elements
     // when reconstructing bucket "tabs" to cut down on the number of elements
@@ -205,17 +173,9 @@ export default class BucketBar extends Delegator {
 
   _buildTabs() {
     this.tabs.forEach((tabEl, index) => {
-      let bucketHeight;
       const anchorCount = this.buckets[index].anchors.length;
-      // Positioning logic currently _relies_ on their being interstitial
-      // buckets that have no anchors but do have positions. Positioning
-      // is averaged between this bucket's position and the _next_ bucket's
-      // position. For now. TODO: Fix this
-      const pos =
-        (this.buckets[index].position + this.buckets[index + 1]?.position) / 2;
-
       tabEl.className = 'annotator-bucket-indicator';
-      tabEl.style.top = `${pos}px`;
+      tabEl.style.top = `${this.buckets[index].position}px`;
       tabEl.style.display = '';
 
       if (anchorCount) {
@@ -229,34 +189,20 @@ export default class BucketBar extends Delegator {
         tabEl.style.display = 'none';
       }
 
-      if (this.isNavigationBucket(index)) {
-        bucketHeight = BUCKET_NAV_SIZE;
-        tabEl.classList.toggle('upper', this.isUpper(index));
-        tabEl.classList.toggle('lower', this.isLower(index));
-      } else {
-        bucketHeight = BUCKET_SIZE;
-        tabEl.classList.remove('upper');
-        tabEl.classList.remove('lower');
-      }
-
-      tabEl.style.marginTop = (-1 * bucketHeight) / 2 + 'px';
+      tabEl.classList.toggle('upper', this.isUpper(index));
+      tabEl.classList.toggle('lower', this.isLower(index));
     });
   }
 
   isUpper(i) {
-    return i === 1;
+    return i === 0;
   }
 
   isLower(i) {
-    return i === this.buckets.length - 2;
+    return i === this.buckets.length - 1;
   }
 
   isNavigationBucket(i) {
     return this.isUpper(i) || this.isLower(i);
   }
 }
-
-// Export constants
-BucketBar.BUCKET_SIZE = BUCKET_SIZE;
-BucketBar.BUCKET_NAV_SIZE = BUCKET_NAV_SIZE;
-BucketBar.BUCKET_TOP_THRESHOLD = BUCKET_TOP_THRESHOLD;

src/annotator/plugin/test/bucket-bar-test.js

@@ -46,11 +46,8 @@ describe('BucketBar', () => {
     };
 
     fakeBucketUtil = {
+      anchorBuckets: sinon.stub().returns([]),
       findClosestOffscreenAnchor: sinon.stub(),
-      constructPositionPoints: sinon
-        .stub()
-        .returns({ above: [], below: [], points: [] }),
-      buildBuckets: sinon.stub().returns([]),
     };
 
     fakeHighlighter = {
@@ -105,16 +102,16 @@ describe('BucketBar', () => {
   describe('updating buckets', () => {
     it('should update buckets when the window is resized', () => {
       bucketBar = createBucketBar();
-      assert.notCalled(fakeBucketUtil.buildBuckets);
+      assert.notCalled(fakeBucketUtil.anchorBuckets);
       window.dispatchEvent(new Event('resize'));
-      assert.calledOnce(fakeBucketUtil.buildBuckets);
+      assert.calledOnce(fakeBucketUtil.anchorBuckets);
     });
 
     it('should update buckets when the window is scrolled', () => {
       bucketBar = createBucketBar();
-      assert.notCalled(fakeBucketUtil.buildBuckets);
+      assert.notCalled(fakeBucketUtil.anchorBuckets);
       window.dispatchEvent(new Event('scroll'));
-      assert.calledOnce(fakeBucketUtil.buildBuckets);
+      assert.calledOnce(fakeBucketUtil.anchorBuckets);
     });
 
     context('when scrollables provided', () => {
@@ -144,11 +141,11 @@ describe('BucketBar', () => {
         bucketBar = createBucketBar({
           scrollables: ['.scrollable-1', '.scrollable-2'],
         });
-        assert.notCalled(fakeBucketUtil.buildBuckets);
+        assert.notCalled(fakeBucketUtil.anchorBuckets);
         scrollableEls[0].dispatchEvent(new Event('scroll'));
-        assert.calledOnce(fakeBucketUtil.buildBuckets);
+        assert.calledOnce(fakeBucketUtil.anchorBuckets);
         scrollableEls[1].dispatchEvent(new Event('scroll'));
-        assert.calledTwice(fakeBucketUtil.buildBuckets);
+        assert.calledTwice(fakeBucketUtil.anchorBuckets);
       });
     });
 
@@ -165,8 +162,10 @@ describe('BucketBar', () => {
       // Create fake anchors and render buckets.
       const anchors = [createAnchor()];
 
-      fakeBucketUtil.buildBuckets.returns([
+      fakeBucketUtil.anchorBuckets.returns([
+        { anchors: [], position: 137 }, // Upper navigation
         { anchors: [anchors[0]], position: 250 },
+        { anchors: [], position: 400 }, // Lower navigation
       ]);
 
       bucketBar.annotator.anchors = anchors;
@@ -256,21 +255,17 @@ describe('BucketBar', () => {
       ];
       // These two anchors are considered to be offscreen upwards
       fakeAbove = [fakeAnchors[0], fakeAnchors[1]];
+      fakeBelow = [fakeAnchors[5]];
       // These buckets are on-screen
       fakeBuckets = [
+        { anchors: fakeAbove, position: 137 },
         { anchors: [fakeAnchors[2], fakeAnchors[3]], position: 350 },
-        { anchors: [], position: 450 }, // This is an empty bucket
         { anchors: [fakeAnchors[4]], position: 550 },
+        { anchors: fakeBelow, position: 600 },
       ];
       // This anchor is offscreen below
-      fakeBelow = [fakeAnchors[5]];
 
-      fakeBucketUtil.constructPositionPoints.returns({
-        above: fakeAbove,
-        below: fakeBelow,
-        points: [],
-      });
-      fakeBucketUtil.buildBuckets.returns(fakeBuckets.slice());
+      fakeBucketUtil.anchorBuckets.returns(fakeBuckets.slice());
     });
 
     describe('navigation bucket tabs', () => {
@@ -302,7 +297,7 @@ describe('BucketBar', () => {
 
         // Resetting this return is necessary to return a fresh array reference
         // on next update
-        fakeBucketUtil.buildBuckets.returns(fakeBuckets.slice());
+        fakeBucketUtil.anchorBuckets.returns(fakeBuckets.slice());
         bucketBar.update();
         assert.equal(bucketBar.tabs.length, bucketBar.buckets.length);
         assert.notExists(bucketBar.element.querySelector('.extraTab'));
@@ -370,12 +365,7 @@ describe('BucketBar', () => {
     });
 
     it('does not display empty bucket tabs', () => {
-      fakeBucketUtil.buildBuckets.returns([]);
-      fakeBucketUtil.constructPositionPoints.returns({
-        above: [],
-        below: [],
-        points: [],
-      });
+      fakeBucketUtil.anchorBuckets.returns([]);
       bucketBar.update();
 
       const allBuckets = bucketBar.element.querySelectorAll(

src/styles/annotator/bucket-bar.scss

@@ -28,13 +28,14 @@
     right: 0;
     pointer-events: all;
     position: absolute;
-    margin-top: -8px;
     line-height: 1;
     height: 16px;
     width: 26px;
     -webkit-tap-highlight-color: rgba(255, 255, 255, 0);
     text-align: center;
     cursor: pointer;
+    // Vertically center the element, which is 16px high
+    margin-top: -8px;
 
     .label {
       @include reset.reset-box-model;
@@ -86,10 +87,18 @@
         border-right: solid transparent;
         margin-top: 0;
       }
+      & .label {
+        // Vertical alignment tweak to better center the label in the indicator
+        margin-top: -1px;
+      }
     }
 
     &.upper {
       border-radius: 2px 2px 4px 4px;
+      // Vertically center the element (which is 22px high) by adding a negative
+      // top margin in conjunction with an inline style `top` position (set
+      // in code)
+      margin-top: -11px;
 
       &:before,
       &:after {
@@ -111,6 +120,7 @@
     }
 
     &.lower {
+      margin-top: 0;
       border-radius: 4px 4px 2px 2px;
 
       &:before,