From d5412cff4e420b734eda0e9169419ee4b9500cc0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Thu, 5 Apr 2018 10:08:48 +0200 Subject: [PATCH] FIX: scrolling was off sometimes Also changed "Jump To Post" to go to the post index in the stream rather than the post number --- .../lib/raw-handlebars.js.es6 | 12 +-- .../components/scrolling-post-stream.js.es6 | 15 ++-- .../discourse/controllers/jump-to-post.js.es6 | 6 +- .../discourse/controllers/topic.js.es6 | 14 +++- .../discourse/lib/keyboard-shortcuts.js.es6 | 11 +-- .../javascripts/discourse/lib/lock-on.js.es6 | 44 +++++------ .../discourse/lib/offset-calculator.js.es6 | 78 ++++++++----------- 7 files changed, 80 insertions(+), 100 deletions(-) diff --git a/app/assets/javascripts/discourse-common/lib/raw-handlebars.js.es6 b/app/assets/javascripts/discourse-common/lib/raw-handlebars.js.es6 index 4b547a68828..2900e5d6704 100644 --- a/app/assets/javascripts/discourse-common/lib/raw-handlebars.js.es6 +++ b/app/assets/javascripts/discourse-common/lib/raw-handlebars.js.es6 @@ -18,13 +18,15 @@ RawHandlebars.helpers['get'] = function(context, options) { var firstContext = options.contexts[0]; var val = firstContext[context]; - if (context.indexOf('controller') === 0) { - context = context.replace(/^controller\./, ''); + if (context.indexOf('controller.') === 0) { + context = context.slice(context.indexOf('.') + 1); } - if (val && val.isDescriptor) { return Em.get(firstContext, context); } - val = val === undefined ? Em.get(firstContext, context): val; - return val; + if (val && val.isDescriptor) { + return Em.get(firstContext, context); + } + + return val === undefined ? Em.get(firstContext, context) : val; }; // adds compatability so this works with stringParams diff --git a/app/assets/javascripts/discourse/components/scrolling-post-stream.js.es6 b/app/assets/javascripts/discourse/components/scrolling-post-stream.js.es6 index 74d6b378b7d..79acf111840 100644 --- a/app/assets/javascripts/discourse/components/scrolling-post-stream.js.es6 +++ b/app/assets/javascripts/discourse/components/scrolling-post-stream.js.es6 @@ -54,8 +54,7 @@ export default MountWidget.extend({ const scrollTop = $body.scrollTop(); // This hack is for when swapping out many cloaked views at once - // when using keyboard navigation. It could suddenly move the - // scroll + // when using keyboard navigation. It could suddenly move the scroll if (this.prevHeight === height && scrollTop !== this.prevScrollTop) { $body.scrollTop(this.prevScrollTop); } @@ -80,7 +79,7 @@ export default MountWidget.extend({ const postsWrapperTop = $('.posts-wrapper').offset().top; const $posts = this.$('.onscreen-post, .cloaked-post'); const viewportTop = windowTop - slack; - const topView = findTopView($posts, viewportTop, postsWrapperTop, 0, $posts.length-1); + const topView = findTopView($posts, viewportTop, postsWrapperTop, 0, $posts.length - 1); let windowBottom = windowTop + windowHeight; let viewportBottom = windowBottom + slack; @@ -93,7 +92,7 @@ export default MountWidget.extend({ let percent = null; const offset = offsetCalculator(); - const topCheck = Math.ceil(windowTop + offset); + const topCheck = Math.ceil(windowTop + offset + 5); // uncomment to debug the eyeline /* @@ -102,7 +101,7 @@ export default MountWidget.extend({ $('body').prepend('
'); $eyeline = $('.debug-eyeline'); } - $eyeline.css({ height: '1px', width: '100%', backgroundColor: 'blue', position: 'absolute', top: `${topCheck}px` }); + $eyeline.css({ height: '5px', width: '100%', backgroundColor: 'blue', position: 'absolute', top: `${topCheck}px`, zIndex: 999999 }); */ let allAbove = true; @@ -115,7 +114,7 @@ export default MountWidget.extend({ if (!$post) { break; } const viewTop = $post.offset().top; - const postHeight = $post.height(); + const postHeight = $post.outerHeight(true); const viewBottom = Math.ceil(viewTop + postHeight); allAbove = allAbove && (viewTop < topCheck); @@ -170,7 +169,7 @@ export default MountWidget.extend({ this.sendAction('topVisibleChanged', { post: first, refresh: topRefresh }); } - const last = posts.objectAt(onscreen[onscreen.length-1]); + const last = posts.objectAt(onscreen[onscreen.length - 1]); if (this._bottomVisible !== last) { this._bottomVisible = last; this.sendAction('bottomVisibleChanged', { post: last, refresh }); @@ -184,7 +183,7 @@ export default MountWidget.extend({ } if (percent !== null) { - if (percent > 1.0) { percent = 1.0; } + percent = Math.max(0.0, Math.min(1.0, percent)); if (changedPost || (this._currentPercent !== percent)) { this._currentPercent = percent; diff --git a/app/assets/javascripts/discourse/controllers/jump-to-post.js.es6 b/app/assets/javascripts/discourse/controllers/jump-to-post.js.es6 index 94ccc5595a4..06fae57eb80 100644 --- a/app/assets/javascripts/discourse/controllers/jump-to-post.js.es6 +++ b/app/assets/javascripts/discourse/controllers/jump-to-post.js.es6 @@ -6,10 +6,8 @@ export default Ember.Controller.extend(ModalFunctionality, { actions: { jump() { - let where = parseInt(this.get('postNumber')); - if (where < 1) { where = 1; } - const max = this.get('topic.postStream.filteredPostsCount'); - if (where > max) { where = max; } + const max = this.get("topic.postStream.filteredPostsCount"); + const where = Math.min(max, Math.max(1, parseInt(this.get("postNumber")))); this.jumpToIndex(where); this.send('closeModal'); diff --git a/app/assets/javascripts/discourse/controllers/topic.js.es6 b/app/assets/javascripts/discourse/controllers/topic.js.es6 index 5e1fc792727..3a8782ab4a9 100644 --- a/app/assets/javascripts/discourse/controllers/topic.js.es6 +++ b/app/assets/javascripts/discourse/controllers/topic.js.es6 @@ -452,17 +452,17 @@ export default Ember.Controller.extend(BufferedContent, { }, jumpToIndex(index) { - this._jumpToPostId(this.get('model.postStream.stream')[index - 1]); + this._jumpToIndex(index); }, jumpToPostPrompt() { const postText = prompt(I18n.t('topic.progress.jump_prompt_long')); if (postText === null) { return; } - const postNumber = parseInt(postText, 10); - if (postNumber === 0) { return; } + const postIndex = parseInt(postText, 10); + if (postIndex === 0) { return; } - this._jumpToPostId(this.get('model.postStream').findPostIdForPostNumber(postNumber)); + this._jumpToIndex(postIndex); }, jumpToPost(postNumber) { @@ -755,6 +755,12 @@ export default Ember.Controller.extend(BufferedContent, { } }, + _jumpToIndex(index) { + const stream = this.get("model.postStream.stream"); + index = Math.max(1, Math.min(stream.length, index)); + this._jumpToPostId(stream[index - 1]); + }, + _jumpToPostId(postId) { if (!postId) { Ember.Logger.warn("jump-post code broken - requested an index outside the stream array"); diff --git a/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6 b/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6 index 18453287e68..766a77e0240 100644 --- a/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6 +++ b/app/assets/javascripts/discourse/lib/keyboard-shortcuts.js.es6 @@ -1,6 +1,6 @@ import DiscourseURL from 'discourse/lib/url'; import Composer from 'discourse/models/composer'; -import { scrollTopFor } from 'discourse/lib/offset-calculator'; +import { minimumOffset } from 'discourse/lib/offset-calculator'; const bindings = { '!': {postAction: 'showFlags'}, @@ -318,10 +318,10 @@ export default { const $selected = ($articles.filter('.selected').length !== 0) ? $articles.filter('.selected') : $articles.filter('[data-islastviewedtopic=true]'); + let index = $articles.index($selected); - if ($selected.length !== 0) { //boundries check - // loop is not allowed + if ($selected.length !== 0) { if (direction === -1 && index === 0) { return; } if (direction === 1 && index === ($articles.length - 1) ) { return; } } @@ -349,14 +349,12 @@ export default { const $article = $articles.eq(index + direction); if ($article.length > 0) { - $articles.removeClass('selected'); $article.addClass('selected'); if ($article.is('.topic-post')) { $('a.tabLoc', $article).focus(); this._scrollToPost($article); - } else { this._scrollList($article, direction); } @@ -364,8 +362,7 @@ export default { }, _scrollToPost($article) { - const pos = $article.offset(); - $(window).scrollTop(scrollTopFor(pos.top)); + $(window).scrollTop($article.offset().top - minimumOffset()); }, _scrollList($article) { diff --git a/app/assets/javascripts/discourse/lib/lock-on.js.es6 b/app/assets/javascripts/discourse/lib/lock-on.js.es6 index 111b713a359..fe20031ab3a 100644 --- a/app/assets/javascripts/discourse/lib/lock-on.js.es6 +++ b/app/assets/javascripts/discourse/lib/lock-on.js.es6 @@ -1,13 +1,13 @@ -import { scrollTopFor } from 'discourse/lib/offset-calculator'; +import { minimumOffset } from "discourse/lib/offset-calculator"; -// Dear traveller, you are entering a zone where we are at war with the browser -// the browser is insisting on positioning scrollTop per the location it was in -// the past, we are insisting on it being where we want it to be -// The hack is just to keep trying over and over to position the scrollbar (up to 1 minute) +// Dear traveller, you are entering a zone where we are at war with the browser. +// The browser is insisting on positioning scrollTop per the location it was in +// the past, we are insisting on it being where we want it to be. +// The hack is just to keep trying over and over to position the scrollbar (up to 1 second). // // The root cause is that a "refresh" on a topic page will almost never be at the // same position it was in the past, the URL points to the post at the top of the -// page, so a refresh will try to bring that post into view causing drift +// page, so a refresh will try to bring that post into view causing drift. // // Additionally if you loaded multiple batches of posts, on refresh they will not // be loaded. @@ -18,29 +18,29 @@ import { scrollTopFor } from 'discourse/lib/offset-calculator'; // 1. onbeforeunload ensure we are scrolled to the right spot // 2. give up on the scrollbar and implement it ourselves (something that will happen) +const LOCK_DURATION_MS = 1000; const SCROLL_EVENTS = "scroll.lock-on touchmove.lock-on mousedown.lock-on wheel.lock-on DOMMouseScroll.lock-on mousewheel.lock-on keyup.lock-on"; +const SCROLL_TYPES = ["mousedown", "mousewheel", "touchmove", "wheel"]; function within(threshold, x, y) { - return Math.abs(x-y) < threshold; + return Math.abs(x - y) < threshold; } export default class LockOn { constructor(selector, options) { this.selector = selector; this.options = options || {}; - this.offsetTop = null; } elementTop() { - const selected = $(this.selector); - if (selected && selected.offset && selected.offset()) { - const result = selected.offset().top; - return scrollTopFor(result); + const $selected = $(this.selector); + if ($selected && $selected.offset && $selected.offset()) { + return $selected.offset().top - minimumOffset(); } } clearLock(interval) { - $('body,html').off(SCROLL_EVENTS); + $("body, html").off(SCROLL_EVENTS); clearInterval(interval); if (this.options.finished) { this.options.finished(); @@ -54,9 +54,7 @@ export default class LockOn { $(window).scrollTop(previousTop); const interval = setInterval(() => { - let top = this.elementTop(); - if (top < 0) { top = 0; } - + const top = Math.max(0, this.elementTop()); const scrollTop = $(window).scrollTop(); if (typeof(top) === "undefined" || isNaN(top)) { @@ -68,20 +66,14 @@ export default class LockOn { previousTop = top; } - // We commit suicide after 3s just to clean up - const nowTime = new Date().getTime(); - if (nowTime - startedAt > 1000) { + // Commit suicide after a little while + if (new Date().getTime() - startedAt > LOCK_DURATION_MS) { return this.clearLock(interval); } }, 50); - $('body,html').off(SCROLL_EVENTS).on(SCROLL_EVENTS, e => { - if ( e.which > 0 || - e.type === "mousedown" || - e.type === "mousewheel" || - e.type === "touchmove" || - e.type === "wheel" - ) { + $("body, html").off(SCROLL_EVENTS).on(SCROLL_EVENTS, e => { + if (e.which > 0 || SCROLL_TYPES.includes(e.type)) { this.clearLock(interval); } }); diff --git a/app/assets/javascripts/discourse/lib/offset-calculator.js.es6 b/app/assets/javascripts/discourse/lib/offset-calculator.js.es6 index b354e271b44..41d50903bcb 100644 --- a/app/assets/javascripts/discourse/lib/offset-calculator.js.es6 +++ b/app/assets/javascripts/discourse/lib/offset-calculator.js.es6 @@ -1,50 +1,36 @@ export function scrollTopFor(y) { - return y - offsetCalculator(y); + return y - offsetCalculator(); } -export default function offsetCalculator(y) { - const $header = $('header'); - const $container = $('.posts-wrapper'); - const containerOffset = $container.offset(); - - let titleHeight = 0; - const scrollTop = y || $(window).scrollTop(); - - if (!containerOffset || scrollTop < containerOffset.top) { - titleHeight = $('#topic-title').height() || 0; - } - - const rawWinHeight = $(window).height(); - const windowHeight = rawWinHeight - titleHeight; - - const eyeTarget = (windowHeight / 10); - const headerHeight = $header.outerHeight(true); - const expectedOffset = titleHeight - ($header.find('.contents').height() || 0) + (eyeTarget * 2); - const ideal = headerHeight + ((expectedOffset < 0) ? 0 : expectedOffset); - - if ($container.length === 0) { return expectedOffset; } - - const topPos = $container.offset().top; - - const docHeight = $(document).height(); - let scrollPercent = Math.min((scrollTop / (docHeight-rawWinHeight)), 1.0); - - let inter = topPos - scrollTop + ($container.height() * scrollPercent); - if (inter < headerHeight + eyeTarget) { - inter = headerHeight + eyeTarget; - } - - if (inter > ideal) { - const bottom = $('#topic-bottom').offset().top; - const switchPos = bottom - rawWinHeight - ideal; - - if (scrollTop > switchPos) { - const p = Math.max(Math.min((scrollTop + inter - switchPos) / rawWinHeight, 1.0), 0.0); - return ((1 - p) * ideal) + (p * inter); - } else { - return ideal; - } - } - - return inter; +export function minimumOffset() { + const $header = $("header.d-header"); + const headerHeight = $header.outerHeight(true) || 0; + const headerPositionTop = $header.position().top; + return headerHeight + headerPositionTop; +} + +export default function offsetCalculator() { + const min = minimumOffset(); + + // on mobile, just use the header + if ($("html").hasClass("mobile-view")) return min; + + const $window = $(window); + const windowHeight = $window.height(); + const documentHeight = $(document).height(); + const topicBottomOffsetTop = $("#topic-bottom").offset().top; + + // the footer is bigger than the window, we can scroll down past the last post + if (documentHeight - windowHeight > topicBottomOffsetTop) return min; + + const scrollTop = $window.scrollTop(); + const visibleBottomHeight = scrollTop + windowHeight - topicBottomOffsetTop; + + if (visibleBottomHeight > 0) { + const bottomHeight = documentHeight - topicBottomOffsetTop; + const offset = (windowHeight - bottomHeight) * visibleBottomHeight / bottomHeight; + return Math.max(min, offset); + } + + return min; }