From bdec564d1417c51ef6958784fdd6375fd05da4fc Mon Sep 17 00:00:00 2001 From: David Taylor Date: Tue, 23 Jul 2024 10:24:44 +0100 Subject: [PATCH] DEV: Refactor header topic-info handling (#27989) - Move topic-title on-screen detection to intersection-observer (via new modifier), and add a boolean to header service which indicates whether it's on-screen - Move scroll-direction from Mixin to dedicated service. Teach it to pause scroll monitoring while transitions are in progress, to avoid reporting false changes in scroll direction. Also resets to a 'neutral' state after each navigation, which indicates the the user has not yet scrolled - When entering a topic view, notify the header service which post is being targeted. It can then make an educated guess about whether the topic title is likely to be in-view - Update header service `topicInfoVisible` to be a declarative getter, based on the three refactored sources of truth mentioned above - Update legacy widget header to use the header service for topic info All of these changes mean that the header no longer 'flickers' when navigating into topics on mobile. As well as the improved UX, this should also improve our Cumulative Layout Shift (CLS) web vital metrics. --- .../app/components/discourse-topic.js | 123 +------- .../discourse/app/components/footer-nav.js | 268 ++++++++---------- .../discourse/app/components/site-header.js | 21 +- .../discourse/app/components/topic-title.gjs | 5 + .../app/mixins/mobile-scroll-direction.js | 62 ---- .../app/modifiers/observe-intersection.js | 13 + .../discourse/app/routes/topic-from-params.js | 5 +- .../discourse/app/services/header.js | 72 ++++- .../app/services/scroll-direction.js | 137 +++++++++ spec/system/header_spec.rb | 28 ++ 10 files changed, 388 insertions(+), 346 deletions(-) delete mode 100644 app/assets/javascripts/discourse/app/mixins/mobile-scroll-direction.js create mode 100644 app/assets/javascripts/discourse/app/modifiers/observe-intersection.js create mode 100644 app/assets/javascripts/discourse/app/services/scroll-direction.js diff --git a/app/assets/javascripts/discourse/app/components/discourse-topic.js b/app/assets/javascripts/discourse/app/components/discourse-topic.js index 2e14817089e..126ad57a20f 100644 --- a/app/assets/javascripts/discourse/app/components/discourse-topic.js +++ b/app/assets/javascripts/discourse/app/components/discourse-topic.js @@ -1,21 +1,15 @@ import { getOwner } from "@ember/application"; import Component from "@ember/component"; import { alias } from "@ember/object/computed"; -import { schedule, scheduleOnce, throttle } from "@ember/runloop"; -import { service } from "@ember/service"; +import { schedule, scheduleOnce } from "@ember/runloop"; import { isBlank } from "@ember/utils"; import $ from "jquery"; import ClickTrack from "discourse/lib/click-track"; -import DiscourseURL from "discourse/lib/url"; import { highlightPost } from "discourse/lib/utilities"; -import MobileScrollDirection from "discourse/mixins/mobile-scroll-direction"; import Scrolling from "discourse/mixins/scrolling"; -import discourseLater from "discourse-common/lib/later"; import { bind, observes } from "discourse-common/utils/decorators"; -const MOBILE_SCROLL_DIRECTION_CHECK_THROTTLE = 300; - -export default Component.extend(Scrolling, MobileScrollDirection, { +export default Component.extend(Scrolling, { userFilters: alias("topic.userFilters"), classNameBindings: [ "multiSelect", @@ -24,25 +18,18 @@ export default Component.extend(Scrolling, MobileScrollDirection, { "topic.category.read_restricted:read_restricted", "topic.deleted:deleted-topic", ], - header: service(), menuVisible: true, SHORT_POST: 1200, postStream: alias("topic.postStream"), dockAt: 0, - _lastShowTopic: null, - - mobileScrollDirection: null, - pauseHeaderTopicUpdate: false, - @observes("enteredAt") _enteredTopic() { // Ember is supposed to only call observers when values change but something // in our view set up is firing this observer with the same value. This check // prevents scrolled from being called twice if (this.enteredAt && this.lastEnteredAt !== this.enteredAt) { - this._lastShowTopic = null; schedule("afterRender", this.scrolled); this.set("lastEnteredAt", this.enteredAt); } @@ -54,53 +41,10 @@ export default Component.extend(Scrolling, MobileScrollDirection, { } }, - _hideTopicInHeader() { - this.appEvents.trigger("header:hide-topic"); - this.header.topicInfoVisible = false; - this._lastShowTopic = false; - }, - - _showTopicInHeader(topic) { - if (this.pauseHeaderTopicUpdate) { - return; - } - this.appEvents.trigger("header:show-topic", topic); - this.header.topicInfoVisible = true; - this._lastShowTopic = true; - }, - - _updateTopic(topic, debounceDuration) { - if (topic === null) { - this._hideTopicInHeader(); - - if (debounceDuration && !this.pauseHeaderTopicUpdate) { - this.pauseHeaderTopicUpdate = true; - this._lastShowTopic = true; - - discourseLater(() => { - this._lastShowTopic = false; - this.pauseHeaderTopicUpdate = false; - }, debounceDuration); - } - - return; - } - - const offset = window.pageYOffset || document.documentElement.scrollTop; - this._lastShowTopic = this.shouldShowTopicInHeader(topic, offset); - - if (this._lastShowTopic) { - this._showTopicInHeader(topic); - } else { - this._hideTopicInHeader(); - } - }, - init() { this._super(...arguments); this.appEvents.on("discourse:focus-changed", this, "gotFocus"); this.appEvents.on("post:highlight", this, "_highlightPost"); - this.appEvents.on("header:update-topic", this, "_updateTopic"); }, didInsertElement() { @@ -119,10 +63,8 @@ export default Component.extend(Scrolling, MobileScrollDirection, { this._super(...arguments); // this happens after route exit, stuff could have trickled in - this._hideTopicInHeader(); this.appEvents.off("discourse:focus-changed", this, "gotFocus"); this.appEvents.off("post:highlight", this, "_highlightPost"); - this.appEvents.off("header:update-topic", this, "_updateTopic"); }, willDestroyElement() { @@ -133,8 +75,6 @@ export default Component.extend(Scrolling, MobileScrollDirection, { // Unbind link tracking $(this.element).off("click.discourse-redirect", ".cooked a, a.track-link"); - - this.resetExamineDockCache(); }, gotFocus(hasFocus) { @@ -143,20 +83,6 @@ export default Component.extend(Scrolling, MobileScrollDirection, { } }, - resetExamineDockCache() { - this.set("dockAt", 0); - }, - - shouldShowTopicInHeader(topic, offset) { - // On mobile, we show the header topic if the user has scrolled past the topic - // title and the current scroll direction is down - // On desktop the user only needs to scroll past the topic title. - return ( - offset > this.dockAt && - (this.site.desktopView || this.mobileScrollDirection === "down") - ); - }, - // The user has scrolled the window, or it is finished rendering and ready for processing. @bind scrolled() { @@ -165,54 +91,9 @@ export default Component.extend(Scrolling, MobileScrollDirection, { } const offset = window.pageYOffset || document.documentElement.scrollTop; - if (this.dockAt === 0) { - const title = document.querySelector("#topic-title"); - if (title) { - this.set("dockAt", title.getBoundingClientRect().top + window.scrollY); - } - } - this.set("hasScrolled", offset > 0); - const showTopic = this.shouldShowTopicInHeader(this.topic, offset); - - if (showTopic !== this._lastShowTopic) { - if (showTopic) { - this._showTopicInHeader(this.topic); - } else { - if (!DiscourseURL.isJumpScheduled()) { - const loadingNear = this.topic.get("postStream.loadingNearPost") || 1; - if (loadingNear === 1) { - this._hideTopicInHeader(); - } - } - } - } - - // Since the user has scrolled, we need to check the scroll direction on mobile. - // We use throttle instead of debounce because we want the switch to occur - // at the start of the scroll. This feels a lot more snappy compared to waiting - // for the scroll to end if we debounce. - if (this.site.mobileView && this.hasScrolled) { - throttle( - this, - this.calculateDirection, - offset, - MOBILE_SCROLL_DIRECTION_CHECK_THROTTLE - ); - } - // Trigger a scrolled event this.appEvents.trigger("topic:scrolled", offset); }, - - // We observe the scroll direction on mobile and if it's down, we show the topic - // in the header, otherwise, we hide it. - @observes("mobileScrollDirection") - toggleMobileHeaderTopic() { - return this.appEvents.trigger( - "header:update-topic", - this.mobileScrollDirection === "down" ? this.topic : null - ); - }, }); diff --git a/app/assets/javascripts/discourse/app/components/footer-nav.js b/app/assets/javascripts/discourse/app/components/footer-nav.js index 1464e5c4443..7cf400d4fa8 100644 --- a/app/assets/javascripts/discourse/app/components/footer-nav.js +++ b/app/assets/javascripts/discourse/app/components/footer-nav.js @@ -1,168 +1,144 @@ -import { throttle } from "@ember/runloop"; +import { service } from "@ember/service"; import MountWidget from "discourse/components/mount-widget"; import { postRNWebviewMessage } from "discourse/lib/utilities"; -import MobileScrollDirection from "discourse/mixins/mobile-scroll-direction"; -import Scrolling from "discourse/mixins/scrolling"; -import { observes } from "discourse-common/utils/decorators"; +import { SCROLLED_UP, UNSCROLLED } from "discourse/services/scroll-direction"; +import { bind, observes } from "discourse-common/utils/decorators"; -const MOBILE_SCROLL_DIRECTION_CHECK_THROTTLE = 150; +const FooterNavComponent = MountWidget.extend({ + widget: "footer-nav", + classNames: ["footer-nav", "visible"], + scrollDirection: service(), + routeHistory: [], + currentRouteIndex: 0, + canGoBack: false, + canGoForward: false, + backForwardClicked: null, -const FooterNavComponent = MountWidget.extend( - Scrolling, - MobileScrollDirection, - { - widget: "footer-nav", - mobileScrollDirection: null, - scrollEventDisabled: false, - classNames: ["footer-nav", "visible"], - routeHistory: [], - currentRouteIndex: 0, - canGoBack: false, - canGoForward: false, - backForwardClicked: null, + buildArgs() { + return { + canGoBack: this.canGoBack, + canGoForward: this.canGoForward, + }; + }, - buildArgs() { - return { - canGoBack: this.canGoBack, - canGoForward: this.canGoForward, - }; - }, + didInsertElement() { + this._super(...arguments); + this.appEvents.on("page:changed", this, "_routeChanged"); - didInsertElement() { - this._super(...arguments); - this.appEvents.on("page:changed", this, "_routeChanged"); + if (this.capabilities.isAppWebview) { + this.appEvents.on("modal:body-shown", this, "_modalOn"); + this.appEvents.on("modal:body-dismissed", this, "_modalOff"); + } - if (this.capabilities.isAppWebview) { - this.appEvents.on("modal:body-shown", this, "_modalOn"); - this.appEvents.on("modal:body-dismissed", this, "_modalOff"); - } + if (this.capabilities.isIpadOS) { + document.documentElement.classList.add("footer-nav-ipad"); + } else { + this.appEvents.on("composer:opened", this, "_composerOpened"); + this.appEvents.on("composer:closed", this, "_composerClosed"); + document.documentElement.classList.add("footer-nav-visible"); + } - if (this.capabilities.isIpadOS) { - document.documentElement.classList.add("footer-nav-ipad"); - } else { - this.bindScrolling(); - window.addEventListener("resize", this.scrolled, false); - this.appEvents.on("composer:opened", this, "_composerOpened"); - this.appEvents.on("composer:closed", this, "_composerClosed"); - document.documentElement.classList.add("footer-nav-visible"); - } - }, + this.scrollDirection.addObserver( + "lastScrollDirection", + this.toggleMobileFooter + ); + }, - willDestroyElement() { - this._super(...arguments); - this.appEvents.off("page:changed", this, "_routeChanged"); + willDestroyElement() { + this._super(...arguments); + this.appEvents.off("page:changed", this, "_routeChanged"); - if (this.capabilities.isAppWebview) { - this.appEvents.off("modal:body-shown", this, "_modalOn"); - this.appEvents.off("modal:body-removed", this, "_modalOff"); - } + if (this.capabilities.isAppWebview) { + this.appEvents.off("modal:body-shown", this, "_modalOn"); + this.appEvents.off("modal:body-removed", this, "_modalOff"); + } - if (this.capabilities.isIpadOS) { - document.documentElement.classList.remove("footer-nav-ipad"); - } else { - this.unbindScrolling(); - window.removeEventListener("resize", this.scrolled); - this.appEvents.off("composer:opened", this, "_composerOpened"); - this.appEvents.off("composer:closed", this, "_composerClosed"); - } - }, + if (this.capabilities.isIpadOS) { + document.documentElement.classList.remove("footer-nav-ipad"); + } else { + this.unbindScrolling(); + window.removeEventListener("resize", this.scrolled); + this.appEvents.off("composer:opened", this, "_composerOpened"); + this.appEvents.off("composer:closed", this, "_composerClosed"); + } - // The user has scrolled the window, or it is finished rendering and ready for processing. - scrolled() { - if ( - this.isDestroyed || - this.isDestroying || - this._state !== "inDOM" || - this.scrollEventDisabled - ) { - return; - } + this.scrollDirection.removeObserver( + "lastScrollDirection", + this.toggleMobileFooter + ); + }, - throttle( - this, - this.calculateDirection, - window.pageYOffset, - MOBILE_SCROLL_DIRECTION_CHECK_THROTTLE + @bind + toggleMobileFooter() { + const visible = [UNSCROLLED, SCROLLED_UP].includes( + this.scrollDirection.lastScrollDirection + ); + this.element.classList.toggle("visible", visible); + document.documentElement.classList.toggle("footer-nav-visible", visible); + }, + + _routeChanged(route) { + // only update route history if not using back/forward nav + if (this.backForwardClicked) { + this.backForwardClicked = null; + return; + } + + this.routeHistory.push(route.url); + this.set("currentRouteIndex", this.routeHistory.length); + + this.queueRerender(); + }, + + _composerOpened() { + this.set("mobileScrollDirection", "down"); + this.set("scrollEventDisabled", true); + }, + + _composerClosed() { + this.set("mobileScrollDirection", null); + this.set("scrollEventDisabled", false); + }, + + _modalOn() { + const backdrop = document.querySelector(".modal-backdrop"); + if (backdrop) { + postRNWebviewMessage( + "headerBg", + getComputedStyle(backdrop)["background-color"] ); - }, + } + }, - // We observe the scroll direction on mobile and if it's down, we show the topic - // in the header, otherwise, we hide it. - @observes("mobileScrollDirection") - toggleMobileFooter() { - this.element.classList.toggle( - "visible", - this.mobileScrollDirection === null ? true : false + _modalOff() { + const dheader = document.querySelector(".d-header"); + if (dheader) { + postRNWebviewMessage( + "headerBg", + getComputedStyle(dheader)["background-color"] ); - document.documentElement.classList.toggle( - "footer-nav-visible", - this.mobileScrollDirection === null ? true : false - ); - }, + } + }, - _routeChanged(route) { - // only update route history if not using back/forward nav - if (this.backForwardClicked) { - this.backForwardClicked = null; - return; - } + goBack() { + this.set("currentRouteIndex", this.currentRouteIndex - 1); + this.backForwardClicked = true; + window.history.back(); + }, - this.routeHistory.push(route.url); - this.set("currentRouteIndex", this.routeHistory.length); + goForward() { + this.set("currentRouteIndex", this.currentRouteIndex + 1); + this.backForwardClicked = true; + window.history.forward(); + }, - this.queueRerender(); - }, + @observes("currentRouteIndex") + setBackForward() { + let index = this.currentRouteIndex; - _composerOpened() { - this.set("mobileScrollDirection", "down"); - this.set("scrollEventDisabled", true); - }, - - _composerClosed() { - this.set("mobileScrollDirection", null); - this.set("scrollEventDisabled", false); - }, - - _modalOn() { - const backdrop = document.querySelector(".modal-backdrop"); - if (backdrop) { - postRNWebviewMessage( - "headerBg", - getComputedStyle(backdrop)["background-color"] - ); - } - }, - - _modalOff() { - const dheader = document.querySelector(".d-header"); - if (dheader) { - postRNWebviewMessage( - "headerBg", - getComputedStyle(dheader)["background-color"] - ); - } - }, - - goBack() { - this.set("currentRouteIndex", this.currentRouteIndex - 1); - this.backForwardClicked = true; - window.history.back(); - }, - - goForward() { - this.set("currentRouteIndex", this.currentRouteIndex + 1); - this.backForwardClicked = true; - window.history.forward(); - }, - - @observes("currentRouteIndex") - setBackForward() { - let index = this.currentRouteIndex; - - this.set("canGoBack", index > 1 || document.referrer ? true : false); - this.set("canGoForward", index < this.routeHistory.length ? true : false); - }, - } -); + this.set("canGoBack", index > 1 || document.referrer ? true : false); + this.set("canGoForward", index < this.routeHistory.length ? true : false); + }, +}); export default FooterNavComponent; diff --git a/app/assets/javascripts/discourse/app/components/site-header.js b/app/assets/javascripts/discourse/app/components/site-header.js index e1d58087af7..aa2016c311a 100644 --- a/app/assets/javascripts/discourse/app/components/site-header.js +++ b/app/assets/javascripts/discourse/app/components/site-header.js @@ -1,4 +1,5 @@ import { DEBUG } from "@glimmer/env"; +import { getOwner } from "@ember/owner"; import { schedule } from "@ember/runloop"; import { waitForPromise } from "@ember/test-waiters"; import ItsATrap from "@discourse/itsatrap"; @@ -212,9 +213,14 @@ const SiteHeaderComponent = MountWidget.extend( } }, - setTopic(topic) { + setTopic() { + const header = getOwner(this).lookup("service:header"); + if (header.topicInfoVisible) { + this._topic = header.topicInfo; + } else { + this._topic = null; + } this.eventDispatched("dom:clean", "header"); - this._topic = topic; this.queueRerender(); }, @@ -231,8 +237,9 @@ const SiteHeaderComponent = MountWidget.extend( this._resizeDiscourseMenuPanel = () => this.afterRender(); window.addEventListener("resize", this._resizeDiscourseMenuPanel); - this.appEvents.on("header:show-topic", this, "setTopic"); - this.appEvents.on("header:hide-topic", this, "setTopic"); + const headerService = getOwner(this).lookup("service:header"); + headerService.addObserver("topicInfoVisible", this, "setTopic"); + headerService.topicInfoVisible; // Access property to set up observer this.appEvents.on("user-menu:rendered", this, "_animateMenu"); @@ -299,9 +306,9 @@ const SiteHeaderComponent = MountWidget.extend( this._super(...arguments); window.removeEventListener("resize", this._resizeDiscourseMenuPanel); - - this.appEvents.off("header:show-topic", this, "setTopic"); - this.appEvents.off("header:hide-topic", this, "setTopic"); + getOwner(this) + .lookup("service:header") + .removeObserver("topicInfoVisible", this, "setTopic"); this.appEvents.off("dom:clean", this, "_cleanDom"); this.appEvents.off("user-menu:rendered", this, "_animateMenu"); diff --git a/app/assets/javascripts/discourse/app/components/topic-title.gjs b/app/assets/javascripts/discourse/app/components/topic-title.gjs index 3576fe987e4..0f629d28f6f 100644 --- a/app/assets/javascripts/discourse/app/components/topic-title.gjs +++ b/app/assets/javascripts/discourse/app/components/topic-title.gjs @@ -3,8 +3,10 @@ import { hash } from "@ember/helper"; import { on } from "@ember/modifier"; import { action } from "@ember/object"; import didInsert from "@ember/render-modifiers/modifiers/did-insert"; +import { service } from "@ember/service"; import PluginOutlet from "discourse/components/plugin-outlet"; import { isiPad } from "discourse/lib/utilities"; +import observeIntersection from "discourse/modifiers/observe-intersection"; export let topicTitleDecorators = []; @@ -17,6 +19,8 @@ export function resetTopicTitleDecorators() { } export default class TopicTitle extends Component { + @service header; + @action applyDecorators(element) { const fancyTitle = element.querySelector(".fancy-title"); @@ -54,6 +58,7 @@ export default class TopicTitle extends Component {
diff --git a/app/assets/javascripts/discourse/app/mixins/mobile-scroll-direction.js b/app/assets/javascripts/discourse/app/mixins/mobile-scroll-direction.js deleted file mode 100644 index 76cf825edb6..00000000000 --- a/app/assets/javascripts/discourse/app/mixins/mobile-scroll-direction.js +++ /dev/null @@ -1,62 +0,0 @@ -import Mixin from "@ember/object/mixin"; -import $ from "jquery"; -import discourseDebounce from "discourse-common/lib/debounce"; - -// Small buffer so that very tiny scrolls don't trigger mobile header switch -const MOBILE_SCROLL_TOLERANCE = 5; - -export default Mixin.create({ - _lastScroll: null, - _bottomHit: 0, - - calculateDirection(offset) { - // Difference between this scroll and the one before it. - const delta = Math.floor(offset - this._lastScroll); - - // This is a tiny scroll, so we ignore it. - if (delta <= MOBILE_SCROLL_TOLERANCE && delta >= -MOBILE_SCROLL_TOLERANCE) { - return; - } - - // don't calculate when resetting offset (i.e. going to /latest or to next topic in suggested list) - if (offset === 0) { - return; - } - - const prevDirection = this.mobileScrollDirection; - const currDirection = delta > 0 ? "down" : null; - - const distanceToBottom = Math.floor( - $("body").height() - offset - $(window).height() - ); - - // Handle Safari top overscroll first - if (offset < 0) { - this.set("mobileScrollDirection", null); - } else if (currDirection !== prevDirection && distanceToBottom > 0) { - this.set("mobileScrollDirection", currDirection); - } - - // We store this to compare against it the next time the user scrolls - this._lastScroll = Math.floor(offset); - - // Not at the bottom yet - if (distanceToBottom > 0) { - this._bottomHit = 0; - return; - } - - // If the user reaches the very bottom of the topic, we only want to reset - // this scroll direction after a second scroll down. This is a nicer event - // similar to what Safari and Chrome do. - discourseDebounce(this, this._setBottomHit, 1000); - - if (this._bottomHit === 1) { - this.set("mobileScrollDirection", null); - } - }, - - _setBottomHit() { - this._bottomHit = 1; - }, -}); diff --git a/app/assets/javascripts/discourse/app/modifiers/observe-intersection.js b/app/assets/javascripts/discourse/app/modifiers/observe-intersection.js new file mode 100644 index 00000000000..719f86c92a3 --- /dev/null +++ b/app/assets/javascripts/discourse/app/modifiers/observe-intersection.js @@ -0,0 +1,13 @@ +import { modifier } from "ember-modifier"; + +export default modifier((element, [callback], { threshold = 1 }) => { + const observer = new IntersectionObserver((entries) => { + entries.forEach(callback, { threshold }); + }); + + observer.observe(element); + + return () => { + observer.disconnect(); + }; +}); diff --git a/app/assets/javascripts/discourse/app/routes/topic-from-params.js b/app/assets/javascripts/discourse/app/routes/topic-from-params.js index 242ebb7354c..76521095bd9 100644 --- a/app/assets/javascripts/discourse/app/routes/topic-from-params.js +++ b/app/assets/javascripts/discourse/app/routes/topic-from-params.js @@ -39,19 +39,20 @@ export default class TopicFromParams extends DiscourseRoute { }); } - afterModel() { + afterModel(model) { const topic = this.modelFor("topic"); if (topic.isPrivateMessage && topic.suggested_topics) { this.pmTopicTrackingState.startTracking(); } - this.header.topicInfo = topic; + this.header.enterTopic(topic, model.nearPost); } deactivate() { super.deactivate(...arguments); this.controllerFor("topic").unsubscribe(); + this.header.clearTopic(); } setupController(controller, params, { _discourse_anchor }) { diff --git a/app/assets/javascripts/discourse/app/services/header.js b/app/assets/javascripts/discourse/app/services/header.js index 1212aae523f..38ec17a9704 100644 --- a/app/assets/javascripts/discourse/app/services/header.js +++ b/app/assets/javascripts/discourse/app/services/header.js @@ -1,15 +1,20 @@ import { tracked } from "@glimmer/tracking"; import { registerDestructor } from "@ember/destroyable"; +import { action } from "@ember/object"; +import { dependentKeyCompat } from "@ember/object/compat"; import Service, { service } from "@ember/service"; import { TrackedMap } from "@ember-compat/tracked-built-ins"; import { disableImplicitInjections } from "discourse/lib/implicit-injections"; import deprecated from "discourse-common/lib/deprecated"; +import { SCROLLED_UP } from "./scroll-direction"; const VALID_HEADER_BUTTONS_TO_HIDE = ["search", "login", "signup"]; @disableImplicitInjections export default class Header extends Service { @service siteSettings; + @service scrollDirection; + @service site; /** * The topic currently viewed on the page. @@ -20,14 +25,7 @@ export default class Header extends Service { */ @tracked topicInfo = null; - /** - * Indicates whether the topic information is visible on the header. - * - * The information is updated when the user scrolls the page. - * - * @type {boolean} - */ - @tracked topicInfoVisible = false; + @tracked mainTopicTitleVisible = false; @tracked hamburgerVisible = false; @tracked userVisible = false; @@ -48,6 +46,33 @@ export default class Header extends Service { return this.topicInfoVisible ? this.topicInfo : null; } + /** + * Indicates whether topic info should be displayed + * in the header. + */ + @dependentKeyCompat // For legacy `site-header` observer compat + get topicInfoVisible() { + if (!this.topicInfo) { + // Not on a topic page + return false; + } + + if (this.mainTopicTitleVisible) { + // Title is already visible on screen, no need to duplicate + return false; + } + + if ( + this.site.mobileView && + this.scrollDirection.lastScrollDirection === SCROLLED_UP + ) { + // On mobile, we hide the topic info when scrolling up + return false; + } + + return true; + } + get useGlimmerHeader() { if (this.siteSettings.glimmer_header_mode === "disabled") { return false; @@ -103,4 +128,35 @@ export default class Header extends Service { }); return Array.from(buttonsToHide); } + + /** + * Called by a modifier attached to the main topic title element. + */ + @action + titleIntersectionChanged(e) { + if (e.isIntersecting) { + this.mainTopicTitleVisible = true; + } else if (e.boundingClientRect.top > 0) { + // Title is below the curent viewport position. Unusual, but can be caused with + // small viewport and/or large headers. Treat same as if title is on screen. + this.mainTopicTitleVisible = true; + } else { + this.mainTopicTitleVisible = false; + } + } + + /** + * Called whenever a topic route is entered. Sets the current topicInfo, + * and makes a guess about whether the main topic title is likely to be visible + * on initial load. The IntersectionObserver will correct this later if needed. + */ + enterTopic(topic, postNumber) { + this.topicInfo = topic; + this.mainTopicTitleVisible = !postNumber || postNumber === 1; + } + + clearTopic() { + this.topicInfo = null; + this.mainTopicTitleVisible = false; + } } diff --git a/app/assets/javascripts/discourse/app/services/scroll-direction.js b/app/assets/javascripts/discourse/app/services/scroll-direction.js new file mode 100644 index 00000000000..451ec07dcac --- /dev/null +++ b/app/assets/javascripts/discourse/app/services/scroll-direction.js @@ -0,0 +1,137 @@ +import { tracked } from "@glimmer/tracking"; +import { next, throttle } from "@ember/runloop"; +import Service, { service } from "@ember/service"; +import { disableImplicitInjections } from "discourse/lib/implicit-injections"; +import discourseDebounce from "discourse-common/lib/debounce"; +import { bind } from "discourse-common/utils/decorators"; + +// Small buffer so that very tiny scrolls don't trigger mobile header switch +const MOBILE_SCROLL_TOLERANCE = 5; + +const PAUSE_AFTER_TRANSITION_MS = 1000; + +export const UNSCROLLED = Symbol("unscrolled"), + SCROLLED_DOWN = Symbol("scroll-down"), + SCROLLED_UP = Symbol("scroll-up"); + +@disableImplicitInjections +export default class ScrollDirection extends Service { + @service router; + @tracked lastScrollDirection = UNSCROLLED; + + #lastScroll = null; + #bottomHit = 0; + #paused = false; + + constructor() { + super(...arguments); + this.routeDidChange(); + window.addEventListener("scroll", this.onScroll, { passive: true }); + this.router.on("routeWillChange", this.routeWillChange); + this.router.on("routeDidChange", this.routeDidChange); + } + + willDestroy() { + window.removeEventListener("scroll", this.onScroll); + this.router.off("routeDidChange", this.routeDidChange); + } + + @bind + routeWillChange() { + // Pause detection until the transition is over + this.#paused = true; + } + + @bind + routeDidChange() { + this.#paused = true; + + // User hasn't scrolled yet on this route + this.lastScrollDirection = UNSCROLLED; + + // Wait for the initial DOM render to be done + next(() => { + // Then allow a bit of extra time for any DOM shifts to settle + discourseDebounce(this.unpause, PAUSE_AFTER_TRANSITION_MS); + }); + } + + @bind + unpause() { + this.#paused = false; + } + + @bind + onScroll() { + if (this.#paused) { + this.#lastScroll = window.scrollY; + return; + } else { + throttle(this.handleScroll, 100, false); + } + } + + @bind + handleScroll() { + // Unfortunately no public API for this + // eslint-disable-next-line ember/no-private-routing-service + if (this.router._router._routerMicrolib.activeTransition) { + // console.log("activetransition"); + return; + } + + const offset = window.scrollY; + this.calculateDirection(offset); + } + + calculateDirection(offset) { + // Difference between this scroll and the one before it. + const delta = Math.floor(offset - this.#lastScroll); + + // This is a tiny scroll, so we ignore it. + if (delta <= MOBILE_SCROLL_TOLERANCE && delta >= -MOBILE_SCROLL_TOLERANCE) { + return; + } + + // don't calculate when resetting offset (i.e. going to /latest or to next topic in suggested list) + if (offset === 0) { + return; + } + + const prevDirection = this.lastScrollDirection; + const currDirection = delta > 0 ? SCROLLED_DOWN : SCROLLED_UP; + + const distanceToBottom = Math.floor( + document.body.clientHeight - offset - window.innerHeight + ); + + // Handle Safari top overscroll first + if (offset < 0) { + this.lastScrollDirection = UNSCROLLED; + } else if (currDirection !== prevDirection && distanceToBottom > 0) { + this.lastScrollDirection = currDirection; + } + + // We store this to compare against it the next time the user scrolls + this.#lastScroll = Math.floor(offset); + + if (distanceToBottom > 0) { + this.#bottomHit = 0; + } else { + // If the user reaches the very bottom of the topic, we only want to reset + // this scroll direction after a second scroll down. This is a nicer event + // similar to what Safari and Chrome do. + discourseDebounce(this, this.#setBottomHit, 1000); + + if (this.#bottomHit === 1) { + this.lastScrollDirection = UNSCROLLED; + } + } + + this.lastScrollTimestamp = Date.now(); + } + + #setBottomHit() { + this.#bottomHit = 1; + } +} diff --git a/spec/system/header_spec.rb b/spec/system/header_spec.rb index 0697d2448fa..b04a2c84bea 100644 --- a/spec/system/header_spec.rb +++ b/spec/system/header_spec.rb @@ -259,4 +259,32 @@ RSpec.describe "Glimmer Header", type: :system do expect(search).to have_no_search_menu_visible end end + + describe "mobile topic-info" do + fab!(:topic) + fab!(:posts) { Fabricate.times(5, :post, topic: topic) } + + it "only shows when scrolled down", mobile: true do + visit "/t/#{topic.slug}/#{topic.id}" + + expect(page).to have_css("#topic-title") # Main topic title + expect(page).to have_css("header.d-header .auth-buttons .login-button") # header buttons visible when no topic-info in header + + page.execute_script("document.querySelector('#post_4').scrollIntoView()") + + expect(page).not_to have_css("header.d-header .auth-buttons .login-button") # No header buttons + expect(page).to have_css("header.d-header .title-wrapper .topic-link") # Title is shown in header + + page.execute_script("window.scrollTo(0, 0)") + expect(page).to have_css("#topic-title") # Main topic title + expect(page).to have_css("header.d-header .auth-buttons .login-button") # header buttons visible when no topic-info in header + end + + it "shows when navigating direct to a later post", mobile: true do + visit "/t/#{topic.slug}/#{topic.id}/4" + + expect(page).not_to have_css("header.d-header .auth-buttons .login-button") # No header buttons + expect(page).to have_css("header.d-header .title-wrapper .topic-link") # Title is shown in header + end + end end