From c5b4e6b7084e8257979adec87f97d4800b7bec57 Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 12 Jul 2021 17:00:14 +0200 Subject: [PATCH 1/6] Add modal stack to allow better boost modal and media modal interaction. --- .../features/ui/containers/modal_container.js | 4 ++-- app/javascript/flavours/glitch/reducers/modal.js | 14 +++++--------- 2 files changed, 7 insertions(+), 11 deletions(-) diff --git a/app/javascript/flavours/glitch/features/ui/containers/modal_container.js b/app/javascript/flavours/glitch/features/ui/containers/modal_container.js index f074002e44..e13e745e6d 100644 --- a/app/javascript/flavours/glitch/features/ui/containers/modal_container.js +++ b/app/javascript/flavours/glitch/features/ui/containers/modal_container.js @@ -3,8 +3,8 @@ import { closeModal } from 'flavours/glitch/actions/modal'; import ModalRoot from '../components/modal_root'; const mapStateToProps = state => ({ - type: state.get('modal').modalType, - props: state.get('modal').modalProps, + type: state.getIn(['modal', 0, 'modalType'], null), + props: state.getIn(['modal', 0, 'modalProps'], {}), }); const mapDispatchToProps = dispatch => ({ diff --git a/app/javascript/flavours/glitch/reducers/modal.js b/app/javascript/flavours/glitch/reducers/modal.js index 52b05d69be..f8fdc29954 100644 --- a/app/javascript/flavours/glitch/reducers/modal.js +++ b/app/javascript/flavours/glitch/reducers/modal.js @@ -1,19 +1,15 @@ import { MODAL_OPEN, MODAL_CLOSE } from 'flavours/glitch/actions/modal'; import { TIMELINE_DELETE } from 'flavours/glitch/actions/timelines'; +import { Stack as ImmutableStack, Map as ImmutableMap } from 'immutable'; -const initialState = { - modalType: null, - modalProps: {}, -}; - -export default function modal(state = initialState, action) { +export default function modal(state = ImmutableStack(), action) { switch(action.type) { case MODAL_OPEN: - return { modalType: action.modalType, modalProps: action.modalProps }; + return state.unshift(ImmutableMap({ modalType: action.modalType, modalProps: action.modalProps })); case MODAL_CLOSE: - return (action.modalType === undefined || action.modalType === state.modalType) ? initialState : state; + return (action.modalType === undefined || action.modalType === state.getIn([0, 'modalType'])) ? state.shift() : state; case TIMELINE_DELETE: - return (state.modalProps.statusId === action.id) ? initialState : state; + return state.filterNot((modal) => modal.get('modalProps').statusId === action.id); default: return state; } From 6e3d5cbca26995e99d6ca8b1e3191edc539d87eb Mon Sep 17 00:00:00 2001 From: Claire Date: Mon, 12 Jul 2021 17:55:40 +0200 Subject: [PATCH 2/6] Fix and simplify browser history handling in relation to modals This simplifies the logic to: - when the last modal gets closed and we're in our history buffer state, go back - whenever a modal is open, ensure we're in a history buffer state by potentially pushing one --- .../flavours/glitch/components/modal_root.js | 34 ++++++++++++------- .../features/ui/components/modal_root.js | 8 ++--- 2 files changed, 24 insertions(+), 18 deletions(-) diff --git a/app/javascript/flavours/glitch/components/modal_root.js b/app/javascript/flavours/glitch/components/modal_root.js index 913234d325..7b5a630e5b 100644 --- a/app/javascript/flavours/glitch/components/modal_root.js +++ b/app/javascript/flavours/glitch/components/modal_root.js @@ -76,10 +76,13 @@ export default class ModalRoot extends React.PureComponent { this.activeElement = null; }).catch(console.error); - this.handleModalClose(); + this._handleModalClose(); } if (this.props.children && !prevProps.children) { - this.handleModalOpen(); + this._handleModalOpen(); + } + if (this.props.children) { + this._ensureHistoryBuffer(); } } @@ -88,22 +91,29 @@ export default class ModalRoot extends React.PureComponent { window.removeEventListener('keydown', this.handleKeyDown); } - handleModalClose () { + _handleModalOpen () { + this._modalHistoryKey = Date.now(); + this.unlistenHistory = this.history.listen((_, action) => { + if (action === 'POP') { + this.props.onClose(); + } + }); + } + + _handleModalClose () { this.unlistenHistory(); - const state = this.history.location.state; - if (state && state.mastodonModalOpen) { + const { state } = this.history.location; + if (state && state.mastodonModalKey === this._modalHistoryKey) { this.history.goBack(); } } - handleModalOpen () { - const history = this.history; - const state = {...history.location.state, mastodonModalOpen: true}; - history.push(history.location.pathname, state); - this.unlistenHistory = history.listen(() => { - this.props.onClose(); - }); + _ensureHistoryBuffer () { + const { pathname, state } = this.history.location; + if (!state || state.mastodonModalKey !== this._modalHistoryKey) { + this.history.push(pathname, { ...state, mastodonModalKey: this._modalHistoryKey }); + } } getSiblings = () => { diff --git a/app/javascript/flavours/glitch/features/ui/components/modal_root.js b/app/javascript/flavours/glitch/features/ui/components/modal_root.js index 0fd70de34b..2636e79f53 100644 --- a/app/javascript/flavours/glitch/features/ui/components/modal_root.js +++ b/app/javascript/flavours/glitch/features/ui/components/modal_root.js @@ -59,12 +59,8 @@ export default class ModalRoot extends React.PureComponent { backgroundColor: null, }; - getSnapshotBeforeUpdate () { - return { visible: !!this.props.type }; - } - - componentDidUpdate (prevProps, prevState, { visible }) { - if (visible) { + componentDidUpdate () { + if (!!this.props.type) { document.body.classList.add('with-modals--active'); document.documentElement.style.marginRight = `${getScrollbarWidth()}px`; } else { From 99f28c17dea35d0eec90a74c5fe95f60b6ad2f9e Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 13 Jul 2021 11:07:16 +0200 Subject: [PATCH 3/6] Fix scroll handling with modals --- app/javascript/flavours/glitch/components/scrollable_list.js | 3 +-- app/javascript/flavours/glitch/components/status_action_bar.js | 2 +- app/javascript/flavours/glitch/containers/mastodon.js | 2 +- .../flavours/glitch/features/account_gallery/index.js | 3 +-- app/javascript/flavours/glitch/features/status/index.js | 3 +-- 5 files changed, 5 insertions(+), 8 deletions(-) diff --git a/app/javascript/flavours/glitch/components/scrollable_list.js b/app/javascript/flavours/glitch/components/scrollable_list.js index cc8d9f1f31..5d0a065618 100644 --- a/app/javascript/flavours/glitch/components/scrollable_list.js +++ b/app/javascript/flavours/glitch/components/scrollable_list.js @@ -265,8 +265,7 @@ class ScrollableList extends PureComponent { } defaultShouldUpdateScroll = (prevRouterProps, { location }) => { - if ((((prevRouterProps || {}).location || {}).state || {}).mastodonModalOpen) return false; - return !(location.state && location.state.mastodonModalOpen); + return !(prevRouterProps?.location?.state?.mastodonModalKey || location.state?.mastodonModalKey); } handleLoadPending = e => { diff --git a/app/javascript/flavours/glitch/components/status_action_bar.js b/app/javascript/flavours/glitch/components/status_action_bar.js index 74bfd948ed..206ae74c87 100644 --- a/app/javascript/flavours/glitch/components/status_action_bar.js +++ b/app/javascript/flavours/glitch/components/status_action_bar.js @@ -147,7 +147,7 @@ class StatusActionBar extends ImmutablePureComponent { handleOpen = () => { let state = {...this.context.router.history.location.state}; - if (state.mastodonModalOpen) { + if (state.mastodonModalKey) { this.context.router.history.replace(`/statuses/${this.props.status.get('id')}`, { mastodonBackSteps: (state.mastodonBackSteps || 0) + 1 }); } else { state.mastodonBackSteps = (state.mastodonBackSteps || 0) + 1; diff --git a/app/javascript/flavours/glitch/containers/mastodon.js b/app/javascript/flavours/glitch/containers/mastodon.js index bcdd9b54eb..131303fd32 100644 --- a/app/javascript/flavours/glitch/containers/mastodon.js +++ b/app/javascript/flavours/glitch/containers/mastodon.js @@ -41,7 +41,7 @@ export default class Mastodon extends React.PureComponent { } shouldUpdateScroll (_, { location }) { - return !(location.state && location.state.mastodonModalOpen); + return !(location.state?.mastodonModalKey); } render () { diff --git a/app/javascript/flavours/glitch/features/account_gallery/index.js b/app/javascript/flavours/glitch/features/account_gallery/index.js index 2a43d1ed22..83d6233561 100644 --- a/app/javascript/flavours/glitch/features/account_gallery/index.js +++ b/app/javascript/flavours/glitch/features/account_gallery/index.js @@ -105,8 +105,7 @@ class AccountGallery extends ImmutablePureComponent { } shouldUpdateScroll = (prevRouterProps, { location }) => { - if ((((prevRouterProps || {}).location || {}).state || {}).mastodonModalOpen) return false; - return !(location.state && location.state.mastodonModalOpen); + return !(prevRouterProps?.location?.state?.mastodonModalKey || location.state?.mastodonModalKey); } setColumnRef = c => { diff --git a/app/javascript/flavours/glitch/features/status/index.js b/app/javascript/flavours/glitch/features/status/index.js index 513a6227ff..230966f2a8 100644 --- a/app/javascript/flavours/glitch/features/status/index.js +++ b/app/javascript/flavours/glitch/features/status/index.js @@ -508,8 +508,7 @@ class Status extends ImmutablePureComponent { } shouldUpdateScroll = (prevRouterProps, { location }) => { - if ((((prevRouterProps || {}).location || {}).state || {}).mastodonModalOpen) return false; - return !(location.state && location.state.mastodonModalOpen); + return !(prevRouterProps?.location?.state?.mastodonModalKey || location.state?.mastodonModalKey); } render () { From 84fbe4d030e3176fffaf49ac8eec0c0602b1ba87 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 13 Jul 2021 12:40:15 +0200 Subject: [PATCH 4/6] Refactor shouldUpdateScroll stuff --- .../flavours/glitch/components/scrollable_list.js | 11 +++-------- .../flavours/glitch/components/status_list.js | 1 - .../glitch/containers/scroll_container.js | 15 +++++++++++++++ .../glitch/features/account_gallery/index.js | 8 ++------ .../flavours/glitch/features/directory/index.js | 7 +++---- .../glitch/features/notifications/index.js | 4 +--- .../flavours/glitch/features/status/index.js | 8 ++------ .../flavours/glitch/features/ui/index.js | 2 +- 8 files changed, 27 insertions(+), 29 deletions(-) create mode 100644 app/javascript/flavours/glitch/containers/scroll_container.js diff --git a/app/javascript/flavours/glitch/components/scrollable_list.js b/app/javascript/flavours/glitch/components/scrollable_list.js index 5d0a065618..16f13afa49 100644 --- a/app/javascript/flavours/glitch/components/scrollable_list.js +++ b/app/javascript/flavours/glitch/components/scrollable_list.js @@ -1,5 +1,5 @@ import React, { PureComponent } from 'react'; -import { ScrollContainer } from 'react-router-scroll-4'; +import ScrollContainer from 'flavours/glitch/containers/scroll_container'; import PropTypes from 'prop-types'; import IntersectionObserverArticleContainer from 'flavours/glitch/containers/intersection_observer_article_container'; import LoadMore from './load_more'; @@ -34,7 +34,6 @@ class ScrollableList extends PureComponent { onScrollToTop: PropTypes.func, onScroll: PropTypes.func, trackScroll: PropTypes.bool, - shouldUpdateScroll: PropTypes.func, isLoading: PropTypes.bool, showLoading: PropTypes.bool, hasMore: PropTypes.bool, @@ -264,10 +263,6 @@ class ScrollableList extends PureComponent { this.props.onLoadMore(); } - defaultShouldUpdateScroll = (prevRouterProps, { location }) => { - return !(prevRouterProps?.location?.state?.mastodonModalKey || location.state?.mastodonModalKey); - } - handleLoadPending = e => { e.preventDefault(); this.props.onLoadPending(); @@ -281,7 +276,7 @@ class ScrollableList extends PureComponent { } render () { - const { children, scrollKey, trackScroll, shouldUpdateScroll, showLoading, isLoading, hasMore, numPending, prepend, alwaysPrepend, append, emptyMessage, onLoadMore } = this.props; + const { children, scrollKey, trackScroll, showLoading, isLoading, hasMore, numPending, prepend, alwaysPrepend, append, emptyMessage, onLoadMore } = this.props; const { fullscreen } = this.state; const childrenCount = React.Children.count(children); @@ -347,7 +342,7 @@ class ScrollableList extends PureComponent { if (trackScroll) { return ( - + {scrollableArea} ); diff --git a/app/javascript/flavours/glitch/components/status_list.js b/app/javascript/flavours/glitch/components/status_list.js index 60cc23f4b0..9095e087ee 100644 --- a/app/javascript/flavours/glitch/components/status_list.js +++ b/app/javascript/flavours/glitch/components/status_list.js @@ -18,7 +18,6 @@ export default class StatusList extends ImmutablePureComponent { onScrollToTop: PropTypes.func, onScroll: PropTypes.func, trackScroll: PropTypes.bool, - shouldUpdateScroll: PropTypes.func, isLoading: PropTypes.bool, isPartial: PropTypes.bool, hasMore: PropTypes.bool, diff --git a/app/javascript/flavours/glitch/containers/scroll_container.js b/app/javascript/flavours/glitch/containers/scroll_container.js new file mode 100644 index 0000000000..595f3155ff --- /dev/null +++ b/app/javascript/flavours/glitch/containers/scroll_container.js @@ -0,0 +1,15 @@ +import { ScrollContainer as OriginalScrollContainer } from 'react-router-scroll-4'; + +// ScrollContainer is used to automatically scroll to the top when pushing a +// new history state and remembering the scroll position when going back. +// There are a few things we need to do differently, though. +const defaultShouldUpdateScroll = (prevRouterProps, { location }) => { + return !(prevRouterProps?.location?.state?.mastodonModalKey || location.state?.mastodonModalKey); +} + +export default +class ScrollContainer extends OriginalScrollContainer { + static defaultProps = { + shouldUpdateScroll: defaultShouldUpdateScroll, + }; +} diff --git a/app/javascript/flavours/glitch/features/account_gallery/index.js b/app/javascript/flavours/glitch/features/account_gallery/index.js index 83d6233561..434a47dfca 100644 --- a/app/javascript/flavours/glitch/features/account_gallery/index.js +++ b/app/javascript/flavours/glitch/features/account_gallery/index.js @@ -11,7 +11,7 @@ import ImmutablePureComponent from 'react-immutable-pure-component'; import { getAccountGallery } from 'flavours/glitch/selectors'; import MediaItem from './components/media_item'; import HeaderContainer from 'flavours/glitch/features/account_timeline/containers/header_container'; -import { ScrollContainer } from 'react-router-scroll-4'; +import ScrollContainer from 'flavours/glitch/containers/scroll_container'; import LoadMore from 'flavours/glitch/components/load_more'; import MissingIndicator from 'flavours/glitch/components/missing_indicator'; import { openModal } from 'flavours/glitch/actions/modal'; @@ -104,10 +104,6 @@ class AccountGallery extends ImmutablePureComponent { this.handleScrollToBottom(); } - shouldUpdateScroll = (prevRouterProps, { location }) => { - return !(prevRouterProps?.location?.state?.mastodonModalKey || location.state?.mastodonModalKey); - } - setColumnRef = c => { this.column = c; } @@ -164,7 +160,7 @@ class AccountGallery extends ImmutablePureComponent { - +
diff --git a/app/javascript/flavours/glitch/features/directory/index.js b/app/javascript/flavours/glitch/features/directory/index.js index 858a8fa55a..cde5926e06 100644 --- a/app/javascript/flavours/glitch/features/directory/index.js +++ b/app/javascript/flavours/glitch/features/directory/index.js @@ -12,7 +12,7 @@ import AccountCard from './components/account_card'; import RadioButton from 'flavours/glitch/components/radio_button'; import classNames from 'classnames'; import LoadMore from 'flavours/glitch/components/load_more'; -import { ScrollContainer } from 'react-router-scroll-4'; +import ScrollContainer from 'flavours/glitch/containers/scroll_container'; const messages = defineMessages({ title: { id: 'column.directory', defaultMessage: 'Browse profiles' }, @@ -40,7 +40,6 @@ class Directory extends React.PureComponent { isLoading: PropTypes.bool, accountIds: ImmutablePropTypes.list.isRequired, dispatch: PropTypes.func.isRequired, - shouldUpdateScroll: PropTypes.func, columnId: PropTypes.string, intl: PropTypes.object.isRequired, multiColumn: PropTypes.bool, @@ -125,7 +124,7 @@ class Directory extends React.PureComponent { } render () { - const { isLoading, accountIds, intl, columnId, multiColumn, domain, shouldUpdateScroll } = this.props; + const { isLoading, accountIds, intl, columnId, multiColumn, domain } = this.props; const { order, local } = this.getParams(this.props, this.state); const pinned = !!columnId; @@ -163,7 +162,7 @@ class Directory extends React.PureComponent { multiColumn={multiColumn} /> - {multiColumn && !pinned ? {scrollableArea} : scrollableArea} + {multiColumn && !pinned ? {scrollableArea} : scrollableArea} ); } diff --git a/app/javascript/flavours/glitch/features/notifications/index.js b/app/javascript/flavours/glitch/features/notifications/index.js index 6fc951e374..075e729b18 100644 --- a/app/javascript/flavours/glitch/features/notifications/index.js +++ b/app/javascript/flavours/glitch/features/notifications/index.js @@ -99,7 +99,6 @@ class Notifications extends React.PureComponent { notifications: ImmutablePropTypes.list.isRequired, showFilterBar: PropTypes.bool.isRequired, dispatch: PropTypes.func.isRequired, - shouldUpdateScroll: PropTypes.func, intl: PropTypes.object.isRequired, isLoading: PropTypes.bool, isUnread: PropTypes.bool, @@ -220,7 +219,7 @@ class Notifications extends React.PureComponent { } render () { - const { intl, notifications, shouldUpdateScroll, isLoading, isUnread, columnId, multiColumn, hasMore, numPending, showFilterBar, lastReadId, canMarkAsRead, needsNotificationPermission } = this.props; + const { intl, notifications, isLoading, isUnread, columnId, multiColumn, hasMore, numPending, showFilterBar, lastReadId, canMarkAsRead, needsNotificationPermission } = this.props; const { notifCleaning, notifCleaningActive } = this.props; const { animatingNCD } = this.state; const pinned = !!columnId; @@ -273,7 +272,6 @@ class Notifications extends React.PureComponent { onLoadPending={this.handleLoadPending} onScrollToTop={this.handleScrollToTop} onScroll={this.handleScroll} - shouldUpdateScroll={shouldUpdateScroll} bindToDocument={!multiColumn} > {scrollableContent} diff --git a/app/javascript/flavours/glitch/features/status/index.js b/app/javascript/flavours/glitch/features/status/index.js index 230966f2a8..9dbba47726 100644 --- a/app/javascript/flavours/glitch/features/status/index.js +++ b/app/javascript/flavours/glitch/features/status/index.js @@ -32,7 +32,7 @@ import { initBlockModal } from 'flavours/glitch/actions/blocks'; import { initReport } from 'flavours/glitch/actions/reports'; import { initBoostModal } from 'flavours/glitch/actions/boosts'; import { makeGetStatus } from 'flavours/glitch/selectors'; -import { ScrollContainer } from 'react-router-scroll-4'; +import ScrollContainer from 'flavours/glitch/containers/scroll_container'; import ColumnBackButton from 'flavours/glitch/components/column_back_button'; import ColumnHeader from '../../components/column_header'; import StatusContainer from 'flavours/glitch/containers/status_container'; @@ -507,10 +507,6 @@ class Status extends ImmutablePureComponent { this.setState({ fullscreen: isFullscreen() }); } - shouldUpdateScroll = (prevRouterProps, { location }) => { - return !(prevRouterProps?.location?.state?.mastodonModalKey || location.state?.mastodonModalKey); - } - render () { let ancestors, descendants; const { setExpansion } = this; @@ -561,7 +557,7 @@ class Status extends ImmutablePureComponent { )} /> - +
{ancestors} diff --git a/app/javascript/flavours/glitch/features/ui/index.js b/app/javascript/flavours/glitch/features/ui/index.js index 1149eb14e5..ad063f01b9 100644 --- a/app/javascript/flavours/glitch/features/ui/index.js +++ b/app/javascript/flavours/glitch/features/ui/index.js @@ -212,7 +212,7 @@ class SwitchingColumnsArea extends React.PureComponent { - + From 19ea6618b18ac5c11d4b0ac4eb8acb2276db6a89 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 13 Jul 2021 12:57:07 +0200 Subject: [PATCH 5/6] Small scroll/history behavior fixup to take weird browser patterns into account --- app/javascript/flavours/glitch/containers/scroll_container.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/javascript/flavours/glitch/containers/scroll_container.js b/app/javascript/flavours/glitch/containers/scroll_container.js index 595f3155ff..740e266cbe 100644 --- a/app/javascript/flavours/glitch/containers/scroll_container.js +++ b/app/javascript/flavours/glitch/containers/scroll_container.js @@ -4,7 +4,8 @@ import { ScrollContainer as OriginalScrollContainer } from 'react-router-scroll- // new history state and remembering the scroll position when going back. // There are a few things we need to do differently, though. const defaultShouldUpdateScroll = (prevRouterProps, { location }) => { - return !(prevRouterProps?.location?.state?.mastodonModalKey || location.state?.mastodonModalKey); + // If the change is caused by opening a modal, do not scroll to top + return !(location.state?.mastodonModalKey && location.state?.mastodonModalKey !== prevRouterProps?.location?.state?.mastodonModalKey); } export default From e4270cb55a133f4cb93290ff88b3fd2a3aa9f536 Mon Sep 17 00:00:00 2001 From: Claire Date: Tue, 13 Jul 2021 13:49:40 +0200 Subject: [PATCH 6/6] Please CodeClimate --- app/javascript/flavours/glitch/containers/scroll_container.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/javascript/flavours/glitch/containers/scroll_container.js b/app/javascript/flavours/glitch/containers/scroll_container.js index 740e266cbe..d21ff63687 100644 --- a/app/javascript/flavours/glitch/containers/scroll_container.js +++ b/app/javascript/flavours/glitch/containers/scroll_container.js @@ -6,11 +6,13 @@ import { ScrollContainer as OriginalScrollContainer } from 'react-router-scroll- const defaultShouldUpdateScroll = (prevRouterProps, { location }) => { // If the change is caused by opening a modal, do not scroll to top return !(location.state?.mastodonModalKey && location.state?.mastodonModalKey !== prevRouterProps?.location?.state?.mastodonModalKey); -} +}; export default class ScrollContainer extends OriginalScrollContainer { + static defaultProps = { shouldUpdateScroll: defaultShouldUpdateScroll, }; + }