Fix PostStream Reply Scroll (#2366)

- Add an index to reply placeholder so we can scroll to it directly when replying.
- Stop pretending that the currently broken `bottom` scroll functionality works, and explicitly call it `reply` scrolling to be clearer
- Directly get target from state
- Explicitly scroll to placeholder on reply
- Clean up scrollToItem code a bit
- Account for edge case where index is undefined when scrolling to post

Co-authored-by: Wadim Kalmykov <36057469+w-4@users.noreply.github.com>

js/src/forum/components/DiscussionPage.js

@@ -82,7 +82,6 @@ export default class DiscussionPage extends Page {
                     {PostStream.component({
                       discussion,
                       stream: this.stream,
-                      targetPost: this.stream.targetPost,
                       onPositionChange: this.positionChanged.bind(this),
                     })}
                   </div>

js/src/forum/components/PostStream.js

@@ -97,7 +97,7 @@ export default class PostStream extends Component {
     // is not already doing so, then show a 'write a reply' placeholder.
     if (viewingEnd && (!app.session.user || this.discussion.canReply())) {
       items.push(
-        <div className="PostStream-item" key="reply" oncreate={postFadeIn}>
+        <div className="PostStream-item" key="reply" data-index={this.stream.count()} oncreate={postFadeIn}>
           {ReplyPlaceholder.component({ discussion: this.discussion })}
         </div>
       );
@@ -129,16 +129,15 @@ export default class PostStream extends Component {
    * Start scrolling, if appropriate, to a newly-targeted post.
    */
   triggerScroll() {
-    if (!this.attrs.targetPost || !this.stream.needsScroll) return;
+    if (!this.stream.needsScroll) return;
 
-    const newTarget = this.attrs.targetPost;
+    const target = this.stream.targetPost;
     this.stream.needsScroll = false;
 
-    if ('number' in newTarget) {
+    if ('number' in target) {
-      this.scrollToNumber(newTarget.number, this.stream.animateScroll);
+      this.scrollToNumber(target.number, this.stream.animateScroll);
-    } else if ('index' in newTarget) {
+    } else if ('index' in target) {
-      const backwards = newTarget.index === this.stream.count() - 1;
+      this.scrollToIndex(target.index, this.stream.animateScroll, target.reply);
-      this.scrollToIndex(newTarget.index, this.stream.animateScroll, backwards);
     }
   }
 
@@ -309,18 +308,17 @@ export default class PostStream extends Component {
    *
    * @param {Integer} index
    * @param {Boolean} animate
-   * @param {Boolean} bottom Whether or not to scroll to the bottom of the post
+   * @param {Boolean} reply Whether or not to scroll to the reply placeholder.
-   *     at the given index, instead of the top of it.
    * @return {jQuery.Deferred}
    */
-  scrollToIndex(index, animate, bottom) {
+  scrollToIndex(index, animate, reply) {
-    const $item = this.$(`.PostStream-item[data-index=${index}]`);
+    const $item = reply ? $('.PostStream-item:last-child') : this.$(`.PostStream-item[data-index=${index}]`);
 
-    return this.scrollToItem($item, animate, true, bottom).then(() => {
+    this.scrollToItem($item, animate, true, reply);
-      if (index == this.stream.count() - 1) {
+
-        this.flashItem(this.$('.PostStream-item:last-child'));
+    if (reply) {
-      }
+      this.flashItem($item);
-    });
+    }
   }
 
   /**
@@ -330,11 +328,10 @@ export default class PostStream extends Component {
    * @param {Boolean} animate
    * @param {Boolean} force Whether or not to force scrolling to the item, even
    *     if it is already in the viewport.
-   * @param {Boolean} bottom Whether or not to scroll to the bottom of the post
+   * @param {Boolean} reply Whether or not to scroll to the reply placeholder.
-   *     at the given index, instead of the top of it.
    * @return {jQuery.Deferred}
    */
-  scrollToItem($item, animate, force, bottom) {
+  scrollToItem($item, animate, force, reply) {
     const $container = $('html, body').stop(true);
     const index = $item.data('index');
 
@@ -345,10 +342,10 @@ export default class PostStream extends Component {
       const scrollBottom = scrollTop + $(window).height();
 
       // If the item is already in the viewport, we may not need to scroll.
-      // If we're scrolling to the bottom of an item, then we'll make sure the
+      // If we're scrolling to the reply placeholder, we'll make sure its
       // bottom will line up with the top of the composer.
       if (force || itemTop < scrollTop || itemBottom > scrollBottom) {
-        const top = bottom ? itemBottom - $(window).height() + app.composer.computedHeight() : $item.is(':first-child') ? 0 : itemTop;
+        const top = reply ? itemBottom - $(window).height() + app.composer.computedHeight() : $item.is(':first-child') ? 0 : itemTop;
 
         if (!animate) {
           $container.scrollTop(top);
@@ -362,7 +359,7 @@ export default class PostStream extends Component {
       // We manually set the index because we want to display the index of the
       // exact post we've scrolled to, not just that of the first post within viewport.
       this.updateScrubber();
-      this.stream.index = index;
+      if (index !== undefined) this.stream.index = index + 1;
     };
 
     // If we don't update this before the scroll, the scrubber will start
@@ -373,18 +370,22 @@ export default class PostStream extends Component {
     return Promise.all([$container.promise(), this.stream.loadPromise]).then(() => {
       m.redraw.sync();
 
-      // After post data has been loaded in, we will attempt to scroll back
+      // Rendering post contents will probably throw off our position.
-      // to the top of the requested post (or to the top of the page if the
+      // To counter this, we'll scroll either:
-      // first post was requested). In some cases, we may have scrolled to
+      //   - To the reply placeholder (aligned with composer top)
-      // the end of the available post range, in which case, the next range
+      //   - To the top of the page if we're on the first post
-      // of posts will be loaded in. However, in those cases, the post we
+      //   - To the top of a post (if that post exists)
-      // requested won't exist, so scrolling to it would cause an error.
+      // If the post does not currently exist, it's probably
-      // Accordingly, we start by checking that it's offset is defined.
+      // outside of the range we loaded in, so we won't adjust anything,
-      const offset = $(`.PostStream-item[data-index=${index}]`).offset();
+      // as it will soon be rendered by the "load more" system.
-      if (index === 0) {
+      let itemOffset;
+      if (reply) {
+        const $placeholder = $('.PostStream-item:last-child');
+        $(window).scrollTop($placeholder.offset().top + $placeholder.height() - $(window).height() + app.composer.computedHeight());
+      } else if (index === 0) {
         $(window).scrollTop(0);
-      } else if (offset) {
+      } else if ((itemOffset = $(`.PostStream-item[data-index=${index}]`).offset())) {
-        $(window).scrollTop($(`.PostStream-item[data-index=${index}]`).offset().top - this.getMarginTop());
+        $(window).scrollTop(itemOffset.top - this.getMarginTop());
       }
 
       // We want to adjust this again after posts have been loaded in

js/src/forum/states/PostStreamState.js

@@ -96,7 +96,9 @@ class PostStreamState {
     // If we want to go to the reply preview, then we will go to the end of the
     // discussion and then scroll to the very bottom of the page.
     if (number === 'reply') {
-      return this.goToLast();
+      const resultPromise = this.goToLast();
+      this.targetPost.reply = true;
+      return resultPromise;
     }
 
     this.paused = true;