From 6aeeed8c9acde1137a791f4f31e565450bb785a8 Mon Sep 17 00:00:00 2001 From: Robert Knight Date: Tue, 4 Jul 2023 13:32:40 +0100 Subject: [PATCH] Don't close the sidebar for pointer events within the bounds of the sidebar Fix an issue where clicking in transparent areas of the bucket bar could close the sidebar when side-by-side mode is disabled. Such clicks are allowed to bubble up to `document.body` because the user may be trying to click on a scrollbar thumb or other element in this area. However we don't want these clicks to close the sidebar like clicking elsewhere on the document does, as this can be annoying if the user was trying to click on eg. a sidebar bucket but missed. In fixing this, the separate mouse and touch event listeners have been replaced with a pointer event listener, as this is now supported in all of our target browsers. --- src/annotator/guest.ts | 33 +++++++++++++++++++---------- src/annotator/test/guest-test.js | 36 ++++++++++++++++++++++---------- 2 files changed, 47 insertions(+), 22 deletions(-) diff --git a/src/annotator/guest.ts b/src/annotator/guest.ts index 2ee949c5f0c..20af64e5dd8 100644 --- a/src/annotator/guest.ts +++ b/src/annotator/guest.ts @@ -214,6 +214,11 @@ export class Guest extends TinyEmitter implements Annotator, Destroyable { /** Channel for guest-sidebar communication. */ private _sidebarRPC: PortRPC; + /** + * The most recently received sidebar layout information from the host frame. + */ + private _sidebarLayout: SidebarLayout | null; + private _bucketBarClient: BucketBarClient; private _listeners: ListenerCollection; @@ -306,6 +311,7 @@ export class Guest extends TinyEmitter implements Annotator, Destroyable { this._connectHost(hostFrame); this._sidebarRPC = new PortRPC(); + this._sidebarLayout = null; this._connectSidebar(); this._bucketBarClient = new BucketBarClient({ @@ -330,7 +336,7 @@ export class Guest extends TinyEmitter implements Annotator, Destroyable { // them from propagating out of their shadow roots, and hence clicking on // elements in the sidebar's vertical toolbar or adder won't close the // sidebar. - const maybeCloseSidebar = (element: Element) => { + const maybeCloseSidebar = (event: PointerEvent) => { // Don't hide the sidebar if event was disabled because the sidebar // doesn't overlap the content. if (this._integration.sideBySideActive()) { @@ -338,7 +344,19 @@ export class Guest extends TinyEmitter implements Annotator, Destroyable { } // Don't hide the sidebar if the event comes from an element that contains a highlight - if (annotationsAt(element).length) { + if (annotationsAt(event.target as Element).length) { + return; + } + + // If the click is within the bounds of the sidebar, ignore it. We don't + // want to close the sidebar if the user clicks eg. in transparent areas + // of the toolbar / bucket bar along the edge. Clicks within the sidebar + // iframe won't be received by the guest frame(s) at all. + if ( + frameFillsAncestor(window, this._hostFrame) && + this._sidebarLayout?.expanded && + window.innerWidth - event.clientX < this._sidebarLayout.width + ) { return; } @@ -354,15 +372,7 @@ export class Guest extends TinyEmitter implements Annotator, Destroyable { } }); - this._listeners.add(this.element, 'mousedown', ({ target }) => { - maybeCloseSidebar(target as Element); - }); - - // Allow taps on the document to hide the sidebar as well as clicks. - // On iOS < 13 (2019), elements like h2 or div don't emit 'click' events. - this._listeners.add(this.element, 'touchstart', ({ target }) => { - maybeCloseSidebar(target as Element); - }); + this._listeners.add(this.element, 'pointerdown', maybeCloseSidebar); this._listeners.add(this.element, 'mouseover', ({ target }) => { const tags = annotationsAt(target as Element); @@ -848,6 +858,7 @@ export class Guest extends TinyEmitter implements Annotator, Destroyable { * @param sidebarLayout */ fitSideBySide(sidebarLayout: SidebarLayout) { + this._sidebarLayout = sidebarLayout; this._integration.fitSideBySide(sidebarLayout); } diff --git a/src/annotator/test/guest-test.js b/src/annotator/test/guest-test.js index 642c2f9a84d..571cb8e2b4d 100644 --- a/src/annotator/test/guest-test.js +++ b/src/annotator/test/guest-test.js @@ -624,23 +624,37 @@ describe('Guest', () => { fakeSidebarFrame?.remove(); }); - it('hides sidebar on user "mousedown" or "touchstart" events in the document', () => { - for (let event of ['mousedown', 'touchstart']) { - rootElement.dispatchEvent(new Event(event)); + context('clicks/taps on the document', () => { + const simulateClick = (element = rootElement, clientX = 0) => + element.dispatchEvent( + new PointerEvent('pointerdown', { bubbles: true, clientX }) + ); + + it('hides sidebar', () => { + simulateClick(); assert.calledWith(sidebarRPC().call, 'closeSidebar'); - sidebarRPC().call.resetHistory(); - } - }); + }); + + it('does not hide sidebar if target is a highlight', () => { + simulateClick(fakeHighlight); + assert.notCalled(sidebarRPC().call); + }); - it('does not hide sidebar if side-by-side mode is active', () => { - for (let event of ['mousedown', 'touchstart']) { + it('does not hide sidebar if side-by-side mode is active', () => { fakeIntegration.sideBySideActive.returns(true); + simulateClick(); + assert.notCalled(sidebarRPC().call); + }); + + it('does not hide sidebar if event is within the bounds of the sidebar', () => { + createGuest(); + emitHostEvent('sidebarLayoutChanged', { expanded: true, width: 300 }); - rootElement.dispatchEvent(new Event(event)); + // Simulate click on the left edge of the sidebar. + simulateClick(rootElement, window.innerWidth - 295); assert.notCalled(sidebarRPC().call); - sidebarRPC().call.resetHistory(); - } + }); }); it('does not reposition the adder if hidden when the window is resized', () => {