Route compared-pane annotation scroll and retry sidebar thread scroll

src/elements/common/flowTypes.js

@@ -35,6 +35,9 @@ type Page = {
 
 type AdditionalVersionInfo = {
     currentVersionId?: ?string,
+    // Annotation-path version changes vs versions-sidebar clicks. Compare mode
+    // must not forward annotation-driven changes to the host (that unmounts the compared pane).
+    origin?: 'annotation',
     updateVersionToCurrent: () => void,
 };
 

src/elements/content-preview/ContentPreview.js

@@ -89,6 +89,11 @@ type StartAt = {
     value: number,
 };
 
+type AnnotationScrollRequest = {
+    annotation: Annotation,
+    deferScrollToOnload?: boolean,
+};
+
 type Props = {
     accessPattern?: 'file_list' | 'direct_link' | 'shared_link',
     advancedContentInsights: {
@@ -117,6 +122,7 @@ type Props = {
      * future major version.
      */
     metadataApiHost?: string,
+    annotationScrollRequest?: ?AnnotationScrollRequest,
     appHost: string,
     autoFocus: boolean,
     boxAnnotations?: Object,
@@ -153,6 +159,7 @@ type Props = {
     onAnnotatorEvent: Function,
     onBeforeNavigate?: (targetFileId: string) => boolean | Promise<boolean>,
     onClose?: Function,
+    onComparedAnnotationSelect: (annotation: Annotation, deferScrollToOnload?: boolean) => void,
     onContentInsightsEventReport: Function,
     onDownload: Function,
     onLoad: Function,
@@ -354,6 +361,7 @@ class ContentPreview extends React.PureComponent<Props, State> {
         loadingIndicatorDelayMs: 0,
         onAnnotator: noop,
         onAnnotatorEvent: noop,
+        onComparedAnnotationSelect: noop,
         onContentInsightsEventReport: noop,
         onDownload: noop,
         onError: noop,
@@ -633,6 +641,15 @@ class ContentPreview extends React.PureComponent<Props, State> {
             this.setState({ selectedVersion: undefined });
         }
 
+        // The compared pane has no sidebar of its own, so the pane that does forwards
+        // annotations belonging to this version. A pane mounting for a newly compared version
+        // already deep links through fileOptions, so this only covers one already showing it.
+        const { annotationScrollRequest } = this.props;
+        if (annotationScrollRequest && annotationScrollRequest !== prevProps.annotationScrollRequest) {
+            const { annotation, deferScrollToOnload } = annotationScrollRequest;
+            this.handleAnnotationSelect(annotation, deferScrollToOnload);
+        }
+
         if (haveExperiencesChanged && this.preview && this.preview.updateExperiences) {
             this.preview.updateExperiences(previewExperiences);
         }
@@ -1569,13 +1586,20 @@ class ContentPreview extends React.PureComponent<Props, State> {
      * @param {object} [additionalVersionInfo] - extra info about the version
      */
     onVersionChange = (version?: BoxItemVersion, additionalVersionInfo: AdditionalVersionInfo = {}): void => {
-        const { onVersionChange }: Props = this.props;
+        const { isComparing, onVersionChange }: Props = this.props;
+        const { currentVersionId, origin } = additionalVersionInfo;
         this.updateVersionToCurrent = additionalVersionInfo.updateVersionToCurrent;
 
-        onVersionChange(version, additionalVersionInfo);
-        // Host still gets the event so the compared pane can follow comparedVersion.
+        // Annotation clicks rewrite the activity path's fileVersionId. While comparing, the host
+        // reads a change back to the current version as leaving comparison and unmounts the
+        // compared pane, so those stay local. Annotations on any other version are forwarded so
+        // the compared pane follows the thread. Versions-sidebar clicks do not set origin.
+        const isReturnToCurrentVersion = !version || version.id === currentVersionId;
+        if (!(isComparing && origin === 'annotation' && isReturnToCurrentVersion)) {
+            onVersionChange(version, additionalVersionInfo);
+        }
         // The left pane stays on current for the whole comparison session.
-        if (!this.props.isComparing) {
+        if (!isComparing) {
             this.setState({
                 selectedVersion: version,
             });
@@ -1615,20 +1639,30 @@ class ContentPreview extends React.PureComponent<Props, State> {
         videoPlayer.addEventListener('loadeddata', handleLoadedData);
     };
 
-    handleAnnotationSelect = ({ file_version, id, target }: Annotation, deferScrollToOnload: boolean = false) => {
-        if (this.props.isComparing) {
-            return;
-        }
-
+    handleAnnotationSelect = (annotation: Annotation, deferScrollToOnload: boolean = false) => {
+        const { file_version, id, target } = annotation;
+        const { isComparing, onComparedAnnotationSelect }: Props = this.props;
         const { location = {} } = target;
         const { file } = this.state;
         const annotationFileVersionId = getProp(file_version, 'id');
         const currentFileVersionId = getProp(file, 'file_version.id');
         const currentPreviewFileVersionId = getProp(this.getVersionToPreview(), 'id', currentFileVersionId);
         const unit = startAtTypes[location.type];
         const viewer = this.getViewer();
+        const isOtherVersion = !!annotationFileVersionId && annotationFileVersionId !== currentPreviewFileVersionId;
 
-        if (unit && annotationFileVersionId && annotationFileVersionId !== currentPreviewFileVersionId) {
+        // Each pane stays on its own version for the whole comparison, so route by version
+        // instead of switching: annotations on the compared version belong to the other pane.
+        if (isComparing) {
+            if (isOtherVersion) {
+                onComparedAnnotationSelect(annotation, deferScrollToOnload);
+            } else {
+                this.emitScrollToAnnotation(id, target);
+            }
+            return;
+        }
+
+        if (unit && isOtherVersion) {
             // Frame.value is milliseconds; Preview SDK startAt expects seconds for video.
             const value = location.type === 'frame' ? convertTimestampToSeconds(location.value) : location.value;
             this.setState({
@@ -1939,16 +1973,25 @@ const MemoConnectedContentPreview = React.memo(ConnectedContentPreview);
 function ContentPreviewWithComparison(props: ContentPreviewProps) {
     const { comparedVersion, ...rest } = props;
     const [comparedSlot, setComparedSlot] = React.useState<?HTMLDivElement>(null);
+    const [annotationScrollRequest, setAnnotationScrollRequest] = React.useState<?AnnotationScrollRequest>(null);
     const comparedVersionId = comparedVersion && comparedVersion.id;
     const isComparing = comparedVersionId != null && comparedVersionId !== '';
 
+    // A new object every time so selecting the same annotation twice still scrolls.
+    const handleComparedAnnotationSelect = React.useCallback(
+        (annotation, deferScrollToOnload) => setAnnotationScrollRequest({ annotation, deferScrollToOnload }),
+        [],
+    );
+
     return (
         <React.Fragment>
             <MemoConnectedContentPreview
                 {...rest}
+                annotationScrollRequest={undefined}
                 collection={isComparing ? EMPTY_COLLECTION : rest.collection}
                 comparedSlotRef={setComparedSlot}
                 isComparing={isComparing}
+                onComparedAnnotationSelect={handleComparedAnnotationSelect}
             />
             {comparedSlot && isComparing
                 ? createPortal(
@@ -1957,8 +2000,8 @@ function ContentPreviewWithComparison(props: ContentPreviewProps) {
                           key={comparedVersionId}
                           accessPattern={undefined}
                           advancedContentInsights={undefined}
+                          annotationScrollRequest={annotationScrollRequest}
                           autoFocus={false}
-                          boxAnnotations={undefined}
                           collection={EMPTY_COLLECTION}
                           componentRef={undefined}
                           comparedSlotRef={undefined}
@@ -1975,6 +2018,7 @@ function ContentPreviewWithComparison(props: ContentPreviewProps) {
                           onAnnotator={noop}
                           onAnnotatorEvent={noop}
                           onBeforeNavigate={undefined}
+                          onComparedAnnotationSelect={noop}
                           onContentInsightsEventReport={noop}
                           onError={noop}
                           onLoad={noop}
@@ -1986,8 +2030,16 @@ function ContentPreviewWithComparison(props: ContentPreviewProps) {
                           previewVersion={comparedVersion}
                           resin={undefined}
                           renderCustomPreview={undefined}
-                          showAnnotations={false}
+                          // Inherit host showAnnotations + boxAnnotations so this pane
+                          // creates a second annotator (PREVIEW-1818). Create stays off:
+                          // controls only hide the toolbar; discoverability would still
+                          // open the comment composer on text select.
+                          enableAnnotationsDiscoverability={false}
+                          enableAnnotationsImageDiscoverability={false}
+                          enableAnnotationsOnlyControls={false}
                           showAnnotationsControls={false}
+                          showAnnotationsDrawing={false}
+                          showAnnotationsDrawingCreate={false}
                       />,
                       comparedSlot,
                   )

src/elements/content-preview/__tests__/ContentPreview.test.js

@@ -1796,6 +1796,115 @@ describe('elements/content-preview/ContentPreview', () => {
             expect(onVersionChange).toHaveBeenCalledWith(version, {});
             expect(wrapper.state('selectedVersion')).toBeUndefined();
         });
+
+        test('should not notify the host when an annotation returns to the current version while comparing', () => {
+            const onVersionChange = jest.fn();
+            const wrapper = getWrapper({ isComparing: true, onVersionChange });
+            const instance = wrapper.instance();
+            const version = { id: '12345' };
+
+            instance.onVersionChange(version, {
+                currentVersionId: '12345',
+                origin: 'annotation',
+                updateVersionToCurrent: jest.fn(),
+            });
+
+            expect(onVersionChange).not.toHaveBeenCalled();
+            expect(wrapper.state('selectedVersion')).toBeUndefined();
+        });
+
+        test('should notify the host for an annotation on another version so the compared pane follows', () => {
+            const onVersionChange = jest.fn();
+            const wrapper = getWrapper({ isComparing: true, onVersionChange });
+            const instance = wrapper.instance();
+            const version = { id: '999' };
+            const additionalVersionInfo = { currentVersionId: '12345', origin: 'annotation' };
+
+            instance.onVersionChange(version, additionalVersionInfo);
+
+            expect(onVersionChange).toHaveBeenCalledWith(version, additionalVersionInfo);
+            expect(wrapper.state('selectedVersion')).toBeUndefined();
+        });
+
+        test('should not notify the host for the annotation-driven version reset when comparing', () => {
+            // SidebarPanels resets the version (null) when the sidebar leaves the versions route.
+            // When the exit is caused by opening an annotation thread, the reset is tagged with
+            // origin so the side-by-side comparison stays open.
+            const onVersionChange = jest.fn();
+            const wrapper = getWrapper({ isComparing: true, onVersionChange });
+            const instance = wrapper.instance();
+
+            instance.onVersionChange(null, { origin: 'annotation' });
+
+            expect(onVersionChange).not.toHaveBeenCalled();
+        });
+
+        test('should notify the host for the annotation-driven version reset when not comparing', () => {
+            const onVersionChange = jest.fn();
+            const wrapper = getWrapper({ onVersionChange });
+            const instance = wrapper.instance();
+
+            instance.onVersionChange(null, { origin: 'annotation' });
+
+            expect(onVersionChange).toHaveBeenCalledWith(null, { origin: 'annotation' });
+        });
+    });
+
+    describe('handleAnnotationSelect while comparing', () => {
+        const getAnnotation = fileVersionId => ({
+            id: 'anno-1',
+            file_version: { id: fileVersionId },
+            target: { location: { type: 'page', value: 3 } },
+        });
+
+        const getComparingWrapper = (overrideProps = {}) => {
+            const wrapper = getWrapper({ isComparing: true, ...overrideProps });
+            const instance = wrapper.instance();
+            instance.setState({ file: { id: '123', file_version: { id: 'CURRENT' } } });
+            instance.emitScrollToAnnotation = jest.fn();
+            return { instance, wrapper };
+        };
+
+        test('should scroll in this pane for an annotation on the version it previews', () => {
+            const onComparedAnnotationSelect = jest.fn();
+            const { instance } = getComparingWrapper({ onComparedAnnotationSelect });
+            const annotation = getAnnotation('CURRENT');
+
+            instance.handleAnnotationSelect(annotation);
+
+            expect(instance.emitScrollToAnnotation).toHaveBeenCalledWith('anno-1', annotation.target);
+            expect(onComparedAnnotationSelect).not.toHaveBeenCalled();
+        });
+
+        test('should hand an annotation on another version to the compared pane', () => {
+            const onComparedAnnotationSelect = jest.fn();
+            const { instance } = getComparingWrapper({ onComparedAnnotationSelect });
+            const annotation = getAnnotation('OLD');
+
+            instance.handleAnnotationSelect(annotation, true);
+
+            expect(onComparedAnnotationSelect).toHaveBeenCalledWith(annotation, true);
+            expect(instance.emitScrollToAnnotation).not.toHaveBeenCalled();
+        });
+
+        test('should not change the version this pane previews', () => {
+            const { instance, wrapper } = getComparingWrapper();
+
+            instance.handleAnnotationSelect(getAnnotation('OLD'));
+
+            expect(wrapper.state('startAt')).toBeUndefined();
+        });
+
+        test('should scroll to a forwarded annotation when the request prop changes', () => {
+            const wrapper = getWrapper();
+            const instance = wrapper.instance();
+            instance.handleAnnotationSelect = jest.fn();
+            const annotation = getAnnotation('OLD');
+
+            wrapper.setProps({ annotationScrollRequest: { annotation, deferScrollToOnload: false } });
+
+            expect(instance.handleAnnotationSelect).toHaveBeenCalledWith(annotation, false);
+        });
     });
 
     describe('handleAnnotationSelect', () => {
@@ -2888,6 +2997,30 @@ describe('elements/content-preview/ContentPreview', () => {
             expect(wrapper.childAt(1).props().children.props.loadingIndicatorDelayMs).toBe(0);
         });
 
+        test('should inherit host annotations on the compared instance but keep create controls off', () => {
+            const boxAnnotations = jest.fn();
+            const wrapper = shallow(
+                <ContentPreviewWithComparison
+                    boxAnnotations={boxAnnotations}
+                    comparedVersion={{ id: '456' }}
+                    fileId="123"
+                    logger={{ onReadyMetric: jest.fn(), onPreviewMetric: jest.fn() }}
+                    showAnnotations
+                    showAnnotationsControls
+                />,
+            );
+
+            wrapper.childAt(0).props().comparedSlotRef(document.createElement('div'));
+            wrapper.update();
+
+            const comparedProps = wrapper.childAt(1).props().children.props;
+            expect(comparedProps.showAnnotations).toBe(true);
+            expect(comparedProps.boxAnnotations).toBe(boxAnnotations);
+            expect(comparedProps.showAnnotationsControls).toBe(false);
+            expect(comparedProps.enableAnnotationsDiscoverability).toBe(false);
+            expect(comparedProps.showAnnotationsDrawingCreate).toBe(false);
+        });
+
         test('should not forward the host onMetric to the compared instance', () => {
             const onMetric = jest.fn();
             const wrapper = shallow(

src/elements/content-sidebar/SidebarPanels.js

@@ -130,6 +130,7 @@ const LoadableVersionsSidebar = SidebarUtils.getAsyncSidebarContent(
 );
 
 const SIDEBAR_PATH_VERSIONS = '/:sidebar(activity|details)/versions/:versionId?';
+const SIDEBAR_PATH_ANNOTATIONS = '/:sidebar/annotations/:fileVersionId/:annotationId?';
 
 class SidebarPanels extends React.Component<Props, State> {
     boxAISidebar: ElementRefType = React.createRef();
@@ -168,9 +169,15 @@ class SidebarPanels extends React.Component<Props, State> {
         const { location, onVersionChange } = this.props;
         const { location: prevLocation } = prevProps;
 
-        // Reset the current version id if the wrapping versions route is no longer active
+        // Reset the current version id if the wrapping versions route is no longer active.
+        // Tag annotation-driven exits (versions -> annotation thread) so hosts comparing
+        // versions side by side can ignore the reset and keep the comparison open.
         if (onVersionChange && this.getVersionsMatchPath(prevLocation) && !this.getVersionsMatchPath(location)) {
-            onVersionChange(null);
+            if (matchPath(location.pathname, SIDEBAR_PATH_ANNOTATIONS)) {
+                onVersionChange(null, { origin: 'annotation' });
+            } else {
+                onVersionChange(null);
+            }
         }
     }